fix(auth): create default token for third-party registrations - #4374
Open
iamliuyin wants to merge 3 commits into
Open
fix(auth): create default token for third-party registrations#4374iamliuyin wants to merge 3 commits into
iamliuyin wants to merge 3 commits into
Conversation
Ensure users created through OAuth/OIDC and WeChat registration receive the same default token as password registrations when GENERATE_DEFAULT_TOKEN is enabled. Constraint: Keep returning users from receiving duplicate default tokens on login Rejected: Duplicating token creation logic in each registration path | would keep behavior divergent across auth flows Confidence: high Scope-risk: narrow
Contributor
WalkthroughfindOrCreateOAuthUser now returns a created flag; handlers create a default token for newly created OAuth/WeChat users via createDefaultTokenForUser. Token-generation vs insertion failures are mapped to sentinel errors and surfaced as specific i18n API errors; Register also delegates default-token creation to the new helper. Changes
Sequence DiagramsequenceDiagram
participant Client as Client/OAuth Provider
participant Handler as OAuth/WeChat Handler
participant Finder as findOrCreateOAuthUser
participant TokenHelper as createDefaultTokenForUser
participant DB as Database
Client->>Handler: Auth request (OAuth/WeChat)
Handler->>Finder: findOrCreateOAuthUser(oauthUser, session)
activate Finder
Finder->>DB: Lookup or insert user
alt new user created
Finder-->>Handler: (*User, true, nil)
else existing or other path
Finder-->>Handler: (*User, false, nil) or (nil, false, err)
end
deactivate Finder
alt created == true
Handler->>TokenHelper: createDefaultTokenForUser(user.Id, user.Username)
activate TokenHelper
TokenHelper->>TokenHelper: generate token key
TokenHelper->>DB: insert token
TokenHelper-->>Handler: nil or error (sentinel)
deactivate TokenHelper
alt token error
Handler-->>Client: API error (mapped i18n or raw)
else success
Handler->>Handler: continue status checks / respond success
end
else created == false
Handler->>Handler: continue status checks / respond
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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: 1
🧹 Nitpick comments (2)
controller/user.go (2)
119-152: Considererrors.Isfor sentinel comparisons and document the partial-failure behavior.Two small concerns with the new helper:
- The helper returns the sentinel values directly, so the
err == errGenerateDefaultTokenKeyswitch at Line 234 works today. However, if a future change ever wraps these (e.g.,fmt.Errorf("...: %w", errCreateDefaultToken)), theswitch errcomparisons in all three callers silently fall through tocommon.ApiErrorand the user sees the raw internal error. Prefererrors.Isor aswitchwitherrors.Isguards to be forward-compatible.- If
token.Insert()fails, the user row has already been committed (inRegister,HandleOAuth, andWeChatAuth). The caller returns an API error but the user cannot retry token creation on next login — returning users skipcreateDefaultTokenForUser. Consider either logging this as a recoverable condition and still completing login, or documenting that the admin must manually create a token. At minimum,common.SysLogthe underlyingtoken.Inserterror before returningerrCreateDefaultTokenso operators can diagnose the failure.Suggested diff
if err := token.Insert(); err != nil { + common.SysLog(fmt.Sprintf("failed to insert default token for user %d: %s", userId, err.Error())) return errCreateDefaultToken }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/user.go` around lines 119 - 152, The createDefaultTokenForUser helper currently returns sentinel errors directly and doesn't log the underlying token.Insert failure; change its error handling to use errors.Is in callers by ensuring this function wraps underlying errors (or better: return the original error) and explicitly SysLog the token.Insert error before returning errCreateDefaultToken so operators can diagnose failures; specifically, in createDefaultTokenForUser (function name) call common.SysLog with the actual err from token.Insert (and optionally wrap it) and update callers' error checks to use errors.Is(err, errGenerateDefaultTokenKey) / errors.Is(err, errCreateDefaultToken) rather than direct equality so future wrapping won't break behavior.
233-243: Minor: useerrors.Isand drop redundantreturn.
switch errworks because sentinels are returned unwrapped, butswitch { case errors.Is(err, errGenerateDefaultTokenKey): ... }is more idiomatic in Go and robust against future wrapping. Same applies to the identical blocks incontroller/oauth.go(Lines 122-129) andcontroller/wechat.go(Lines 107-114).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/user.go` around lines 233 - 243, Replace the switch-on-error value with idiomatic errors.Is checks and remove the redundant return: after calling createDefaultTokenForUser (in controller/user.go) check if err != nil then if errors.Is(err, errGenerateDefaultTokenKey) call common.ApiErrorI18n(c, i18n.MsgUserDefaultTokenFailed) else if errors.Is(err, errCreateDefaultToken) call common.ApiErrorI18n(c, i18n.MsgCreateDefaultTokenErr) else call common.ApiError(c, err); do the same change for the identical blocks that reference createDefaultTokenForUser/errGenerateDefaultTokenKey/errCreateDefaultToken in controller/oauth.go and controller/wechat.go so the code is robust to wrapped errors and eliminate the extra return where control already exits after responding.
🤖 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/wechat.go`:
- Around line 106-116: Replace the hardcoded Chinese error messages in
controller/wechat.go where createDefaultTokenForUser is handled: instead of
calling common.ApiErrorMsg for the sentinel errors errGenerateDefaultTokenKey
and errCreateDefaultToken, call common.ApiErrorI18n with the corresponding i18n
keys i18n.MsgUserDefaultTokenFailed and i18n.MsgCreateDefaultTokenErr
respectively, keeping the default branch as common.ApiError(c, err); also add
the import "github.com/QuantumNous/new-api/i18n" to the file imports so the i18n
symbols are available.
---
Nitpick comments:
In `@controller/user.go`:
- Around line 119-152: The createDefaultTokenForUser helper currently returns
sentinel errors directly and doesn't log the underlying token.Insert failure;
change its error handling to use errors.Is in callers by ensuring this function
wraps underlying errors (or better: return the original error) and explicitly
SysLog the token.Insert error before returning errCreateDefaultToken so
operators can diagnose failures; specifically, in createDefaultTokenForUser
(function name) call common.SysLog with the actual err from token.Insert (and
optionally wrap it) and update callers' error checks to use errors.Is(err,
errGenerateDefaultTokenKey) / errors.Is(err, errCreateDefaultToken) rather than
direct equality so future wrapping won't break behavior.
- Around line 233-243: Replace the switch-on-error value with idiomatic
errors.Is checks and remove the redundant return: after calling
createDefaultTokenForUser (in controller/user.go) check if err != nil then if
errors.Is(err, errGenerateDefaultTokenKey) call common.ApiErrorI18n(c,
i18n.MsgUserDefaultTokenFailed) else if errors.Is(err, errCreateDefaultToken)
call common.ApiErrorI18n(c, i18n.MsgCreateDefaultTokenErr) else call
common.ApiError(c, err); do the same change for the identical blocks that
reference
createDefaultTokenForUser/errGenerateDefaultTokenKey/errCreateDefaultToken in
controller/oauth.go and controller/wechat.go so the code is robust to wrapped
errors and eliminate the extra return where control already exits after
responding.
🪄 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: f0a025bb-0693-471a-80d5-6d9e43f7088d
📒 Files selected for processing (3)
controller/oauth.gocontroller/user.gocontroller/wechat.go
Use errors.Is for default token creation failures, log token insert errors for operators, and keep WeChat registration error messaging localized. Constraint: Keep the PR scoped to the third-party registration token fix Rejected: Expanding the flow to auto-retry or silently continue after token creation failure | changes product behavior beyond this bug fix Confidence: high Scope-risk: narrow
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
controller/oauth.go (2)
1-15:⚠️ Potential issue | 🔴 CriticalMissing
errorsimport breaks the build.Lines 123 and 125 call
errors.Is, but theerrorspackage is not in the import block. The package will not compile (confirmed by golangci-lint:undefined: errors).🛠️ Proposed fix
import ( + "errors" "fmt" "net/http" "strconv" "github.com/QuantumNous/new-api/common" "github.com/QuantumNous/new-api/i18n" "github.com/QuantumNous/new-api/model" "github.com/QuantumNous/new-api/oauth" "github.com/gin-contrib/sessions" "github.com/gin-gonic/gin" "gorm.io/gorm" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/oauth.go` around lines 1 - 15, The build breaks because the errors package is used (errors.Is called at the sites that reference it) but not imported in the controller package; add the standard "errors" import to the import block in controller (alongside fmt, net/http, strconv, etc.) so calls to errors.Is compile successfully—look for the usages of errors.Is around the OAuth-related handlers in this file to verify the fix.
120-138:⚠️ Potential issue | 🔴 CriticalAdd missing
"errors"import to enable compilation.The review's analysis of the default-token failure logic is sound: the token creation happens after user persistence, and on retry the token is never recreated since
created=false. This matches password-register behavior. However, the code useserrors.Is()at lines 123 and 125 without importing the"errors"package, which will cause a compilation failure.import ( "errors" "fmt" "net/http" ... )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/oauth.go` around lines 120 - 138, The code in controller/oauth.go uses errors.Is() when checking errors returned from createDefaultTokenForUser (referenced symbols: createDefaultTokenForUser, errGenerateDefaultTokenKey, errCreateDefaultToken) but the "errors" package is not imported; add the standard library import "errors" to the file's import block so the calls to errors.Is compile successfully.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@controller/oauth.go`:
- Around line 1-15: The build breaks because the errors package is used
(errors.Is called at the sites that reference it) but not imported in the
controller package; add the standard "errors" import to the import block in
controller (alongside fmt, net/http, strconv, etc.) so calls to errors.Is
compile successfully—look for the usages of errors.Is around the OAuth-related
handlers in this file to verify the fix.
- Around line 120-138: The code in controller/oauth.go uses errors.Is() when
checking errors returned from createDefaultTokenForUser (referenced symbols:
createDefaultTokenForUser, errGenerateDefaultTokenKey, errCreateDefaultToken)
but the "errors" package is not imported; add the standard library import
"errors" to the file's import block so the calls to errors.Is compile
successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b6cb2b1f-db49-4c15-bdfe-f0b916fa29ab
📒 Files selected for processing (3)
controller/oauth.gocontroller/user.gocontroller/wechat.go
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/wechat.go
Add the standard errors import required by the new errors.Is checks in the OAuth default token flow. Constraint: Keep the follow-up scoped to the review-reported compile issue Confidence: high Scope-risk: narrow
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/oauth.go`:
- Around line 121-133: The handler currently aborts after
createDefaultTokenForUser fails, leaving the committed user without a token;
change this to log the error instead of returning so execution continues to
setupLogin (use processLogger or common.Logger to record the err from
createDefaultTokenForUser), and also make setupLogin (or the login flow)
idempotently attempt to create a default token when a returning user has zero
tokens by calling createDefaultTokenForUser when token count == 0 (so failures
are retried on subsequent logins); update any error handling around
createDefaultTokenForUser to distinguish fatal vs non-fatal paths and ensure
created/GENERATE_DEFAULT_TOKEN logic remains consistent.
🪄 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: cd2ce456-97b3-4570-bfe4-6bdc3801102b
📒 Files selected for processing (1)
controller/oauth.go
Comment on lines
+121
to
+133
| if created { | ||
| if err := createDefaultTokenForUser(user.Id, user.Username); err != nil { | ||
| switch { | ||
| case errors.Is(err, errGenerateDefaultTokenKey): | ||
| common.ApiErrorI18n(c, i18n.MsgUserDefaultTokenFailed) | ||
| case errors.Is(err, errCreateDefaultToken): | ||
| common.ApiErrorI18n(c, i18n.MsgCreateDefaultTokenErr) | ||
| default: | ||
| common.ApiError(c, err) | ||
| } | ||
| return | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Newly created user is left without a default token if token creation fails.
If createDefaultTokenForUser fails, the handler returns an error response, but the user row (and OAuth binding) has already been committed in step 7. On the user's next login, created will be false, so the default token creation path will never run again — that user is permanently missing their default token despite GENERATE_DEFAULT_TOKEN=true.
Consider one of:
- Log the token-creation failure and continue to
setupLogin(degraded but recoverable — user can create a token manually), or - Additionally trigger default-token creation on subsequent logins when the user has zero tokens, so the failure is self-healing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/oauth.go` around lines 121 - 133, The handler currently aborts
after createDefaultTokenForUser fails, leaving the committed user without a
token; change this to log the error instead of returning so execution continues
to setupLogin (use processLogger or common.Logger to record the err from
createDefaultTokenForUser), and also make setupLogin (or the login flow)
idempotently attempt to create a default token when a returning user has zero
tokens by calling createDefaultTokenForUser when token count == 0 (so failures
are retried on subsequent logins); update any error handling around
createDefaultTokenForUser to distinguish fatal vs non-fatal paths and ensure
created/GENERATE_DEFAULT_TOKEN logic remains consistent.
Calcium-Ion
force-pushed
the
main
branch
2 times, most recently
from
August 30, 2026 15:03
51fdfc5 to
2b6f1df
Compare
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.
Summary
GENERATE_DEFAULT_TOKENis enabledTest plan
GENERATE_DEFAULT_TOKEN=trueand confirm a default token is createdGENERATE_DEFAULT_TOKEN=trueand confirm a default token is createdGENERATE_DEFAULT_TOKEN=trueand confirm a default token is createdSummary by CodeRabbit
New Features
Bug Fixes