Skip to content

Moved supported OAuth grantTypes, responseTypes and AuthMethods to configuration - #4296

Merged
senthalan merged 1 commit into
thunder-id:mainfrom
anushasunkada:local_main
Jul 23, 2026
Merged

Moved supported OAuth grantTypes, responseTypes and AuthMethods to configuration#4296
senthalan merged 1 commit into
thunder-id:mainfrom
anushasunkada:local_main

Conversation

@anushasunkada

@anushasunkada anushasunkada commented Jul 23, 2026

Copy link
Copy Markdown
Member

Purpose

DCR — dynamic client registration endpoint + discovery metadata is based on the "oauth.dcr.enabled" flag. By default value is true.
Token revocation — /oauth2/revoke endpoint + deny-list enforcement on the token-validation hot path is now enabled on the "oauth.token_revocation.enabled" flag. By default value is true.
Logout — RP-initiated logout endpoint + end_session_endpoint discovery metadata depends on "oauth.logout.enabled" flag. By default value is true.
CIBA — gated by allowed_grant_types, now also skips initializing the CIBA service entirely when the grant type isn't allowed. if "allowed_grant_types" is empty all the available grant types are supported.

-->

Approach

Related Issues

Related PRs

  • N/A

Checklist

  • Followed the contribution guidelines.
  • Manual test round performed and verified.
  • Documentation provided. (Add links if there are any)
    • Ran Vale and fixed all errors and warnings
  • Tests provided. (Add links if there are any)
    • Unit Tests
    • Integration Tests
  • Breaking changes. (Fill if applicable)
    • Breaking changes section filled.
    • breaking change label added.

Security checks

  • Followed secure coding standards in WSO2 Secure Coding Guidelines
  • Confirmed that this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.

Summary by CodeRabbit

  • New Features
    • Added configurable allowlists for permitted OAuth grant types, response types, and token endpoint authentication methods.
    • Added feature toggles for Dynamic Client Registration, token revocation, and logout.
    • Added support for the Token Exchange grant handler.
  • Bug Fixes
    • Disabled OAuth features no longer expose related endpoints or handlers.
    • Improved resilience by skipping revocation enforcement when not configured; CIBA callbacks now return a controlled 400 when CIBA isn’t enabled.
  • Documentation
    • Updated OAuth configuration docs for DCR and registration constraints.
  • Tests
    • Expanded coverage for discovery, CIBA callbacks, and allowlist/endpoint behavior.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

OAuth features now use explicit configuration for enablement and supported policies. Initialization, grant handling, discovery metadata, application schemas, token validation, callback dispatch, and engine provider validation reflect those settings and handle disabled optional services safely.

Changes

OAuth configuration and engine wiring

Layer / File(s) Summary
Engine provider contracts
backend/.mockery.public.yml, backend/pkg/thunderidengine/..., backend/pkg/thunderidengine/engine_test.go
Additional provider mocks and required engine-context validations were added, along with the WithIDPProvider option.
OAuth feature configuration and initialization
backend/pkg/thunderidengine/config/config.go, backend/cmd/server/..., backend/internal/oauth/..., backend/internal/inboundclient/service.go, install/helm/..., backend/tests/resources/deployment.yaml, docs/content/deployment/configuration.mdx
DCR, token revocation, logout, and OAuth allowlists were added to configuration; deployment templates expose the settings, and related services and grant handlers are initialized or validated conditionally.
Configured OAuth policies in discovery and application schemas
backend/internal/application/..., backend/internal/oauth/oauth2/constants/..., backend/internal/oauth/oauth2/discovery/..., backend/internal/system/i18n/core/defaults.go
Configured allowlists now drive application validation, tool schemas, and discovery metadata, while optional endpoints are omitted when disabled.
Disabled-service handling and validation
backend/internal/oauth/oauth2/callback/..., backend/internal/oauth/oauth2/granthandlers/..., backend/internal/oauth/oauth2/tokenservice/...
CIBA callbacks, refresh-token rotation, and token validation tolerate absent optional services, with corresponding tests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OAuthConfig
  participant OAuthInit
  participant GrantHandlers
  participant Discovery
  OAuthConfig->>OAuthInit: provide feature toggles and allowlists
  OAuthInit->>GrantHandlers: initialize allowed grant handlers
  OAuthInit->>Discovery: initialize enabled OAuth services
  Discovery->>OAuthConfig: read enabled endpoints and supported values
  Discovery->>GrantHandlers: inspect supported grant types
  Discovery-->>OAuthConfig: return filtered metadata
