Add Integration Tests for New jwt-auth, mcp-auth and mcp-authz Improvements - #2980
Conversation
📝 WalkthroughWalkthroughJWT and MCP integration tests now cover custom JWT claims, scope and claim authorization formats, deprecated-parameter precedence, token forwarding, header interactions, multi-rule authorization, governance, and exclusions. ChangesAuthentication and authorization coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" 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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/it/features/jwt-auth.feature (1)
550-555: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrecedence scenarios assert only the rejection path. Both new-vs-deprecated precedence scenarios would still pass if the policy rejected every request, so they do not actually prove the new param is evaluated. Add a matching positive case to each.
gateway/it/features/jwt-auth.feature#L550-L555: add a token with scopeapi:deployexpecting 200.gateway/it/features/jwt-auth.feature#L652-L657: add a token with claimrole=superadminexpecting 200.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/it/features/jwt-auth.feature` around lines 550 - 555, The precedence scenarios only test rejection, so they do not verify that the new policy parameters are accepted. In gateway/it/features/jwt-auth.feature lines 550-555, add a positive request using scope api:deploy and assert status 200; in lines 652-657, add a positive request using claim role=superadmin and assert status 200. Keep the existing rejection cases unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/it/features/jwt-auth.feature`:
- Around line 550-555: The precedence scenarios only test rejection, so they do
not verify that the new policy parameters are accepted. In
gateway/it/features/jwt-auth.feature lines 550-555, add a positive request using
scope api:deploy and assert status 200; in lines 652-657, add a positive request
using claim role=superadmin and assert status 200. Keep the existing rejection
cases unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5c4dcca-d4b4-4764-b383-fb547bd367f8
📒 Files selected for processing (4)
gateway/it/features/jwt-auth.featuregateway/it/features/mcp_policies.featuregateway/it/steps_jwt.gotests/mock-servers/mock-jwks/main.go
5974bd1 to
54d5dea
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
gateway/it/steps_jwt.go (1)
185-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the duplicated token fetch in
iGetJWTTokenWithIssuerAndScope.
fetchTokenFromis a verbatim copy of lines 119-137, and the existing method still hand-rolls?/&concatenation. Delegating it to the new helpers removes the duplication and the fragile query building.♻️ Proposed refactor for lines 103-138
func (j *JWTSteps) iGetJWTTokenWithIssuerAndScope(issuer, scope string) error { - tokenURL := j.mockJWKSURL + "/token" - if issuer != "" { - tokenURL = tokenURL + "?issuer=" + url.QueryEscape(issuer) - } - if scope != "" { - if issuer != "" { - tokenURL = tokenURL + "&" - } else { - tokenURL = tokenURL + "?" - } - tokenURL = tokenURL + "scope=" + url.QueryEscape(scope) - } - - log.Printf("DEBUG: Fetching JWT token from %s", tokenURL) - - resp, err := j.state.HTTPClient.Get(tokenURL) - if err != nil { - return fmt.Errorf("failed to get JWT token from mock JWKS server: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("mock JWKS server returned status %d", resp.StatusCode) - } - - tokenBytes, err := io.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("failed to read token response: %w", err) - } - - j.currentToken = string(tokenBytes) - log.Printf("DEBUG: Obtained JWT token (length: %d)", len(j.currentToken)) - - return nil + return j.iGetJWTTokenWithIssuerScopeAndClaims(issuer, scope, "") }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gateway/it/steps_jwt.go` around lines 185 - 208, Update iGetJWTTokenWithIssuerAndScope to build issuer and scope parameters with the existing query helpers and delegate token retrieval to fetchTokenFrom. Remove its duplicated HTTP GET, status validation, response reading, and currentToken assignment logic; retain fetchTokenFrom as the single implementation for fetching and storing the token.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@gateway/it/steps_jwt.go`:
- Around line 185-208: Update iGetJWTTokenWithIssuerAndScope to build issuer and
scope parameters with the existing query helpers and delegate token retrieval to
fetchTokenFrom. Remove its duplicated HTTP GET, status validation, response
reading, and currentToken assignment logic; retain fetchTokenFrom as the single
implementation for fetching and storing the token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 064a4c94-9181-4f08-a3a8-72e73e951281
📒 Files selected for processing (3)
gateway/it/features/jwt-auth.featuregateway/it/features/mcp_policies.featuregateway/it/steps_jwt.go
Purpose
This PR adds some additional integration test scenarios to test and ensure the functionality of the jwt-auth, mcp-auth and mcp-authz policies after the improvements made for addressing the following set of issues