Open
Conversation
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 new "JWT-direct" custom OAuth provider path (ticket-exchange/validation, JWKS/PEM verification, userinfo parsing), a JWT login/bind controller and API route, expands custom provider schema/validation/registry handling, refactors session identity syncing and bind/login flows, adds frontend support and many tests across layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Browser as Browser (cookies)
participant Controller as JWT Login Controller
participant Session as Session Store
participant Provider as JWTDirectProvider
participant DB as Database
participant Auditor as Audit Logger
Client->>Controller: POST /api/auth/external/:provider/jwt/login (state, token/ticket)
Controller->>Session: validate state == session.oauth_state
Controller->>Provider: ResolveIdentityFromInput(token, ticket, callbackURL, state)
alt Ticket exchange/validate
Provider->>Provider: HTTP exchange/validate (external ticket endpoint)
Provider->>Provider: extract/normalize token or claims
else Direct token
Provider->>Provider: parse & verify JWT (PEM/JWKS)
end
Provider->>Controller: resolved identity (external id, user info, groups/roles)
alt existing session user
Controller->>DB: bind OAuth identity to session user
Controller->>Auditor: record bind audit
Controller-->>Client: {action: "bind", success:true}
else new or merged user
Controller->>DB: findOrCreate user (auto-register/email-merge)
Controller->>DB: sync group/role if enabled
Controller->>Auditor: record login audit
Controller-->>Client: {action: "login", success:true}
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 12
🧹 Nitpick comments (7)
oauth/jwt_direct.go (1)
3-30: Replace directencoding/jsonusage withcommonwrappers.Line 11 imports
encoding/jsondirectly, and it's used at lines 544 and 569. Per coding guidelines, all JSON operations should use the wrapper functions incommon/json.go.Proposed fix
Remove the direct import:
import ( "bytes" "context" "crypto/ecdsa" "crypto/ed25519" "crypto/elliptic" "crypto/rsa" "encoding/base64" - "encoding/json" "encoding/xml"Update the usages:
- if err := json.Unmarshal(body, &payload); err != nil { + if err := common.Unmarshal(body, &payload); err != nil { return nil, fmt.Errorf("parse ticket validation json failed: %w", err)- var payload map[string]any - if err := json.Unmarshal(body, &payload); err == nil { + var payload any + if err := common.Unmarshal(body, &payload); err == nil { if str, ok := payload.(string); ok {As per coding guidelines: "All JSON marshal/unmarshal operations MUST use wrapper functions in
common/json.go."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oauth/jwt_direct.go` around lines 3 - 30, The file jwt_direct.go currently imports encoding/json and uses json.Marshal/json.Unmarshal; remove the direct encoding/json import and replace those calls with the project JSON wrapper functions from common/json.go (use the common package's marshal/unmarshal helpers — e.g., common.MarshalJSON/common.UnmarshalJSON or the actual wrapper names in common/json.go) at the places where json.Marshal and json.Unmarshal are invoked in jwt_direct.go, updating error handling to match the wrapper signatures and keeping the common package import.middleware/auth_test.go (1)
3-20: UsecommonJSON wrappers instead ofencoding/json.The test file imports
encoding/jsondirectly (line 4) and uses it for JSON decoding (lines 155, 179). Per coding guidelines, all JSON operations should use the wrapper functions incommon/json.go.Proposed fix
import ( - "encoding/json" "fmt" "net/http"Then update the JSON decoding calls:
- if err := json.NewDecoder(response.Body).Decode(&payload); err != nil { + if err := common.DecodeJson(response.Body, &payload); err != nil { t.Fatalf("failed to decode session response: %v", err) }- if err := json.NewDecoder(response.Body).Decode(&payload); err != nil { + if err := common.DecodeJson(response.Body, &payload); err != nil { t.Fatalf("failed to decode api response: %v", err) }As per coding guidelines: "All JSON marshal/unmarshal operations MUST use wrapper functions in
common/json.go."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/auth_test.go` around lines 3 - 20, Remove the direct import of "encoding/json" and switch to the JSON wrapper helpers provided in the common package (the helpers from common/json.go). Replace any usage of encoding/json functions such as json.Unmarshal and json.NewDecoder(...).Decode with the corresponding common helpers (use the common package's JSON decode/unmarshal functions) in the test file, and update the imports to include the common package instead of encoding/json so all JSON operations use the common wrapper.oauth/registry.go (1)
130-134: Consider adding a log when non-oauth_codeproviders are removed in incremental updates.The delete-and-return path is correct, but currently silent; a short log would improve traceability during provider kind transitions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oauth/registry.go` around lines 130 - 134, When config.IsOAuthCode() returns false and you remove the provider from providers and customProviderSlugs, add a short log message that records the provider slug and that it was removed due to non-oauth_code kind; e.g., call the module's logger (or log.Printf) before the deletes to emit something like "removing provider <config.Slug>: not oauth_code" so incremental updates are traceable for config.IsOAuthCode(), providers, customProviderSlugs, and config.Slug.oauth/registry_test.go (1)
9-28: Cover the unregister-on-update branch too.This test starts from an empty registry, so it never exercises the
delete(...)path inRegisterOrUpdateCustomProviderwhen an existingoauth_codeprovider is updated tojwt_direct. A regression there would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@oauth/registry_test.go` around lines 9 - 28, The test currently only covers adding a jwt_direct provider from an empty registry; extend TestRegisterOrUpdateCustomProviderSkipsJWTDirect to also exercise the “unregister-on-update” branch by first registering a provider with Kind set to model.CustomOAuthProviderKindOAuthCode (using RegisterOrUpdateCustomProvider with the same slug), asserting it exists via GetProvider/IsCustomProvider, then calling RegisterOrUpdateCustomProvider again with Kind model.CustomOAuthProviderKindJWTDirect and asserting the provider is removed (GetProvider returns nil and IsCustomProvider is false); keep UnregisterCustomProvider cleanup calls to reset state.controller/misc.go (1)
133-136: Move the provider DB lookup out ofOptionMapRWMutex.
GetEnabledCustomOAuthProviders()does DB I/O while the read lock taken at Line 43 is still held. That makes every/statusrequest keepcommon.OptionMapwriters blocked behind database latency. Fetch the providers before taking the lock, or narrow the lock to the actual map reads.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/misc.go` around lines 133 - 136, The DB call GetEnabledCustomOAuthProviders() is being made while holding common.OptionMapRWMutex (read lock) which can block writers; move the call to GetEnabledCustomOAuthProviders() so it runs before acquiring OptionMapRWMutex, or alternatively reduce the locked section to only the map access (i.e., call GetEnabledCustomOAuthProviders() first, then lock OptionMapRWMutex only around reads of common.OptionMap and any use of customProviders), ensuring symbols mentioned (GetEnabledCustomOAuthProviders, customProviders, common.OptionMapRWMutex, common.OptionMap) are updated accordingly.controller/bind_session_test.go (1)
92-105: Assert that unauthenticated WeChat binds never hit the upstream server.Right now the mock always returns success, so this test still passes if
WeChatBindcontacts WeChat before checking the session. Tracking zero upstream requests would lock in the intended “fail before external I/O” behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/bind_session_test.go` around lines 92 - 105, The test must assert that an unauthenticated WeChat bind does not contact the upstream server: change the httptest.NewServer handler for wechatServer to record/atomically count incoming requests (e.g., increment an int32 or send to a channel) instead of just returning success, keep common.WeChatAuthEnabled, common.WeChatServerAddress and common.WeChatServerToken as-is, call performBindSessionTestRequest(...) and then assert the recorded request count is zero; reference the wechatServer handler and performBindSessionTestRequest so the assertion guarantees WeChatBind didn't call the upstream before session checks.web/src/components/settings/CustomOAuthSetting.jsx (1)
1378-1396: Consider simplifying the nested ternary for readability.The rules definition uses nested ternaries that could be flattened.
Suggested simplification
<Form.Input field='issuer' label={t('Issuer')} placeholder='https://issuer.example.com' rules={ - isJWTUserInfoMode - ? [] - : isJWTTicketValidateMode - ? [] - : [{ required: true, message: t('请输入 Issuer') }] + (isJWTUserInfoMode || isJWTTicketValidateMode) + ? [] + : [{ required: true, message: t('请输入 Issuer') }] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/CustomOAuthSetting.jsx` around lines 1378 - 1396, The nested ternary in the Form.Input for field 'issuer' (in CustomOAuthSetting.jsx) is hard to read; extract the rules logic into a small boolean or helper (e.g., compute a variable like issuerRules or a function shouldRequireIssuer using isJWTUserInfoMode and isJWTTicketValidateMode) and pass that variable to the rules prop instead of the nested ternary, keeping the existing behavior (empty array when either isJWTUserInfoMode or isJWTTicketValidateMode is true, otherwise [{ required: true, message: t('请输入 Issuer') }]); update the extraText usage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/custom_oauth_jwt_test.go`:
- Around line 3-28: Remove the direct encoding/json import from
controller/custom_oauth_jwt_test.go and replace all usages of
json.NewDecoder().Decode(...) (in the helper functions that parse JSON
responses) with the project's wrapper common.DecodeJson(...); if the code uses
json.RawMessage, switch to common.RawMessage if available otherwise keep the
type but still parse via common.DecodeJson; update the imports to drop
encoding/json and ensure helper functions (the ones at the JSON decode call
sites) call common.DecodeJson with the same target types.
In `@controller/custom_oauth_jwt.go`:
- Around line 118-119: The audit success is being recorded before the session is
actually persisted; move the call to recordCustomOAuthJWTAudit(audit) so it runs
only after setupLogin(result.User, c) completes successfully (i.e., after
setupLogin returns without error and the session save inside it has succeeded).
Update the flow in the function containing recordCustomOAuthJWTAudit and
setupLogin to call setupLogin(result.User, c) first, check its error/return
status, and then call recordCustomOAuthJWTAudit(audit) only on success; keep the
same audit variable and context (audit, result.User, c) so the audit entry
remains identical when moved.
- Around line 67-74: The JWT-login business failures (e.g., invalid oauth_state
and other 400/403 branches) must be returned in the normal JSON envelope so
submitJWTLogin() doesn’t treat them as transport errors and retry; change the
HTTP status for these error responses from non-2xx (e.g., http.StatusForbidden /
http.StatusBadRequest) to http.StatusOK while keeping the JSON body
(success:false, message: i18n.T(...)) and still calling
recordCustomOAuthJWTAudit(audit). Update the error-response sites in this file
(the invalid state branch around the state/session check and the other 400/403
branch around lines ~125-130) to return 200 with the existing JSON payload
instead of 4xx.
In `@controller/custom_oauth.go`:
- Around line 41-48: The provider response is exposing sensitive JSON blobs
TicketExchangeHeaders and TicketExchangeExtraParams; update the serialization so
toCustomOAuthProviderResponse() (and any code paths used for read/create/update
responses) omits TicketExchangeHeaders and TicketExchangeExtraParams from
returned structs while still preserving them in storage; locate references to
the struct fields TicketExchangeHeaders and TicketExchangeExtraParams and change
the response mapping to exclude them (or explicitly whitelist safe fields) so
admin UI/read APIs no longer receive those secrets.
- Line 409: The update path currently ignores empty client_secret so it can
never be cleared; change the request model's ClientSecret from string to *string
(ClientSecret *string `json:"client_secret"`) and update the code that applies
the secret (the block that currently checks the ClientSecret string for
non-emptiness) to check for nil vs non-nil: if ClientSecret != nil then assign
the stored secret to *ClientSecret (allowing an empty string to clear it), and
if ClientSecret == nil then leave the existing secret untouched; update any
validation/serialization accordingly so API callers can send null (to keep) or
an explicit "" (to clear).
In `@middleware/auth.go`:
- Around line 126-133: The response for a failed syncSessionUserIdentity uses
http.StatusOK (200) which is inconsistent with other auth failures; change the
response status to http.StatusUnauthorized (401) and keep the same JSON body and
abort logic so it matches the handling used for loadSessionUserIdentity
failures; update the HTTP status in the block that calls
syncSessionUserIdentity(session, user) (the same branch that currently returns
200) so all authentication/authorization failures return 401.
In `@oauth/registry.go`:
- Around line 108-111: The summary log still reports the total DB rows even
after skipping configs that return false from config.IsOAuthCode(), causing a
misleading loaded-provider count; fix by introducing a loadedCount (or increment
an existing counter) inside the loop only when a provider is actually registered
(i.e., after the continue is not hit) and use that loadedCount in the final
summary log instead of the total DB rows, referencing the existing
config.IsOAuthCode(), config.Name and config.Slug skip logic so the increment
happens where providers are truly loaded.
In `@web/src/components/auth/LoginForm.jsx`:
- Around line 135-137: The code calls .some and .filter on
status.custom_oauth_providers which may not be an array; guard these accesses
with Array.isArray checks: for the browser-login check (the expression using
(status.custom_oauth_providers || []).some(...)) update it to first test
Array.isArray(status.custom_oauth_providers) and only call .some when true
(otherwise return false), and likewise for the other block that uses
status.custom_oauth_providers.filter(...) (only call .filter when
Array.isArray(...) is true, otherwise treat as empty array/result); reference
the symbol status.custom_oauth_providers and the functions using .some/.filter
to locate and fix both occurrences.
In `@web/src/components/auth/OAuth2Callback.jsx`:
- Around line 170-201: The branch handling providerKind === 'jwt_direct' only
treats jwt_acquire_mode === 'ticket_exchange' as a ticket callback; update the
jwtAcquireMode check to also treat 'ticket_validate' as a ticket callback so
ticket params are extracted and handled correctly. Specifically, in the
jwtAcquireMode conditional around pickFirstParamValue([... 'ticket','st']) and
submitJWTLogin({ state, ticket }), change the condition to include
'ticket_validate' (e.g. jwtAcquireMode === 'ticket_exchange' || jwtAcquireMode
=== 'ticket_validate') so that pickFirstParamValue, the ticket presence check,
showError/navigation, and submitJWTLogin are executed for both modes; leave the
JWT token extraction path (pickFirstParamValue([...
'id_token','token','jwt','access_token']) and submitJWTLogin({ state, id_token:
jwtToken })) unchanged.
In `@web/src/components/auth/RegisterForm.jsx`:
- Around line 133-135: Guard against non-array shapes for
status.custom_oauth_providers before calling array methods: replace direct uses
like (status.custom_oauth_providers || []).some(...) and .filter(...) with a
safe check/coercion such as const customProviders =
Array.isArray(status.custom_oauth_providers) ? status.custom_oauth_providers :
[]; then call customProviders.some(...) and customProviders.filter(...). Update
all occurrences in RegisterForm (references: status.custom_oauth_providers, the
some/filter usages) to use this Array.isArray guard so malformed but truthy
values won't throw.
In `@web/src/helpers/api.js`:
- Around line 55-71: The helper supportsCustomProviderBrowserLogin currently
only treats jwt_acquire_mode === 'ticket_exchange' as the ticket flow; update
the condition(s) in supportsCustomProviderBrowserLogin (and the similar block
around the other occurrence) to treat both 'ticket_exchange' and
'ticket_validate' as ticket-based flows so that providers with jwt_acquire_mode
=== 'ticket_validate' will return true when authorization_endpoint is present;
locate getCustomProviderKind(provider) usage and change the check from ===
'ticket_exchange' to check for either mode (e.g., provider?.jwt_acquire_mode ===
'ticket_exchange' || provider?.jwt_acquire_mode === 'ticket_validate') so the
browser login and authorize URL generation follow the ticket flow for both
modes.
- Around line 75-79: The hard-coded Chinese error strings in
web/src/helpers/api.js should be localized; replace direct Error messages (e.g.,
the throws for missing/invalid auth endpoint URL and the other occurrences noted
at lines 103-108 and 398-430) to use the shared i18n instance or return
translation keys instead of literal text. Update the helper (the validation
function that throws these Errors) to call the shared i18n translate function
(or throw an object containing a stable translation key like
"error.authEndpoint.missing"/"error.authEndpoint.invalid") and ensure callers or
UI components resolve the key via react-i18next so locale switching works
consistently. Ensure all other similar throws in this file follow the same
pattern.
---
Nitpick comments:
In `@controller/bind_session_test.go`:
- Around line 92-105: The test must assert that an unauthenticated WeChat bind
does not contact the upstream server: change the httptest.NewServer handler for
wechatServer to record/atomically count incoming requests (e.g., increment an
int32 or send to a channel) instead of just returning success, keep
common.WeChatAuthEnabled, common.WeChatServerAddress and
common.WeChatServerToken as-is, call performBindSessionTestRequest(...) and then
assert the recorded request count is zero; reference the wechatServer handler
and performBindSessionTestRequest so the assertion guarantees WeChatBind didn't
call the upstream before session checks.
In `@controller/misc.go`:
- Around line 133-136: The DB call GetEnabledCustomOAuthProviders() is being
made while holding common.OptionMapRWMutex (read lock) which can block writers;
move the call to GetEnabledCustomOAuthProviders() so it runs before acquiring
OptionMapRWMutex, or alternatively reduce the locked section to only the map
access (i.e., call GetEnabledCustomOAuthProviders() first, then lock
OptionMapRWMutex only around reads of common.OptionMap and any use of
customProviders), ensuring symbols mentioned (GetEnabledCustomOAuthProviders,
customProviders, common.OptionMapRWMutex, common.OptionMap) are updated
accordingly.
In `@middleware/auth_test.go`:
- Around line 3-20: Remove the direct import of "encoding/json" and switch to
the JSON wrapper helpers provided in the common package (the helpers from
common/json.go). Replace any usage of encoding/json functions such as
json.Unmarshal and json.NewDecoder(...).Decode with the corresponding common
helpers (use the common package's JSON decode/unmarshal functions) in the test
file, and update the imports to include the common package instead of
encoding/json so all JSON operations use the common wrapper.
In `@oauth/jwt_direct.go`:
- Around line 3-30: The file jwt_direct.go currently imports encoding/json and
uses json.Marshal/json.Unmarshal; remove the direct encoding/json import and
replace those calls with the project JSON wrapper functions from common/json.go
(use the common package's marshal/unmarshal helpers — e.g.,
common.MarshalJSON/common.UnmarshalJSON or the actual wrapper names in
common/json.go) at the places where json.Marshal and json.Unmarshal are invoked
in jwt_direct.go, updating error handling to match the wrapper signatures and
keeping the common package import.
In `@oauth/registry_test.go`:
- Around line 9-28: The test currently only covers adding a jwt_direct provider
from an empty registry; extend TestRegisterOrUpdateCustomProviderSkipsJWTDirect
to also exercise the “unregister-on-update” branch by first registering a
provider with Kind set to model.CustomOAuthProviderKindOAuthCode (using
RegisterOrUpdateCustomProvider with the same slug), asserting it exists via
GetProvider/IsCustomProvider, then calling RegisterOrUpdateCustomProvider again
with Kind model.CustomOAuthProviderKindJWTDirect and asserting the provider is
removed (GetProvider returns nil and IsCustomProvider is false); keep
UnregisterCustomProvider cleanup calls to reset state.
In `@oauth/registry.go`:
- Around line 130-134: When config.IsOAuthCode() returns false and you remove
the provider from providers and customProviderSlugs, add a short log message
that records the provider slug and that it was removed due to non-oauth_code
kind; e.g., call the module's logger (or log.Printf) before the deletes to emit
something like "removing provider <config.Slug>: not oauth_code" so incremental
updates are traceable for config.IsOAuthCode(), providers, customProviderSlugs,
and config.Slug.
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 1378-1396: The nested ternary in the Form.Input for field 'issuer'
(in CustomOAuthSetting.jsx) is hard to read; extract the rules logic into a
small boolean or helper (e.g., compute a variable like issuerRules or a function
shouldRequireIssuer using isJWTUserInfoMode and isJWTTicketValidateMode) and
pass that variable to the rules prop instead of the nested ternary, keeping the
existing behavior (empty array when either isJWTUserInfoMode or
isJWTTicketValidateMode is true, otherwise [{ required: true, message: t('请输入
Issuer') }]); update the extraText usage unchanged.
🪄 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: 08412f6e-9eab-4985-b0de-cac63d6c2c83
📒 Files selected for processing (26)
controller/bind_session_test.gocontroller/custom_oauth.gocontroller/custom_oauth_jwt.gocontroller/custom_oauth_jwt_test.gocontroller/custom_oauth_update_test.gocontroller/misc.gocontroller/oauth.gocontroller/telegram.gocontroller/user.gocontroller/wechat.gomiddleware/auth.gomiddleware/auth_test.gomodel/custom_oauth_provider.gooauth/jwt_direct.gooauth/jwt_direct_test.gooauth/provider.gooauth/registry.gooauth/registry_test.gorouter/api-router.goweb/src/components/auth/LoginForm.jsxweb/src/components/auth/OAuth2Callback.jsxweb/src/components/auth/RegisterForm.jsxweb/src/components/settings/CustomOAuthSetting.jsxweb/src/components/settings/personal/cards/AccountManagement.jsxweb/src/components/table/tokens/modals/EditTokenModal.jsxweb/src/helpers/api.js
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/settings/CustomOAuthSetting.jsx (1)
164-212:⚠️ Potential issue | 🟡 MinorUse Chinese source keys for the new translations.
These additions pass English strings to
t()(Query,OAuth 2.0 / OIDC Code Flow,OAuth Code,Configuration, etc.). In this repo that won’t line up with the locale JSON structure, so the new UI text will stay untranslated or create a second key style.As per coding guidelines, "Translation files in
web/src/i18n/locales/{lang}.jsonmust be flat JSON with Chinese source strings as keys. UseuseTranslation()hook and callt('中文key')in components."Also applies to: 258-294, 779-785, 929-953
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/CustomOAuthSetting.jsx` around lines 164 - 212, The option and label constants (CUSTOM_OAUTH_KIND_OPTIONS, JWT_SOURCE_OPTIONS, JWT_ACQUIRE_MODE_OPTIONS, JWT_IDENTITY_MODE_OPTIONS, TICKET_EXCHANGE_METHOD_OPTIONS, TICKET_EXCHANGE_PAYLOAD_MODE_OPTIONS, JWT_MAPPING_MODE_OPTIONS, and DISCOVERY_FIELD_LABELS) currently pass English strings directly to t(); change each label value to the corresponding Chinese source key used in the locale JSON and update component usage to call t('中文键名') instead of t('English string') so translations resolve correctly (ensure each new Chinese key matches the keys in web/src/i18n/locales/{lang}.json and update any occurrences referenced later in the file at the noted ranges as well).
🧹 Nitpick comments (4)
controller/misc.go (1)
43-46: Cache the enabled-provider payload instead of querying it inGetStatus.
GetStatusis a bootstrap/public endpoint, andmodel.GetEnabledCustomOAuthProviders()is a direct database query. That makes every status load depend on the DB for data that only changes on provider CRUD. Consider caching the serialized provider list and invalidating it on create/update/delete/reload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/misc.go` around lines 43 - 46, GetStatus currently calls model.GetEnabledCustomOAuthProviders() (assigning to customProviders) on every request; change this to read a cached serialized provider payload instead and only fall back to loading+serializing once on cache miss. Implement a small in-memory cache (or use existing cache utility) keyed for the enabled-provider payload, store the serialized result the controller expects, and return that from GetStatus. Ensure all provider lifecycle operations (create/update/delete/reload functions) clear or refresh the same cache key so subsequent GetStatus calls see updates without hitting the DB on every bootstrap/public request.controller/custom_oauth_jwt.go (2)
56-62: Use i18n for user-facing error messages.The error message on line 60 is hardcoded in Chinese. For consistency with the rest of the controller (which uses
common.ApiErrorI18n), consider using an i18n key.♻️ Proposed fix
var req customOAuthJWTLoginRequest if err := c.ShouldBind(&req); err != nil { audit.FailureReason = "invalid_request" recordCustomOAuthJWTAudit(audit) - common.ApiErrorMsg(c, "无效的请求参数: "+err.Error()) + common.ApiErrorI18n(c, i18n.MsgInvalidRequestParams) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/custom_oauth_jwt.go` around lines 56 - 62, Replace the hardcoded Chinese error string in the c.ShouldBind error path with an i18n call: in the block where req is bound (customOAuthJWTLoginRequest) and on error you already set audit.FailureReason and call recordCustomOAuthJWTAudit, call common.ApiErrorI18n(c, "error.invalid_request_params", map[string]interface{}{"detail": err.Error()}) (or the project's existing i18n key pattern) instead of common.ApiErrorMsg so the user-facing message uses the translation system; keep the audit and return behavior unchanged.
147-159: Hardcoded Chinese error messages in validation logic.The error messages on lines 152 and 158 are hardcoded in Chinese. These errors are passed through
handleCustomOAuthJWTLoginErrorand may be displayed to users. Consider using i18n-compatible error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/custom_oauth_jwt.go` around lines 147 - 159, The validation branch in the login flow uses hardcoded Chinese error strings (fmt.Errorf("未提供票据") and fmt.Errorf("未提供 JWT 令牌")) which are passed into handleCustomOAuthJWTLoginError; replace these with i18n-aware errors or error constants and return a machine-readable error value (e.g., an error variable or code) that handleCustomOAuthJWTLoginError can translate for display. Update the providerConfig.RequiresTicketAcquire() branch and the rawToken check to return standardized errors (or call your localization helper) instead of literal Chinese text, and ensure any audit.FailureReason remains unchanged so the audit uses the same failure keys.web/src/components/auth/OAuth2Callback.jsx (1)
85-88: RedundantlocalStorage.setItemcall.Line 86 manually sets
localStorage.setItem('user', ...), butsetUserData(data)on line 87 already performs the same operation. Remove the redundant call.♻️ Proposed fix
userDispatch({ type: 'login', payload: data }); - localStorage.setItem('user', JSON.stringify(data)); setUserData(data); updateAPI();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/auth/OAuth2Callback.jsx` around lines 85 - 88, Remove the redundant localStorage write: the explicit localStorage.setItem('user', JSON.stringify(data)) call is unnecessary because setUserData(data) already persists the user; in the OAuth2 callback flow, keep userDispatch({ type: 'login', payload: data }); call setUserData(data); and then call updateAPI(); but delete the localStorage.setItem(...) line so only setUserData handles storage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/custom_oauth_jwt.go`:
- Around line 64-71: Replace the unsafe type assertion on
session.Get("oauth_state") with a safe comma-ok check: retrieve session :=
sessions.Default(c), read state := strings.TrimSpace(req.State) as before, then
do val, ok := session.Get("oauth_state").(string) and treat any non-string or
missing value as invalid (set audit.FailureReason = "invalid_state", call
recordCustomOAuthJWTAudit(audit), call common.ApiErrorI18n(c,
i18n.MsgOAuthStateInvalid) and return). Ensure you also compare state != val
only when ok is true so corrupted/non-string session values don't cause a panic.
In `@middleware/auth_test.go`:
- Around line 35-60: The helper setupAuthMiddlewareTestDB mutates package
globals (model.DB, model.LOG_DB and
common.UsingSQLite/UsingMySQL/UsingPostgreSQL/common.RedisEnabled) but only
closes the temporary DB; capture the previous values before changing them and
restore them in the t.Cleanup closure. Specifically, save oldDB := model.DB,
oldLogDB := model.LOG_DB and the prior booleans for common.UsingSQLite,
common.UsingMySQL, common.UsingPostgreSQL, common.RedisEnabled at the start of
setupAuthMiddlewareTestDB, then in the existing t.Cleanup first close the temp
sqlite connection and then set model.DB = oldDB, model.LOG_DB = oldLogDB and
restore the common.* flags so later tests see the original state.
In `@middleware/auth.go`:
- Around line 33-42: The current loadSessionUserIdentity function always calls
model.GetUserById, causing a DB lookup on every authenticated request; change it
to fetch only the minimal identity fields (e.g., id, username, role, status,
group) via a lightweight method (e.g., model.GetUserIdentityById or a new method
that selects only those columns) or implement a short-lived cache keyed by
session id with explicit invalidation on revoke; update loadSessionUserIdentity
to call that new identity-only function or cache lookup instead of
model.GetUserById to avoid the heavy hot-path DB read while preserving
revocation semantics.
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 421-442: The current setFormValues call spreads ...provider last
which lets empty legacy provider fields overwrite the computed defaults (e.g.,
jwt_identity_mode, jwt_acquire_mode, authorization_service_field,
ticket_exchange_*), so move the spread to be first (i.e., spread provider before
the explicit normalized fields) in the setFormValues invocation to ensure the
explicit fallback/normalized values (in setFormValues) take precedence over
possibly empty properties on provider; adjust the setFormValues call around the
provider object to place ...provider at the beginning (and optionally filter
undefined/null if needed) so normalized values in this form are not clobbered by
legacy empty strings.
---
Outside diff comments:
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 164-212: The option and label constants
(CUSTOM_OAUTH_KIND_OPTIONS, JWT_SOURCE_OPTIONS, JWT_ACQUIRE_MODE_OPTIONS,
JWT_IDENTITY_MODE_OPTIONS, TICKET_EXCHANGE_METHOD_OPTIONS,
TICKET_EXCHANGE_PAYLOAD_MODE_OPTIONS, JWT_MAPPING_MODE_OPTIONS, and
DISCOVERY_FIELD_LABELS) currently pass English strings directly to t(); change
each label value to the corresponding Chinese source key used in the locale JSON
and update component usage to call t('中文键名') instead of t('English string') so
translations resolve correctly (ensure each new Chinese key matches the keys in
web/src/i18n/locales/{lang}.json and update any occurrences referenced later in
the file at the noted ranges as well).
---
Nitpick comments:
In `@controller/custom_oauth_jwt.go`:
- Around line 56-62: Replace the hardcoded Chinese error string in the
c.ShouldBind error path with an i18n call: in the block where req is bound
(customOAuthJWTLoginRequest) and on error you already set audit.FailureReason
and call recordCustomOAuthJWTAudit, call common.ApiErrorI18n(c,
"error.invalid_request_params", map[string]interface{}{"detail": err.Error()})
(or the project's existing i18n key pattern) instead of common.ApiErrorMsg so
the user-facing message uses the translation system; keep the audit and return
behavior unchanged.
- Around line 147-159: The validation branch in the login flow uses hardcoded
Chinese error strings (fmt.Errorf("未提供票据") and fmt.Errorf("未提供 JWT 令牌")) which
are passed into handleCustomOAuthJWTLoginError; replace these with i18n-aware
errors or error constants and return a machine-readable error value (e.g., an
error variable or code) that handleCustomOAuthJWTLoginError can translate for
display. Update the providerConfig.RequiresTicketAcquire() branch and the
rawToken check to return standardized errors (or call your localization helper)
instead of literal Chinese text, and ensure any audit.FailureReason remains
unchanged so the audit uses the same failure keys.
In `@controller/misc.go`:
- Around line 43-46: GetStatus currently calls
model.GetEnabledCustomOAuthProviders() (assigning to customProviders) on every
request; change this to read a cached serialized provider payload instead and
only fall back to loading+serializing once on cache miss. Implement a small
in-memory cache (or use existing cache utility) keyed for the enabled-provider
payload, store the serialized result the controller expects, and return that
from GetStatus. Ensure all provider lifecycle operations
(create/update/delete/reload functions) clear or refresh the same cache key so
subsequent GetStatus calls see updates without hitting the DB on every
bootstrap/public request.
In `@web/src/components/auth/OAuth2Callback.jsx`:
- Around line 85-88: Remove the redundant localStorage write: the explicit
localStorage.setItem('user', JSON.stringify(data)) call is unnecessary because
setUserData(data) already persists the user; in the OAuth2 callback flow, keep
userDispatch({ type: 'login', payload: data }); call setUserData(data); and then
call updateAPI(); but delete the localStorage.setItem(...) line so only
setUserData handles storage.
🪄 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: 107c31e5-9955-4885-8781-374785cd5325
📒 Files selected for processing (18)
controller/bind_session_test.gocontroller/custom_oauth.gocontroller/custom_oauth_jwt.gocontroller/custom_oauth_jwt_test.gocontroller/custom_oauth_update_test.gocontroller/misc.gocontroller/user.gocontroller/wechat.gomiddleware/auth.gomiddleware/auth_test.gooauth/jwt_direct.gooauth/registry.gooauth/registry_test.goweb/src/components/auth/LoginForm.jsxweb/src/components/auth/OAuth2Callback.jsxweb/src/components/auth/RegisterForm.jsxweb/src/components/settings/CustomOAuthSetting.jsxweb/src/helpers/api.js
✅ Files skipped from review due to trivial changes (2)
- web/src/components/auth/LoginForm.jsx
- controller/custom_oauth_jwt_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- web/src/components/auth/RegisterForm.jsx
- controller/bind_session_test.go
- oauth/registry_test.go
- controller/custom_oauth.go
- controller/custom_oauth_update_test.go
- oauth/jwt_direct.go
Contributor
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/components/settings/CustomOAuthSetting.jsx (1)
507-510:⚠️ Potential issue | 🟡 MinorUse field labels here, not internal keys.
fieldis values likeclient_idandauthorization_endpoint, so this toast shows untranslated implementation names andt()can't resolve stable locale entries. Map the required fields to the existing human-readable labels before building the message.As per coding guidelines, translation files in
web/src/i18n/locales/{lang}.jsonmust be flat JSON with Chinese source strings as keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/CustomOAuthSetting.jsx` around lines 507 - 510, The toast currently displays internal keys (from requiredFields/currentValues) like client_id which are not user-friendly or translatable; update the validation in the loop that calls showError (the block iterating over requiredFields) to map each internal field key to its human-readable label (use the component's existing label mapping or add a fieldLabels object) and call showError(t('请填写 {fieldLabel}', { fieldLabel: fieldLabels[field] } )) or equivalent so the message uses the localized/human label instead of the raw key; ensure the string passed to t() is the flat Chinese source string required by web/src/i18n/locales/*.json.
♻️ Duplicate comments (2)
middleware/auth_test.go (1)
34-72:⚠️ Potential issue | 🟡 MinorRestore Gin mode in cleanup.
gin.SetMode(gin.TestMode)is process-global. Leaving it set after this helper returns leaks test-only Gin state into later tests.💡 Suggested fix
func setupAuthMiddlewareTestDB(t *testing.T) { t.Helper() + oldGinMode := gin.Mode() oldDB := model.DB oldLogDB := model.LOG_DB @@ t.Cleanup(func() { + gin.SetMode(oldGinMode) sqlDB, err := db.DB() if err == nil { _ = sqlDB.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/auth_test.go` around lines 34 - 72, The helper setupAuthMiddlewareTestDB sets gin.SetMode(gin.TestMode) but never restores the previous Gin mode; capture the current mode before changing (e.g., oldGinMode := gin.Mode()), and in the t.Cleanup closure restore it with gin.SetMode(oldGinMode) alongside the existing cleanup steps so that setupAuthMiddlewareTestDB leaves Gin's global mode unchanged after tests.web/src/components/settings/CustomOAuthSetting.jsx (1)
548-550:⚠️ Potential issue | 🟠 MajorThe edit flow still can’t express “clear this
client_secret”.The backend now distinguishes
null(keep existing) from""(clear), but the form still starts blank with “留空则保持原有密钥” semantics and this submit branch strips empty strings. That leaves no path for an admin to intentionally remove a stored secret, including after switching a provider tojwt_direct. Track field dirtiness or add an explicit clear action so""only reaches the API when the user really chose to clear it.Also applies to: 1165-1188
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/CustomOAuthSetting.jsx` around lines 548 - 550, The current submit branch in CustomOAuthSetting.jsx removes payload.client_secret when editingProvider and the input is an empty string, preventing an admin from intentionally clearing a stored secret; change this by tracking whether the client_secret field was modified (e.g., introduce a clientSecretDirty flag or an explicit "clear secret" control) and use that flag in the submit logic instead of payload.client_secret === ''. Update the code that currently checks editingProvider and payload.client_secret (the delete payload.client_secret branch) to only delete the key when the field was not dirtied/left untouched, and send an explicit empty string ("" ) when the user intentionally cleared the secret (or send null/omit otherwise) so the backend can distinguish keep vs clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/custom_oauth_jwt.go`:
- Around line 175-177: Replace direct use of identity.User.ProviderUserID when
assigning audit fields with a redacted or hashed representation: compute a
deterministic, non-reversible identifier (e.g., HMAC-SHA256 or a one-way hash
with a server-side salt) via a helper like redactOAuthID(...) and assign that to
audit.ExternalID (and any other audit fields currently set from
identity.User.ProviderUserID in the later block covering the other occurrences).
Update places using audit.ExternalID in SysLog/RecordLog to persist the
redacted/hash value so raw PII is never written to logs or DB; keep use of
safeOAuthAuditValue(identity.Group) and oauthRoleLabel(identity.Role) unchanged.
Ensure the helper is deterministic so records for the same provider ID can be
correlated without exposing the plain value.
In `@web/src/components/auth/OAuth2Callback.jsx`:
- Around line 134-148: The !success branch in submitJWTLogin currently only
shows a toast and returns, leaving the OAuth2Callback page stuck on the <Loading
/> spinner; update that branch in submitJWTLogin (the function that calls
handleCallbackSuccess and uses MAX_RETRIES, retry) to immediately
showError(message || t('授权失败')), then navigate away (e.g.,
navigate('/console/personal')) and return to stop further processing; ensure
this behavior mirrors the catch branch so business-level failures
(success:false) do not hang the callback spinner and do not trigger retries.
---
Outside diff comments:
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 507-510: The toast currently displays internal keys (from
requiredFields/currentValues) like client_id which are not user-friendly or
translatable; update the validation in the loop that calls showError (the block
iterating over requiredFields) to map each internal field key to its
human-readable label (use the component's existing label mapping or add a
fieldLabels object) and call showError(t('请填写 {fieldLabel}', { fieldLabel:
fieldLabels[field] } )) or equivalent so the message uses the localized/human
label instead of the raw key; ensure the string passed to t() is the flat
Chinese source string required by web/src/i18n/locales/*.json.
---
Duplicate comments:
In `@middleware/auth_test.go`:
- Around line 34-72: The helper setupAuthMiddlewareTestDB sets
gin.SetMode(gin.TestMode) but never restores the previous Gin mode; capture the
current mode before changing (e.g., oldGinMode := gin.Mode()), and in the
t.Cleanup closure restore it with gin.SetMode(oldGinMode) alongside the existing
cleanup steps so that setupAuthMiddlewareTestDB leaves Gin's global mode
unchanged after tests.
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 548-550: The current submit branch in CustomOAuthSetting.jsx
removes payload.client_secret when editingProvider and the input is an empty
string, preventing an admin from intentionally clearing a stored secret; change
this by tracking whether the client_secret field was modified (e.g., introduce a
clientSecretDirty flag or an explicit "clear secret" control) and use that flag
in the submit logic instead of payload.client_secret === ''. Update the code
that currently checks editingProvider and payload.client_secret (the delete
payload.client_secret branch) to only delete the key when the field was not
dirtied/left untouched, and send an explicit empty string ("" ) when the user
intentionally cleared the secret (or send null/omit otherwise) so the backend
can distinguish keep vs clear.
🪄 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: a2f6f61c-706c-4507-a9b7-2daba69f8e36
📒 Files selected for processing (14)
common/json.gocontroller/custom_oauth.gocontroller/custom_oauth_jwt.gocontroller/custom_oauth_jwt_test.gocontroller/misc.goi18n/keys.goi18n/locales/en.yamli18n/locales/zh-CN.yamli18n/locales/zh-TW.yamlmiddleware/auth.gomiddleware/auth_test.gomodel/user.goweb/src/components/auth/OAuth2Callback.jsxweb/src/components/settings/CustomOAuthSetting.jsx
✅ Files skipped from review due to trivial changes (6)
- common/json.go
- i18n/locales/en.yaml
- i18n/locales/zh-CN.yaml
- i18n/locales/zh-TW.yaml
- i18n/keys.go
- controller/custom_oauth_jwt_test.go
Contributor
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/components/settings/CustomOAuthSetting.jsx (1)
509-512:⚠️ Potential issue | 🟡 MinorAvoid dynamic translation keys in required-field errors
t(\请填写 ${field}`)generates runtime keys (e.g.,请填写 issuer`) and can show untranslated/internal field IDs to users. Use a static i18n key plus a localized field-label map.Suggested fix
+ const REQUIRED_FIELD_LABELS = { + name: '显示名称', + slug: '标识符 (Slug)', + client_id: '客户端 ID', + client_secret: '客户端密钥', + authorization_endpoint: '授权端点', + token_endpoint: '令牌端点', + user_info_endpoint: '用户信息端点', + issuer: '发行者', + }; ... for (const field of requiredFields) { if (!currentValues[field]) { - showError(t(`请填写 ${field}`)); + const fieldLabel = REQUIRED_FIELD_LABELS[field] || field; + showError(t('请填写 {{field}}', { field: t(fieldLabel) })); return; } }As per coding guidelines
web/src/**/*.{ts,tsx,js,jsx}: "UseuseTranslation()hook and callt('中文key')in components."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/CustomOAuthSetting.jsx` around lines 509 - 512, The validation currently builds dynamic translation keys with t(`请填写 ${field}`) which may expose internal IDs; instead create a static i18n key for the required-field message and a localized field-label map, then use the static key with a translated field label. Update the block that iterates requiredFields (referencing requiredFields, currentValues, showError, and t) to look up a human-friendly label for each field (e.g., fieldLabelMap[field] translated via t or using t('issuer_label')) and call t with the fixed message key (e.g., t('请填写字段', { field: translatedLabel })) before calling showError. Ensure useTranslation() is used in the component and remove any string interpolation inside t(...) so all keys are static.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 500-506: The check that ensures ticket_exchange_url is present for
acquireMode values 'ticket_exchange'/'ticket_validate' is missing URL format
validation, so update the validation in the CustomOAuthSetting component (the
branch using acquireMode and currentValues.ticket_exchange_url) to also verify
ticket_exchange_url is a well-formed HTTP/HTTPS URL (e.g., use the URL
constructor or a strict regex) before proceeding; if the URL is invalid, call
showError(t('票据处理模式必须填写有效的 Ticket Processing URL')) and return. Apply the same
format validation to the other ticket-mode block (the logic referenced around
lines 516-526) so both code paths validate presence and scheme before
continuing.
- Around line 1102-1104: The label prop for the slug field is hardcoded as
label='Slug'; replace it with a localized string using the component's i18n
instance (e.g., label={t('your中文key_for_slug')}) and ensure the component calls
useTranslation() (const { t } = useTranslation()) and imports it if not already
present; update the locale files to include the matching Chinese source key for
the slug label so the t(...) lookup resolves.
---
Outside diff comments:
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 509-512: The validation currently builds dynamic translation keys
with t(`请填写 ${field}`) which may expose internal IDs; instead create a static
i18n key for the required-field message and a localized field-label map, then
use the static key with a translated field label. Update the block that iterates
requiredFields (referencing requiredFields, currentValues, showError, and t) to
look up a human-friendly label for each field (e.g., fieldLabelMap[field]
translated via t or using t('issuer_label')) and call t with the fixed message
key (e.g., t('请填写字段', { field: translatedLabel })) before calling showError.
Ensure useTranslation() is used in the component and remove any string
interpolation inside t(...) so all keys are static.
🪄 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: 239095f0-ee8e-41ec-8698-3f03bc16e652
📒 Files selected for processing (1)
web/src/components/settings/CustomOAuthSetting.jsx
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/components/settings/CustomOAuthSetting.jsx (1)
579-616:⚠️ Potential issue | 🟠 MajorSwitching away from JWT mode does not actually clear the stored JWT config.
controller/custom_oauth.goonly updates pointer fields that are present in the request. Deleting these keys here preserves the old DB values, and several hidden JWT-only fields (jwt_source,jwt_header,issuer, mappings, sync flags, etc.) are still sent anyway. After toggling back tojwt_direct, stale ticket/JWKS/group-role settings can silently reappear.💡 Suggested fix
- if (providerKind !== 'jwt_direct') { - delete payload.jwt_identity_mode; - delete payload.jwt_acquire_mode; - delete payload.authorization_service_field; - delete payload.ticket_exchange_url; - delete payload.ticket_exchange_method; - delete payload.ticket_exchange_payload_mode; - delete payload.ticket_exchange_ticket_field; - delete payload.ticket_exchange_token_field; - delete payload.ticket_exchange_service_field; - delete payload.ticket_exchange_extra_params; - delete payload.ticket_exchange_headers; - } + if (providerKind !== 'jwt_direct') { + Object.assign(payload, { + issuer: '', + audience: '', + jwks_url: '', + public_key: '', + jwt_source: '', + jwt_header: '', + jwt_identity_mode: '', + jwt_acquire_mode: '', + authorization_service_field: '', + ticket_exchange_url: '', + ticket_exchange_method: '', + ticket_exchange_payload_mode: '', + ticket_exchange_ticket_field: '', + ticket_exchange_token_field: '', + ticket_exchange_service_field: '', + ticket_exchange_extra_params: '', + ticket_exchange_headers: '', + group_field: '', + role_field: '', + group_mapping: '', + role_mapping: '', + auto_register: false, + auto_merge_by_email: false, + sync_group_on_login: false, + sync_role_on_login: false, + group_mapping_mode: '', + role_mapping_mode: '', + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/CustomOAuthSetting.jsx` around lines 579 - 616, When switching away from jwt_direct you must send explicit nulls for JWT-specific pointer fields so the controller will clear them; instead of deleting keys from payload in the providerKind !== 'jwt_direct' branch, set each JWT/ticket field to null (e.g. jwt_identity_mode, jwt_acquire_mode, authorization_service_field, ticket_exchange_url, ticket_exchange_method, ticket_exchange_payload_mode, ticket_exchange_ticket_field, ticket_exchange_token_field, ticket_exchange_service_field, ticket_exchange_extra_params, ticket_exchange_headers) and also ensure hidden JWT fields and config like jwt_source, jwt_header, issuer, any mapping/sync flags and group/role mappings are set to null when editingProvider and providerKind !== 'jwt_direct' so the server receives present-but-null pointers and will clear DB values in controller/custom_oauth.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/custom_oauth_jwt_test.go`:
- Around line 46-75: setupCustomOAuthJWTControllerTestDB mutates global state
(model.DB, model.LOG_DB and common.* flags) without restoring them; capture the
previous values at the start of the function and restore them in the existing
t.Cleanup closure after closing the sqlite handle. Specifically, save the
current model.DB and model.LOG_DB and each flag: common.UsingSQLite,
common.UsingMySQL, common.UsingPostgreSQL, common.RedisEnabled,
common.RegisterEnabled, common.QuotaForNewUser, common.QuotaForInvitee,
common.QuotaForInviter, then in t.Cleanup assign those saved values back so
later tests don't inherit the modified/closed DB or altered flags.
In `@controller/custom_oauth_jwt.go`:
- Around line 84-89: Replace direct assignments of audit.FailureReason =
err.Error() with a bounded redacted value (e.g., a short machine-readable code
plus minimal safe message) produced by a helper (create or reuse a helper such
as RedactAuditFailure(err) that returns a safe string), and keep the existing
calls to recordCustomOAuthJWTAudit(audit) and handleCustomOAuthJWTLoginError(c,
err) unchanged; update every occurrence in this file where audit.FailureReason
is set from err (including the sites around
recordCustomOAuthJWTAudit/handleCustomOAuthJWTLoginError and the other
occurrences noted) so the audit trail never contains raw upstream error text or
payloads.
- Around line 74-82: The code currently picks the JWT credential using a fixed
order (firstNonEmpty(req.Token, req.IDToken, req.JWT)), which forces token-first
and breaks claims mode; add a helper like
selectJWTLoginCredential(providerConfig, req) that checks
providerConfig.GetJWTIdentityMode() (compare to
model.CustomJWTIdentityModeUserInfo) and returns firstNonEmpty(req.Token,
req.IDToken, req.JWT) for userinfo mode or firstNonEmpty(req.IDToken, req.JWT,
req.Token) for claims mode, then call completeCustomOAuthJWTLogin using that
helper instead of the fixed firstNonEmpty call to ensure id_token/jwt are
preferred when appropriate.
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 818-824: The new Chinese strings introduced in
CustomOAuthSetting.jsx (e.g., the Tag labels rendered in render(kind): 'JWT 直连'
and 'OAuth 授权码', plus other strings like '添加身份提供商' and the new banner copy
around lines where the component renders) are not present in the zh-TW locale
JSON, causing fallback to Simplified Chinese; run the i18n toolchain (bun run
i18n:extract, bun run i18n:sync, then bun run i18n:lint) to extract and sync
these flat JSON keys into web/src/i18n/locales/zh-TW.json, ensuring every new
t('...') key from CustomOAuthSetting.jsx (and the block around 883-910) is added
with proper zh-TW translations so the UI no longer falls back.
---
Outside diff comments:
In `@web/src/components/settings/CustomOAuthSetting.jsx`:
- Around line 579-616: When switching away from jwt_direct you must send
explicit nulls for JWT-specific pointer fields so the controller will clear
them; instead of deleting keys from payload in the providerKind !== 'jwt_direct'
branch, set each JWT/ticket field to null (e.g. jwt_identity_mode,
jwt_acquire_mode, authorization_service_field, ticket_exchange_url,
ticket_exchange_method, ticket_exchange_payload_mode,
ticket_exchange_ticket_field, ticket_exchange_token_field,
ticket_exchange_service_field, ticket_exchange_extra_params,
ticket_exchange_headers) and also ensure hidden JWT fields and config like
jwt_source, jwt_header, issuer, any mapping/sync flags and group/role mappings
are set to null when editingProvider and providerKind !== 'jwt_direct' so the
server receives present-but-null pointers and will clear DB values in
controller/custom_oauth.go.
🪄 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: 17d55034-da14-44a3-a488-3deee19a5f94
📒 Files selected for processing (7)
controller/custom_oauth_jwt.gocontroller/custom_oauth_jwt_test.goweb/src/components/auth/OAuth2Callback.jsxweb/src/components/settings/CustomOAuthSetting.jsxweb/src/i18n/locales/en.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/i18n/locales/zh-TW.json
🚧 Files skipped from review as they are similar to previous changes (3)
- web/src/i18n/locales/zh-CN.json
- web/src/i18n/locales/en.json
- web/src/components/auth/OAuth2Callback.jsx
Author
Fixes Applied SuccessfullyFixed 5 file(s) based on 4 unresolved CodeRabbit review comment(s). Files modified:
Commit: Validation:
Note:
The latest autofix changes are on the |
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
controller/custom_oauth_jwt.go (1)
107-111:⚠️ Potential issue | 🟠 MajorRoute deferred bind failures through the JWT-login error mapper.
Line 111 bypasses
handleCustomOAuthJWTLoginError(c, err). IfbindOAuthIdentityToUser()returnsOAuthAlreadyBoundErroror another OAuth business error, this branch skips the dedicated i18n/error mapping used everywhere else in this handler and falls back to the generic API error path instead.↩️ Proposed fix
if result.BindAfterStatusCheck { if err := bindOAuthIdentityToUser(result.User, provider, result.ProviderUserID); err != nil { audit.FailureReason = oauthAuditFailureReason(err) recordCustomOAuthJWTAudit(audit) - common.ApiError(c, err) + handleCustomOAuthJWTLoginError(c, err) return } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/custom_oauth_jwt.go` around lines 107 - 111, The branch that handles deferred binding (when result.BindAfterStatusCheck is true) calls bindOAuthIdentityToUser and on error currently sets audit fields, records the audit via recordCustomOAuthJWTAudit and calls common.ApiError(c, err), which bypasses the localized/business error mapping; replace the common.ApiError call with a call to handleCustomOAuthJWTLoginError(c, err) so OAuth business errors (e.g., OAuthAlreadyBoundError) are mapped and translated consistently while keeping the existing audit update via oauthAuditFailureReason and recordCustomOAuthJWTAudit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/custom_oauth_jwt.go`:
- Around line 164-169: The callback URL for ticket modes is currently derived
from request headers via buildCustomOAuthJWTCallbackURL when calling
ResolveIdentityFromInput, which allows caller-controlled values; change this to
require and validate a configured external base URL (ServerAddress) and fail
closed for ticket_exchange/ticket_validate: update
buildCustomOAuthJWTCallbackURL to accept and validate ServerAddress (parse and
ensure a well-formed base URL) and return an error if empty/malformed, stop
using X-Forwarded-* / Request.Host as fallback, and in the controller where
ResolveIdentityFromInput is called (the ResolveIdentityFromInput(...)
invocation) propagate and handle that error by returning a failure for ticket
modes instead of deriving the callback from request headers. Ensure
ResolveIdentityFromInput gets only the validated callback URL and that any code
paths referenced for ticket modes enforce the same validation.
---
Duplicate comments:
In `@controller/custom_oauth_jwt.go`:
- Around line 107-111: The branch that handles deferred binding (when
result.BindAfterStatusCheck is true) calls bindOAuthIdentityToUser and on error
currently sets audit fields, records the audit via recordCustomOAuthJWTAudit and
calls common.ApiError(c, err), which bypasses the localized/business error
mapping; replace the common.ApiError call with a call to
handleCustomOAuthJWTLoginError(c, err) so OAuth business errors (e.g.,
OAuthAlreadyBoundError) are mapped and translated consistently while keeping the
existing audit update via oauthAuditFailureReason and recordCustomOAuthJWTAudit.
🪄 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: 4668100e-b1d1-4514-a836-dfebd7a733c4
📒 Files selected for processing (5)
controller/custom_oauth_jwt.gocontroller/custom_oauth_jwt_test.goweb/src/i18n/locales/en.jsonweb/src/i18n/locales/zh-CN.jsonweb/src/i18n/locales/zh-TW.json
✅ Files skipped from review due to trivial changes (4)
- web/src/i18n/locales/zh-CN.json
- web/src/i18n/locales/zh-TW.json
- controller/custom_oauth_jwt_test.go
- web/src/i18n/locales/en.json
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/custom_oauth_jwt.go`:
- Around line 192-207: When handling the existing-session bind path, ensure you
check the user's account status immediately after getSessionUser(c) and before
calling bindOAuthIdentityToCurrentUser so disabled users cannot link identities;
specifically, after currentUser, currentUserErr := getSessionUser(c) and before
bindOAuthIdentityToCurrentUser(c, provider, identity.User) add a status gate
that sets audit.FailureReason via oauthAuditFailureReason (when audit != nil)
and returns the appropriate error/result if the user is disabled or locked. Keep
existing audit field handling (audit.Action = "bind", audit.TargetUserID =
currentUser.Id) only after the status check passes.
- Around line 107-112: The delayed bind branch uses common.ApiError which
bypasses the OAuth-specific error formatting; after bindOAuthIdentityToUser(...)
returns an error (including OAuthAlreadyBoundError), set audit.FailureReason =
oauthAuditFailureReason(err) and recordCustomOAuthJWTAudit(audit) as you do now,
but replace the common.ApiError(c, err) call with the OAuth error handler (e.g.
handleOAuthError(c, err) or the existing OAuth-specific handler your controller
uses elsewhere) so OAuth-specific errors are returned with the correct
localized/business response, then return.
🪄 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: c08bc5bb-1d34-4620-80a7-785c6c246ac9
📒 Files selected for processing (2)
controller/custom_oauth_jwt.gocontroller/custom_oauth_jwt_test.go
✅ Files skipped from review due to trivial changes (1)
- controller/custom_oauth_jwt_test.go
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
1 participant
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.
📝 变更描述 / Description
ticket_exchange与ticket_validate两种票据换 JWT 模式。claims/userinfo两类身份解析与映射。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
📸 运行证明 / Proof of Work
go test ./oauth ./controller ./middleware -count=1cd web && bun run buildgit diff --check