Loading

Possibly related PRs

Suggested reviewers: rajithacharith, thiva-k, thamindudilshan, brionmario

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.68% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: moving OAuth grant types, response types, and auth methods into configuration.
Description check ✅ Passed The description covers the purpose and related issue, but the Approach section is empty and the checklist is not filled out.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
backend/internal/oauth/oauth2/granthandlers/provider.go (1)

97-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider validating AllowedGrantTypes values at startup.

A typo in oauth.allowed_grant_types silently disables that grant type (handler is never constructed, GetGrantHandler just returns UnSupportedGrantTypeError at request time) with no startup-time signal. Validating configured values against the known providers.GrantType set during initialization would surface misconfiguration immediately instead of at first client request.

🤖 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/provider.go` around lines 97 -
104, Validate each configured value in oauth.allowed_grant_types during OAuth
provider initialization against the known providers.GrantType set, and fail
startup with a clear configuration error when an unknown value is found. Reuse
isGrantTypeAllowed for request-time filtering, but add the startup validation at
the initialization path before grant handlers are constructed.
🤖 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/tools_test.go`:
- Around line 50-53: Update the test fixture in the relevant tools test to use
the approved product domain consistently: change the Hostname and PublicURL
values from thunder.io to thunderid.io while preserving the existing URL scheme
and other fields.

---

Nitpick comments:
In `@backend/internal/oauth/oauth2/granthandlers/provider.go`:
- Around line 97-104: Validate each configured value in
oauth.allowed_grant_types during OAuth provider initialization against the known
providers.GrantType set, and fail startup with a clear configuration error when
an unknown value is found. Reuse isGrantTypeAllowed for request-time filtering,
but add the startup validation at the initialization path before grant handlers
are constructed.
🪄 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: 33ee9938-fe7d-4eed-912b-ca00f0a7fd6f

📥 Commits

Reviewing files that changed from the base of the PR and between 99130c3 and 88240b0.

