Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds configurable JWT scope-claim support across config, schema, docs, authentication API, AccessController wiring, and tests; ensures runtime authentications use the configured claim name for scopes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. Comment |
Router image scan passed✅ No security vulnerabilities found in image: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/config/config.schema.json`:
- Around line 2105-2109: The schema currently allows an empty string for the
"scope_claim" property, which can lead to empty lookup keys and confusing auth
behavior; update the "scope_claim" definition in config.schema.json (the
property named "scope_claim") to disallow empty values by adding a non-empty
constraint (e.g., add "minLength": 1 or a non-empty "pattern") so the schema
validation rejects "" values. Ensure you only modify the "scope_claim"
property's schema object to include that constraint without changing its type or
description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4b272c5-22cd-4860-a1b8-baa8b51e9760
📒 Files selected for processing (11)
router-tests/security/authentication_test.gorouter/core/access_controller.gorouter/core/ratelimiter_test.gorouter/core/supervisor_instance.gorouter/pkg/authentication/authentication.gorouter/pkg/config/config.gorouter/pkg/config/config.schema.jsonrouter/pkg/config/config_test.gorouter/pkg/config/fixtures/full.yamlrouter/pkg/config/testdata/config_defaults.jsonrouter/pkg/config/testdata/config_full.json
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2724 +/- ##
==========================================
- Coverage 63.46% 63.46% -0.01%
==========================================
Files 251 251
Lines 26767 26777 +10
==========================================
+ Hits 16989 16994 +5
- Misses 8415 8419 +4
- Partials 1363 1364 +1
🚀 New features to boost your workflow:
|
…ent-name-for-scopes-claim' into ale/eng-9325-router-allow-different-name-for-scopes-claim
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
router/pkg/authentication/authentication.go (1)
29-37:⚠️ Potential issue | 🟠 Major
Authenticationinterface now requiresSetScopesClaimmethod—verify no external implementations exist.Adding a required method to an exported Go interface is a breaking change. All implementations within the codebase have been updated (
authenticationstruct andFakeAuthenticatortest mock), suggesting this is a coordinated change as part of feature eng-9325. However, if this package is consumed by external implementations outside this monorepo, they will break and must be updated to implementSetScopesClaim. Confirm that no external consumers depend on theAuthenticationinterface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@router/pkg/authentication/authentication.go` around lines 29 - 37, You added SetScopesClaim to the exported Authentication interface which is a breaking change; search for any external consumers or implementations of Authentication (look for references to the Authentication interface symbol and types claiming to implement it), and update them to implement SetScopesClaim (or provide a no-op shim) — ensure internal types like authentication and FakeAuthenticator are updated already, add a short package-level comment documenting the breaking change, and if this package is consumed externally consider bumping the module major version or communicate the required change to downstream consumers so their implementations add SetScopesClaim.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@router/pkg/authentication/authentication.go`:
- Around line 63-68: SetScopesClaim currently allows empty or whitespace-only
scope names which can misroute scope reads/writes; update
authentication.SetScopesClaim to first check a != nil, then trim the input using
strings.TrimSpace and if the trimmed value is empty return without changing
a.scopeClaim, otherwise assign the trimmed value. Add the strings import if
missing and update any callers/tests that assume blank values are accepted.
---
Outside diff comments:
In `@router/pkg/authentication/authentication.go`:
- Around line 29-37: You added SetScopesClaim to the exported Authentication
interface which is a breaking change; search for any external consumers or
implementations of Authentication (look for references to the Authentication
interface symbol and types claiming to implement it), and update them to
implement SetScopesClaim (or provide a no-op shim) — ensure internal types like
authentication and FakeAuthenticator are updated already, add a short
package-level comment documenting the breaking change, and if this package is
consumed externally consider bumping the module major version or communicate the
required change to downstream consumers so their implementations add
SetScopesClaim.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ce16f2f-d3dd-4348-ad2a-b360bd0c4211
📒 Files selected for processing (1)
router/pkg/authentication/authentication.go
Some systems don't use JWT claim "scope" as the claim where they put scopes. In alternative we saw is "scp". This additional config will keep scope as the default value, but will allow users to choose for a different claim if needed.
Summary by CodeRabbit
New Features
Tests
Documentation
Checklist
Open Source AI Manifesto
This project follows the principles of the Open Source AI Manifesto. Please ensure your contribution aligns with its principles.