feat: add provider to pkg #263
Conversation
pkg/info/info.go
Outdated
| type providerInfoKey struct{} | ||
|
|
||
| type ProviderInfo struct { | ||
| IntrospectionResponse *oidc.IntrospectionResponse |
There was a problem hiding this comment.
i wouldn't tie the info to the *oidc.introspectionResponse first of all that can be generic and what about userinfo, non oidc related info, ...?
There was a problem hiding this comment.
That's why the response is part of the struct, if we have other or additional information wie want to add to the context we either can split it or for the moment add it here with a set-function
pkg/provider/config.go
Outdated
| type configurationSAML struct{} | ||
|
|
||
| func OIDCProviderConfiguration(issuer, keyPath, clientID, clientSecret, port, callbackURL string, scopes []string) *configuration { | ||
| return &configuration{ | ||
| oidc: &configurationOIDC{ | ||
| issuer: issuer, | ||
| keyPath: keyPath, | ||
| clientID: clientID, | ||
| clientSecret: clientSecret, | ||
| port: port, | ||
| callbackURL: callbackURL, | ||
| scopes: scopes, | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| func SAMLProviderConfiguration() *configuration { | ||
| return &configuration{ | ||
| saml: &configurationSAML{}, | ||
| } | ||
| } |
There was a problem hiding this comment.
the constructors are never used and saml is not implemented (IMO just omit it currently)
pkg/provider/oidc.go
Outdated
| redirectURI := fmt.Sprintf("http://localhost:%v%v", oidc.port, oidc.callbackURL) | ||
| cookieHandler := httphelper.NewCookieHandler(key, key, httphelper.WithUnsecure()) |
There was a problem hiding this comment.
why is that statically using localhost and http?
pkg/provider/oidc.go
Outdated
| func marshalUserinfo() rp.CodeExchangeUserinfoCallback[*oidc.IDTokenClaims, *oidc.UserInfo] { | ||
| return func(w http.ResponseWriter, r *http.Request, tokens *oidc.Tokens[*oidc.IDTokenClaims], state string, rp rp.RelyingParty, info *oidc.UserInfo) { | ||
| data, err := json.Marshal(info) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusInternalServerError) | ||
| return | ||
| } | ||
| w.Write(data) | ||
| } | ||
| } |
There was a problem hiding this comment.
can be removed was part of the example and then the function to call can be provided as parameter
pkg/provider/oidc.go
Outdated
| pi := info.ProviderInfoFromContext(ctx) | ||
| resp, err := checkIntrospectWithCached(ctx, resourceServer, token, pi.IntrospectionResponse) |
There was a problem hiding this comment.
just for my understanding: how is this intended? and why is the introspection response always cached?
There was a problem hiding this comment.
BTW as it's calling the introspection endpoint, that would mean it's checking for authorization and not authentication
There was a problem hiding this comment.
it's only cached if an introspection response is already in the context, as for example the different interceptors are stacked. It's only calling the introspection endpoint to check if the token is still active, so you suggest to remove the introspection call and only check the token itself?
pkg/provider/oidc.go
Outdated
| return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { | ||
| ctx := req.Context() | ||
|
|
||
| token, err := checkOIDCTokenFromRequest(req) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusUnauthorized) | ||
| return | ||
| } | ||
|
|
||
| pi := info.ProviderInfoFromContext(ctx) | ||
| resp, err := checkIntrospectWithCached(ctx, resourceServer, token, pi.IntrospectionResponse) | ||
| if err != nil { | ||
| http.Error(w, err.Error(), http.StatusForbidden) | ||
| return | ||
| } | ||
| ctx = pi.SetIntrospectionResponse(resp).IntoContext(ctx) | ||
|
|
||
| if err := checkAuthorization(pi.IntrospectionResponse, requestedClaim, requestedValue); err != nil { | ||
| http.Error(w, err.Error(), http.StatusForbidden) | ||
| return | ||
| } | ||
| next.ServeHTTP(w, req.WithContext(ctx)) |
There was a problem hiding this comment.
mostly redundant (apart from the checkAuthorization) and why can on string claims be checked, resp. shouldn't there be multiple options what to be checked (simple authZ check, role check, custom checks, ...)
There was a problem hiding this comment.
can be expanded, started of with this as an example
pkg/provider/oidc.go
Outdated
| func checkIntrospect(ctx context.Context, resourceServer rs.ResourceServer, token string) (*oidc.IntrospectionResponse, error) { | ||
| resp, err := rs.Introspect[*oidc.IntrospectionResponse](ctx, resourceServer, token) | ||
| if err != nil { | ||
| return nil, ErrInvalidToken | ||
| } | ||
| if !resp.Active { | ||
| return nil, ErrInvalidToken | ||
| } | ||
| return resp, nil | ||
| } |
There was a problem hiding this comment.
as mentioned before, i'd make that generic so that developers can handle their custom claims (if needed)
pkg/provider/provider.go
Outdated
| ) | ||
|
|
||
| var ( | ||
| key = []byte("test1234test1234") |
There was a problem hiding this comment.
leftover from the copy, removed
pkg/provider/config.go
Outdated
| type configurationOIDC struct { | ||
| issuer string | ||
| keyPath string | ||
| clientID string | ||
| clientSecret string | ||
| port string | ||
| callbackURL string | ||
| scopes []string | ||
| } | ||
|
|
||
| func (c *configurationOIDC) validRS() bool { | ||
| return c.issuer != "" && c.keyPath != "" | ||
| } | ||
|
|
||
| func (c *configurationOIDC) validRP() bool { | ||
| return c.issuer != "" && c.clientID != "" && c.port != "" && c.callbackURL != "" | ||
| } |
There was a problem hiding this comment.
why is there only a single config (authentication and authorization)? e.g. what if i want jwt-profile for both, authentication and authorization?
There was a problem hiding this comment.
and IMO it's to OIDC focused. I'd go with a domain or alike on the provider configuration itself, which represents the zitadel domain to use. IMO easy understandable for devs and regardless of what protocol is used later on it can be used. In case of OIDC this will correspond to the issuer, but the same value can then also be used for the client to call the API and so on
There was a problem hiding this comment.
If you want a combination then you only have to instantiate multiple providers, as maybe for these 2 functions, only the minimum required values are checked.
Changed the issuer to host, port, and insecure attributes, to concatenate redirectURI and issuer from.
|
closing in favor of the next branch |
Definition of Ready