⛔ Files ignored due to path filters (5)
  • backend/tests/mocks/designprovidermock/DesignProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/i18nprovidermock/I18nProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/idpprovidermock/IDPProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/ouprovidermock/OrganizationUnitProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/resourceserverprovidermock/ResourceServerProvider_mock.go is excluded by !**/*_mock.go
📒 Files selected for processing (21)
  • backend/.mockery.public.yml
  • backend/cmd/server/config/default.json
  • backend/cmd/server/servicemanager.go
  • backend/internal/application/tools.go
  • backend/internal/application/tools_test.go
  • backend/internal/oauth/init.go
  • backend/internal/oauth/oauth2/callback/callback.go
  • backend/internal/oauth/oauth2/callback/callback_test.go
  • backend/internal/oauth/oauth2/constants/constants.go
  • backend/internal/oauth/oauth2/discovery/discovery_test.go
  • backend/internal/oauth/oauth2/discovery/service.go
  • backend/internal/oauth/oauth2/granthandlers/provider.go
  • backend/internal/oauth/oauth2/granthandlers/provider_test.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
  • backend/internal/oauth/oauth2/tokenservice/validator.go
  • backend/internal/oauth/oauth2/tokenservice/validator_test.go
  • backend/pkg/thunderidengine/config/config.go
  • backend/pkg/thunderidengine/engine.go
  • backend/pkg/thunderidengine/engine_test.go
  • backend/tests/resources/deployment.yaml

Comment thread backend/internal/application/tools_test.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
backend/internal/application/tools_test.go (1)

51-53: ⚠️ Potential issue | 🔴 Critical

Use the approved product name in the test fixture.

🔴 Incorrect product name: thunder must be ThunderID (or the appropriate template placeholder for the file type). Bare thunder/Thunder/THUNDER is not an accepted short form of the product name.

Replace thunder.io at Lines [51], [53], and [59] with thunderid.io.

Proposed fix
-			Hostname:   "thunder.io",
+			Hostname:   "thunderid.io",
-			PublicURL:  "https://thunder.io",
+			PublicURL:  "https://thunderid.io",
-			Issuer:     "https://thunder.io",
+			Issuer:     "https://thunderid.io",

As per path instructions, bare thunder/Thunder/THUNDER is not an accepted short form of the product name.

Also applies to: 58-60

🤖 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/application/tools_test.go` around lines 51 - 53, Update the
test fixture fields in the relevant tools test, including Hostname and
PublicURL, to use thunderid.io instead of thunder.io. Ensure all occurrences in
the fixture are updated consistently while preserving the existing URL and port
structure.

Source: Path instructions

🧹 Nitpick comments (1)
docs/content/deployment/configuration.mdx (1)

455-459: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Document all new OAuth feature toggles and empty-list behavior.

At Line [455], add rows for oauth.token_revocation.enabled and oauth.logout.enabled, which are also introduced by this PR. At Line [459], document that an empty oauth.allowed_grant_types list enables all available grant types; the allowlist also affects service initialization and discovery behavior.

As per path instructions, documentation must cover all new configuration options and behavioral changes.

🤖 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 `@docs/content/deployment/configuration.mdx` around lines 455 - 459, The OAuth
configuration table must document the new oauth.token_revocation.enabled and
oauth.logout.enabled toggles. Update the oauth.allowed_grant_types description
to state that an empty list enables all available grant types and that the
allowlist affects service initialization and discovery behavior.

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.

Duplicate comments:
In `@backend/internal/application/tools_test.go`:
- Around line 51-53: Update the test fixture fields in the relevant tools test,
including Hostname and PublicURL, to use thunderid.io instead of thunder.io.
Ensure all occurrences in the fixture are updated consistently while preserving
the existing URL and port structure.

---

Nitpick comments:
In `@docs/content/deployment/configuration.mdx`:
- Around line 455-459: The OAuth configuration table must document the new
oauth.token_revocation.enabled and oauth.logout.enabled toggles. Update the
oauth.allowed_grant_types description to state that an empty list enables all
available grant types and that the allowlist affects service initialization and
discovery behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: bacd1ddb-8b49-497d-97e5-15f86236a04b

📥 Commits

Reviewing files that changed from the base of the PR and between 88240b0 and 44b8fa9.

⛔ Files ignored due to path filters (5)
  • backend/tests/mocks/designprovidermock/DesignProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/i18nprovidermock/I18nProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/idpprovidermock/IDPProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/ouprovidermock/OrganizationUnitProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/resourceserverprovidermock/ResourceServerProvider_mock.go is excluded by !**/*_mock.go
📒 Files selected for processing (24)
  • backend/.mockery.public.yml
  • backend/cmd/server/config/default.json
  • backend/cmd/server/servicemanager.go
  • backend/internal/application/tools.go
  • backend/internal/application/tools_test.go
  • backend/internal/oauth/init.go
  • backend/internal/oauth/oauth2/callback/callback.go
  • backend/internal/oauth/oauth2/callback/callback_test.go
  • backend/internal/oauth/oauth2/constants/constants.go
  • backend/internal/oauth/oauth2/discovery/discovery_test.go
  • backend/internal/oauth/oauth2/discovery/service.go
  • backend/internal/oauth/oauth2/granthandlers/provider.go
  • backend/internal/oauth/oauth2/granthandlers/provider_test.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
  • backend/internal/oauth/oauth2/tokenservice/validator.go
  • backend/internal/oauth/oauth2/tokenservice/validator_test.go
  • backend/pkg/thunderidengine/config/config.go
  • backend/pkg/thunderidengine/engine.go
  • backend/pkg/thunderidengine/engine_test.go
  • backend/tests/resources/deployment.yaml
  • docs/content/deployment/configuration.mdx
  • install/helm/conf/deployment.yaml
  • install/helm/values.yaml
