Repository: zitadel/oidc
Pull request #467 (2023-10-24 13:26:21)

fix(server): do not get client by id for introspection
As introspection is a Oauth mechanism for resource servers only, it does not make sense to get an oidc client by ID. The original OP did not do this and now we make the server behavior similar.

Tested in zitadel commit https://github.com/zitadel/zitadel/commit/ac5122d2acc3da79b2bb92e5cd5c5aeeabb22872

### Definition of Ready

- [x] I am happy with the code
- [x] Short description of the feature/issue is added in the pr description
- [x] PR is linked to the corresponding user story
- [x] Acceptance criteria are met
- [x] All open todos and follow ups are defined in a new ticket and justified
- [x] Deviations from the acceptance criteria and design are agreed with the PO and documented.
- [x] No debug or dead code
- [x] My code has no repetitions
- [x] Critical parts are tested automatically
- [x] Where possible E2E tests are implemented
- [x] Documentation/examples are up-to-date
- [x] All non-functional requirements are met
- [x] Functionality of the acceptance criteria is checked manually on the dev system.



codecov[bot]: ## [Codecov](https://app.codecov.io/gh/zitadel/oidc/pull/467?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) Report
> Merging [#467](https://app.codecov.io/gh/zitadel/oidc/pull/467?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (2d7d92a) into [main](https://app.codecov.io/gh/zitadel/oidc/commit/bab53998599bfe56f1dc3c6a1c3f52b8a97b3a6b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (bab5399) will **decrease** coverage by `0.08%`.
> The diff coverage is `72.50%`.

```diff
@@            Coverage Diff             @@
##             main     #467      +/-   ##
==========================================
- Coverage   60.34%   60.27%   -0.08%     
==========================================
  Files          78       78              
  Lines        6713     6738      +25     
==========================================
+ Hits         4051     4061      +10     
- Misses       2370     2381      +11     
- Partials      292      296       +4     
```


| [Files](https://app.codecov.io/gh/zitadel/oidc/pull/467?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) | Coverage Δ | |
|---|---|---|
| [pkg/op/server.go](https://app.codecov.io/gh/zitadel/oidc/pull/467?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL3NlcnZlci5nbw==) | `81.08% <100.00%> (ø)` | |
| [pkg/op/server\_http.go](https://app.codecov.io/gh/zitadel/oidc/pull/467?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL3NlcnZlcl9odHRwLmdv) | `98.73% <100.00%> (-1.01%)` | :arrow_down: |
| [pkg/op/token\_intospection.go](https://app.codecov.io/gh/zitadel/oidc/pull/467?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL3Rva2VuX2ludG9zcGVjdGlvbi5nbw==) | `43.24% <ø> (ø)` | |
| [pkg/op/server\_legacy.go](https://app.codecov.io/gh/zitadel/oidc/pull/467?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL3NlcnZlcl9sZWdhY3kuZ28=) | `44.80% <31.25%> (-1.24%)` | :arrow_down: |



Patch:
diff --git a/pkg/op/server.go b/pkg/op/server.go
index a9cdcf5..829618c 100644
--- a/pkg/op/server.go
+++ b/pkg/op/server.go
@@ -127,7 +127,7 @@ type Server interface {
 	// Introspect handles the OAuth 2.0 Token Introspection endpoint.
 	// https://datatracker.ietf.org/doc/html/rfc7662
 	// The recommended Response Data type is [oidc.IntrospectionResponse].
-	Introspect(context.Context, *ClientRequest[oidc.IntrospectionRequest]) (*Response, error)
+	Introspect(context.Context, *Request[IntrospectionRequest]) (*Response, error)
 
 	// UserInfo handles the UserInfo endpoint and returns Claims about the authenticated End-User.
 	// https://openid.net/specs/openid-connect-core-1_0.html#UserInfo
@@ -329,7 +329,7 @@ func (UnimplementedServer) DeviceToken(ctx context.Context, r *ClientRequest[oid
 	return nil, unimplementedGrantError(oidc.GrantTypeDeviceCode)
 }
 
-func (UnimplementedServer) Introspect(ctx context.Context, r *ClientRequest[oidc.IntrospectionRequest]) (*Response, error) {
+func (UnimplementedServer) Introspect(ctx context.Context, r *Request[IntrospectionRequest]) (*Response, error) {
 	return nil, unimplementedError(r)
 }
 
diff --git a/pkg/op/server_http.go b/pkg/op/server_http.go
index 750f7a9..6d379c6 100644
--- a/pkg/op/server_http.go
+++ b/pkg/op/server_http.go
@@ -102,7 +102,7 @@ func (s *webServer) createRouter() {
 	s.endpointRoute(s.endpoints.Authorization, s.authorizeHandler)
 	s.endpointRoute(s.endpoints.DeviceAuthorization, s.withClient(s.deviceAuthorizationHandler))
 	s.endpointRoute(s.endpoints.Token, s.tokensHandler)
-	s.endpointRoute(s.endpoints.Introspection, s.withClient(s.introspectionHandler))
+	s.endpointRoute(s.endpoints.Introspection, s.introspectionHandler)
 	s.endpointRoute(s.endpoints.Userinfo, s.userInfoHandler)
 	s.endpointRoute(s.endpoints.Revocation, s.withClient(s.revocationHandler))
 	s.endpointRoute(s.endpoints.EndSession, s.endSessionHandler)
@@ -136,7 +136,21 @@ func (s *webServer) withClient(handler clientHandler) http.HandlerFunc {
 }
 
 func (s *webServer) verifyRequestClient(r *http.Request) (_ Client, err error) {
-	if err = r.ParseForm(); err != nil {
+	cc, err := s.parseClientCredentials(r)
+	if err != nil {
+		return nil, err
+	}
+	return s.server.VerifyClient(r.Context(), &Request[ClientCredentials]{
+		Method: r.Method,
+		URL:    r.URL,
+		Header: r.Header,
+		Form:   r.Form,
+		Data:   cc,
+	})
+}
+
+func (s *webServer) parseClientCredentials(r *http.Request) (_ *ClientCredentials, err error) {
+	if err := r.ParseForm(); err != nil {
 		return nil, oidc.ErrInvalidRequest().WithDescription("error parsing form").WithParent(err)
 	}
 	cc := new(ClientCredentials)
@@ -160,13 +174,7 @@ func (s *webServer) verifyRequestClient(r *http.Request) (_ Client, err error) {
 	if cc.ClientAssertion != "" && cc.ClientAssertionType != oidc.ClientAssertionTypeJWTAssertion {
 		return nil, oidc.ErrInvalidRequest().WithDescription("invalid client_assertion_type %s", cc.ClientAssertionType)
 	}
-	return s.server.VerifyClient(r.Context(), &Request[ClientCredentials]{
-		Method: r.Method,
-		URL:    r.URL,
-		Header: r.Header,
-		Form:   r.Form,
-		Data:   cc,
-	})
+	return cc, nil
 }
 
 func (s *webServer) authorizeHandler(w http.ResponseWriter, r *http.Request) {
@@ -378,8 +386,13 @@ func (s *webServer) deviceTokenHandler(w http.ResponseWriter, r *http.Request, c
 	resp.writeOut(w)
 }
 
-func (s *webServer) introspectionHandler(w http.ResponseWriter, r *http.Request, client Client) {
-	if client.AuthMethod() == oidc.AuthMethodNone {
+func (s *webServer) introspectionHandler(w http.ResponseWriter, r *http.Request) {
+	cc, err := s.parseClientCredentials(r)
+	if err != nil {
+		WriteError(w, r, err, s.getLogger(r.Context()))
+		return
+	}
+	if cc.ClientSecret == "" && cc.ClientAssertion == "" {
 		WriteError(w, r, oidc.ErrInvalidClient().WithDescription("client must be authenticated"), s.getLogger(r.Context()))
 		return
 	}
@@ -392,7 +405,7 @@ func (s *webServer) introspectionHandler(w http.ResponseWriter, r *http.Request,
 		WriteError(w, r, oidc.ErrInvalidRequest().WithDescription("token missing"), s.getLogger(r.Context()))
 		return
 	}
-	resp, err := s.server.Introspect(r.Context(), newClientRequest(r, request, client))
+	resp, err := s.server.Introspect(r.Context(), newRequest(r, &IntrospectionRequest{cc, request}))
 	if err != nil {
 		WriteError(w, r, err, s.getLogger(r.Context()))
 		return
diff --git a/pkg/op/server_legacy.go b/pkg/op/server_legacy.go
index 2006e90..deb1abc 100644
--- a/pkg/op/server_legacy.go
+++ b/pkg/op/server_legacy.go
@@ -18,6 +18,7 @@ type ExtendedLegacyServer interface {
 	Server
 	Provider() OpenIDProvider
 	Endpoints() Endpoints
+	AuthCallbackURL() func(context.Context, string) string
 }
 
 // RegisterLegacyServer registers a [LegacyServer] or an extension thereof.
@@ -31,7 +32,7 @@ func RegisterLegacyServer(s ExtendedLegacyServer, options ...ServerOption) http.
 	options = append(options,
 		WithHTTPMiddleware(intercept(provider.IssuerFromRequest)),
 		WithSetRouter(func(r chi.Router) {
-			r.HandleFunc(authCallbackPath(provider), authorizeCallbackHandler(provider))
+			r.HandleFunc(s.Endpoints().Authorization.Relative()+authCallbackPathSuffix, authorizeCallbackHandler(provider))
 		}),
 	)
 	return RegisterServer(s, s.Endpoints(), options...)
@@ -75,6 +76,13 @@ func (s *LegacyServer) Endpoints() Endpoints {
 	return s.endpoints
 }
 
+// AuthCallbackURL builds the url for the redirect (with the requestID) after a successful login
+func (s *LegacyServer) AuthCallbackURL() func(context.Context, string) string {
+	return func(ctx context.Context, requestID string) string {
+		return s.endpoints.Authorization.Absolute(IssuerFromContext(ctx)) + authCallbackPathSuffix + "?id=" + requestID
+	}
+}
+
 func (s *LegacyServer) Health(_ context.Context, r *Request[struct{}]) (*Response, error) {
 	return NewResponse(Status{Status: "ok"}), nil
 }
@@ -307,13 +315,30 @@ func (s *LegacyServer) DeviceToken(ctx context.Context, r *ClientRequest[oidc.De
 	return NewResponse(resp), nil
 }
 
-func (s *LegacyServer) Introspect(ctx context.Context, r *ClientRequest[oidc.IntrospectionRequest]) (*Response, error) {
+func (s *LegacyServer) authenticateResourceClient(ctx context.Context, cc *ClientCredentials) (string, error) {
+	if cc.ClientAssertion != "" {
+		if jp, ok := s.provider.(ClientJWTProfile); ok {
+			return ClientJWTAuth(ctx, oidc.ClientAssertionParams{ClientAssertion: cc.ClientAssertion}, jp)
+		}
+		return "", oidc.ErrInvalidClient().WithDescription("client_assertion not supported")
+	}
+	if err := s.provider.Storage().AuthorizeClientIDSecret(ctx, cc.ClientID, cc.ClientSecret); err != nil {
+		return "", oidc.ErrUnauthorizedClient().WithParent(err)
+	}
+	return cc.ClientID, nil
+}
+
+func (s *LegacyServer) Introspect(ctx context.Context, r *Request[IntrospectionRequest]) (*Response, error) {
+	clientID, err := s.authenticateResourceClient(ctx, r.Data.ClientCredentials)
+	if err != nil {
+		return nil, err
+	}
 	response := new(oidc.IntrospectionResponse)
 	tokenID, subject, ok := getTokenIDAndSubject(ctx, s.provider, r.Data.Token)
 	if !ok {
 		return NewResponse(response), nil
 	}
-	err := s.provider.Storage().SetIntrospectionFromToken(ctx, response, tokenID, subject, r.Client.GetID())
+	err = s.provider.Storage().SetIntrospectionFromToken(ctx, response, tokenID, subject, clientID)
 	if err != nil {
 		return NewResponse(response), nil
 	}
diff --git a/pkg/op/token_intospection.go b/pkg/op/token_intospection.go
index 21b79c3..9c45ef8 100644
--- a/pkg/op/token_intospection.go
+++ b/pkg/op/token_intospection.go
@@ -65,3 +65,8 @@ func ParseTokenIntrospectionRequest(r *http.Request, introspector Introspector)
 
 	return req.Token, clientID, nil
 }
+
+type IntrospectionRequest struct {
+	*ClientCredentials
+	*oidc.IntrospectionRequest
+}


Test patch:
diff --git a/pkg/op/server_http_test.go b/pkg/op/server_http_test.go
index 86fe7ed..4eac4a0 100644
--- a/pkg/op/server_http_test.go
+++ b/pkg/op/server_http_test.go
@@ -1001,14 +1001,12 @@ func Test_webServer_introspectionHandler(t *testing.T) {
 	tests := []struct {
 		name    string
 		decoder httphelper.Decoder
-		client  Client
 		r       *http.Request
 		want    webServerResult
 	}{
 		{
 			name:    "decoder error",
 			decoder: schema.NewDecoder(),
-			client:  newClient(clientTypeUserAgent),
 			r:       httptest.NewRequest(http.MethodPost, "/", strings.NewReader("foo=bar")),
 			want: webServerResult{
 				wantStatus: http.StatusBadRequest,
@@ -1018,8 +1016,7 @@ func Test_webServer_introspectionHandler(t *testing.T) {
 		{
 			name:    "public client",
 			decoder: testDecoder,
-			client:  newClient(clientTypeNative),
-			r:       httptest.NewRequest(http.MethodPost, "/", strings.NewReader("foo=bar")),
+			r:       httptest.NewRequest(http.MethodPost, "/", strings.NewReader("client_id=123")),
 			want: webServerResult{
 				wantStatus: http.StatusBadRequest,
 				wantBody:   `{"error":"invalid_client", "error_description":"client must be authenticated"}`,
@@ -1028,8 +1025,7 @@ func Test_webServer_introspectionHandler(t *testing.T) {
 		{
 			name:    "token missing",
 			decoder: testDecoder,
-			client:  newClient(clientTypeWeb),
-			r:       httptest.NewRequest(http.MethodPost, "/", strings.NewReader("foo=bar")),
+			r:       httptest.NewRequest(http.MethodPost, "/", strings.NewReader("client_id=123&client_secret=SECRET")),
 			want: webServerResult{
 				wantStatus: http.StatusBadRequest,
 				wantBody:   `{"error":"invalid_request", "error_description":"token missing"}`,
@@ -1038,8 +1034,7 @@ func Test_webServer_introspectionHandler(t *testing.T) {
 		{
 			name:    "unimplemented Introspect called",
 			decoder: testDecoder,
-			client:  newClient(clientTypeWeb),
-			r:       httptest.NewRequest(http.MethodPost, "/", strings.NewReader("token=xxx")),
+			r:       httptest.NewRequest(http.MethodPost, "/", strings.NewReader("client_id=123&client_secret=SECRET&token=xxx")),
 			want: webServerResult{
 				wantStatus: UnimplementedStatusCode,
 				wantBody:   `{"error":"server_error", "error_description":"/ not implemented on this server"}`,
@@ -1053,7 +1048,7 @@ func Test_webServer_introspectionHandler(t *testing.T) {
 				decoder: tt.decoder,
 				logger:  slog.Default(),
 			}
-			runWebServerClientTest(t, s.introspectionHandler, tt.r, tt.client, tt.want)
+			runWebServerTest(t, s.introspectionHandler, tt.r, tt.want)
 		})
 	}
 }