sso-proxy: use internal/pkg/sessions by shrayolacrayon · Pull Request #137 · buzzfeed/sso

Skip to content

Navigation Menu

Sign in

Appearance settings

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up

Appearance settings

Conversation

@shrayolacrayon

Copy link

@shrayolacrayon shrayolacrayon commented

Jan 2, 2019

edited

Loading

Changes all session store and csrf store logic to use internal/pkg/sessions.

Fixes #43.

RFR @buzzfeed/infra-security @buzzfeed/sso-maintainers

@shrayolacrayon shrayolacrayon force-pushed the shraya/sso-proxy-internal-sessionstate branch from aee4fbb to f432868 Compare

January 2, 2019 21:38
func (ms *MockSessionStore) LoadSession(*http.Request) (*SessionState, error) {
if ms.Session == nil {
return nil, http.ErrNoCookie
}
Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pretty trivial but might be cleaner and more concise to to set ms.LoadError to http.ErrNoCookie here?

katzdm reacted with thumbs up emoji

katzdm

katzdm previously approved these changes Jan 16, 2019

benjsto

benjsto previously approved these changes Jan 16, 2019

@shrayolacrayon shrayolacrayon dismissed stale reviews from benjsto and katzdm via 4713a33

January 18, 2019 17:04

@shrayolacrayon shrayolacrayon force-pushed the shraya/sso-proxy-internal-sessionstate branch from 9f32476 to 4713a33 Compare

January 18, 2019 17:04

@shrayolacrayon shrayolacrayon merged commit 97b15cb into master

Jan 18, 2019

@shrayolacrayon shrayolacrayon deleted the shraya/sso-proxy-internal-sessionstate branch

January 18, 2019 18:06

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@loganmeetsworld loganmeetsworld loganmeetsworld approved these changes

@benjsto benjsto benjsto left review comments

@katzdm katzdm katzdm left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shrayolacrayon @benjsto @katzdm @loganmeetsworld