fix(oauth): fix OIDC and custom OAuth binding flows - #5216
Open
TomyJan wants to merge 15 commits into
Open
Conversation
- Derive OAuth redirect URIs from request headers before falling back to ServerAddress - Improve OIDC token exchange diagnostics - Fix custom OAuth profile binding to use provider slug and OAuth state - Show custom OAuth provider user IDs in admin binding management - Add configurable auto-link policy for custom OAuth providers - Remove custom OAuth bindings when users are deleted - Add custom OAuth UI controls, translations, and built-in OIDC guidance
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a configurable ChangesCustom OAuth Auto-Link Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
oauth/oidc.go (1)
158-158: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse
common.DecodeJson()instead ofjson.NewDecoder()for consistency.Line 158 directly uses
json.NewDecoder(res.Body).Decode(&oidcUser), which violates the coding guideline requiring wrapper functions fromcommon/json.go. The same file'sExchangeTokenmethod was updated to usecommon.Unmarshal(line 101), butGetUserInfowas left unchanged.♻️ Proposed fix to align with coding guidelines
- var oidcUser oidcUser - err = json.NewDecoder(res.Body).Decode(&oidcUser) - if err != nil { + body, err := io.ReadAll(res.Body) + if err != nil { + logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] GetUserInfo read body error: %s", err.Error())) + return nil, err + } + + var oidcUser oidcUser + if err := common.DecodeJson(io.NopCloser(strings.NewReader(string(body))), &oidcUser); err != nil { logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] GetUserInfo decode error: %s", err.Error())) return nil, err }Or more simply, following the pattern used in
ExchangeToken:+ body, err := io.ReadAll(res.Body) + if err != nil { + logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] GetUserInfo read body error: %s", err.Error())) + return nil, err + } + var oidcUser oidcUser - err = json.NewDecoder(res.Body).Decode(&oidcUser) - if err != nil { + if err := common.Unmarshal(body, &oidcUser); err != nil { logger.LogError(ctx, fmt.Sprintf("[OAuth-OIDC] GetUserInfo decode error: %s", err.Error())) return nil, err }As per coding guidelines: All JSON marshal/unmarshal operations MUST use wrapper functions from
common/json.go:common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.DecodeJson(), orcommon.GetJsonType(). Do NOT directly import or callencoding/jsonin business code.🤖 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/oidc.go` at line 158, GetUserInfo currently decodes the HTTP response with json.NewDecoder(res.Body).Decode(&oidcUser) which breaks the guideline; replace that call with the wrapper common.DecodeJson(res.Body, &oidcUser) (matching the pattern used in ExchangeToken which uses common.Unmarshal) and remove any direct encoding/json usage from this function so all JSON decoding uses the common package.web/default/src/features/users/components/dialogs/user-binding-dialog.tsx (1)
250-278:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winType error:
labelfallback now yieldsstring | number.Changing
custom_oauth_providers[].idtonumber(Line 88) breaks the twolabelassignments below, sinceBindingItem.labelis typedstring(Line 71):
- Line 255:
provider.name || provider.id→string | number- Line 269:
binding.provider_name || binding.provider_id→string | number(provider_idis nownumber)
bun run typecheckwill fail on both. Wrap the numeric fallback inString(...).🐛 Proposed fix
- label: provider.name || provider.id, + label: provider.name || String(provider.id),- label: binding.provider_name || binding.provider_id, + label: binding.provider_name || String(binding.provider_id),🤖 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/default/src/features/users/components/dialogs/user-binding-dialog.tsx` around lines 250 - 278, The label fallbacks can be number|string now because custom_oauth_providers[].id and binding.provider_id are numbers; update the two label assignments so they always produce strings: in the loop over customProviders set label to provider.name || String(provider.id), and in the loop over oauthBindings set label to binding.provider_name || String(binding.provider_id). Locate the code that builds the items array (the customProviders loop and the oauthBindings loop that push into items, referencing variables customProviders, oauthBindings, oauthBindingMap, items and the BindingItem.label type) and wrap the numeric fallbacks with String(...) accordingly.web/default/src/features/system-settings/auth/custom-oauth/components/provider-form-dialog.tsx (1)
134-154:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd missing
auto_link_policyfield to new provider reset.When resetting the form for a new provider (no existing provider), the
auto_link_policyfield is missing from the reset object. For consistency with the editing flow (line 130) and other fields likeauth_style(line 151), this field should be explicitly set.Proposed fix
well_known: '', auth_style: 0, + auto_link_policy: 'none', access_policy: '', access_denied_message: '',🤖 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/default/src/features/system-settings/auth/custom-oauth/components/provider-form-dialog.tsx` around lines 134 - 154, The form reset for creating a new provider in the provider-form-dialog component omits the auto_link_policy field; update the branch that calls form.reset when props.open && !props.provider to include an explicit auto_link_policy entry (matching the edited-provider default used when props.provider exists), so add auto_link_policy alongside fields like auth_style in the object passed to form.reset to ensure consistent defaults between create and edit flows.
🤖 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 `@web/default/src/features/profile/components/tabs/account-bindings-tab.tsx`:
- Around line 118-137: The popup is opened only after an await (getOAuthState),
which can cause browsers to block it; modify handleBindCustomOAuth so it opens
the popup synchronously before any awaits (call window.open immediately to get a
popup window reference), then perform getOAuthState() and build the
authorization URL and set popup.location.href (or fallback to
popup.opener.location) to the final URL; ensure you still return early if
provider lacks required fields but do this check before opening the popup, and
reference handleBindCustomOAuth, getOAuthState, provider.authorization_endpoint,
provider.client_id, provider.slug and window.open when making the change.
In `@web/default/src/features/system-settings/auth/custom-oauth/types.ts`:
- Around line 228-232: AUTO_LINK_POLICY_OPTIONS uses full-sentence labelKey
strings; change each labelKey to hierarchical i18n keys (e.g., replace 'Do not
auto-link existing users' -> 'auth.oauth.autoLink.none', 'Auto-link by email' ->
'auth.oauth.autoLink.emailVerified', 'Auto-link by username' ->
'auth.oauth.autoLink.username') and update the corresponding locale files
(en/fr/ja/ru/vi/zh) to add these keys and translations; also search usages of
AUTO_LINK_POLICY_OPTIONS and any direct references to the old sentence keys and
update them to use the new hierarchical keys so i18n lookups continue to work.
In `@web/default/src/features/system-settings/auth/oauth-section.tsx`:
- Around line 451-455: Replace the hard-coded English sentence passed to the
translation function in the FormDescription (the t(...) call inside the
component rendering the OIDC description) with a hierarchical translation key
like 'auth.oauth.oidc.builtInDescription', and add that key to the project's
locale files (all supported locales) with the original full descriptive text as
the value so the UI shows the same message via
t('auth.oauth.oidc.builtInDescription').
---
Outside diff comments:
In `@oauth/oidc.go`:
- Line 158: GetUserInfo currently decodes the HTTP response with
json.NewDecoder(res.Body).Decode(&oidcUser) which breaks the guideline; replace
that call with the wrapper common.DecodeJson(res.Body, &oidcUser) (matching the
pattern used in ExchangeToken which uses common.Unmarshal) and remove any direct
encoding/json usage from this function so all JSON decoding uses the common
package.
In
`@web/default/src/features/system-settings/auth/custom-oauth/components/provider-form-dialog.tsx`:
- Around line 134-154: The form reset for creating a new provider in the
provider-form-dialog component omits the auto_link_policy field; update the
branch that calls form.reset when props.open && !props.provider to include an
explicit auto_link_policy entry (matching the edited-provider default used when
props.provider exists), so add auto_link_policy alongside fields like auth_style
in the object passed to form.reset to ensure consistent defaults between create
and edit flows.
In `@web/default/src/features/users/components/dialogs/user-binding-dialog.tsx`:
- Around line 250-278: The label fallbacks can be number|string now because
custom_oauth_providers[].id and binding.provider_id are numbers; update the two
label assignments so they always produce strings: in the loop over
customProviders set label to provider.name || String(provider.id), and in the
loop over oauthBindings set label to binding.provider_name ||
String(binding.provider_id). Locate the code that builds the items array (the
customProviders loop and the oauthBindings loop that push into items,
referencing variables customProviders, oauthBindings, oauthBindingMap, items and
the BindingItem.label type) and wrap the numeric fallbacks with String(...)
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0adf57eb-4acf-4757-966f-73d429d41b2d
📒 Files selected for processing (22)
controller/custom_oauth.gocontroller/oauth.gomodel/custom_oauth_provider.gomodel/task_cas_test.gomodel/user.gomodel/user_oauth_binding_delete_test.gooauth/generic.gooauth/oidc.gooauth/redirect_uri.goweb/default/src/features/profile/api.tsweb/default/src/features/profile/components/tabs/account-bindings-tab.tsxweb/default/src/features/system-settings/auth/custom-oauth/components/provider-form-dialog.tsxweb/default/src/features/system-settings/auth/custom-oauth/types.tsweb/default/src/features/system-settings/auth/oauth-section.tsxweb/default/src/features/users/api.tsweb/default/src/features/users/components/dialogs/user-binding-dialog.tsxweb/default/src/i18n/locales/en.jsonweb/default/src/i18n/locales/fr.jsonweb/default/src/i18n/locales/ja.jsonweb/default/src/i18n/locales/ru.jsonweb/default/src/i18n/locales/vi.jsonweb/default/src/i18n/locales/zh.json
- Open the custom OAuth popup synchronously before fetching OAuth state so browsers do not block the binding window - Decode OIDC userinfo responses through common.DecodeJson instead of encoding/json directly - Preserve auto_link_policy defaults when resetting the custom OAuth provider create form - Normalize custom provider IDs to strings when rendering admin binding labels
Author
|
Anyone review? |
|
Is this PR still progressing? |
Author
Done. Some new conflicts have been resolved. |
|
I'm waiting for this PR to be merged. |
|
No one resolved the conflicts? The issue described as “修复自定义 OAuth 在用户中心绑定时错误使用 provider 数字 ID 的问题，改为使用 provider slug” still occurs in version rc.21 |
Author
It's been handled. If needed, you can download the binary from the release on my fork. |
11 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
4 participants
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.
Important
📝 变更描述 / Description
ServerAddressid的问题，改为使用 providerslugstatenoneemail_verifiedusername删除用户时同步清理自定义 OAuth 绑定关系已在 fix(oauth): clear bindings when hard deleting users #5582 修复🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
#5215
✅ 提交前检查项 / Checklist
Bug fix，我已提交或关联对应 Issue，且不会将设计取舍、预期不一致或理解偏差直接归类为 bug。📸 运行证明 / Proof of Work
涉及流程问题, 不太好用截图表示. 已经自测相关修复全部生效
Summary by CodeRabbit
New Features
Bug Fixes
Documentation