🚧 Files skipped from review as they are similar to previous changes (19)
  • backend/internal/oauth/oauth2/granthandlers/refresh_token.go
  • backend/tests/resources/deployment.yaml
  • backend/cmd/server/servicemanager.go
  • backend/internal/oauth/oauth2/granthandlers/provider_test.go
  • backend/internal/oauth/init.go
  • backend/internal/oauth/oauth2/callback/callback_test.go
  • backend/pkg/thunderidengine/engine.go
  • backend/internal/application/tools.go
  • backend/internal/oauth/oauth2/tokenservice/validator.go
  • backend/cmd/server/config/default.json
  • backend/internal/oauth/oauth2/callback/callback.go
  • backend/internal/oauth/oauth2/constants/constants.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
  • backend/internal/oauth/oauth2/tokenservice/validator_test.go
  • backend/internal/oauth/oauth2/granthandlers/provider.go
  • backend/pkg/thunderidengine/config/config.go
  • backend/pkg/thunderidengine/engine_test.go
  • backend/internal/oauth/oauth2/discovery/service.go
  • backend/internal/oauth/oauth2/discovery/discovery_test.go

@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.63946% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
backend/internal/inboundclient/service.go 93.93% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

Comment thread backend/tests/resources/deployment.yaml
@senthalan

Copy link
Copy Markdown
Member

I don't see the configurations like allowed_auth_methods, allowed_response_types and allowed_grant_types have been used to validate the oauth client details during the creation and update paths. @anushasunkada

