Add Audience to authserver test client fixture#4814
Conversation
Fosite validates that a granted audience appears in the client's own `GetAudience()` list when handling refresh tokens. The main `setupTestServer` helper registered the test client without an `Audience` field, so the check failed with HTTP 400 after the token handler began defaulting to the sole `AllowedAudience` on the initial `authorization_code` exchange. DCR registration always sets `Audience` to `AllowedAudiences`; align the test fixture with that production behaviour.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4814 +/- ##
==========================================
+ Coverage 68.93% 68.99% +0.06%
==========================================
Files 518 518
Lines 54980 54985 +5
==========================================
+ Hits 37900 37937 +37
+ Misses 14151 14121 -30
+ Partials 2929 2927 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
LGTM — clean one-line fix aligning the test fixture with production DCR behavior.
Extended reasoning...
Overview
This PR adds a single line to setupTestServer in pkg/authserver/integration_test.go, registering the test fosite client with Audience: []string{testAudience}. The change aligns the test fixture with how DCR registration sets Audience = AllowedAudiences in production.
Security Risks
None. This is a test-only change with no production code paths affected.
Level of Scrutiny
Low. The change is a one-line addition to a test helper, follows an identical pattern already present in setupTestServerWithOIDCProvider and setupTestServerWithTwoUpstreams, and directly mirrors the production behavior described in the PR. No logic change, no new feature surface.
Other Factors
Two nit-level inline comments were flagged: a stale workaround comment and a hardcoded literal that should use the testAudience constant. Both are test-code quality issues only, with no functional impact today. The hardcoded literal is a mild maintenance hazard but does not warrant blocking this fix.
| ResponseTypes: []string{"code"}, | ||
| GrantTypes: []string{"authorization_code", "refresh_token"}, | ||
| Scopes: options.scopes, | ||
| Audience: []string{testAudience}, |
There was a problem hiding this comment.
🟡 The comment at line 1449 in TestIntegration_RefreshPreservesUpstreamTokenBinding — // Exchange code for tokens (no resource/audience to avoid audience mismatch on refresh) — is now stale after this PR adds Audience: []string{testAudience} to the client fixture. The comment describes a workaround for the exact fosite audience-validation failure this PR fixes, and now actively misleads future readers into thinking an audience mismatch still occurs on refresh.
Extended reasoning...
What the bug is: The comment at line 1449 reads // Exchange code for tokens (no resource/audience to avoid audience mismatch on refresh). Before this PR, the test client (setupTestServer) registered a fosite.DefaultClient with no Audience field. When fosite processed a refresh_token grant and the original authorization_code exchange had granted an audience (e.g. testAudience via resource=testAudience), fosite's audience validation would fail because testAudience was not in the client's GetAudience() list. The workaround was to omit the resource parameter during code exchange entirely, hence the comment.
The specific code path: This PR adds Audience: []string{testAudience} at line 142 in setupTestServer. This means the registered client's GetAudience() now returns [testAudience]. Fosite's refresh token handler checks that any previously-granted audience is in the client's GetAudience() list. With the fix in place, passing resource=testAudience during code exchange and subsequently using refresh_token will succeed — the audience mismatch fosite was throwing no longer occurs.
Why existing code doesn't prevent it: The code itself is functionally correct — omitting the resource parameter is still a valid OAuth request. But the comment's stated reason is no longer accurate. Future readers who see the comment will incorrectly believe that specifying resource=testAudience would still cause an audience mismatch on refresh, when the whole point of this PR is to eliminate that failure mode.
Impact: No functional regression, but the comment actively contradicts the behavior enabled by this PR. A developer reading the test would think the audience mismatch bug still exists, might propagate the workaround pattern to new tests, or (worse) might be confused when adding resource=testAudience to this test and finding it works fine despite the comment warning against it.
How to fix: Update the comment at line 1449 to remove the outdated workaround explanation. It could simply read // Exchange code for tokens or // Exchange code for tokens without specifying resource (audience binding is optional here) to accurately reflect that omitting resource is a valid choice, not a required workaround.
Step-by-step proof: (1) Before this PR: setupTestServer registers a client with no Audience field → GetAudience() returns []. (2) If resource=testAudience were passed in the code exchange, fosite grants testAudience in the access token. (3) On refresh_token grant, fosite validates the granted audience against client.GetAudience() → testAudience not in [] → HTTP 400. (4) Workaround: omit resource, no audience is granted, refresh succeeds. (5) After this PR: GetAudience() returns [testAudience] → step (3) now passes, so the workaround comment's premise is false.
| ResponseTypes: []string{"code"}, | ||
| GrantTypes: []string{"authorization_code", "refresh_token"}, | ||
| Scopes: options.scopes, | ||
| Audience: []string{testAudience}, |
There was a problem hiding this comment.
🟡 In setupTestServer, the PR correctly uses testAudience for the client's Audience field (line 142), but AllowedAudiences on line 174 still uses the hardcoded literal "https://mcp.example.com" instead of the constant. If testAudience is ever changed, Audience and AllowedAudiences would diverge, resurrecting the exact HTTP 400 audience validation failure this PR fixes. Line 174 should read: AllowedAudiences: []string{testAudience}.
Extended reasoning...
What the bug is and how it manifests
The PR adds Audience: []string{testAudience} to the fosite.DefaultClient registration in setupTestServer (line 142), correctly using the testAudience constant (value: "https://mcp.example.com"). However, the companion field AllowedAudiences at line 174 in the same function was not updated and still reads []string{"https://mcp.example.com"} as a hardcoded string literal. This creates an asymmetry within the same function: Audience uses the constant but AllowedAudiences uses the literal.
The specific code path that triggers it
Fosite validates audience during both the initial authorization code exchange and refresh token grants. It checks that (a) the requested/granted audience appears in the server's AllowedAudiences list and (b) the audience is present in the client's own GetAudience() list. Both values must agree. If testAudience is changed to a new string (e.g., "https://mcp.example.com/v2"), setupTestServer would register a client with Audience=["https://mcp.example.com/v2"] but configure the server with AllowedAudiences=["https://mcp.example.com"]. The fosite audience validation would then fail with HTTP 400.
Why existing code does not prevent it
The two other setup helpers in the same file — setupTestServerWithOIDCProvider and setupTestServerWithTwoUpstreams — both correctly use AllowedAudiences: []string{testAudience}. The intra-function inconsistency in setupTestServer is not caught by any lint rule or compiler check since both the literal and the constant currently resolve to the same string.
What the impact would be
Today there is no functional failure because testAudience == "https://mcp.example.com" and the literal matches. However, this is a latent maintenance hazard: any future rename of testAudience would silently misalign the two fields in setupTestServer alone. The resulting HTTP 400 audience validation failure would be confusing to diagnose since the PR being fixed was precisely about this kind of mismatch.
How to fix it
Change line 174 in setupTestServer from:
AllowedAudiences: []string{"https://mcp.example.com"},to:
AllowedAudiences: []string{testAudience},This aligns setupTestServer with the pattern already used by both setupTestServerWithOIDCProvider and setupTestServerWithTwoUpstreams.
Step-by-step proof
- Developer changes
testAudience = "https://mcp.example.com/v2"to version the audience. setupTestServernow registers a client withAudience: []string{"https://mcp.example.com/v2"}(viatestAudience).- But
AllowedAudiencesstays[]string{"https://mcp.example.com"}(the old literal, unchanged). - Any test using
setupTestServerthat requestsresource=https://mcp.example.com/v2will fail: fosite rejects the audience because"https://mcp.example.com/v2"is not inAllowedAudiences. - Tests using
setupTestServerWithOIDCProviderorsetupTestServerWithTwoUpstreamscontinue to pass because they usetestAudiencefor both fields. - The regression would be indistinguishable from the original bug this PR was meant to fix.
Fosite validates that a granted audience appears in the client's own
GetAudience()list when handling refresh tokens. The mainsetupTestServerhelper registered the test client without anAudiencefield, so the check failed with HTTP 400 after the token handler began defaulting to the soleAllowedAudienceon the initialauthorization_codeexchange.DCR registration always sets
AudiencetoAllowedAudiences; align the test fixture with that production behaviour.