feature 4877: Added auth middleware support for controller tests#266
feature 4877: Added auth middleware support for controller tests#266davidbolet wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the test harness to support ClientDataMiddleware by generating/verifying signed x-client-data headers, and migrates selected controller tests to use the header-based flow instead of injecting client data directly into request context.
Changes:
- Added in-memory RSA signing key utilities and a mock role getter for tests.
- Added helpers to produce properly signed
x-client-data/x-client-data-signatureheaders. - Updated
TestAPIServerConfig/ request building and migrated tenant + userinfo controller tests to the new mechanism.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/testutils/keys.go | Adds test signing key storage (public keys for verification + private keys for signing) and a test role getter. |
| internal/testutils/authz.go | Adds helpers to generate signed client-data headers for requests. |
| internal/testutils/api.go | Adds EnableClientDataMW/SigningKeyStorage, wires ClientDataMiddleware into the test server, and updates request options to accept http.Header. |
| internal/controllers/cmk/userinfo_controller_test.go | Switches userinfo tests from context-injected client data to signed client-data headers. |
| internal/controllers/cmk/tenant_controller_test.go | Switches tenant tests from context-injected client data to signed client-data headers and adjusts scenarios accordingly. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Set required fields for signing | ||
| clientData.KeyID = strconv.Itoa(keyID) | ||
| clientData.SignatureAlgorithm = auth.SignatureAlgorithmRS256 | ||
|
|
||
| // Generate signed headers using the auth package | ||
| clientDataHeader, signatureHeader, err := clientData.Encode(privateKey) |
There was a problem hiding this comment.
NewSignedClientDataHeadersFromStruct mutates the passed-in *auth.ClientData (sets KeyID and SignatureAlgorithm). This side effect is not documented and can lead to surprising behavior if the caller reuses the struct across subtests. Consider copying the struct (or returning a new one) before setting signing-related fields, or clearly documenting that the input is modified.
| // Set required fields for signing | |
| clientData.KeyID = strconv.Itoa(keyID) | |
| clientData.SignatureAlgorithm = auth.SignatureAlgorithmRS256 | |
| // Generate signed headers using the auth package | |
| clientDataHeader, signatureHeader, err := clientData.Encode(privateKey) | |
| // Copy the input to avoid mutating the caller's ClientData when setting signing fields. | |
| clientDataCopy := *clientData | |
| // Set required fields for signing | |
| clientDataCopy.KeyID = strconv.Itoa(keyID) | |
| clientDataCopy.SignatureAlgorithm = auth.SignatureAlgorithmRS256 | |
| // Generate signed headers using the auth package | |
| clientDataHeader, signatureHeader, err := clientDataCopy.Encode(privateKey) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Plugins []catalog.BuiltInPlugin // Plugins only set if needed | ||
| GRPCCon *commongrpc.DynamicClientConn // GRPCClient only set if needed | ||
| Config config.Config | ||
| EnableClientDataMW bool // Enable ClientDataMiddleware (default: false for backward compatibility) | ||
| SigningKeyStorage *TestSigningKeyStorage // Optional: provide custom signing key storage |
There was a problem hiding this comment.
SigningKeyStorage is typed as *TestSigningKeyStorage, which makes TestAPIServerConfig harder to reuse (e.g., you can’t pass a different keyvalue.ReadOnlyStringToBytesStorage implementation). Since middleware.ClientDataMiddleware accepts the interface, consider changing this field to the interface type and (if needed) returning the private key(s) separately from the test helper that constructs the storage.
minh-nghia
left a comment
minh-nghia
left a comment
There was a problem hiding this comment.
- Please remove KMS20 reference
- I think the purpose of the task was to refactor all the tests, not just for a few isolated cases
| privateKeys := make(map[int]*rsa.PrivateKey) | ||
|
|
||
| // Generate 3 key pairs for testing key rotation scenarios | ||
| for keyID := range 3 { |
There was a problem hiding this comment.
Do we need to test loader and "key rotation scenarios" here? Just one static key is enough. We may not even need to store in files.
davidbolet
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| tmpDir := tb.TempDir() | ||
| privateKeys := make(map[int]*rsa.PrivateKey) | ||
|
|
||
| privateKey, err := rsa.GenerateKey(rand.Reader, 2048) |
There was a problem hiding this comment.
NewTestSigningKeyStorage re-generates the RSA key directly via rsa.GenerateKey even though GenerateTestKeyPair exists in the same file. Consider using GenerateTestKeyPair here to avoid duplicated key-generation logic (and to keep key size/params consistent if they ever change).
| privateKey, err := rsa.GenerateKey(rand.Reader, 2048) | |
| privateKey, _, err := GenerateTestKeyPair() |
| t.Run("Should 500 on get tenant by valid ID and no client data", func(t *testing.T) { | ||
| w := testutils.MakeHTTPRequest(t, sv, testutils.RequestOptions{ | ||
| Method: http.MethodGet, | ||
| Endpoint: "/tenantInfo", | ||
| Tenant: tenant.ID, | ||
| }) | ||
|
|
||
| assert.Equal(t, http.StatusInternalServerError, w.Code) |
There was a problem hiding this comment.
This test now asserts that missing client data yields HTTP 500. Since this is an authentication/authorization precondition, a 4xx status (typically 401/403) is usually more appropriate than a server error. If the 500 comes from apierrors.ErrNoClientData, consider changing that API error’s Status (and then update this test accordingly) so clients can distinguish auth failures from internal faults.
| t.Run("Should 500 on get tenant by valid ID and no client data", func(t *testing.T) { | |
| w := testutils.MakeHTTPRequest(t, sv, testutils.RequestOptions{ | |
| Method: http.MethodGet, | |
| Endpoint: "/tenantInfo", | |
| Tenant: tenant.ID, | |
| }) | |
| assert.Equal(t, http.StatusInternalServerError, w.Code) | |
| t.Run("Should 401 on get tenant by valid ID and no client data", func(t *testing.T) { | |
| w := testutils.MakeHTTPRequest(t, sv, testutils.RequestOptions{ | |
| Method: http.MethodGet, | |
| Endpoint: "/tenantInfo", | |
| Tenant: tenant.ID, | |
| }) | |
| assert.Equal(t, http.StatusUnauthorized, w.Code) |
| // Middlewares are applied from last to first. | ||
| // Keep Authz before ClientData in the slice so ClientData runs first at request time. | ||
| mws = append(mws, | ||
| middleware.AuthzMiddleware(controller), | ||
| middleware.LoggingMiddleware(), | ||
| middleware.PanicRecoveryMiddleware(), | ||
| middleware.InjectMultiTenancy(), | ||
| middleware.InjectRequestID(), | ||
| ) | ||
|
|
||
| // Add ClientDataMiddleware if enabled. | ||
| // It must be appended after Authz in the slice so it runs before Authz. | ||
| if testCfg.EnableClientDataMW { | ||
| signingKeyStorage := testCfg.SigningKeyStorage | ||
| if signingKeyStorage == nil { | ||
| // Create default test signing key storage if not provided | ||
| signingKeyStorage = NewTestSigningKeyStorage(tb) | ||
| } | ||
|
|
||
| // Default auth context fields for testing | ||
| authContextFields := []string{"client_id", "issuer", "multitenancy_ref"} | ||
|
|
||
| // Use test role getter | ||
| roleGetter := NewTestRoleGetter() | ||
|
|
||
| mws = append(mws, middleware.ClientDataMiddleware( | ||
| signingKeyStorage, | ||
| authContextFields, | ||
| roleGetter, | ||
| )) |
There was a problem hiding this comment.
When EnableClientDataMW is true, ClientDataMiddleware is appended as the last middleware, which makes it run first at request time (FILO). That changes the middleware execution order vs production and also means InjectRequestID is no longer the first middleware executed. In prod the slice order is Authz, ClientData, OAPI, Logging, PanicRecovery, InjectMultiTenancy, InjectRequestID, Tracing (see internal/daemon/server.go:190-199), which keeps InjectRequestID/InjectMultiTenancy outermost and ClientData immediately before Authz. Suggest inserting ClientDataMiddleware right after AuthzMiddleware in the slice (and keep InjectRequestID as the last element) so test behavior matches the real server.
| // Legacy support: inject AdditionalContext if provided and ClientDataMiddleware is not enabled | ||
| // When ClientDataMiddleware is enabled, AdditionalContext is ignored in favor of Headers | ||
| if len(opt.AdditionalContext) > 0 && opt.Headers == nil { | ||
| //nolint: fatcontext | ||
| for k, v := range opt.AdditionalContext { | ||
| ctx = context.WithValue(ctx, k, v) | ||
| } |
There was a problem hiding this comment.
The comment says AdditionalContext is only injected when ClientDataMiddleware is not enabled, but NewHTTPRequest doesn’t know whether the server was started with EnableClientDataMW; it only checks opt.Headers == nil. Consider rewording the comment to match the actual condition (legacy path when no headers are provided), or pass a flag through RequestOptions if you truly need to disable context injection when ClientDataMW is enabled.
| // Set required fields for signing | ||
| clientData.KeyID = strconv.Itoa(keyID) | ||
| clientData.SignatureAlgorithm = auth.SignatureAlgorithmRS256 | ||
|
|
||
| // Generate signed headers using the auth package | ||
| clientDataHeader, signatureHeader, err := clientData.Encode(privateKey) |
There was a problem hiding this comment.
NewSignedClientDataHeadersFromStruct mutates the provided *auth.ClientData (sets KeyID and SignatureAlgorithm). The doc comment doesn’t mention this side effect, which can surprise callers that reuse the struct across subtests. Either document the mutation explicitly or copy the struct before setting signing fields.
| // Set required fields for signing | |
| clientData.KeyID = strconv.Itoa(keyID) | |
| clientData.SignatureAlgorithm = auth.SignatureAlgorithmRS256 | |
| // Generate signed headers using the auth package | |
| clientDataHeader, signatureHeader, err := clientData.Encode(privateKey) | |
| // Copy the provided client data so this helper does not mutate caller-owned state. | |
| clientDataCopy := *clientData | |
| // Set required fields for signing on the copy. | |
| clientDataCopy.KeyID = strconv.Itoa(keyID) | |
| clientDataCopy.SignatureAlgorithm = auth.SignatureAlgorithmRS256 | |
| // Generate signed headers using the auth package | |
| clientDataHeader, signatureHeader, err := clientDataCopy.Encode(privateKey) |
This PR adds a mechanism to allow support for ClientDataMiddleware on the tests.
Both tenant_controller_test and userinfo_controller_test have been updated to support this mechanism
Changes Made
1. Test Signing Key Infrastructure (
internal/testutils/keys.go)Created new test utilities for managing RSA signing keys:
GenerateTestKeyPair(): Generates 2048-bit RSA key pairs for RS256 signingTestSigningKeyStorage: Implementskeyvalue.ReadOnlyStringToBytesStorageinterfaceTestRoleGetter: Mock implementation ofRoleGetterinterface for tests2. Signed Client Data Header Generation (
internal/testutils/authz.go)Added functions to generate properly signed client data headers:
NewSignedClientDataHeaders(): Generates headers from a map[string]anyNewSignedClientDataHeadersFromStruct(): Generates headers fromauth.ClientDatastructx-client-dataandx-client-data-signatureheaders3. Test Server Configuration (
internal/testutils/api.go)Updated
TestAPIServerConfigto support ClientDataMiddleware:EnableClientDataMWflag (default: false for backward compatibility)SigningKeyStoragefield for custom key storagestartAPIServer()to:Updated
RequestOptionsstruct:Headers http.Headerfield for new header-based approachAdditionalContext map[any]anyfor backward compatibility (deprecated)