@anushasunkada
anushasunkada force-pushed the local_main branch 2 times, most recently from 1647960 to 7c2882f Compare July 23, 2026 18:02

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/inboundclient/service.go`:
- Around line 1070-1111: Add focused table-driven tests covering non-empty OAuth
allow-lists for validateWithAllowedGrantTypes, validateWithAllowedResponseTypes,
and validateAllowedTokenEndpointAuthMethod. Exercise both create and update
validation paths, asserting configured values are accepted and disabled grants,
response types, and auth methods are rejected, while preserving existing
empty-list coverage.

In `@install/helm/conf/deployment.yaml`:
- Around line 248-261: The OAuth configuration template currently renders only
DCR and method lists, omitting the new token revocation and logout feature
toggles. Update the relevant OAuth section in the deployment template to
serialize configuration.oauth.tokenRevocation.enabled and
configuration.oauth.logout.enabled alongside dcr.enabled, preserving the
existing Helm value structure and rendering style.
🪄 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: 6b5dad78-231d-4ba1-a235-e28dcc1146b9

📥 Commits

Reviewing files that changed from the base of the PR and between 1647960 and 7c2882f.

⛔ Files ignored due to path filters (4)
  • backend/tests/mocks/designprovidermock/DesignProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/i18nprovidermock/I18nProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/ouprovidermock/OrganizationUnitProvider_mock.go is excluded by !**/*_mock.go
  • backend/tests/mocks/resourceserverprovidermock/ResourceServerProvider_mock.go is excluded by !**/*_mock.go
📒 Files selected for processing (29)
  • backend/.mockery.public.yml
  • backend/cmd/server/config/default.json
  • backend/cmd/server/servicemanager.go
  • backend/internal/application/error_constants.go
  • backend/internal/application/service.go
  • backend/internal/application/service_test.go
  • backend/internal/application/tools.go
  • backend/internal/application/tools_test.go
  • backend/internal/inboundclient/service.go
  • backend/internal/oauth/init.go
  • backend/internal/oauth/oauth2/callback/callback.go
  • backend/internal/oauth/oauth2/callback/callback_test.go
  • backend/internal/oauth/oauth2/constants/constants.go
  • backend/internal/oauth/oauth2/discovery/discovery_test.go
  • backend/internal/oauth/oauth2/discovery/service.go
  • backend/internal/oauth/oauth2/granthandlers/provider.go
  • backend/internal/oauth/oauth2/granthandlers/provider_test.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
  • backend/internal/oauth/oauth2/tokenservice/validator.go
  • backend/internal/oauth/oauth2/tokenservice/validator_test.go
  • backend/internal/system/i18n/core/defaults.go
  • backend/pkg/thunderidengine/config/config.go
  • backend/pkg/thunderidengine/engine.go
  • backend/pkg/thunderidengine/engine_test.go
  • backend/tests/resources/deployment.yaml
  • docs/content/deployment/configuration.mdx
  • install/helm/conf/deployment.yaml
  • install/helm/values.yaml
💤 Files with no reviewable changes (2)
  • backend/internal/system/i18n/core/defaults.go
  • backend/internal/application/error_constants.go
🚧 Files skipped from review as they are similar to previous changes (20)
  • backend/tests/resources/deployment.yaml
  • backend/internal/oauth/oauth2/granthandlers/refresh_token.go
  • install/helm/values.yaml
  • backend/internal/oauth/oauth2/callback/callback.go
  • backend/internal/oauth/init.go
  • backend/pkg/thunderidengine/engine.go
  • backend/internal/oauth/oauth2/tokenservice/validator_test.go
  • backend/internal/oauth/oauth2/constants/constants.go
  • backend/internal/oauth/oauth2/tokenservice/validator.go
  • backend/internal/application/tools_test.go
  • backend/.mockery.public.yml
  • docs/content/deployment/configuration.mdx
  • backend/internal/application/tools.go
  • backend/internal/oauth/oauth2/granthandlers/refresh_token_test.go
  • backend/cmd/server/servicemanager.go
  • backend/internal/oauth/oauth2/callback/callback_test.go
  • backend/internal/oauth/oauth2/granthandlers/provider.go
  • backend/pkg/thunderidengine/config/config.go
  • backend/internal/oauth/oauth2/discovery/discovery_test.go
  • backend/internal/oauth/oauth2/discovery/service.go

Comment on lines +1070 to +1111
// validateAllowedGrantTypes rejects grant types not permitted by the deployment's configured
// oauth.allowed_grant_types allow-list. An empty allow-list permits all grant types.
func validateWithAllowedGrantTypes(grantTypes []string) error {
allowed := config.GetServerRuntime().Config.OAuth.AllowedGrantTypes
for _, grantType := range grantTypes {
if !providers.GrantType(grantType).IsValid() {
return ErrOAuthInvalidGrantType
}
if len(allowed) > 0 && !slices.Contains(allowed, grantType) {
return ErrOAuthInvalidGrantType
}
}
return nil
}

// validateAllowedResponseTypes rejects response types not permitted by the deployment's configured
// oauth.allowed_response_types allow-list. An empty allow-list permits all response types.
func validateWithAllowedResponseTypes(responseTypes []string) error {
allowed := config.GetServerRuntime().Config.OAuth.AllowedResponseTypes
for _, responseType := range responseTypes {
if !providers.ResponseType(responseType).IsValid() {
return ErrOAuthInvalidResponseType
}
if len(allowed) > 0 && !slices.Contains(allowed, responseType) {
return ErrOAuthInvalidResponseType
}
}
return nil
}

// validateAllowedTokenEndpointAuthMethod rejects a token endpoint auth method not permitted by the
// deployment's configured oauth.allowed_auth_methods allow-list. An empty allow-list permits all methods.
func validateWithAllowedTokenEndpointAuthMethod(method string) error {
if !providers.TokenEndpointAuthMethod(method).IsValid() {
return ErrOAuthInvalidTokenEndpointAuthMethod
}
allowed := config.GetServerRuntime().Config.OAuth.AllowedAuthMethods
if len(allowed) == 0 || slices.Contains(allowed, method) {
return nil
}
return ErrOAuthInvalidTokenEndpointAuthMethod
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add configured allow-list enforcement tests.

The supplied tests only initialize OAuthConfig{} with empty lists, so they do not verify that each non-empty allow-list accepts allowed values and rejects disabled grants, response types, and auth methods. Add focused table-driven coverage for both create and update validation paths. As per 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/inboundclient/service.go` around lines 1070 - 1111, Add
focused table-driven tests covering non-empty OAuth allow-lists for
validateWithAllowedGrantTypes, validateWithAllowedResponseTypes, and
validateAllowedTokenEndpointAuthMethod. Exercise both create and update
validation paths, asserting configured values are accepted and disabled grants,
response types, and auth methods are rejected, while preserving existing
empty-list coverage.

Source: Coding guidelines

