-
Notifications
You must be signed in to change notification settings - Fork 45
Add JWT authentication integration tests #705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughAdds JWT authentication integration tests: mock JWKS service, BDD feature file, Go step definitions and test wiring, Docker Compose and test config updates, and CI/workflow changes including a Copilot guidance doc and build steps for the mock-jwks image. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Test Suite
participant MockJWKS as Mock JWKS Server
participant Gateway as API Gateway
participant Backend as Backend Service
TestSuite->>MockJWKS: GET /token?issuer=...
MockJWKS-->>TestSuite: 200 OK with JWT
TestSuite->>TestSuite: store JWT
TestSuite->>Gateway: HTTP request with Authorization: Bearer {JWT}
Gateway->>MockJWKS: Fetch/validate keys (JWKS URI)
MockJWKS-->>Gateway: JWKS (keys)
Gateway->>Gateway: Validate JWT (issuer/audience/alg)
Gateway->>Backend: Forward request (on success)
Backend-->>Gateway: Response
Gateway-->>TestSuite: 200 OK / 401 Unauthorized
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/copilot-instructions.md:
- Around line 26-28: Avoid hardcoded line numbers; instead open
gateway/it/docker-compose.test.yaml, locate the gateway-controller service's
image entry (the line containing
ghcr.io/wso2/api-platform/gateway-controller-coverage:<version>) and remove the
"-coverage" suffix so it becomes
ghcr.io/wso2/api-platform/gateway-controller:<version>; repeat the same
pattern-based replacement for any other occurrences (e.g., the ones referenced
around lines 47–49) rather than pointing to specific line numbers.
In `@gateway/it/docker-compose.test.yaml`:
- Around line 125-135: Add a Docker healthcheck for the mock-jwks service
(container_name it-mock-jwks) in docker-compose.test.yaml so test runs wait
until the JWKS endpoint is reachable; implement a healthcheck that queries the
JWKS endpoint (e.g., GET / .well-known/jwks.json) with a short timeout and
conservative retries/interval/start_period to avoid flakes, and ensure any
test-suite waiting logic uses the container health status if applicable.
🧹 Nitpick comments (4)
.github/workflows/copilot-setup-steps.yml (1)
40-48: Consider pinning Helm and kubectl versions for reproducibility.Using
version: 'latest'for both Helm and kubectl can cause unexpected build failures when new versions are released with breaking changes. Pinning to specific versions ensures consistent behavior across runs.♻️ Suggested improvement
- name: Install Helm uses: azure/setup-helm@v4 with: - version: 'latest' + version: 'v3.14.0' # Pin to a specific stable version - name: Install kubectl uses: azure/setup-kubectl@v4 with: - version: 'latest' + version: 'v1.29.0' # Pin to a specific stable version.github/copilot-instructions.md (1)
19-41: Consider automating the image switching workflow.The manual edit-test-revert workflow for switching between coverage and non-coverage images is error-prone. Consider these alternatives:
- Use an environment variable or make target that overrides the image name dynamically
- Create separate docker-compose files (e.g.,
docker-compose.test.yamlfor CI with coverage,docker-compose.local.yamlfor local testing)- Add a pre-commit git hook to prevent accidentally committing the non-coverage image reference
- Use docker-compose override files (
docker-compose.override.yaml) for local customizationgateway/it/steps_jwt.go (2)
68-73: Harden token URL construction to avoid path/query edge cases.String concatenation can yield double slashes or drop existing query parts. Using
url.JoinPath+url.Valuesis safer and more idiomatic.♻️ Proposed refactor
- tokenURL := j.mockJWKSURL + "/token" - if issuer != "" { - tokenURL = tokenURL + "?issuer=" + url.QueryEscape(issuer) - } + tokenURL, err := url.JoinPath(j.mockJWKSURL, "token") + if err != nil { + return fmt.Errorf("invalid mock JWKS URL: %w", err) + } + if issuer != "" { + u, err := url.Parse(tokenURL) + if err != nil { + return fmt.Errorf("invalid token URL: %w", err) + } + q := u.Query() + q.Set("issuer", issuer) + u.RawQuery = q.Encode() + tokenURL = u.String() + }
99-107: Verify HTTP helper consistency for GET requests.
iSendGETRequestWithJWTTokenusesSendGETRequest, while the other helpers useISendGETRequest/ISendPOSTRequest. If these differ in response-state handling, the subsequent assertions may read stale state. Please confirm or align to the same helper.🔧 Suggested alignment
- return j.httpSteps.SendGETRequest(url) + return j.httpSteps.ISendGETRequest(url)
Summary
This PR introduces comprehensive integration tests for JWT authentication.
Summary by CodeRabbit
Tests
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.