Repository: zitadel/oidc
Pull request #466 (2023-10-23 13:54:29)

feat(op): allow Legacy Server extension
This change splits the constructor and registration of the Legacy Server. This allows it to be extended by struct embedding.

Related https://github.com/zitadel/zitadel/issues/6619

### 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/466?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) Report
> Merging [#466](https://app.codecov.io/gh/zitadel/oidc/pull/466?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (506b5ce) into [main](https://app.codecov.io/gh/zitadel/oidc/commit/434b2e62d8f2703a86b95a4dacc9647777aad491?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) (434b2e6) will **increase** coverage by `0.97%`.
> The diff coverage is `100.00%`.

```diff
@@            Coverage Diff             @@
##             main     #466      +/-   ##
==========================================
+ Coverage   60.30%   61.27%   +0.97%     
==========================================
  Files          78       74       -4     
  Lines        6698     6608      -90     
==========================================
+ Hits         4039     4049      +10     
+ Misses       2369     2269     -100     
  Partials      290      290              
```


| [Files](https://app.codecov.io/gh/zitadel/oidc/pull/466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel) | Coverage Δ | |
|---|---|---|
| [pkg/op/server\_http.go](https://app.codecov.io/gh/zitadel/oidc/pull/466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL3NlcnZlcl9odHRwLmdv) | `99.73% <100.00%> (+<0.01%)` | :arrow_up: |
| [pkg/op/server\_legacy.go](https://app.codecov.io/gh/zitadel/oidc/pull/466?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel#diff-cGtnL29wL3NlcnZlcl9sZWdhY3kuZ28=) | `46.15% <100.00%> (+1.27%)` | :arrow_up: |

... and [4 files with indirect coverage changes](https://app.codecov.io/gh/zitadel/oidc/pull/466/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=zitadel)



Patch:
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
index 644b23f..ab22f8d 100644
--- a/.github/workflows/release.yml
+++ b/.github/workflows/release.yml
@@ -2,6 +2,7 @@ name: Release
 on:
   push:
     branches:
+      - "2.11.x"
       - main
       - next
     tags-ignore:
diff --git a/.releaserc.js b/.releaserc.js
index e8eea8e..c87b1d1 100644
--- a/.releaserc.js
+++ b/.releaserc.js
@@ -1,5 +1,6 @@
 module.exports = {
     branches: [
+        {name: "2.11.x"},
         {name: "main"},
         {name: "next", prerelease: true},
     ],
diff --git a/SECURITY.md b/SECURITY.md
index d682630..a32b842 100644
--- a/SECURITY.md
+++ b/SECURITY.md
@@ -1,6 +1,6 @@
 # Security Policy
 
-At ZITADEL we are extremely grateful for security aware people that disclose vulnerabilities to us and the open source community. All reports will be investigated by our team.
+Please refer to the security policy [on zitadel/zitadel](https://github.com/zitadel/zitadel/blob/main/SECURITY.md) which is applicable for all open source repositories of our organization.
 
 ## Supported Versions
 
@@ -9,43 +9,12 @@ We currently support the following version of the OIDC framework:
 | Version  | Supported          | Branch      | Details                              |
 | -------- | ------------------ | ----------- | ------------------------------------ |
 | 0.x.x    | :x:                |             | not maintained                       |
-| <1.13    | :x:                |             | not maintained                       |
-| 1.13.x   | :lock: :warning:   | [1.13.x][1] | security only, [community effort][2] |
-| 2.x.x    | :heavy_check_mark: | [main][3]   | supported                            |
-| 3.0.0-xx | :white_check_mark: | [next][4]   | [developement branch][5]             |
+| <2.11    | :x:                |             | not maintained                       |
+| 2.11.x   | :lock: :warning:   | [2.11.x][1] | security only, [community effort][2] |
+| 3.x.x    | :heavy_check_mark: | [main][3]   | supported                            |
+| 4.0.0-xx | :white_check_mark: | [next][4]   | [development branch]                 |
 
-[1]: https://github.com/zitadel/oidc/tree/1.13.x
-[2]: https://github.com/zitadel/oidc/discussions/378
+[1]: https://github.com/zitadel/oidc/tree/2.11.x
+[2]: https://github.com/zitadel/oidc/discussions/458
 [3]: https://github.com/zitadel/oidc/tree/main
 [4]: https://github.com/zitadel/oidc/tree/next
-[5]: https://github.com/zitadel/oidc/milestone/2
-
-## Reporting a vulnerability
-
-To file a incident, please disclose by email to security@zitadel.com with the security details.
-
-At the moment GPG encryption is no yet supported, however you may sign your message at will.
-
-### When should I report a vulnerability
-
-* You think you discovered a ...
-  * ... potential security vulnerability in the SDK
-  * ... vulnerability in another project that this SDK bases on
-* For projects with their own vulnerability reporting and disclosure process, please report it directly there
-
-### When should I NOT report a vulnerability
-
-* You need help applying security related updates
-* Your issue is not security related
-
-## Security Vulnerability Response
-
-TBD
-
-## Public Disclosure
-
-All accepted and mitigated vulnerabilities will be published on the [Github Security Page](https://github.com/zitadel/oidc/security/advisories)
-
-### Timing
-
-We think it is crucial to publish advisories `ASAP` as mitigations are ready. But due to the unknown nature of the disclosures the time frame can range from 7 to 90 days.
diff --git a/example/server/exampleop/op.go b/example/server/exampleop/op.go
index 74018da..baa2662 100644
--- a/example/server/exampleop/op.go
+++ b/example/server/exampleop/op.go
@@ -80,7 +80,7 @@ func SetupServer(issuer string, storage Storage, logger *slog.Logger, wrapServer
 
 	handler := http.Handler(provider)
 	if wrapServer {
-		handler = op.NewLegacyServer(provider, *op.DefaultEndpoints)
+		handler = op.RegisterLegacyServer(op.NewLegacyServer(provider, *op.DefaultEndpoints))
 	}
 
 	// we register the http handler of the OP on the root, so that the discovery endpoint (/.well-known/openid-configuration)
diff --git a/pkg/op/server_http.go b/pkg/op/server_http.go
index 96ee7a5..750f7a9 100644
--- a/pkg/op/server_http.go
+++ b/pkg/op/server_http.go
@@ -25,11 +25,13 @@ func RegisterServer(server Server, endpoints Endpoints, options ...ServerOption)
 	decoder.IgnoreUnknownKeys(true)
 
 	ws := &webServer{
+		router:    chi.NewRouter(),
 		server:    server,
 		endpoints: endpoints,
 		decoder:   decoder,
 		logger:    slog.Default(),
 	}
+	ws.router.Use(cors.New(defaultCORSOptions).Handler)
 
 	for _, option := range options {
 		option(ws)
@@ -45,7 +47,14 @@ type ServerOption func(s *webServer)
 // the Server's router.
 func WithHTTPMiddleware(m ...func(http.Handler) http.Handler) ServerOption {
 	return func(s *webServer) {
-		s.middleware = m
+		s.router.Use(m...)
+	}
+}
+
+// WithSetRouter allows customization or the Server's router.
+func WithSetRouter(set func(chi.Router)) ServerOption {
+	return func(s *webServer) {
+		set(s.router)
 	}
 }
 
@@ -67,12 +76,15 @@ func WithFallbackLogger(logger *slog.Logger) ServerOption {
 }
 
 type webServer struct {
-	http.Handler
-	server     Server
-	middleware []func(http.Handler) http.Handler
-	endpoints  Endpoints
-	decoder    httphelper.Decoder
-	logger     *slog.Logger
+	server    Server
+	router    *chi.Mux
+	endpoints Endpoints
+	decoder   httphelper.Decoder
+	logger    *slog.Logger
+}
+
+func (s *webServer) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	s.router.ServeHTTP(w, r)
 }
 
 func (s *webServer) getLogger(ctx context.Context) *slog.Logger {
@@ -83,27 +95,23 @@ func (s *webServer) getLogger(ctx context.Context) *slog.Logger {
 }
 
 func (s *webServer) createRouter() {
-	router := chi.NewRouter()
-	router.Use(cors.New(defaultCORSOptions).Handler)
-	router.Use(s.middleware...)
-	router.HandleFunc(healthEndpoint, simpleHandler(s, s.server.Health))
-	router.HandleFunc(readinessEndpoint, simpleHandler(s, s.server.Ready))
-	router.HandleFunc(oidc.DiscoveryEndpoint, simpleHandler(s, s.server.Discovery))
-
-	s.endpointRoute(router, s.endpoints.Authorization, s.authorizeHandler)
-	s.endpointRoute(router, s.endpoints.DeviceAuthorization, s.withClient(s.deviceAuthorizationHandler))
-	s.endpointRoute(router, s.endpoints.Token, s.tokensHandler)
-	s.endpointRoute(router, s.endpoints.Introspection, s.withClient(s.introspectionHandler))
-	s.endpointRoute(router, s.endpoints.Userinfo, s.userInfoHandler)
-	s.endpointRoute(router, s.endpoints.Revocation, s.withClient(s.revocationHandler))
-	s.endpointRoute(router, s.endpoints.EndSession, s.endSessionHandler)
-	s.endpointRoute(router, s.endpoints.JwksURI, simpleHandler(s, s.server.Keys))
-	s.Handler = router
+	s.router.HandleFunc(healthEndpoint, simpleHandler(s, s.server.Health))
+	s.router.HandleFunc(readinessEndpoint, simpleHandler(s, s.server.Ready))
+	s.router.HandleFunc(oidc.DiscoveryEndpoint, simpleHandler(s, s.server.Discovery))
+
+	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.Userinfo, s.userInfoHandler)
+	s.endpointRoute(s.endpoints.Revocation, s.withClient(s.revocationHandler))
+	s.endpointRoute(s.endpoints.EndSession, s.endSessionHandler)
+	s.endpointRoute(s.endpoints.JwksURI, simpleHandler(s, s.server.Keys))
 }
 
-func (s *webServer) endpointRoute(router *chi.Mux, e *Endpoint, hf http.HandlerFunc) {
+func (s *webServer) endpointRoute(e *Endpoint, hf http.HandlerFunc) {
 	if e != nil {
-		router.HandleFunc(e.Relative(), hf)
+		s.router.HandleFunc(e.Relative(), hf)
 		s.logger.Info("registered route", "endpoint", e.Relative())
 	}
 }
diff --git a/pkg/op/server_legacy.go b/pkg/op/server_legacy.go
index 5907e28..2006e90 100644
--- a/pkg/op/server_legacy.go
+++ b/pkg/op/server_legacy.go
@@ -10,37 +10,69 @@ import (
 	"github.com/zitadel/oidc/v3/pkg/oidc"
 )
 
-// LegacyServer is an implementation of [Server[] that
-// simply wraps a [OpenIDProvider].
+// ExtendedLegacyServer allows embedding [LegacyServer] in a struct,
+// so that its methods can be individually overridden.
+//
+// EXPERIMENTAL: may change until v4
+type ExtendedLegacyServer interface {
+	Server
+	Provider() OpenIDProvider
+	Endpoints() Endpoints
+}
+
+// RegisterLegacyServer registers a [LegacyServer] or an extension thereof.
+// It takes care of registering the IssuerFromRequest middleware
+// and Authorization Callback Routes.
+// Neither are part of the bare [Server] interface.
+//
+// EXPERIMENTAL: may change until v4
+func RegisterLegacyServer(s ExtendedLegacyServer, options ...ServerOption) http.Handler {
+	provider := s.Provider()
+	options = append(options,
+		WithHTTPMiddleware(intercept(provider.IssuerFromRequest)),
+		WithSetRouter(func(r chi.Router) {
+			r.HandleFunc(authCallbackPath(provider), authorizeCallbackHandler(provider))
+		}),
+	)
+	return RegisterServer(s, s.Endpoints(), options...)
+}
+
+// LegacyServer is an implementation of [Server] that
+// simply wraps an [OpenIDProvider].
 // It can be used to transition from the former Provider/Storage
 // interfaces to the new Server interface.
+//
+// EXPERIMENTAL: may change until v4
 type LegacyServer struct {
 	UnimplementedServer
 	provider  OpenIDProvider
 	endpoints Endpoints
 }
 
-// NewLegacyServer wraps provider in a `Server` and returns a handler which is
-// the Server's router.
+// NewLegacyServer wraps provider in a `Server` implementation
 //
 // Only non-nil endpoints will be registered on the router.
 // Nil endpoints are disabled.
 //
-// The passed endpoints is also set to the provider,
-// to be consistent with the discovery config.
+// The passed endpoints is also used for the discovery config,
+// and endpoints already set to the provider are ignored.
 // Any `With*Endpoint()` option used on the provider is
 // therefore ineffective.
-func NewLegacyServer(provider OpenIDProvider, endpoints Endpoints) http.Handler {
-	server := RegisterServer(&LegacyServer{
+//
+// EXPERIMENTAL: may change until v4
+func NewLegacyServer(provider OpenIDProvider, endpoints Endpoints) *LegacyServer {
+	return &LegacyServer{
 		provider:  provider,
 		endpoints: endpoints,
-	}, endpoints, WithHTTPMiddleware(intercept(provider.IssuerFromRequest)))
+	}
+}
 
-	router := chi.NewRouter()
-	router.Mount("/", server)
-	router.HandleFunc(authCallbackPath(provider), authorizeCallbackHandler(provider))
+func (s *LegacyServer) Provider() OpenIDProvider {
+	return s.provider
+}
 
-	return router
+func (s *LegacyServer) Endpoints() Endpoints {
+	return s.endpoints
 }
 
 func (s *LegacyServer) Health(_ context.Context, r *Request[struct{}]) (*Response, error) {
@@ -336,9 +368,14 @@ func (s *LegacyServer) EndSession(ctx context.Context, r *Request[oidc.EndSessio
 	if err != nil {
 		return nil, err
 	}
-	err = s.provider.Storage().TerminateSession(ctx, session.UserID, session.ClientID)
+	redirect := session.RedirectURI
+	if fromRequest, ok := s.provider.Storage().(CanTerminateSessionFromRequest); ok {
+		redirect, err = fromRequest.TerminateSessionFromRequest(ctx, session)
+	} else {
+		err = s.provider.Storage().TerminateSession(ctx, session.UserID, session.ClientID)
+	}
 	if err != nil {
 		return nil, err
 	}
-	return NewRedirect(session.RedirectURI), nil
+	return NewRedirect(redirect), nil
 }


Test patch:
diff --git a/pkg/op/server_http_routes_test.go b/pkg/op/server_http_routes_test.go
index c7767d2..c50e989 100644
--- a/pkg/op/server_http_routes_test.go
+++ b/pkg/op/server_http_routes_test.go
@@ -32,7 +32,7 @@ func jwtProfile() (string, error) {
 }
 
 func TestServerRoutes(t *testing.T) {
-	server := op.NewLegacyServer(testProvider, *op.DefaultEndpoints)
+	server := op.RegisterLegacyServer(op.NewLegacyServer(testProvider, *op.DefaultEndpoints))
 
 	storage := testProvider.Storage().(routesTestStorage)
 	ctx := op.ContextWithIssuer(context.Background(), testIssuer)