Comment on lines +248 to +261
enabled: {{ .Values.configuration.oauth.dcr.enabled }}
insecure: {{ .Values.configuration.oauth.dcr.insecure }}
allowed_auth_methods:
{{- range .Values.configuration.oauth.allowedAuthMethods }}
- {{ . | quote }}
{{- end }}
allowed_response_types:
{{- range .Values.configuration.oauth.allowedResponseTypes }}
- {{ . | quote }}
{{- end }}
allowed_grant_types:
{{- range .Values.configuration.oauth.allowedGrantTypes }}
- {{ . | quote }}
{{- end }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Render revocation and logout feature toggles.

Unlike dcr.enabled, this template has no path to serialize token_revocation.enabled or logout.enabled. Helm deployments therefore cannot configure the two new OAuth feature gates.

Proposed fix
   dcr:
     enabled: {{ .Values.configuration.oauth.dcr.enabled }}
     insecure: {{ .Values.configuration.oauth.dcr.insecure }}
+  token_revocation:
+    enabled: {{ .Values.configuration.oauth.tokenRevocation.enabled }}
+  logout:
+    enabled: {{ .Values.configuration.oauth.logout.enabled }}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
enabled: {{ .Values.configuration.oauth.dcr.enabled }}
insecure: {{ .Values.configuration.oauth.dcr.insecure }}
allowed_auth_methods:
{{- range .Values.configuration.oauth.allowedAuthMethods }}
- {{ . | quote }}
{{- end }}
allowed_response_types:
{{- range .Values.configuration.oauth.allowedResponseTypes }}
- {{ . | quote }}
{{- end }}
allowed_grant_types:
{{- range .Values.configuration.oauth.allowedGrantTypes }}
- {{ . | quote }}
{{- end }}
enabled: {{ .Values.configuration.oauth.dcr.enabled }}
insecure: {{ .Values.configuration.oauth.dcr.insecure }}
token_revocation:
enabled: {{ .Values.configuration.oauth.tokenRevocation.enabled }}
logout:
enabled: {{ .Values.configuration.oauth.logout.enabled }}
allowed_auth_methods:
{{- range .Values.configuration.oauth.allowedAuthMethods }}
- {{ . | quote }}
{{- end }}
allowed_response_types:
{{- range .Values.configuration.oauth.allowedResponseTypes }}
- {{ . | quote }}
{{- end }}
allowed_grant_types:
{{- range .Values.configuration.oauth.allowedGrantTypes }}
- {{ . | quote }}
{{- end }}
🤖 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 `@install/helm/conf/deployment.yaml` around lines 248 - 261, The OAuth
configuration template currently renders only DCR and method lists, omitting the
new token revocation and logout feature toggles. Update the relevant OAuth
section in the deployment template to serialize
configuration.oauth.tokenRevocation.enabled and
configuration.oauth.logout.enabled alongside dcr.enabled, preserving the
existing Helm value structure and rendering style.

Comment thread backend/internal/application/service.go Outdated
Comment on lines +1306 to +1346
@@ -1334,7 +1340,10 @@ func translateOAuthValidationError(err error) *tidcommon.ServiceError {

// OAuth: token endpoint auth method
case errors.Is(err, inboundclient.ErrOAuthInvalidTokenEndpointAuthMethod):
return &ErrorInvalidTokenEndpointAuthMethod
return tidcommon.CustomServiceError(ErrorInvalidOAuthConfiguration, tidcommon.I18nMessage{
Key: "error.applicationservice.invalid_token_endpoint_auth_method",
DefaultValue: "Invalid token endpoint authentication method",
})

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we keep this as it is. With the Application level error codes defined in backend/internal/application/error_constants.go without this custom error

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For all 3 checks?


// DCRConfig holds the Dynamic Client Registration configuration.
type DCRConfig struct {
Enabled bool `yaml:"enabled" json:"enabled"`

@thiva-k thiva-k Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be *bool instead? We are setting the default as true, so if we override this to false via deployment.yaml, the merge logic will ignore zero values(false is the zero value for bool).

Signed-off-by: anushasunkada <anushasunkada@gmail.com>
@senthalan senthalan added the trigger-pr-builder Add when the PR is ready for CI; starts the PR Builder for this and all later pushes label Jul 23, 2026
@senthalan
senthalan enabled auto-merge July 23, 2026 20:04
@senthalan
senthalan added this pull request to the merge queue Jul 23, 2026
Merged via the queue into thunder-id:main with commit 4ef4c49 Jul 23, 2026
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

trigger-pr-builder Add when the PR is ready for CI; starts the PR Builder for this and all later pushes Type/Improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants