Skip to content
This repository has been archived by the owner on Jan 27, 2021. It is now read-only.

introduce oidc middleware #12

Merged
merged 1 commit into from
Dec 11, 2019
Merged

introduce oidc middleware #12

merged 1 commit into from
Dec 11, 2019

Conversation

butonic
Copy link
Member

@butonic butonic commented Dec 10, 2019

based on https://github.com/cs3org/reva/tree/master/pkg/auth/manager/oidc

It needs quite a bit of configuration. Will sort that out while testing with @DeepDiver1975

changelog/unreleased/issue-8 Outdated Show resolved Hide resolved
func OpenIDConnect(opt OIDCOptions) func(http.Handler) http.Handler {

var l log.Logger
if opt.Logger == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are using the same options pattern we are using already everywhere within the ocis repos you can define defaults much cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, so I move the options to individual functions inside pkg/ocis/options.go and implement a newOptions(o Option...) that fills the options object that is private to the middleware. right? at least somewhat?

Copy link
Member Author

Choose a reason for hiding this comment

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

@tboerger currently the middlewares all take explicit arguments ... that would be quite a list in this case ... I'd prefer to have a func OpenIDConnect(opts ...Option) func(http.Handler) http.Handler { signature than 9 parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

ok. @tboerger Does this make sense? I am asking because all other middlewares take explicit parameters ...

Copy link
Contributor

Choose a reason for hiding this comment

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

func OpenIDConnect(opts ...Option) func(http.Handler) http.Handler { is what I was refering to. That's the scheme we are using for all the ocis repos.

Copy link
Member Author

Choose a reason for hiding this comment

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

but for middlewares like Static we have func Static(root string, fs http.FileSystem) func(http.Handler) http.Handler {. They don't follow that pattern because ... it's only two parameters? When do we start using the options pattern ... if that is a thing ... 3 parameters? 4? when the number starts to suck?

Just trying to find something we can note down as a coding guideline ... somewhere...

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed I have not done that for these 2 cases of the middlewares because there are only 2 options. Feel free to add options for all of them to keep it consistent with the other repos.

middleware/openidconnect.go Outdated Show resolved Hide resolved
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
@DeepDiver1975 DeepDiver1975 marked this pull request as ready for review December 11, 2019 14:32
@DeepDiver1975 DeepDiver1975 merged commit ab52855 into master Dec 11, 2019
@delete-merged-branch delete-merged-branch bot deleted the oidc-middleware branch December 11, 2019 14:39
@DeepDiver1975 DeepDiver1975 mentioned this pull request Dec 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants