Resolve Permission resolution in authorization_code/CIBA flows is not scoped to the requested resource server - #4190
Merged
rajithacharith merged 1 commit intoJul 24, 2026
Conversation
Contributor
📝 WalkthroughWalkthroughChangesResource-server resolution is delegated to a default-aware provider, OAuth flows propagate the selected identifier into flow runtime data, and authorization evaluations are scoped to that server. PAR, CIBA, SSO, refresh-token, end-to-end, and integration tests cover explicit, default, missing, and cross-resource-server scenarios. Resource-server-scoped authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant OAuthService
participant ResourceProvider
participant FlowExecutor
participant AuthorizationService
Client->>OAuthService: request resource-bound authorization
OAuthService->>ResourceProvider: resolve target resource server
OAuthService->>FlowExecutor: store resource-server identifier
FlowExecutor->>ResourceProvider: resolve resource-server ID
FlowExecutor->>AuthorizationService: evaluate permissions for that resource server
Possibly related PRs
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. 🔧 ESLint
docs/content/guides/protocols/oauth-oidc/resource-indicators.mdxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. tests/e2e/utils/authentication/admin-api-auth.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. tests/e2e/utils/server-setup/mfa-setup.tsParsing error: error TS5012: Cannot read file '/tsconfig.json': ENOENT: no such file or directory, open '/tsconfig.json'. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/flow/common/constants.go`:
- Around line 141-143: Update the relevant OAuth 2.0 documentation to describe
resource-server binding and downscoping via RFC 8707’s resource parameter or the
application default, including that permission evaluation is scoped to the
selected server. Document that SSO sessions are resource-bound and cannot reuse
permission scopes across different resource servers, referencing
RuntimeKeyResourceServerID for the corresponding behavior.
In `@backend/internal/oauth/oauth2/authz/service.go`:
- Around line 317-353: The new audience-binding behavior requires documentation
updates: document explicit resource resolution, defaultResourceServer fallback,
and invalid_target behavior for OAuth2 authorize/PAR/CIBA flows in the relevant
API guide; document the defaultResourceServer configuration key and
ResourceServerID value; and update SDK documentation to explain
resource-server-specific token aud claims and permission scopes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c5e40906-33ad-40e1-9ca5-5e17f26339d7
📒 Files selected for processing (19)
backend/internal/flow/common/constants.gobackend/internal/flow/executor/authz_executor.gobackend/internal/flow/executor/authz_executor_test.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/init.gobackend/internal/oauth/oauth2/authz/init_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/authz/service_test.gobackend/internal/oauth/oauth2/ciba/service.gobackend/internal/oauth/oauth2/ciba/service_test.gobackend/internal/oauth/oauth2/par/init.gobackend/internal/oauth/oauth2/par/service.gobackend/internal/oauth/oauth2/par/service_test.gotests/integration/oauth/authz/authz_scope_test.gotests/integration/oauth/sso/sso_reuse_test.gotests/integration/oauth/sso/suite_test.gotests/integration/oauth/token/refresh_token_test.go
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a security flaw in ThunderID’s OAuth/OIDC flows where permission evaluation could ignore the targeted resource server, allowing same-named permissions on different resource servers to be incorrectly treated as equivalent. It introduces a request-scoped internal resource-server binding (resolved from RFC 8707 resource or a configured default) and ensures authorization decisions, token issuance, PAR redemption, CIBA, refresh-token behavior, and SSO session reuse consistently honor that binding.
Changes:
- Resolve and carry a single target resource server (or leave unbound for OIDC-only/scopeless requests), and thread its internal ID through the flow runtime context.
- Scope authorization evaluations to the bound resource-server ID and prevent that request-scoped binding from being reused via SSO checkpoints.
- Add/extend unit and integration tests covering cross-resource-server permission collisions, PAR validation behavior, CIBA runtime propagation, and SSO session reuse isolation.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/integration/oauth/token/refresh_token_test.go | Updates the auth-code portion of the refresh-token integration flow to include an explicit RFC 8707 resource binding. |
| tests/integration/oauth/sso/suite_test.go | Adds fixtures and helpers to create two colliding-permission resource servers and drive resource-bound authorize/token exchanges for SSO reuse testing. |
| tests/integration/oauth/sso/sso_reuse_test.go | Adds an integration regression test ensuring SSO checkpoint reuse does not leak resource-server permission context across requests. |
| tests/integration/oauth/authz/authz_scope_test.go | Extends integration coverage to validate permission isolation across two resource servers with colliding scope strings. |
| backend/internal/oauth/oauth2/par/service.go | Validates resource binding feasibility at PAR push-time (including default RS handling) while deferring authoritative binding/downscoping to redeem-time. |
| backend/internal/oauth/oauth2/par/service_test.go | Updates PAR unit tests for the new validation behavior and adds coverage for “no resource + (no/default) RS” cases. |
| backend/internal/oauth/oauth2/par/init.go | Wires server-config service into PAR initialization for default resource server resolution. |
| backend/internal/oauth/oauth2/ciba/service.go | Propagates the resolved resource-server ID into flow runtime data for CIBA initiations. |
| backend/internal/oauth/oauth2/ciba/service_test.go | Adds unit coverage ensuring the CIBA runtime resource-server ID is set (or empty for OIDC-only). |
| backend/internal/oauth/oauth2/authz/service.go | Centralizes resource binding + per-resource-server downscoping in the shared initiation path and carries resource-server ID through runtime data. |
| backend/internal/oauth/oauth2/authz/service_test.go | Adds unit tests for explicit resource binding, default RS fallback, OIDC-only unbound behavior, and “no resource + no default” rejection. |
| backend/internal/oauth/oauth2/authz/init.go | Wires server-config service into the authorize endpoint initialization. |
| backend/internal/oauth/oauth2/authz/init_test.go | Updates init tests for the new authorize initializer signature. |
| backend/internal/oauth/init.go | Passes server-config service through to PAR and authorize initialization at the OAuth module level. |
| backend/internal/flow/executor/session_executor.go | Prevents request-scoped resource-server binding from being snapshotted into reusable SSO checkpoint data. |
| backend/internal/flow/executor/session_executor_test.go | Verifies resource-server ID is excluded from sanitized checkpoint snapshots. |
| backend/internal/flow/executor/authz_executor.go | Scopes access evaluations by including the runtime resource-server ID in authorization requests. |
| backend/internal/flow/executor/authz_executor_test.go | Adds unit coverage asserting evaluations are scoped to the requested resource server. |
| backend/internal/flow/common/constants.go | Introduces a runtime key for the internal resource-server ID binding. |
ImalshaD
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 21, 2026 07:25
cfa0fac to
2b1edcf
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/content/guides/guides/protocols/oauth-oidc/resource-indicators.mdx`:
- Line 18: Revise the paragraph describing SSO binding so it applies only to
permission-bearing requests. State that requests with a resource bind to that
resource, while OIDC-only or scopeless requests without resource remain unbound
and use the client ID as the audience; remove the claim that every request uses
the configured default resource server.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 86772555-95c9-4264-858d-fa973465e217
📒 Files selected for processing (20)
backend/internal/flow/common/constants.gobackend/internal/flow/executor/authz_executor.gobackend/internal/flow/executor/authz_executor_test.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/init.gobackend/internal/oauth/oauth2/authz/init_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/authz/service_test.gobackend/internal/oauth/oauth2/ciba/service.gobackend/internal/oauth/oauth2/ciba/service_test.gobackend/internal/oauth/oauth2/par/init.gobackend/internal/oauth/oauth2/par/service.gobackend/internal/oauth/oauth2/par/service_test.godocs/content/guides/guides/protocols/oauth-oidc/resource-indicators.mdxtests/integration/oauth/authz/authz_scope_test.gotests/integration/oauth/sso/sso_reuse_test.gotests/integration/oauth/sso/suite_test.gotests/integration/oauth/token/refresh_token_test.go
🚧 Files skipped from review as they are similar to previous changes (18)
- backend/internal/oauth/oauth2/authz/init_test.go
- backend/internal/flow/executor/session_executor.go
- tests/integration/oauth/token/refresh_token_test.go
- tests/integration/oauth/sso/sso_reuse_test.go
- backend/internal/oauth/oauth2/authz/init.go
- backend/internal/flow/executor/session_executor_test.go
- backend/internal/oauth/oauth2/par/init.go
- backend/internal/oauth/init.go
- backend/internal/oauth/oauth2/par/service.go
- backend/internal/oauth/oauth2/ciba/service.go
- backend/internal/oauth/oauth2/ciba/service_test.go
- backend/internal/flow/executor/authz_executor.go
- backend/internal/flow/executor/authz_executor_test.go
- backend/internal/oauth/oauth2/authz/service.go
- backend/internal/oauth/oauth2/par/service_test.go
- backend/internal/oauth/oauth2/authz/service_test.go
- tests/integration/oauth/sso/suite_test.go
- tests/integration/oauth/authz/authz_scope_test.go
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 21, 2026 08:46
2b1edcf to
e57feb6
Compare
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 21, 2026 09:27
e57feb6 to
95c13de
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
tests/integration/oauth/sso/suite_test.go:321
- TearDownSuite currently logs (and effectively ignores) any error deleting rs-A/rs-B, even though the comment says only RES-1006 dependency errors are expected. This can mask real cleanup failures and leak fixtures into subsequent integration runs.
// rs-A/rs-B carry actions; deletion may report a dependency error (RES-1006) which is harmless on
// the temporary test database, so log rather than fail.
for _, rsID := range []string{ts.rsAID, ts.rsBID} {
if rsID != "" {
if err := testutils.DeleteResourceServer(rsID); err != nil {
ts.T().Logf("Failed to delete SSO scope resource server %s: %v", rsID, err)
}
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 21, 2026 13:34
95c13de to
962a9fb
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/internal/flow/executor/authz_executor.go (1)
197-210: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winType-assertion failure is silently swallowed.
cfg, _ := merged.(resource.DefaultResourceServerConfig)discards theokresult. IfGetMergedConfigever returns an unexpected type (contract drift, misregistration), this silently falls through tocfg.ResourceServerID == "", causing all requested permissions to be dropped with no diagnostic trail.♻️ Proposed fix
- cfg, _ := merged.(resource.DefaultResourceServerConfig) - return cfg.ResourceServerID + cfg, ok := merged.(resource.DefaultResourceServerConfig) + if !ok { + a.logger.Error(ctx.Context, "Unexpected type for default resource server config") + return "" + } + return cfg.ResourceServerID🤖 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/flow/executor/authz_executor.go` around lines 197 - 210, The defaultResourceServerID method must validate the type assertion from GetMergedConfig instead of discarding its success result. Check whether merged is a resource.DefaultResourceServerConfig, log an error through a.logger with the relevant context when it is not, and return an empty string; preserve the existing ResourceServerID return for valid configuration.
🤖 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/flow/executor/authz_executor.go`:
- Around line 197-210: The defaultResourceServerID method must validate the type
assertion from GetMergedConfig instead of discarding its success result. Check
whether merged is a resource.DefaultResourceServerConfig, log an error through
a.logger with the relevant context when it is not, and return an empty string;
preserve the existing ResourceServerID return for valid configuration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e16d367a-8d0d-4c72-b903-dfdae7424c40
📒 Files selected for processing (25)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/authz_executor.gobackend/internal/flow/executor/authz_executor_test.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/init.gobackend/internal/oauth/oauth2/authz/init_test.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/authz/service_test.gobackend/internal/oauth/oauth2/ciba/service.gobackend/internal/oauth/oauth2/ciba/service_test.gobackend/internal/oauth/oauth2/par/init.gobackend/internal/oauth/oauth2/par/service.gobackend/internal/oauth/oauth2/par/service_test.gobackend/pkg/thunderidengine/engine.godocs/content/guides/guides/protocols/oauth-oidc/resource-indicators.mdxtests/e2e/run-e2e.shtests/integration/flow/authentication/authz_test.gotests/integration/oauth/authz/authz_scope_test.gotests/integration/oauth/sso/sso_reuse_test.gotests/integration/oauth/sso/suite_test.gotests/integration/oauth/token/refresh_token_test.go
🚧 Files skipped from review as they are similar to previous changes (10)
- backend/internal/flow/executor/session_executor.go
- backend/internal/flow/executor/session_executor_test.go
- backend/internal/oauth/init.go
- backend/internal/oauth/oauth2/authz/init.go
- tests/integration/oauth/token/refresh_token_test.go
- backend/internal/oauth/oauth2/ciba/service_test.go
- backend/internal/oauth/oauth2/par/init.go
- tests/integration/oauth/sso/sso_reuse_test.go
- tests/integration/oauth/authz/authz_scope_test.go
- backend/internal/oauth/oauth2/par/service_test.go
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 21, 2026 14:44
962a9fb to
39f860b
Compare
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 24, 2026 02:44
39f860b to
2726ee6
Compare
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 24, 2026 02:49
2726ee6 to
a544df7
Compare
Contributor
Author
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/internal/oauth/oauth2/granthandlers/refresh_token.go (1)
140-192: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the non-RS identifier from refresh token validation
ResolveDefaultAudience(ClientID)can return a configuredAccessToken.DefaultAudiencethat is not theclient_id, and refresh-token audiences are persisted as[]string. The currentaudience == tokenRequest.ClientIDcheck can therefore misclassify an OIDC-only refresh token and callGetResourceServerByIdentifierwith that custom audience. Derive the "not resource-server-bound" identifier from the validated refresh token claims before comparing it to the requested resource or resolving a resource server.🤖 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 140 - 192, Update the refresh-token audience handling around the `audience == tokenRequest.ClientID` check to derive the validated non-resource-server identifier from the refresh token claims, rather than assuming it is `tokenRequest.ClientID`. Use that identifier when validating the requested resource and determining whether to keep only OIDC scopes; resolve a resource server only for other audiences.backend/internal/oauth/oauth2/authz/service_test.go (1)
2032-2033: 📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win🔴 Incorrect product name:
thundermust beThunderID(or the appropriate template placeholder for the file type). Barethunder/Thunder/THUNDERis not an accepted short form of the product name.This occurs in the changed ACR test literals on Lines 2032, 2064, 2096, 2123, 2154, and 2182.
Also applies to: 2064-2065, 2095-2096, 2123-2124, 2154-2155, 2182-2182
🤖 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/authz/service_test.go` around lines 2032 - 2033, Update the changed ACR test literals in the relevant test cases to use the product name “ThunderID” instead of the bare “thunder” prefix, including every occurrence of the password and generated-code ACR values identified in the comment. Preserve the existing ACR value structure and test behavior.Source: Path instructions
🧹 Nitpick comments (2)
backend/internal/oauth/oauth2/granthandlers/authorization_code_test.go (1)
114-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate resource-mock stub between
SetupTestandstubDefaultResourceServer.Lines 116-126 duplicate
stubDefaultResourceServer(lines 176-186) verbatim. HaveSetupTestcall the helper instead of inlining the same stub twice.♻️ Proposed dedup
suite.mockResourceService = resourcemock.NewResourceServiceInterfaceMock(suite.T()) - - // Resolve any explicit resource identifier to an echo RS (ID == Identifier); an empty identifier - // resolves to the configured default resource server, as the default-aware provider does. - suite.mockResourceService.On("GetResourceServerByIdentifier", mock.Anything, mock.Anything). - Return(func(_ context.Context, identifier string) *providers.ResourceServer { - if identifier == "" { - return &providers.ResourceServer{ID: testDefaultRSID, Identifier: testDefaultRSIdentifier} - } - return &providers.ResourceServer{ID: identifier, Identifier: identifier} - }, func(_ context.Context, _ string) *tidcommon.ServiceError { - return nil - }).Maybe() - suite.mockResourceService.On("ValidatePermissions", mock.Anything, mock.Anything, mock.Anything). - Return([]string{}, nil).Maybe() + suite.stubDefaultResourceServer()Also applies to: 173-187
🤖 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/authorization_code_test.go` around lines 114 - 127, Update SetupTest to call the existing stubDefaultResourceServer helper instead of defining the duplicate GetResourceServerByIdentifier and ValidatePermissions mocks inline. Remove the redundant inline stubs while preserving the helper’s default and explicit resource-server behavior.backend/pkg/thunderidengine/engine.go (1)
95-95: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConfirm the hardcoded brand string.
Is this hardcoded brand name intentional? If it is configurable, source it from runtime configuration or a named constant instead.
🤖 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/pkg/thunderidengine/engine.go` at line 95, Update the debug message in the runtimeCryptoSvc initialization path to avoid embedding the hardcoded “ThunderID” brand directly; use the appropriate runtime configuration value or existing named constant if the brand is configurable, while preserving the message’s meaning.Source: Path instructions
🤖 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.
Outside diff comments:
In `@backend/internal/oauth/oauth2/authz/service_test.go`:
- Around line 2032-2033: Update the changed ACR test literals in the relevant
test cases to use the product name “ThunderID” instead of the bare “thunder”
prefix, including every occurrence of the password and generated-code ACR values
identified in the comment. Preserve the existing ACR value structure and test
behavior.
In `@backend/internal/oauth/oauth2/granthandlers/refresh_token.go`:
- Around line 140-192: Update the refresh-token audience handling around the
`audience == tokenRequest.ClientID` check to derive the validated
non-resource-server identifier from the refresh token claims, rather than
assuming it is `tokenRequest.ClientID`. Use that identifier when validating the
requested resource and determining whether to keep only OIDC scopes; resolve a
resource server only for other audiences.
---
Nitpick comments:
In `@backend/internal/oauth/oauth2/granthandlers/authorization_code_test.go`:
- Around line 114-127: Update SetupTest to call the existing
stubDefaultResourceServer helper instead of defining the duplicate
GetResourceServerByIdentifier and ValidatePermissions mocks inline. Remove the
redundant inline stubs while preserving the helper’s default and explicit
resource-server behavior.
In `@backend/pkg/thunderidengine/engine.go`:
- Line 95: Update the debug message in the runtimeCryptoSvc initialization path
to avoid embedding the hardcoded “ThunderID” brand directly; use the appropriate
runtime configuration value or existing named constant if the brand is
configurable, while preserving the message’s meaning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7e4cf6d4-0157-46a2-b450-e4df3e78ae56
📒 Files selected for processing (33)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/authz_executor.gobackend/internal/flow/executor/authz_executor_test.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/authz/service_test.gobackend/internal/oauth/oauth2/ciba/init.gobackend/internal/oauth/oauth2/ciba/service.gobackend/internal/oauth/oauth2/ciba/service_test.gobackend/internal/oauth/oauth2/granthandlers/authorization_code.gobackend/internal/oauth/oauth2/granthandlers/authorization_code_test.gobackend/internal/oauth/oauth2/granthandlers/client_credentials.gobackend/internal/oauth/oauth2/granthandlers/client_credentials_test.gobackend/internal/oauth/oauth2/granthandlers/init.gobackend/internal/oauth/oauth2/granthandlers/jwt_bearer.gobackend/internal/oauth/oauth2/granthandlers/jwt_bearer_test.gobackend/internal/oauth/oauth2/granthandlers/provider.gobackend/internal/oauth/oauth2/granthandlers/provider_test.gobackend/internal/oauth/oauth2/granthandlers/refresh_token.gobackend/internal/oauth/oauth2/granthandlers/refresh_token_test.gobackend/internal/oauth/oauth2/granthandlers/token_exchange.gobackend/internal/oauth/oauth2/granthandlers/token_exchange_test.gobackend/internal/oauth/oauth2/par/service.gobackend/internal/oauth/oauth2/par/service_test.gobackend/internal/oauth/oauth2/resourceindicators/resourceindicators.gobackend/internal/oauth/oauth2/resourceindicators/resourceindicators_test.gobackend/internal/resource/default_aware_provider.gobackend/internal/resource/default_aware_provider_test.gobackend/pkg/thunderidengine/engine.go
💤 Files with no reviewable changes (1)
- backend/internal/oauth/oauth2/granthandlers/init.go
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/internal/flow/executor/session_executor.go
- backend/internal/flow/common/constants.go
- backend/internal/flow/executor/session_executor_test.go
- backend/internal/flow/executor/authz_executor.go
- backend/internal/flow/executor/authz_executor_test.go
KD23243
added
Type/Bug
trigger-pr-builder
ImalshaD
force-pushed
the
fix/resource-server-aware-authorization
branch
from
July 24, 2026 03:50
a544df7 to
7bf3ecc
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/utils/authentication/admin-api-auth.ts (1)
32-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate admin-token-acquisition logic; this PR had to patch it twice.
Both files implement the same admin
/flow/executelogin sequence with identical hardcoded app ID and flow secret. Addingresource_server_identifierrequired editing both independently — exactly the drift risk duplicated logic creates.
tests/e2e/utils/authentication/admin-api-auth.ts#L32-L66: keep this as the single canonicalgetAdminToken(request)implementation.tests/e2e/utils/server-setup/mfa-setup.ts#L181-L231: replace the privategetAdminTokenbody with a call to the exportedgetAdminTokenfromadmin-api-auth.ts, passingthis.request, instead of maintaining a second copy.🤖 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/e2e/utils/authentication/admin-api-auth.ts` around lines 32 - 66, Use getAdminToken in tests/e2e/utils/authentication/admin-api-auth.ts:32-66 as the canonical implementation and leave it unchanged. In tests/e2e/utils/server-setup/mfa-setup.ts:181-231, remove the private duplicated login logic and delegate to the exported getAdminToken, passing this.request; update imports and callers as needed.tests/integration/oauth/sso/suite_test.go (1)
252-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSame resource-server-A/B + scope-role fixture is duplicated across sibling suites.
Based on cross-file evidence, the same
rs-A/rs-B/scope-role setup and teardown (identicalSetupSuite/TearDownSuitebodies) also appears in other integration suites in this PR's cohort (e.g.authz_scope_test.go,refresh_token_test.go,authz_test.go). Consider extracting a sharedtestutilshelper (e.g.CreateCrossResourceServerFixture) that returns the two resource-server IDs, scope user ID, and role ID, to avoid four near-identical copies drifting independently. Not blocking for this PR given the project's stated preference for minimal, focused changes.Also applies to: 310-323
🤖 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/sso/suite_test.go` around lines 252 - 288, Extract the duplicated cross-resource-server setup and teardown used by the sibling integration suites into a shared testutils helper, such as CreateCrossResourceServerFixture. Have it create both resource servers, the scope user, and the scope role, returning their IDs; update the affected SetupSuite and TearDownSuite implementations to use the helper while preserving the existing fixture values and cleanup behavior.
🤖 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 `@docs/content/guides/protocols/oauth-oidc/resource-indicators.mdx`:
- Around line 18-19: Update the final sentence in the resource-binding
description to state that unbound OIDC-only and scopeless requests use
token.accessToken.defaultAudience as the audience when configured, falling back
to client_id only when it is unset.
---
Nitpick comments:
In `@tests/e2e/utils/authentication/admin-api-auth.ts`:
- Around line 32-66: Use getAdminToken in
tests/e2e/utils/authentication/admin-api-auth.ts:32-66 as the canonical
implementation and leave it unchanged. In
tests/e2e/utils/server-setup/mfa-setup.ts:181-231, remove the private duplicated
login logic and delegate to the exported getAdminToken, passing this.request;
update imports and callers as needed.
In `@tests/integration/oauth/sso/suite_test.go`:
- Around line 252-288: Extract the duplicated cross-resource-server setup and
teardown used by the sibling integration suites into a shared testutils helper,
such as CreateCrossResourceServerFixture. Have it create both resource servers,
the scope user, and the scope role, returning their IDs; update the affected
SetupSuite and TearDownSuite implementations to use the helper while preserving
the existing fixture values and cleanup behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fdc5330d-00f2-4ad4-87d7-47307149a607
📒 Files selected for processing (43)
backend/cmd/server/servicemanager.gobackend/internal/flow/common/constants.gobackend/internal/flow/executor/authz_executor.gobackend/internal/flow/executor/authz_executor_test.gobackend/internal/flow/executor/register.gobackend/internal/flow/executor/session_executor.gobackend/internal/flow/executor/session_executor_test.gobackend/internal/oauth/init.gobackend/internal/oauth/oauth2/authz/service.gobackend/internal/oauth/oauth2/authz/service_test.gobackend/internal/oauth/oauth2/ciba/init.gobackend/internal/oauth/oauth2/ciba/service.gobackend/internal/oauth/oauth2/ciba/service_test.gobackend/internal/oauth/oauth2/granthandlers/authorization_code.gobackend/internal/oauth/oauth2/granthandlers/authorization_code_test.gobackend/internal/oauth/oauth2/granthandlers/client_credentials.gobackend/internal/oauth/oauth2/granthandlers/client_credentials_test.gobackend/internal/oauth/oauth2/granthandlers/init.gobackend/internal/oauth/oauth2/granthandlers/jwt_bearer.gobackend/internal/oauth/oauth2/granthandlers/jwt_bearer_test.gobackend/internal/oauth/oauth2/granthandlers/provider.gobackend/internal/oauth/oauth2/granthandlers/provider_test.gobackend/internal/oauth/oauth2/granthandlers/refresh_token.gobackend/internal/oauth/oauth2/granthandlers/refresh_token_test.gobackend/internal/oauth/oauth2/granthandlers/token_exchange.gobackend/internal/oauth/oauth2/granthandlers/token_exchange_test.gobackend/internal/oauth/oauth2/par/service.gobackend/internal/oauth/oauth2/par/service_test.gobackend/internal/oauth/oauth2/resourceindicators/resourceindicators.gobackend/internal/oauth/oauth2/resourceindicators/resourceindicators_test.gobackend/internal/resource/default_aware_provider.gobackend/internal/resource/default_aware_provider_test.gobackend/pkg/thunderidengine/engine.godocs/content/guides/protocols/oauth-oidc/resource-indicators.mdxtests/e2e/run-e2e.shtests/e2e/tests/sample-app-authentication/README-MFA.mdtests/e2e/utils/authentication/admin-api-auth.tstests/e2e/utils/server-setup/mfa-setup.tstests/integration/flow/authentication/authz_test.gotests/integration/oauth/authz/authz_scope_test.gotests/integration/oauth/sso/sso_reuse_test.gotests/integration/oauth/sso/suite_test.gotests/integration/oauth/token/refresh_token_test.go
💤 Files with no reviewable changes (2)
- backend/internal/oauth/oauth2/granthandlers/provider_test.go
- backend/internal/oauth/oauth2/granthandlers/init.go
🚧 Files skipped from review as they are similar to previous changes (33)
- backend/internal/flow/executor/session_executor.go
- backend/internal/flow/executor/register.go
- backend/internal/flow/common/constants.go
- backend/internal/oauth/oauth2/par/service.go
- tests/integration/oauth/token/refresh_token_test.go
- backend/internal/flow/executor/session_executor_test.go
- backend/internal/oauth/oauth2/granthandlers/refresh_token.go
- backend/internal/oauth/oauth2/granthandlers/token_exchange.go
- backend/internal/resource/default_aware_provider.go
- backend/internal/oauth/oauth2/ciba/init.go
- tests/integration/flow/authentication/authz_test.go
- backend/internal/resource/default_aware_provider_test.go
- backend/pkg/thunderidengine/engine.go
- backend/internal/oauth/init.go
- backend/internal/oauth/oauth2/granthandlers/authorization_code.go
- backend/internal/oauth/oauth2/ciba/service.go
- backend/internal/oauth/oauth2/granthandlers/client_credentials.go
- backend/internal/oauth/oauth2/granthandlers/provider.go
- tests/integration/oauth/sso/sso_reuse_test.go
- backend/internal/oauth/oauth2/authz/service.go
- backend/internal/oauth/oauth2/granthandlers/client_credentials_test.go
- backend/internal/flow/executor/authz_executor.go
- backend/internal/oauth/oauth2/granthandlers/authorization_code_test.go
- backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
- backend/internal/oauth/oauth2/granthandlers/jwt_bearer_test.go
- backend/cmd/server/servicemanager.go
- backend/internal/oauth/oauth2/resourceindicators/resourceindicators.go
- backend/internal/oauth/oauth2/authz/service_test.go
- backend/internal/oauth/oauth2/resourceindicators/resourceindicators_test.go
- tests/integration/oauth/authz/authz_scope_test.go
- backend/internal/oauth/oauth2/granthandlers/token_exchange_test.go
- backend/internal/oauth/oauth2/par/service_test.go
- backend/internal/oauth/oauth2/ciba/service_test.go
thiva-k
approved these changes
Jul 24, 2026
thiva-k
reviewed
Jul 24, 2026
| // NewDefaultAwareResourceServerProvider wraps base so that GetResourceServerByIdentifier resolves the | ||
| // configured default resource server when the identifier is empty. base and serverConfigService must | ||
| // both be non-nil. | ||
| func NewDefaultAwareResourceServerProvider( |
Contributor
There was a problem hiding this comment.
Need to revisit the naming and initialization pattern in a follow-up
Contributor
Author
There was a problem hiding this comment.
Will bring with a followup PR
thiva-k
reviewed
Jul 24, 2026
| "No resource server service available; dropping requested permission scopes") | ||
| return "" | ||
| } | ||
| rs, svcErr := a.resourceService.GetResourceServerByIdentifier(ctx.Context, identifier) |
Contributor
There was a problem hiding this comment.
Shouldn't we handle server errors here?
Contributor
Author
There was a problem hiding this comment.
Will bring with a followup PR
This was referenced Jul 27, 2026
12 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
Fixes a security issue where requested permissions were evaluated without being scoped to the resource server targeted by the authorization request. When multiple resource servers defined permissions with the same name, a user’s permission on one resource server could incorrectly authorize that permission for another resource server.
This change ensures that permission evaluation and token issuance consistently use the resource server resolved from the OAuth 2.0 Resource Indicator. It also prevents resource-server context from a previous request from leaking through SSO session reuse.
Approach
Resolve the applicable resource server from the authorization request’s resource parameter or the application’s configured default resource server.
Preserve the existing client-ID audience fallback when only OIDC scopes are requested and no resource server is applicable.
Carry the resolved internal resource-server ID through the authorization runtime context.
Scope authorization evaluations to that resource-server ID.
Validate and propagate the resource binding consistently across:
- Authorization Code
- Pushed Authorization Requests
- CIBA
- Refresh Token flows
Exclude the request-scoped resource-server ID from reusable SSO checkpoint data, preventing a previous request’s resource binding from overriding a
subsequent authorization request.
Add unit and integration tests covering:
Related Issues
Related PRs
Checklist
Security checks
(https://security.docs.wso2.com/en/latest/security-guidelines/secure-engineering-guidelines/secure-coding-guidlines/introduction/)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation