Add authorization and credential state check on refresh grant - #4861
Conversation
thiva-k
added
Type/Improvement
trigger-pr-builder
|
Warning Review limit reached
Next review available in: 50 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis change records credential-update timestamps, preserves them during entity updates, and validates them during refresh-token grants. Refresh-token scopes are re-evaluated against current roles and groups. Unit tests, integration tests, API helpers, and OAuth documentation were updated. ChangesCredential and refresh-token security
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RefreshTokenGrantHandler
participant EntityService
participant ActorProvider
participant AuthorizationProvider
Client->>RefreshTokenGrantHandler: Submit refresh token
RefreshTokenGrantHandler->>EntityService: Check subject and client credential timestamps
EntityService-->>RefreshTokenGrantHandler: Return credential metadata
RefreshTokenGrantHandler->>ActorProvider: Resolve subject groups
ActorProvider-->>RefreshTokenGrantHandler: Return current groups
RefreshTokenGrantHandler->>AuthorizationProvider: Re-evaluate permission scopes
AuthorizationProvider-->>RefreshTokenGrantHandler: Return authorized scopes
RefreshTokenGrantHandler-->>Client: Issue filtered tokens or invalid_grant
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
backend/internal/oauth/oauth2/granthandlers/refresh_token.go (2)
598-626: 🚀 Performance & Scalability | 🔵 TrivialThe refresh path now makes up to five backend calls per request.
Each refresh adds
GetActorfor the subject, optionallyGetInboundClientByID, optionallyGetActorfor the client,GetActorGroups, andEvaluateAccessBatch. Refresh is a high-frequency endpoint, and these calls are serial and on the request thread.Consider adding a short-TTL cache for the credential marker and group memberships, and track latency and error rates for these two providers so a slow authorization engine is visible before it degrades refresh throughput. The TTL bounds how long a revoked permission stays effective, so pick it against the security requirement.
No change is required in this PR.
🤖 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 `@backend/internal/oauth/oauth2/granthandlers/refresh_token.go` around lines 598 - 626, No implementation change is required for this comment; leave the refresh flow around GetActorGroups and EvaluateAccessBatch unchanged.
464-466: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFail closed or remove the nil guard
The supported engine and server paths initialize
actorProviderbefore OAuth setup, andNewrejects a nil provider. This is not a reachable production wiring regression. If the defensive guard remains, returnserver_errorinstead of silently bypassing credential, subject, and scope checks.🤖 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 `@backend/internal/oauth/oauth2/granthandlers/refresh_token.go` around lines 464 - 466, Update the actorProvider handling in the OAuth refresh flow: either remove the unreachable nil guard, or replace its silent nil return with a server_error response. Preserve the credential, subject, and scope validation path whenever actorProvider is available.Source: Learnings
backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go (1)
2180-2185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
Requireinside aMatchedBycallback.
suite.Require().NotEmptyinside the matcher callsruntime.Goexiton failure, from inside mock argument matching. Testify may invoke a matcher more than once when several expectations compete, and a failure there produces a confusing stack rather than a clean assertion failure. Only one expectation is registered here, so the current behavior is fine.Capture the request and assert after
HandleGrantreturns.♻️ Proposed refactor
var evaluatedGroupIDs []string + var capturedEvaluations []providers.AccessEvaluationRequest suite.mockAuthzService.On("EvaluateAccessBatch", mock.Anything, mock.MatchedBy( func(req providers.AccessEvaluationsRequest) bool { - suite.Require().NotEmpty(req.Evaluations) - evaluatedGroupIDs = req.Evaluations[0].Subject.GroupIDs + capturedEvaluations = req.Evaluations return true })).Return(allowAllEvaluations, nil)Then after the call:
suite.Require().NotEmpty(capturedEvaluations) evaluatedGroupIDs = capturedEvaluations[0].Subject.GroupIDs assert.Equal(suite.T(), []string{"group-1", "group-2"}, evaluatedGroupIDs)🤖 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 `@backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go` around lines 2180 - 2185, Update the EvaluateAccessBatch matcher to only capture req.Evaluations without calling suite.Require inside MatchedBy; after HandleGrant returns, assert the captured evaluations are non-empty, assign evaluatedGroupIDs from the first evaluation, and perform the expected group ID assertion there.tests/integration/oauth/token/refresh_token_test.go (2)
756-778: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
updateApplicationinstead of duplicating the payload.
ts.updateApplicationat lines 530-554 builds exactly this payload. The only difference is the secret value, which is already a parameter. The duplicate block will drift from the helper when the application shape changes.♻️ Proposed refactor
- ts.Require().NoError(testutils.UpdateApplication(appID, testutils.Application{ - ID: appID, - Name: "RefreshSecurityRotateApp", - Description: "Application for refresh token security integration tests", - OUID: ts.ouID, - Type: "fullstack", - AuthFlowID: ts.authFlowID, - IsRegistrationFlowEnabled: false, - AllowedUserTypes: []string{refreshSecUserType}, - InboundAuthConfig: []map[string]interface{}{ - { - "type": "oauth2", - "config": map[string]interface{}{ - "clientId": rotateClientID, - "clientSecret": "rotated-secret-value", - "redirectUris": []string{refreshSecRedirectURI}, - "grantTypes": []string{"authorization_code", "refresh_token"}, - "responseTypes": []string{"code"}, - "tokenEndpointAuthMethod": "client_secret_basic", - }, - }, - }, - }), "Failed to rotate client secret") + ts.Require().NoError( + ts.updateApplication(appID, "RefreshSecurityRotateApp", rotateClientID, "rotated-secret-value"), + "Failed to rotate client secret")🤖 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 `@tests/integration/oauth/token/refresh_token_test.go` around lines 756 - 778, Replace the duplicated testutils.UpdateApplication payload in the refresh-token rotation test with the existing ts.updateApplication helper, passing the rotated client secret value as its parameter. Preserve the current application update behavior and error assertion while reusing the helper’s shared payload construction.
728-808: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThese tests depend on the
<=comparison incredentialChangedSince.The token grant and the credential change happen within the same wall-clock second in a fast test run.
credentialChangedSincecomparesiat <= changedAt.Unix(), so a same-second change still rejects the token and the tests are deterministic. If that comparison ever changes to a strict<, these three tests become flaky rather than failing outright.No change is required. The coupling is worth a short comment next to the assertions so a future change to the comparison does not get diagnosed as test flakiness.
🤖 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 `@tests/integration/oauth/token/refresh_token_test.go` around lines 728 - 808, The refresh-token rejection tests rely on same-second credential changes being rejected by the inclusive comparison in credentialChangedSince. Add a concise comment near the assertions in TestRefresh_PasswordReset_RejectsToken, TestRefresh_ClientSecretRotated_RejectsToken, and TestRefresh_ClientSecretRotatedThenAppRenamed_StillRejectsToken documenting this dependency and warning that changing <= to < would make them timing-sensitive.tests/integration/testutils/api_utils.go (1)
2044-2051: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueA single marshal produces the same payload.
credentialsis amap[string]string, so wrapping it directly removes one marshal call and one error branch.♻️ Proposed simplification
- credsJSON, err := json.Marshal(credentials) - if err != nil { - return fmt.Errorf("failed to marshal credentials: %w", err) - } - payload, err := json.Marshal(map[string]json.RawMessage{"credentials": credsJSON}) + payload, err := json.Marshal(map[string]interface{}{"credentials": credentials}) if err != nil { return fmt.Errorf("failed to marshal credential update request: %w", err) }🤖 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 `@tests/integration/testutils/api_utils.go` around lines 2044 - 2051, In the credential update request construction, remove the intermediate credsJSON marshal and pass credentials directly as the map value in the payload used by the surrounding request flow. Eliminate the associated first error branch while preserving the existing error handling for marshaling the final payload.
🤖 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.
Inline comments:
In `@backend/internal/application/service.go`:
- Around line 466-478: Prevent stale full-blob system-attribute writes from
overwriting credentialUpdatedAt by updating the application system-attribute
path around GetEntity/buildSystemAttributes to use an atomic marker-preserving
merge or the same serialization used by credential updates; apply the identical
mechanism to the agent update path in backend/internal/agent/service.go lines
300-301, so both paths retain markers committed concurrently.
In `@backend/internal/oauth/oauth2/granthandlers/refresh_token.go`:
- Around line 590-596: The reauthorizeScopes guard must treat an entity with an
empty ID like an unresolved subject, and the fixture must provide real IDs. In
refresh_token.go, extend reauthorizeScopes’s early-return condition to include
subjectEntity.ID == ""; in refresh_token_test.go, update entityMarkedAt to
accept an id parameter, assign it to providers.Entity.ID, and pass matching IDs
at all three call sites.
- Around line 566-586: Update credentialChangedSince to accept ctx and the
logger, and log malformed SystemAttributes JSON, non-string or empty
credentialUpdatedAt values, and invalid RFC3339 timestamps; keep genuinely
absent markers silent and preserve the existing boolean behavior. Update all
callers to pass the required context and logger, then add tests covering absent,
malformed JSON, invalid type/value, and invalid timestamp cases.
In `@docs/content/guides/protocols/oauth-oidc/refresh-token.mdx`:
- Around line 48-59: Update the refresh-token behavior table and surrounding
explanation to qualify the user-deletion and permission-scope rows: these
effects apply when the subject resolves to an entity, but not when the client
maps `sub` to a user attribute and `resolveSubjectEntity` skips resolution.
State that refresh succeeds with unchanged scopes in that configuration, and
verify the wording against `resolveSubjectEntity` and `reauthorizeScopes`.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go`:
- Around line 2180-2185: Update the EvaluateAccessBatch matcher to only capture
req.Evaluations without calling suite.Require inside MatchedBy; after
HandleGrant returns, assert the captured evaluations are non-empty, assign
evaluatedGroupIDs from the first evaluation, and perform the expected group ID
assertion there.
In `@backend/internal/oauth/oauth2/granthandlers/refresh_token.go`:
- Around line 598-626: No implementation change is required for this comment;
leave the refresh flow around GetActorGroups and EvaluateAccessBatch unchanged.
- Around line 464-466: Update the actorProvider handling in the OAuth refresh
flow: either remove the unreachable nil guard, or replace its silent nil return
with a server_error response. Preserve the credential, subject, and scope
validation path whenever actorProvider is available.
In `@tests/integration/oauth/token/refresh_token_test.go`:
- Around line 756-778: Replace the duplicated testutils.UpdateApplication
payload in the refresh-token rotation test with the existing
ts.updateApplication helper, passing the rotated client secret value as its
parameter. Preserve the current application update behavior and error assertion
while reusing the helper’s shared payload construction.
- Around line 728-808: The refresh-token rejection tests rely on same-second
credential changes being rejected by the inclusive comparison in
credentialChangedSince. Add a concise comment near the assertions in
TestRefresh_PasswordReset_RejectsToken,
TestRefresh_ClientSecretRotated_RejectsToken, and
TestRefresh_ClientSecretRotatedThenAppRenamed_StillRejectsToken documenting this
dependency and warning that changing <= to < would make them timing-sensitive.
In `@tests/integration/testutils/api_utils.go`:
- Around line 2044-2051: In the credential update request construction, remove
the intermediate credsJSON marshal and pass credentials directly as the map
value in the payload used by the surrounding request flow. Eliminate the
associated first error branch while preserving the existing error handling for
marshaling the final payload.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 91e6f81f-74b7-49f4-b2f8-3788f3a0846a
📒 Files selected for processing (19)
backend/internal/agent/constants.gobackend/internal/agent/service.gobackend/internal/agent/service_test.gobackend/internal/application/constants.gobackend/internal/application/declarative_resource.gobackend/internal/application/service.gobackend/internal/application/service_test.gobackend/internal/authnprovider/common/constants.gobackend/internal/entity/service.gobackend/internal/entity/service_test.gobackend/internal/oauth/oauth2/granthandlers/provider.gobackend/internal/oauth/oauth2/granthandlers/refresh_token.gobackend/internal/oauth/oauth2/granthandlers/refresh_token_test.godocs/content/guides/applications/application-settings.mdxdocs/content/guides/applications/manage-applications.mdxdocs/content/guides/protocols/oauth-oidc/refresh-token.mdxdocs/content/key-concepts/tokens.mdxtests/integration/oauth/token/refresh_token_test.gotests/integration/testutils/api_utils.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3cc25df to
e540a7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go (1)
2180-2185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not call
suite.Require()inside amock.MatchedBypredicate.
MatchedBypredicates run during argument matching and again when testify renders a mismatch diagnostic.Requirecallsruntime.Goexiton failure, which aborts the test from inside the matcher and hides the real assertion. Return a boolean from the predicate and assert afterHandleGrant.♻️ Proposed refactor
var evaluatedGroupIDs []string suite.mockAuthzService.On("EvaluateAccessBatch", mock.Anything, mock.MatchedBy( func(req providers.AccessEvaluationsRequest) bool { - suite.Require().NotEmpty(req.Evaluations) + if len(req.Evaluations) == 0 { + return false + } evaluatedGroupIDs = req.Evaluations[0].Subject.GroupIDs return true })).Return(allowAllEvaluations, nil)🤖 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 `@backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go` around lines 2180 - 2185, Update the mock.MatchedBy predicate for EvaluateAccessBatch to avoid calling suite.Require; capture the evaluation data and return a boolean indicating whether Evaluations is non-empty, then assert the required non-empty condition after HandleGrant completes.
🤖 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.
Inline comments:
In `@backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go`:
- Around line 2459-2481: Move suite.refreshClaimsValid() before the
systemAttributes loop so it registers the ValidateRefreshToken expectation only
once. Remove the per-iteration call while preserving the existing
resetActorMocks setup and unreadable-marker assertions for every test input.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go`:
- Around line 2180-2185: Update the mock.MatchedBy predicate for
EvaluateAccessBatch to avoid calling suite.Require; capture the evaluation data
and return a boolean indicating whether Evaluations is non-empty, then assert
the required non-empty condition after HandleGrant completes.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 48d7788a-609b-40ef-b0e1-fde3d2f5127f
📒 Files selected for processing (5)
backend/internal/agent/service_test.gobackend/internal/application/service_test.gobackend/internal/entity/service_test.gobackend/internal/oauth/oauth2/granthandlers/refresh_token.gobackend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/internal/application/service_test.go
- backend/internal/agent/service_test.go
- backend/internal/oauth/oauth2/granthandlers/refresh_token.go
890c059 to
ae940d9
Compare
6113b03 to
7090ba3
Compare
7090ba3 to
dd2236d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
thiva-k
added
trigger-pr-builder
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/internal/entity/service_test.go (2)
720-735: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on the blob passed to the store, not on the input entity.
UpdateEntitymutates the caller'sentityin place at line 255 ofbackend/internal/entity/service.go. This test readsincoming.SystemAttributesafter the call, so it verifies the input mutation rather than the value written to the store. If that in-place mutation is ever removed, the test still passes while the store receives the wrong blob. Capture the argument passed tostore.UpdateEntity, as the two neighbouring tests do forUpdateSystemAttributes.♻️ Proposed change
s.store.On("GetEntity", mock.Anything, stored.ID).Return(*stored, nil) - s.store.On("UpdateEntity", mock.Anything, mock.Anything).Return(nil) + var written json.RawMessage + s.store.On("UpdateEntity", mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + e, _ := args.Get(1).(*providers.Entity) + written = e.SystemAttributes + }).Return(nil) incoming := testEntity("e-preserve-2") incoming.SystemAttributes = json.RawMessage(`{"name":"Renamed"}`) _, err := s.svc.UpdateEntity(s.ctx, incoming.ID, incoming) s.Require().NoError(err) var attrs map[string]interface{} - s.Require().NoError(json.Unmarshal(incoming.SystemAttributes, &attrs)) + s.Require().NoError(json.Unmarshal(written, &attrs))🤖 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 `@backend/internal/entity/service_test.go` around lines 720 - 735, Update TestUpdateEntity_PreservesCredentialMarker to capture the entity argument supplied to store.UpdateEntity and assert the merged SystemAttributes from that captured value, rather than checking incoming.SystemAttributes after UpdateEntity returns. Preserve the existing assertions for the renamed field and credential marker.
682-698: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
mergeReservedAttributeserror branch.
mergeReservedAttributesreturns an error when the incoming blob is present but unparsable (backend/internal/entity/service.golines 933-937). No test exercises that branch. It sits on the credential-invalidation path, so a silent regression there would drop the marker. Add one case where the stored entity holds a marker and the caller passes an invalid blob toUpdateSystemAttributes.Testing new code is required by the coding guidelines: "Write tests for new features and bug fixes, targeting at least 80% coverage."
🤖 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 `@backend/internal/entity/service_test.go` around lines 682 - 698, Add a ServiceTestSuite case covering UpdateSystemAttributes when the stored entity already has the credential-updated marker and the incoming blob is invalid JSON. Assert the operation returns the mergeReservedAttributes error and preserves the existing marker rather than silently replacing the attributes.Source: Coding guidelines
backend/internal/oauth/oauth2/granthandlers/refresh_token.go (1)
596-645: 🚀 Performance & Scalability | 🔵 TrivialConsider the added per-refresh backend calls.
Each refresh-token grant now performs up to five additional lookups:
GetActorfor the subject,GetActorfor the client,GetInboundClientByID,GetActorGroups, andEvaluateAccessBatch. Refresh is a hot path for long-lived sessions. Measure the added latency, and consider caching group membership and the client subject-attribute mapping, both of which change far less often than tokens refresh.🤖 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 `@backend/internal/oauth/oauth2/granthandlers/refresh_token.go` around lines 596 - 645, Profile the refresh-token path around reauthorizeScopes and the related GetActor, GetInboundClientByID, and EvaluateAccessBatch calls to quantify added latency. Then reduce repeated backend work by reusing appropriately scoped cached client subject-attribute mappings and GetActorGroups results, with invalidation or TTL behavior that reflects assignment changes while preserving authorization correctness.
🤖 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 `@backend/internal/entity/service_test.go`:
- Around line 720-735: Update TestUpdateEntity_PreservesCredentialMarker to
capture the entity argument supplied to store.UpdateEntity and assert the merged
SystemAttributes from that captured value, rather than checking
incoming.SystemAttributes after UpdateEntity returns. Preserve the existing
assertions for the renamed field and credential marker.
- Around line 682-698: Add a ServiceTestSuite case covering
UpdateSystemAttributes when the stored entity already has the credential-updated
marker and the incoming blob is invalid JSON. Assert the operation returns the
mergeReservedAttributes error and preserves the existing marker rather than
silently replacing the attributes.
In `@backend/internal/oauth/oauth2/granthandlers/refresh_token.go`:
- Around line 596-645: Profile the refresh-token path around reauthorizeScopes
and the related GetActor, GetInboundClientByID, and EvaluateAccessBatch calls to
quantify added latency. Then reduce repeated backend work by reusing
appropriately scoped cached client subject-attribute mappings and GetActorGroups
results, with invalidation or TTL behavior that reflects assignment changes
while preserving authorization correctness.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fdbe003e-288f-4e88-8252-9ca56c84b99c
📒 Files selected for processing (12)
backend/internal/authnprovider/common/constants.gobackend/internal/entity/service.gobackend/internal/entity/service_test.gobackend/internal/oauth/oauth2/granthandlers/provider.gobackend/internal/oauth/oauth2/granthandlers/refresh_token.gobackend/internal/oauth/oauth2/granthandlers/refresh_token_test.godocs/content/guides/applications/application-settings.mdxdocs/content/guides/applications/manage-applications.mdxdocs/content/guides/protocols/oauth-oidc/refresh-token.mdxdocs/content/key-concepts/tokens.mdxtests/integration/oauth/token/refresh_token_test.gotests/integration/testutils/api_utils.go
🚧 Files skipped from review as they are similar to previous changes (7)
- docs/content/guides/applications/application-settings.mdx
- backend/internal/oauth/oauth2/granthandlers/provider.go
- backend/internal/authnprovider/common/constants.go
- tests/integration/testutils/api_utils.go
- docs/content/guides/applications/manage-applications.mdx
- docs/content/guides/protocols/oauth-oidc/refresh-token.mdx
- backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
3 participants
Purpose
A refresh re-minted tokens carrying stale scopes: unassigning a role, deleting it, or stripping its permissions had no effect until the refresh token expired. A password reset or a client secret rotation also left existing refresh tokens usable.
Approach
credentialUpdatedAtin the entity's system attributes when a password or client secret changes, and reject a refresh token established at or before that instant. The marker is written in the same transaction as the credential, and carried across by the application, agent, and user paths that rebuild the blob.Scoped to the refresh grant, so other grants are unaffected. Already-issued access tokens still live out their TTL, and applications that map
subto a user attribute skip the subject-derived checks.Related Issues
Related PRs
Checklist
breaking changelabel added.Security checks
Summary by CodeRabbit
New Features
Documentation
Tests