feat: support configurable custom OAuth PKCE - #6620
Conversation
WalkthroughCustom OAuth providers now support persisted PKCE configuration. OAuth login and account binding flows exchange structured S256 challenge data, store verifiers server-side, and submit them during token exchange. The frontend validates flow responses and builds provider URLs with conditional PKCE parameters. ChangesCustom OAuth PKCE
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant OAuthFlowAPI
participant OAuthFlowController
participant OAuthProvider
participant TokenEndpoint
Browser->>OAuthFlowAPI: Request OAuth flow details
OAuthFlowAPI->>OAuthFlowController: Create flow and PKCE challenge
OAuthFlowController-->>OAuthFlowAPI: Return flow token and S256 challenge
Browser->>OAuthProvider: Authorize with state and challenge
OAuthProvider-->>OAuthFlowAPI: Return authorization code
OAuthFlowAPI->>OAuthProvider: Exchange code with stored verifier
OAuthProvider->>TokenEndpoint: Submit code and PKCE verifier
TokenEndpoint-->>OAuthProvider: Return access token
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/features/profile/components/tabs/account-bindings-tab.tsx (1)
157-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd account-binding OAuth coverage for custom providers.
The PKCE coverage in
web/src/features/auth/lib/__tests__/custom-oauth-url.test.tsonly coversbuildCustomOAuthUrl. It does not coveraccount-bindings-tab.tsxbindings orhandleBindCustomOAuthcallingcreateOAuthFlowDetails(..., 'bind')and building URLs with PKCE and non-PKCE providers. Add component/module tests for the binding callback, including PKCE andpkce_enabled: falsecases.🤖 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 `@web/src/features/profile/components/tabs/account-bindings-tab.tsx` around lines 157 - 229, Add component or module tests covering handleBindCustomOAuth and startOAuthBinding for custom providers. Verify createOAuthFlowDetails is called with the provider slug and 'bind', and that both PKCE-enabled and pkce_enabled: false providers produce the expected OAuth URL parameters and popup navigation behavior.Source: Coding guidelines
🧹 Nitpick comments (2)
model/custom_oauth_provider_test.go (1)
24-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assertfor independent default checks.Keep
require.NoErroron Line 24 because it is a prerequisite. The comparisons on Lines 25-31 are independent output checks. Usetestify/assertfor them instead ofrequireso one mismatch does not stop the remaining checks.Proposed assertion update
import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ - require.Equal(t, "example-sso", provider.Slug) - require.Equal(t, "openid profile email", provider.Scopes) - require.Equal(t, "sub", provider.UserIdField) - require.Equal(t, "preferred_username", provider.UsernameField) - require.Equal(t, "name", provider.DisplayNameField) - require.Equal(t, "email", provider.EmailField) - require.True(t, provider.PKCEEnabled) + assert.Equal(t, "example-sso", provider.Slug) + assert.Equal(t, "openid profile email", provider.Scopes) + assert.Equal(t, "sub", provider.UserIdField) + assert.Equal(t, "preferred_username", provider.UsernameField) + assert.Equal(t, "name", provider.DisplayNameField) + assert.Equal(t, "email", provider.EmailField) + assert.True(t, provider.PKCEEnabled)As per coding guidelines: “New or substantially rewritten tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal 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 `@model/custom_oauth_provider_test.go` around lines 24 - 31, Keep require.NoError for the validateCustomOAuthProvider prerequisite, and change the independent provider field checks for Slug, Scopes, UserIdField, UsernameField, DisplayNameField, EmailField, and PKCEEnabled to use testify/assert so all default-value mismatches are reported.Source: Coding guidelines
oauth/generic_test.go (1)
41-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
assertfor non-fatal result checks.Line 42 uses
require.NoErrorcorrectly as a fatal setup check. Lines 43-46 check independent response fields (token.AccessToken,received.Get("code_verifier"),received.Get("code"),received.Get("redirect_uri")). Useassert.Equalfor these, so a mismatch in one field does not hide mismatches in the others.♻️ Proposed fix
+ "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) ... token, err := provider.ExchangeToken(context.Background(), "authorization-code", c) require.NoError(t, err) - require.Equal(t, "access-token", token.AccessToken) - require.Equal(t, "verifier-value", received.Get("code_verifier")) - require.Equal(t, "authorization-code", received.Get("code")) - require.Equal(t, "https://dashboard.example.test/oauth/example-sso", received.Get("redirect_uri")) + assert.Equal(t, "access-token", token.AccessToken) + assert.Equal(t, "verifier-value", received.Get("code_verifier")) + assert.Equal(t, "authorization-code", received.Get("code")) + assert.Equal(t, "https://dashboard.example.test/oauth/example-sso", received.Get("redirect_uri"))As per coding guidelines: "New or substantially rewritten tests must use testify/require for setup and fatal assertions and testify/assert for non-fatal 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 `@oauth/generic_test.go` around lines 41 - 47, In the ExchangeToken test, keep require.NoError for the fatal error check, but change the independent field validations for token.AccessToken and the received code_verifier, code, and redirect_uri values to assert.Equal so all mismatches are reported.Source: Coding guidelines
🤖 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 `@model/custom_oauth_provider_test.go`:
- Around line 12-17: Update the validCustomOAuthProvider defaults test fixture
to use an empty Slug so default slug generation is exercised, and add a separate
provider case with PKCEEnabled set to false. Assert both behaviors, preserving
the existing enabled-PKCE and normalized-slug coverage while protecting the
non-PKCE compatibility path.
In `@web/src/i18n/locales/ja.json`:
- Around line 3249-3250: Update the Japanese translation for “Require a proof
key for authorization code exchanges” in the locale resource to use a complete
setting label that explicitly states the proof key is required during
authorization code exchanges, such as the wording requested in the review.
---
Outside diff comments:
In `@web/src/features/profile/components/tabs/account-bindings-tab.tsx`:
- Around line 157-229: Add component or module tests covering
handleBindCustomOAuth and startOAuthBinding for custom providers. Verify
createOAuthFlowDetails is called with the provider slug and 'bind', and that
both PKCE-enabled and pkce_enabled: false providers produce the expected OAuth
URL parameters and popup navigation behavior.
---
Nitpick comments:
In `@model/custom_oauth_provider_test.go`:
- Around line 24-31: Keep require.NoError for the validateCustomOAuthProvider
prerequisite, and change the independent provider field checks for Slug, Scopes,
UserIdField, UsernameField, DisplayNameField, EmailField, and PKCEEnabled to use
testify/assert so all default-value mismatches are reported.
In `@oauth/generic_test.go`:
- Around line 41-47: In the ExchangeToken test, keep require.NoError for the
fatal error check, but change the independent field validations for
token.AccessToken and the received code_verifier, code, and redirect_uri values
to assert.Equal so all mismatches are reported.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f709c800-5932-4144-88b4-c4b0ebca6f3d
📒 Files selected for processing (24)
controller/auth_flow_test.gocontroller/custom_oauth.gocontroller/misc.gocontroller/oauth.gomodel/custom_oauth_provider.gomodel/custom_oauth_provider_test.gooauth/generic.gooauth/generic_test.gooauth/provider.goweb/src/features/auth/api.tsweb/src/features/auth/hooks/use-oauth-login.tsweb/src/features/auth/lib/__tests__/custom-oauth-url.test.tsweb/src/features/auth/lib/oauth.tsweb/src/features/auth/types.tsweb/src/features/profile/components/tabs/account-bindings-tab.tsxweb/src/features/system-settings/auth/custom-oauth/components/provider-form-dialog.tsxweb/src/features/system-settings/auth/custom-oauth/types.tsweb/src/i18n/locales/en.jsonweb/src/i18n/locales/fr.jsonweb/src/i18n/locales/ja.jsonweb/src/i18n/locales/ru.jsonweb/src/i18n/locales/vi.jsonweb/src/i18n/locales/zh-TW.jsonweb/src/i18n/locales/zh.json
| Slug: "Example-SSO", | ||
| ClientId: "client-id", | ||
| AuthorizationEndpoint: "https://sso.example.test/authorize", | ||
| TokenEndpoint: "https://sso.example.test/token", | ||
| UserInfoEndpoint: "https://sso.example.test/userinfo", | ||
| PKCEEnabled: true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise the empty-slug and disabled-PKCE paths.
validCustomOAuthProvider sets Slug to "Example-SSO" at Line 12 and PKCEEnabled to true at Line 17. The defaults test therefore checks slug normalization and enabled-PKCE preservation. It does not fail if empty-slug defaulting breaks or disabled-PKCE providers are changed to PKCE. Set Slug to an empty value for the defaults case and add a case with PKCEEnabled == false.
The PR objective requires preserving the non-PKCE compatibility path.
Proposed test coverage
func TestValidateCustomOAuthProviderAppliesDefaults(t *testing.T) {
provider := validCustomOAuthProvider()
+ provider.Slug = ""
require.NoError(t, validateCustomOAuthProvider(provider))
require.Equal(t, "example-sso", provider.Slug)
}
+
+func TestValidateCustomOAuthProviderPreservesDisabledPKCE(t *testing.T) {
+ provider := validCustomOAuthProvider()
+ provider.PKCEEnabled = false
+
+ require.NoError(t, validateCustomOAuthProvider(provider))
+ require.False(t, provider.PKCEEnabled)
+}As per coding guidelines: “Backend tests must protect real behavior, API contracts, billing/accounting invariants, compatibility, or regression paths.”
Also applies to: 21-31
🤖 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 `@model/custom_oauth_provider_test.go` around lines 12 - 17, Update the
validCustomOAuthProvider defaults test fixture to use an empty Slug so default
slug generation is exercised, and add a separate provider case with PKCEEnabled
set to false. Assert both behaviors, preserving the existing enabled-PKCE and
normalized-slug coverage while protecting the non-PKCE compatibility path.
Source: Coding guidelines
| "PKCE (S256)": "PKCE（S256）", | ||
| "Require a proof key for authorization code exchanges": "認可コード交換に証明鍵を要求", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a complete Japanese setting label.
Line 3250 reads as a fragment and is less clear for Japanese administrators. Use wording that states the requirement explicitly, such as 認可コード交換時に証明鍵を必須にする.
Proposed fix
- "Require a proof key for authorization code exchanges": "認可コード交換に証明鍵を要求",
+ "Require a proof key for authorization code exchanges": "認可コード交換時に証明鍵を必須にする",📝 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.
| "PKCE (S256)": "PKCE（S256）", | |
| "Require a proof key for authorization code exchanges": "認可コード交換に証明鍵を要求", | |
| "PKCE (S256)": "PKCE（S256）", | |
| "Require a proof key for authorization code exchanges": "認可コード交換時に証明鍵を必須にする", |
🤖 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 `@web/src/i18n/locales/ja.json` around lines 3249 - 3250, Update the Japanese
translation for “Require a proof key for authorization code exchanges” in the
locale resource to use a complete setting label that explicitly states the proof
key is required during authorization code exchanges, such as the wording
requested in the review.
2 participants
Summary
pkce_enabled配置，统一使用 OAuth Authorization Code + PKCE S256。Verification
go test ./controller ./oauth ./modelbun test src/features/auth/lib/__tests__/custom-oauth-url.test.tsbun run typecheck(inweb/)bun run build:check(inweb/)该 PR 从
cosohuang/new-apifork 提交，因为当前账号没有上游仓库直接写权限。Summary by CodeRabbit
New Features
Bug Fixes