-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
34de6f5
to
b9281c8
Compare
middleware/openidconnect.go
Outdated
func OpenIDConnect(opt OIDCOptions) func(http.Handler) http.Handler { | ||
|
||
var l log.Logger | ||
if opt.Logger == nil { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>
0753bee
to
d9e0380
Compare
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