Conversation
middlewarex/oidc.go
Outdated
| func OIDC(ctx context.Context, issuer string, opts ...OIDCMiddlewareOption) func(next http.Handler) http.Handler { | ||
| provider, err := oidc.NewProvider(ctx, issuer) | ||
| if err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
Should we panic here or do it like
https://github.com/HGV/iam-api/commit/870634b0f39e3794edc49b43abf0dac9578ecbde
There was a problem hiding this comment.
I would either panic or return the middleware as a struct and an error in this case.
There was a problem hiding this comment.
According to https://go.dev/doc/effective_go#panic it’s acceptable to panic() in this case because the OIDC middleware can’t set itself up. But this could also be seen from another perspective: What if it isn’t a critical error for the app if the OIDC middleware fails. For example, it could fall back to another authentication mechanism. Just hypothesizing. In that case it would be better to return the error during setup instead of panic(). What’s your take on this?
middlewarex/oidc.go
Outdated
| return | ||
| } | ||
|
|
||
| ctx := contextWithIDToken(r.Context(), idToken) |
There was a problem hiding this comment.
Two r.Context() calls could be stored in a variable instead.
middlewarex/oidc.go
Outdated
|
|
||
| return func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| authHeader := r.Header.Get("Authorization") |
There was a problem hiding this comment.
Nit: I would pass that function result directly to validateAuthHeader instead of creating a dedicated variable.
francesconi
force-pushed
the
oidc-middleware
branch
from
692f5f9 to
64e265d
Compare
francesconi
force-pushed
the
oidc-middleware
branch
from
64e265d to
928545e
Compare
4 participants
No description provided.