feat(oauth): implement custom OAuth provider - #2857
Conversation
- Introduce Provider interface pattern for standard OAuth protocols - Create unified controller/oauth.go with common OAuth logic - Add OAuthError type for translatable error messages - Add i18n keys and translations (zh/en) for OAuth messages - Use common.ApiErrorI18n/ApiSuccessI18n for consistent responses - Preserve backward compatibility for existing routes and data
- Add support for custom OAuth providers, including creation, retrieval, updating, and deletion. - Introduce new model and controller for managing custom OAuth providers. - Enhance existing OAuth logic to accommodate custom providers. - Update API routes for custom OAuth provider management. - Include i18n support for custom OAuth-related messages.
WalkthroughAdds a unified, pluggable OAuth system: provider interface and registry, built-in and generic providers, DB-backed custom provider and binding models, centralized OAuth controller (login/bind), admin CRUD APIs and frontend UI for managing and binding custom OAuth providers; removes legacy provider-specific controllers. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Frontend as Frontend(LoginForm)
participant Server as AppServer(controller/oauth.go)
participant Provider as OAuth Provider (oauth/*)
participant DB
User->>Frontend: click provider button
Frontend->>Server: GET /oauth/state (create CSRF state)
Server->>DB: persist session state
Server-->>Frontend: return state
Frontend->>Browser: redirect to provider authorization_endpoint (client_id, redirect_uri, state)
Provider-->>Browser: callback redirect with code & state
Browser->>Server: GET /oauth/:provider?code=...&state=...
Server->>Server: validate state & route (bind vs login)
Server->>Provider: ExchangeToken(code) -> POST token_endpoint
Provider-->>Server: access_token
Server->>Provider: GetUserInfo(access_token)
Provider-->>Server: user info JSON
Server->>DB: find/create user and create/update UserOAuthBinding (transactional)
Server-->>Browser: set session / respond success
Browser->>User: logged in or binding confirmed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 14
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/auth/OAuth2Callback.jsx (1)
71-80:⚠️ Potential issue | 🟡 MinorPreserve error metadata when reporting failures.
showErrorexpects an error object for richer handling. Passing only the message drops that context.🔧 Suggested fix
- showError(error.message || t('授权失败')); + showError(error?.message ? error : new Error(t('授权失败')));
🤖 Fix all issues with AI agents
In `@controller/custom_oauth.go`:
- Around line 298-303: The call to model.GetBindingCountByProviderId(id)
currently ignores its error return so failures default count to 0 and allow
deletion; change the call to capture (count, err) :=
model.GetBindingCountByProviderId(id) and if err != nil log the error and return
an API error (e.g., via common.ApiErrorMsg(c, "...") or similar) instead of
proceeding, otherwise continue to check count > 0; ensure you reference
GetBindingCountByProviderId, the returned err variable, and use
common.ApiErrorMsg to surface the failure to the client.
In `@controller/oauth.go`:
- Around line 56-64: The state validation currently does an unchecked type
assertion session.Get("oauth_state").(string) which can panic if the stored
value isn't a string; update the check in the handler that reads state and
session.Get("oauth_state") to use the safe comma-ok type assertion (e.g., val,
ok := session.Get("oauth_state").(string)), treat missing or non-string
oauth_state as invalid (return the same 403 JSON response), and ensure the
subsequent comparison uses the validated val variable (refer to variables state
and the session.Get("oauth_state") call in controller/oauth.go).
- Around line 236-245: The current username generation (user.Username =
provider.GetProviderPrefix() + strconv.Itoa(model.GetMaxUserId()+1)) can race
under concurrent OAuth registrations; modify the OAuth registration flow in
controller/oauth.go to handle duplicate-username constraint errors by adding
retry logic: wrap the user creation/save call in a small retry loop (e.g., 3-5
attempts) that regenerates user.Username using a fresh model.GetMaxUserId() or
appends an incremental suffix (e.g., _1, _2) when a unique-constraint/database
error is returned, and only return the error after retries are exhausted;
alternatively, switch to a DB-side auto-increment/sequence for user IDs if
available and update the username construction to use that generated ID (refer
to provider.GetProviderPrefix(), model.GetMaxUserId(), user.Username, and the
save/create function where constraint errors are currently handled).
- Around line 164-172: Guard the session id extraction to avoid a panic: instead
of directly using id.(int) in the model.User{Id: ...} construction, retrieve id
:= session.Get("id"), check for nil and perform a safe type assertion like
value, ok := id.(int); if not ok or id is nil, return a suitable API error via
common.ApiError(c, err) (or construct a new error) before calling
user.FillUserById(); then create user := model.User{Id: value} and call
user.FillUserById() as before.
In `@model/custom_oauth_provider.go`:
- Around line 110-157: The validateCustomOAuthProvider function currently
doesn't check provider.AuthStyle, so invalid values outside the supported range
(0–2) can slip through; update validateCustomOAuthProvider to validate or
normalize provider.AuthStyle (the AuthStyle field on CustomOAuthProvider) by
ensuring it's within the allowed values (0,1,2) and return an error if not (or
coerce to a default such as 0), and include this check near the other
required-field validations (after checking TokenEndpoint/UserInfoEndpoint) to
prevent invalid configs breaking token exchange.
- Around line 99-107: The IsSlugTaken function currently ignores DB errors from
query.Count(&count) which can allow slug conflicts on DB failures; update
IsSlugTaken (which uses DB and CustomOAuthProvider) to capture and check the
result of query.Count(&count) (e.g., res := query.Count(&count)), and if
res.Error != nil return true (fail-closed), otherwise return count > 0; preserve
the existing excludeId logic when building the query.
In `@model/user_oauth_binding.go`:
- Around line 9-18: The UserOAuthBinding struct lacks DB-level composite unique
constraints; update the struct tags on the fields UserId, ProviderId, and
ProviderUserId to add GORM composite unique indexes (e.g. add `gorm:"index;not
null;uniqueIndex:ux_user_provider"` on UserId and ProviderId for the (user_id,
provider_id) constraint and `gorm:"type:varchar(256);not
null;uniqueIndex:ux_provider_provideruserid"` on ProviderId and ProviderUserId
for the (provider_id, provider_user_id) constraint), ensuring the same
uniqueIndex names are used on the paired fields (UserId+ProviderId and
ProviderId+ProviderUserId) so the DB enforces uniqueness under concurrency and
then regenerate/run migrations.
- Around line 57-83: IsProviderUserIdTaken currently ignores DB errors and
CreateUserOAuthBinding relies on a racy pre-check; update IsProviderUserIdTaken
to capture and return the Count() error (or return false with an error) so
callers can fail-closed, add a composite unique index tag to the
UserOAuthBinding struct fields (ProviderId and ProviderUserId) using GORM's
`uniqueIndex` on both fields to enforce uniqueness at the DB level, and simplify
CreateUserOAuthBinding to stop relying on the race-prone pre-check — attempt
DB.Create(binding), set CreatedAt, and propagate the DB error, detecting and
returning a clear error when the insert fails due to the unique constraint
(unique violation).
- Around line 85-108: UpdateUserOAuthBinding currently omits input validation
for newProviderUserId and treats any DB error as "not found"; add a guard to
return an error if newProviderUserId is empty (same validation as
CreateUserOAuthBinding), and replace naive err checks with explicit gorm
ErrRecordNotFound handling: when querying existingBinding and binding use
errors.Is(err, gorm.ErrRecordNotFound) to detect absence and otherwise return
the DB error; only proceed to create a new binding or update provider_user_id on
the binding when the record truly doesn't exist or exists, respectively, and
ensure you reference DB, UserOAuthBinding, UpdateUserOAuthBinding and
provider_user_id in the fixes.
In `@oauth/discord.go`:
- Around line 49-98: The ExchangeToken function currently logs the raw
authorization code (logger.LogDebug line with "[OAuth-Discord] ExchangeToken:
code=%s..." using code and min), which risks credential leakage; remove or
redact the code before logging (e.g., log only a fixed-length hash, the code
length, or a masked substring like "*****..."), update the logger call in
ExchangeToken to use the redacted value (and ensure no other logs in this
function print the raw code), and keep the rest of the token exchange logic
unchanged.
In `@oauth/generic.go`:
- Around line 56-115: The ExchangeToken flow currently logs sensitive values
(the authorization code variable `code` and the raw response body `bodyStr`) via
`logger.LogDebug`; remove or redact those values in the `logger.LogDebug` calls
in `ExchangeToken` (references: `code`, `bodyStr`, `p.config.Slug`,
`p.config.TokenEndpoint`) and instead log non-sensitive metadata such as
response status, response length (e.g. len(body)), or a fixed redacted string;
also ensure any other `logger.LogDebug` calls in this function (and the similar
calls around lines noted) do not print `code`, tokens, or raw user info.
In `@oauth/github.go`:
- Around line 123-135: Before decoding the GitHub response in GetUserInfo, check
res.StatusCode and if it's not http.StatusOK (200) log the status and response
body and return a provider-specific OAuth error via NewOAuthError (include
Provider: "GitHub" and the status/code or body in the error metadata) instead of
attempting to json.Decode into gitHubUser; keep the existing decode/error
handling only for 200 responses and use res.Body read/rewind or io.ReadAll to
capture error details for logging and the returned error.
In `@oauth/linuxdo.go`:
- Around line 45-70: In ExchangeToken, determine the redirect scheme from proxy
headers before falling back to c.Request.TLS by checking headers like
X-Forwarded-Proto (and optionally Forwarded) to build redirectURI so callbacks
work behind TLS-terminating proxies; also stop logging raw auth codes in
logger.LogDebug (redact the code or replace with a constant like "<redacted>")
to avoid leaking sensitive data. Ensure you update the logger.LogDebug call that
currently slices code and the redirect URI construction that uses c.Request.TLS
so they use the forwarded-proto check and redaction.
In `@web/src/components/settings/personal/cards/AccountManagement.jsx`:
- Around line 104-137: loadCustomOAuthBindings currently swallows errors and
handleUnbindCustomOAuth replaces errors with a generic message; update both to
surface real error details to showError so global 401/error handling can run and
stale state is avoided: in loadCustomOAuthBindings catch block call showError
with the actual error text (e.g. error.response?.data?.message || error.message)
and ensure setCustomOAuthBindings([]) or leave unchanged as appropriate; in
handleUnbindCustomOAuth, for non-success responses call
showError(res.data.message) (already present) and in the catch call showError
with the actual error text instead of a generic string, keeping the finally
block to reset setCustomOAuthLoading(providerId) as currently implemented.
🧹 Nitpick comments (7)
controller/misc.go (1)
133-155: Sort custom OAuth providers for deterministic ordering.
If the registry iteration order changes, the response order can fluctuate and cause UI churn. Sorting the payload by slug keeps the response stable.🔧 Suggested change (stable ordering)
import ( "encoding/json" "fmt" "net/http" + "sort" "strings" @@ providersInfo := make([]CustomOAuthInfo, 0, len(customProviders)) for _, p := range customProviders { config := p.GetConfig() providersInfo = append(providersInfo, CustomOAuthInfo{ Name: config.Name, Slug: config.Slug, ClientId: config.ClientId, AuthorizationEndpoint: config.AuthorizationEndpoint, Scopes: config.Scopes, }) } + sort.Slice(providersInfo, func(i, j int) bool { + return providersInfo[i].Slug < providersInfo[j].Slug + }) data["custom_oauth_providers"] = providersInfo }web/src/components/auth/LoginForm.jsx (1)
594-609: Consider adding a null/empty array check before mapping.While the
&&short-circuit handlesundefinedandnull, an explicit length check would improve defensive coding and intent clarity.♻️ Optional defensive check
- {status.custom_oauth_providers && - status.custom_oauth_providers.map((provider) => ( + {status.custom_oauth_providers?.length > 0 && + status.custom_oauth_providers.map((provider) => (web/src/components/settings/CustomOAuthSetting.jsx (2)
149-151: Consider addingfetchProvidersto the dependency array or usinguseCallback.The current implementation works (runs once on mount), but ESLint's
react-hooks/exhaustive-depsrule may warn. Consider either disabling the lint rule for this line or wrappingfetchProvidersinuseCallback.♻️ Option 1: Disable lint rule
useEffect(() => { fetchProviders(); + // eslint-disable-next-line react-hooks/exhaustive-deps }, []);
206-211: Error message interpolation bypasses i18n translation.The template literal embeds the field name directly, which won't be translated for non-Chinese users.
♻️ Proposed fix for i18n-compliant error messages
for (const field of requiredFields) { if (!formValues[field]) { - showError(t(`请填写 ${field}`)); + showError(t('请填写必填字段') + ': ' + field); return; } }Alternatively, create a mapping of field names to translation keys for fully localized field names.
controller/oauth.go (1)
263-280: Consider using a transaction for user creation and binding.If
CreateUserOAuthBindingfails afteruser.Insertsucceeds, the user exists without a binding. While logging is reasonable, consider documenting this as expected behavior or using a transaction.controller/custom_oauth.go (2)
223-262: Inconsistent partial update semantics for boolean and integer fields.String fields are conditionally updated only when non-empty, but
Enabled(line 230) andAuthStyle(line 262) are always overwritten. This means:
- A client sending
{"name": "New Name"}will inadvertently setEnabled=falseandAuthStyle=0- No way to distinguish "field not provided" from "explicitly set to false/0"
Consider using pointer types for optional fields in the request struct to distinguish missing vs explicit zero values:
♻️ Suggested approach using pointer types
// In UpdateCustomOAuthProviderRequest: type UpdateCustomOAuthProviderRequest struct { // ... Enabled *bool `json:"enabled"` AuthStyle *int `json:"auth_style"` // ... } // In UpdateCustomOAuthProvider: if req.Enabled != nil { provider.Enabled = *req.Enabled } if req.AuthStyle != nil { provider.AuthStyle = *req.AuthStyle }
341-353: N+1 query problem: fetching provider for each binding individually.For a user with N bindings, this makes N+1 database queries. Consider using a JOIN in the model layer or batch-fetching all relevant providers in one query.
♻️ Suggested optimization approach
Option 1: Add a model method that returns bindings with provider info via JOIN:
// In model layer: func GetUserOAuthBindingsWithProviders(userId int) ([]BindingWithProvider, error) // In controller: bindings, err := model.GetUserOAuthBindingsWithProviders(userId)Option 2: Batch-fetch providers by IDs:
providerIds := make([]int, len(bindings)) for i, b := range bindings { providerIds[i] = b.ProviderId } providers, _ := model.GetCustomOAuthProvidersByIds(providerIds) providerMap := make(map[int]*model.CustomOAuthProvider) for _, p := range providers { providerMap[p.Id] = p } // Then use providerMap[binding.ProviderId] in the loop
| // 1. Validate state (CSRF protection) | ||
| state := c.Query("state") | ||
| if state == "" || session.Get("oauth_state") == nil || state != session.Get("oauth_state").(string) { | ||
| c.JSON(http.StatusForbidden, gin.H{ | ||
| "success": false, | ||
| "message": i18n.T(c, i18n.MsgOAuthStateInvalid), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add type assertion safety check to prevent potential panic.
If session.Get("oauth_state") returns a non-string value, the type assertion .(string) will panic.
🛡️ Proposed fix for safe type assertion
// 1. Validate state (CSRF protection)
state := c.Query("state")
- if state == "" || session.Get("oauth_state") == nil || state != session.Get("oauth_state").(string) {
+ savedState, ok := session.Get("oauth_state").(string)
+ if state == "" || !ok || state != savedState {
c.JSON(http.StatusForbidden, gin.H{
"success": false,
"message": i18n.T(c, i18n.MsgOAuthStateInvalid),
})
return
}🤖 Prompt for AI Agents
In `@controller/oauth.go` around lines 56 - 64, The state validation currently
does an unchecked type assertion session.Get("oauth_state").(string) which can
panic if the stored value isn't a string; update the check in the handler that
reads state and session.Get("oauth_state") to use the safe comma-ok type
assertion (e.g., val, ok := session.Get("oauth_state").(string)), treat missing
or non-string oauth_state as invalid (return the same 403 JSON response), and
ensure the subsequent comparison uses the validated val variable (refer to
variables state and the session.Get("oauth_state") call in controller/oauth.go).
| // Get current user from session | ||
| session := sessions.Default(c) | ||
| id := session.Get("id") | ||
| user := model.User{Id: id.(int)} | ||
| err = user.FillUserById() | ||
| if err != nil { | ||
| common.ApiError(c, err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Add type assertion safety check to prevent potential panic.
If session.Get("id") returns a non-int value, the type assertion .(int) will panic.
🛡️ Proposed fix for safe type assertion
// Get current user from session
session := sessions.Default(c)
id := session.Get("id")
- user := model.User{Id: id.(int)}
+ userId, ok := id.(int)
+ if !ok {
+ common.ApiError(c, fmt.Errorf("invalid session user id"))
+ return
+ }
+ user := model.User{Id: userId}
err = user.FillUserById()🤖 Prompt for AI Agents
In `@controller/oauth.go` around lines 164 - 172, Guard the session id extraction
to avoid a panic: instead of directly using id.(int) in the model.User{Id: ...}
construction, retrieve id := session.Get("id"), check for nil and perform a safe
type assertion like value, ok := id.(int); if not ok or id is nil, return a
suitable API error via common.ApiError(c, err) (or construct a new error) before
calling user.FillUserById(); then create user := model.User{Id: value} and call
user.FillUserById() as before.
|
|
||
| // Set up new user | ||
| user.Username = provider.GetProviderPrefix() + strconv.Itoa(model.GetMaxUserId()+1) | ||
| if oauthUser.DisplayName != "" { | ||
| user.DisplayName = oauthUser.DisplayName | ||
| } else if oauthUser.Username != "" { | ||
| user.DisplayName = oauthUser.Username | ||
| } else { | ||
| user.DisplayName = provider.GetName() + " User" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check User.Insert method implementation
ast-grep --pattern 'func ($_ *User) Insert($_ *User) $_ {
$$$
}'Repository: QuantumNous/new-api
Length of output: 45
🏁 Script executed:
# Search for Username uniqueness constraints and error handling
rg -n 'Username.*unique|duplicate.*username|UNIQUE.*USERNAME|constraint.*username' --type=go -iRepository: QuantumNous/new-api
Length of output: 168
🏁 Script executed:
# Check the GetMaxUserId function implementation
rg -n 'GetMaxUserId' --type=go -A 5Repository: QuantumNous/new-api
Length of output: 1027
🏁 Script executed:
# Look for User model struct definition, particularly Username field
rg -n 'type User struct' --type=go -A 20Repository: QuantumNous/new-api
Length of output: 2406
🏁 Script executed:
# Check for transaction handling or retry logic in oauth.go around the registration
rg -n 'Insert|transaction|BEGIN|COMMIT|retry' --type=go controller/oauth.go -B 2 -A 2Repository: QuantumNous/new-api
Length of output: 155
🏁 Script executed:
# Check error handling after user.Insert in oauth.go around line 259
rg -n 'user.Insert' controller/oauth.go -A 10 -B 5Repository: QuantumNous/new-api
Length of output: 603
🏁 Script executed:
# Check if there's any retry logic or fallback username generation
rg -n 'for.*retry|goto|retry.*insert' controller/oauth.goRepository: QuantumNous/new-api
Length of output: 45
Add retry logic or database-level ID generation to prevent duplicate username failures under concurrent OAuth registrations.
The race condition is real: concurrent requests can read the same GetMaxUserId() value before either commits, generating identical usernames (e.g., provider_101). The database unique constraint on Username prevents data corruption, but the second registration fails with a constraint violation error instead of succeeding with an alternative username. No retry logic exists in the error handling path (line 259-261).
Consider using database auto-increment or implementing retry logic with a fallback suffix to provide graceful concurrent registration handling.
🤖 Prompt for AI Agents
In `@controller/oauth.go` around lines 236 - 245, The current username generation
(user.Username = provider.GetProviderPrefix() +
strconv.Itoa(model.GetMaxUserId()+1)) can race under concurrent OAuth
registrations; modify the OAuth registration flow in controller/oauth.go to
handle duplicate-username constraint errors by adding retry logic: wrap the user
creation/save call in a small retry loop (e.g., 3-5 attempts) that regenerates
user.Username using a fresh model.GetMaxUserId() or appends an incremental
suffix (e.g., _1, _2) when a unique-constraint/database error is returned, and
only return the error after retries are exhausted; alternatively, switch to a
DB-side auto-increment/sequence for user IDs if available and update the
username construction to use that generated ID (refer to
provider.GetProviderPrefix(), model.GetMaxUserId(), user.Username, and the
save/create function where constraint errors are currently handled).
| func (p *DiscordProvider) ExchangeToken(ctx context.Context, code string, c *gin.Context) (*OAuthToken, error) { | ||
| if code == "" { | ||
| return nil, NewOAuthError(i18n.MsgOAuthInvalidCode, nil) | ||
| } | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: code=%s...", code[:min(len(code), 10)]) | ||
|
|
||
| settings := system_setting.GetDiscordSettings() | ||
| redirectUri := fmt.Sprintf("%s/oauth/discord", system_setting.ServerAddress) | ||
| values := url.Values{} | ||
| values.Set("client_id", settings.ClientId) | ||
| values.Set("client_secret", settings.ClientSecret) | ||
| values.Set("code", code) | ||
| values.Set("grant_type", "authorization_code") | ||
| values.Set("redirect_uri", redirectUri) | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: redirect_uri=%s", redirectUri) | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, "POST", "https://discord.com/api/v10/oauth2/token", strings.NewReader(values.Encode())) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | ||
| req.Header.Set("Accept", "application/json") | ||
|
|
||
| client := http.Client{ | ||
| Timeout: 5 * time.Second, | ||
| } | ||
| res, err := client.Do(req) | ||
| if err != nil { | ||
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Discord] ExchangeToken error: %s", err.Error())) | ||
| return nil, NewOAuthErrorWithRaw(i18n.MsgOAuthConnectFailed, map[string]any{"Provider": "Discord"}, err.Error()) | ||
| } | ||
| defer res.Body.Close() | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken response status: %d", res.StatusCode) | ||
|
|
||
| var discordResponse discordOAuthResponse | ||
| err = json.NewDecoder(res.Body).Decode(&discordResponse) | ||
| if err != nil { | ||
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Discord] ExchangeToken decode error: %s", err.Error())) | ||
| return nil, err | ||
| } | ||
|
|
||
| if discordResponse.AccessToken == "" { | ||
| logger.LogError(ctx, "[OAuth-Discord] ExchangeToken failed: empty access token") | ||
| return nil, NewOAuthError(i18n.MsgOAuthTokenFailed, map[string]any{"Provider": "Discord"}) | ||
| } | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken success: scope=%s", discordResponse.Scope) |
There was a problem hiding this comment.
Do not log authorization codes.
Auth codes can be exchanged for access tokens; logging them risks credential leakage.
🔐 Redact the auth code in logs
-logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: code=%s...", code[:min(len(code), 10)])
+logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: code received")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *DiscordProvider) ExchangeToken(ctx context.Context, code string, c *gin.Context) (*OAuthToken, error) { | |
| if code == "" { | |
| return nil, NewOAuthError(i18n.MsgOAuthInvalidCode, nil) | |
| } | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: code=%s...", code[:min(len(code), 10)]) | |
| settings := system_setting.GetDiscordSettings() | |
| redirectUri := fmt.Sprintf("%s/oauth/discord", system_setting.ServerAddress) | |
| values := url.Values{} | |
| values.Set("client_id", settings.ClientId) | |
| values.Set("client_secret", settings.ClientSecret) | |
| values.Set("code", code) | |
| values.Set("grant_type", "authorization_code") | |
| values.Set("redirect_uri", redirectUri) | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: redirect_uri=%s", redirectUri) | |
| req, err := http.NewRequestWithContext(ctx, "POST", "https://discord.com/api/v10/oauth2/token", strings.NewReader(values.Encode())) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | |
| req.Header.Set("Accept", "application/json") | |
| client := http.Client{ | |
| Timeout: 5 * time.Second, | |
| } | |
| res, err := client.Do(req) | |
| if err != nil { | |
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Discord] ExchangeToken error: %s", err.Error())) | |
| return nil, NewOAuthErrorWithRaw(i18n.MsgOAuthConnectFailed, map[string]any{"Provider": "Discord"}, err.Error()) | |
| } | |
| defer res.Body.Close() | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken response status: %d", res.StatusCode) | |
| var discordResponse discordOAuthResponse | |
| err = json.NewDecoder(res.Body).Decode(&discordResponse) | |
| if err != nil { | |
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Discord] ExchangeToken decode error: %s", err.Error())) | |
| return nil, err | |
| } | |
| if discordResponse.AccessToken == "" { | |
| logger.LogError(ctx, "[OAuth-Discord] ExchangeToken failed: empty access token") | |
| return nil, NewOAuthError(i18n.MsgOAuthTokenFailed, map[string]any{"Provider": "Discord"}) | |
| } | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken success: scope=%s", discordResponse.Scope) | |
| func (p *DiscordProvider) ExchangeToken(ctx context.Context, code string, c *gin.Context) (*OAuthToken, error) { | |
| if code == "" { | |
| return nil, NewOAuthError(i18n.MsgOAuthInvalidCode, nil) | |
| } | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: code received") | |
| settings := system_setting.GetDiscordSettings() | |
| redirectUri := fmt.Sprintf("%s/oauth/discord", system_setting.ServerAddress) | |
| values := url.Values{} | |
| values.Set("client_id", settings.ClientId) | |
| values.Set("client_secret", settings.ClientSecret) | |
| values.Set("code", code) | |
| values.Set("grant_type", "authorization_code") | |
| values.Set("redirect_uri", redirectUri) | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken: redirect_uri=%s", redirectUri) | |
| req, err := http.NewRequestWithContext(ctx, "POST", "https://discord.com/api/v10/oauth2/token", strings.NewReader(values.Encode())) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | |
| req.Header.Set("Accept", "application/json") | |
| client := http.Client{ | |
| Timeout: 5 * time.Second, | |
| } | |
| res, err := client.Do(req) | |
| if err != nil { | |
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Discord] ExchangeToken error: %s", err.Error())) | |
| return nil, NewOAuthErrorWithRaw(i18n.MsgOAuthConnectFailed, map[string]any{"Provider": "Discord"}, err.Error()) | |
| } | |
| defer res.Body.Close() | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken response status: %d", res.StatusCode) | |
| var discordResponse discordOAuthResponse | |
| err = json.NewDecoder(res.Body).Decode(&discordResponse) | |
| if err != nil { | |
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Discord] ExchangeToken decode error: %s", err.Error())) | |
| return nil, err | |
| } | |
| if discordResponse.AccessToken == "" { | |
| logger.LogError(ctx, "[OAuth-Discord] ExchangeToken failed: empty access token") | |
| return nil, NewOAuthError(i18n.MsgOAuthTokenFailed, map[string]any{"Provider": "Discord"}) | |
| } | |
| logger.LogDebug(ctx, "[OAuth-Discord] ExchangeToken success: scope=%s", discordResponse.Scope) |
🤖 Prompt for AI Agents
In `@oauth/discord.go` around lines 49 - 98, The ExchangeToken function currently
logs the raw authorization code (logger.LogDebug line with "[OAuth-Discord]
ExchangeToken: code=%s..." using code and min), which risks credential leakage;
remove or redact the code before logging (e.g., log only a fixed-length hash,
the code length, or a masked substring like "*****..."), update the logger call
in ExchangeToken to use the redacted value (and ensure no other logs in this
function print the raw code), and keep the rest of the token exchange logic
unchanged.
| logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken: code=%s...", p.config.Slug, code[:min(len(code), 10)]) | ||
|
|
||
| redirectUri := fmt.Sprintf("%s/oauth/%s", system_setting.ServerAddress, p.config.Slug) | ||
| values := url.Values{} | ||
| values.Set("grant_type", "authorization_code") | ||
| values.Set("code", code) | ||
| values.Set("redirect_uri", redirectUri) | ||
|
|
||
| // Determine auth style | ||
| authStyle := p.config.AuthStyle | ||
| if authStyle == AuthStyleAutoDetect { | ||
| // Default to params style for most OAuth servers | ||
| authStyle = AuthStyleInParams | ||
| } | ||
|
|
||
| var req *http.Request | ||
| var err error | ||
|
|
||
| if authStyle == AuthStyleInParams { | ||
| values.Set("client_id", p.config.ClientId) | ||
| values.Set("client_secret", p.config.ClientSecret) | ||
| } | ||
|
|
||
| req, err = http.NewRequestWithContext(ctx, "POST", p.config.TokenEndpoint, strings.NewReader(values.Encode())) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | ||
| req.Header.Set("Accept", "application/json") | ||
|
|
||
| if authStyle == AuthStyleInHeader { | ||
| // Basic Auth | ||
| credentials := base64.StdEncoding.EncodeToString([]byte(p.config.ClientId + ":" + p.config.ClientSecret)) | ||
| req.Header.Set("Authorization", "Basic "+credentials) | ||
| } | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken: token_endpoint=%s, redirect_uri=%s, auth_style=%d", | ||
| p.config.Slug, p.config.TokenEndpoint, redirectUri, authStyle) | ||
|
|
||
| client := http.Client{ | ||
| Timeout: 20 * time.Second, | ||
| } | ||
| res, err := client.Do(req) | ||
| if err != nil { | ||
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Generic-%s] ExchangeToken error: %s", p.config.Slug, err.Error())) | ||
| return nil, NewOAuthErrorWithRaw(i18n.MsgOAuthConnectFailed, map[string]any{"Provider": p.config.Name}, err.Error()) | ||
| } | ||
| defer res.Body.Close() | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken response status: %d", p.config.Slug, res.StatusCode) | ||
|
|
||
| body, err := io.ReadAll(res.Body) | ||
| if err != nil { | ||
| logger.LogError(ctx, fmt.Sprintf("[OAuth-Generic-%s] ExchangeToken read body error: %s", p.config.Slug, err.Error())) | ||
| return nil, err | ||
| } | ||
|
|
||
| bodyStr := string(body) | ||
| logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken response body: %s", p.config.Slug, bodyStr[:min(len(bodyStr), 500)]) | ||
|
|
There was a problem hiding this comment.
Avoid logging auth codes, tokens, or raw user info.
Debug logs currently include authorization codes and full response bodies, which can leak secrets/PII. Log only metadata (length/status) or redact payloads.
🔐 Redact sensitive OAuth data from logs
-logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken: code=%s...", p.config.Slug, code[:min(len(code), 10)])
+logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken: code received", p.config.Slug)
@@
-bodyStr := string(body)
-logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken response body: %s", p.config.Slug, bodyStr[:min(len(bodyStr), 500)])
+bodyStr := string(body)
+logger.LogDebug(ctx, "[OAuth-Generic-%s] ExchangeToken response body length: %d", p.config.Slug, len(body))
@@
-bodyStr := string(body)
-logger.LogDebug(ctx, "[OAuth-Generic-%s] GetUserInfo response body: %s", p.config.Slug, bodyStr[:min(len(bodyStr), 500)])
+bodyStr := string(body)
+logger.LogDebug(ctx, "[OAuth-Generic-%s] GetUserInfo response body length: %d", p.config.Slug, len(body))Also applies to: 202-204
🤖 Prompt for AI Agents
In `@oauth/generic.go` around lines 56 - 115, The ExchangeToken flow currently
logs sensitive values (the authorization code variable `code` and the raw
response body `bodyStr`) via `logger.LogDebug`; remove or redact those values in
the `logger.LogDebug` calls in `ExchangeToken` (references: `code`, `bodyStr`,
`p.config.Slug`, `p.config.TokenEndpoint`) and instead log non-sensitive
metadata such as response status, response length (e.g. len(body)), or a fixed
redacted string; also ensure any other `logger.LogDebug` calls in this function
(and the similar calls around lines noted) do not print `code`, tokens, or raw
user info.
| func (p *LinuxDOProvider) ExchangeToken(ctx context.Context, code string, c *gin.Context) (*OAuthToken, error) { | ||
| if code == "" { | ||
| return nil, NewOAuthError(i18n.MsgOAuthInvalidCode, nil) | ||
| } | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: code=%s...", code[:min(len(code), 10)]) | ||
|
|
||
| // Get access token using Basic auth | ||
| tokenEndpoint := common.GetEnvOrDefaultString("LINUX_DO_TOKEN_ENDPOINT", "https://connect.linux.do/oauth2/token") | ||
| credentials := common.LinuxDOClientId + ":" + common.LinuxDOClientSecret | ||
| basicAuth := "Basic " + base64.StdEncoding.EncodeToString([]byte(credentials)) | ||
|
|
||
| // Get redirect URI from request | ||
| scheme := "http" | ||
| if c.Request.TLS != nil { | ||
| scheme = "https" | ||
| } | ||
| redirectURI := fmt.Sprintf("%s://%s/api/oauth/linuxdo", scheme, c.Request.Host) | ||
|
|
||
| logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: token_endpoint=%s, redirect_uri=%s", tokenEndpoint, redirectURI) | ||
|
|
||
| data := url.Values{} | ||
| data.Set("grant_type", "authorization_code") | ||
| data.Set("code", code) | ||
| data.Set("redirect_uri", redirectURI) | ||
|
|
There was a problem hiding this comment.
Fix redirect URI scheme behind proxies and redact auth codes.
Using c.Request.TLS alone produces http when TLS terminates at a reverse proxy, breaking OAuth callbacks. Also avoid logging auth codes.
🌐 Respect forwarded headers and redact auth code
-logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: code=%s...", code[:min(len(code), 10)])
+logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: code received")
@@
-// Get redirect URI from request
-scheme := "http"
-if c.Request.TLS != nil {
- scheme = "https"
-}
-redirectURI := fmt.Sprintf("%s://%s/api/oauth/linuxdo", scheme, c.Request.Host)
+// Get redirect URI from request (prefer forwarded headers behind proxies)
+scheme := c.Request.Header.Get("X-Forwarded-Proto")
+if scheme == "" {
+ if c.Request.TLS != nil {
+ scheme = "https"
+ } else {
+ scheme = "http"
+ }
+}
+host := c.Request.Header.Get("X-Forwarded-Host")
+if host == "" {
+ host = c.Request.Host
+}
+redirectURI := fmt.Sprintf("%s://%s/api/oauth/linuxdo", scheme, host)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (p *LinuxDOProvider) ExchangeToken(ctx context.Context, code string, c *gin.Context) (*OAuthToken, error) { | |
| if code == "" { | |
| return nil, NewOAuthError(i18n.MsgOAuthInvalidCode, nil) | |
| } | |
| logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: code=%s...", code[:min(len(code), 10)]) | |
| // Get access token using Basic auth | |
| tokenEndpoint := common.GetEnvOrDefaultString("LINUX_DO_TOKEN_ENDPOINT", "https://connect.linux.do/oauth2/token") | |
| credentials := common.LinuxDOClientId + ":" + common.LinuxDOClientSecret | |
| basicAuth := "Basic " + base64.StdEncoding.EncodeToString([]byte(credentials)) | |
| // Get redirect URI from request | |
| scheme := "http" | |
| if c.Request.TLS != nil { | |
| scheme = "https" | |
| } | |
| redirectURI := fmt.Sprintf("%s://%s/api/oauth/linuxdo", scheme, c.Request.Host) | |
| logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: token_endpoint=%s, redirect_uri=%s", tokenEndpoint, redirectURI) | |
| data := url.Values{} | |
| data.Set("grant_type", "authorization_code") | |
| data.Set("code", code) | |
| data.Set("redirect_uri", redirectURI) | |
| func (p *LinuxDOProvider) ExchangeToken(ctx context.Context, code string, c *gin.Context) (*OAuthToken, error) { | |
| if code == "" { | |
| return nil, NewOAuthError(i18n.MsgOAuthInvalidCode, nil) | |
| } | |
| logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: code received") | |
| // Get access token using Basic auth | |
| tokenEndpoint := common.GetEnvOrDefaultString("LINUX_DO_TOKEN_ENDPOINT", "https://connect.linux.do/oauth2/token") | |
| credentials := common.LinuxDOClientId + ":" + common.LinuxDOClientSecret | |
| basicAuth := "Basic " + base64.StdEncoding.EncodeToString([]byte(credentials)) | |
| // Get redirect URI from request (prefer forwarded headers behind proxies) | |
| scheme := c.Request.Header.Get("X-Forwarded-Proto") | |
| if scheme == "" { | |
| if c.Request.TLS != nil { | |
| scheme = "https" | |
| } else { | |
| scheme = "http" | |
| } | |
| } | |
| host := c.Request.Header.Get("X-Forwarded-Host") | |
| if host == "" { | |
| host = c.Request.Host | |
| } | |
| redirectURI := fmt.Sprintf("%s://%s/api/oauth/linuxdo", scheme, host) | |
| logger.LogDebug(ctx, "[OAuth-LinuxDO] ExchangeToken: token_endpoint=%s, redirect_uri=%s", tokenEndpoint, redirectURI) | |
| data := url.Values{} | |
| data.Set("grant_type", "authorization_code") | |
| data.Set("code", code) | |
| data.Set("redirect_uri", redirectURI) | |
🤖 Prompt for AI Agents
In `@oauth/linuxdo.go` around lines 45 - 70, In ExchangeToken, determine the
redirect scheme from proxy headers before falling back to c.Request.TLS by
checking headers like X-Forwarded-Proto (and optionally Forwarded) to build
redirectURI so callbacks work behind TLS-terminating proxies; also stop logging
raw auth codes in logger.LogDebug (redact the code or replace with a constant
like "<redacted>") to avoid leaking sensitive data. Ensure you update the
logger.LogDebug call that currently slices code and the redirect URI
construction that uses c.Request.TLS so they use the forwarded-proto check and
redaction.
…th user creation and binding - Improve error handling in DeleteCustomOAuthProvider to log and return errors when fetching binding counts. - Refactor user creation and OAuth binding logic to use transactions for atomic operations, ensuring data integrity. - Add unique constraints to UserOAuthBinding model to prevent duplicate bindings. - Enhance GitHub OAuth provider error logging for non-200 responses. - Update AccountManagement component to provide clearer error messages on API failures.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@controller/custom_oauth.go`:
- Around line 167-263: The UpdateCustomOAuthProviderRequest currently has
non-pointer fields for Enabled, AuthStyle and WellKnown, so
UpdateCustomOAuthProvider unconditionally overwrites provider.Enabled,
provider.AuthStyle and provider.WellKnown with zero values when those fields are
omitted; change the request struct fields Enabled, AuthStyle and WellKnown to
pointer types (e.g., *bool, *int, *string) in UpdateCustomOAuthProviderRequest
and then in UpdateCustomOAuthProvider only assign to provider.Enabled,
provider.AuthStyle and provider.WellKnown when the corresponding request
pointers are non-nil (leave existing values otherwise), keeping existing
nil-check logic used for other optional fields like ClientSecret.
In `@controller/oauth.go`:
- Around line 254-258: The code unsafely asserts affCode.(string) which can
panic if session.Get("aff") returns a non-string; change the logic around
affCode, inviterId and model.GetUserIdByAffCode to first check affCode != nil
and perform a safe type assertion (e.g., s, ok := affCode.(string)) or a type
switch, and only call model.GetUserIdByAffCode(s) when ok is true; ensure
inviterId remains 0 when the assertion fails and handle any alternate
representations (e.g., numeric) if needed before calling GetUserIdByAffCode.
In `@model/user.go`:
- Around line 602-608: The UpdateGitHubId method updates the DB but doesn't
clear the cached user entry; after successfully performing
DB.Model(user).Update("github_id", newGitHubId).Error return, call
invalidateUserCache(user.Id) (or updateUserCache(user)) to remove/refresh the
cached record so FillUserByGitHubId() won't read stale data; ensure this cache
invalidation runs only when the DB update succeeds and keep the function
signature unchanged.
🧹 Nitpick comments (2)
model/user.go (2)
432-458: Consolidate user-creation logic to avoid drift.
InsertWithTxlargely duplicatesInsert, andinviterIdis unused here. Consider extracting a shared helper that accepts*gorm.DB(or removing the unused parameter) so both paths stay in sync.
460-474: Log sidebar-config update failures.If
createdUser.Update(false)fails, the sidebar config silently remains unset. Logging the error will make failures actionable.🛠️ Suggested fix
- createdUser.Update(false) + if err := createdUser.Update(false); err != nil { + common.SysLog(fmt.Sprintf("failed to init sidebar config for user %d: %s", createdUser.Id, err.Error())) + }
| affCode := session.Get("aff") | ||
| inviterId := 0 | ||
| if affCode != nil { | ||
| inviterId, _ = model.GetUserIdByAffCode(affCode.(string)) | ||
| } |
There was a problem hiding this comment.
Safeguard affiliate code type assertion.
affCode.(string) can panic if the session value isn’t a string. Use a safe assertion.
🛡️ Suggested fix
- affCode := session.Get("aff")
- inviterId := 0
- if affCode != nil {
- inviterId, _ = model.GetUserIdByAffCode(affCode.(string))
- }
+ affCode := session.Get("aff")
+ inviterId := 0
+ if affCodeStr, ok := affCode.(string); ok && affCodeStr != "" {
+ inviterId, _ = model.GetUserIdByAffCode(affCodeStr)
+ }🤖 Prompt for AI Agents
In `@controller/oauth.go` around lines 254 - 258, The code unsafely asserts
affCode.(string) which can panic if session.Get("aff") returns a non-string;
change the logic around affCode, inviterId and model.GetUserIdByAffCode to first
check affCode != nil and perform a safe type assertion (e.g., s, ok :=
affCode.(string)) or a type switch, and only call model.GetUserIdByAffCode(s)
when ok is true; ensure inviterId remains 0 when the assertion fails and handle
any alternate representations (e.g., numeric) if needed before calling
GetUserIdByAffCode.
| // UpdateGitHubId updates the user's GitHub ID (used for migration from login to numeric ID) | ||
| func (user *User) UpdateGitHubId(newGitHubId string) error { | ||
| if user.Id == 0 { | ||
| return errors.New("user id is empty") | ||
| } | ||
| return DB.Model(user).Update("github_id", newGitHubId).Error | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Inspect cache update paths and GitHubId usage
rg -n "updateUserCache|invalidateUserCache|GitHubId|github_id" --type=goRepository: QuantumNous/new-api
Length of output: 1885
🏁 Script executed:
# Check for cache-related functions and GitHub ID queries
rg -n "FillUserByGitHubId|IsGitHubIdAlreadyTaken|getUser.*GitHub" --type=goRepository: QuantumNous/new-api
Length of output: 324
🏁 Script executed:
# Examine the UpdateGitHubId function and its callers
rg -B3 -A3 "UpdateGitHubId" --type=goRepository: QuantumNous/new-api
Length of output: 970
🏁 Script executed:
# Check if there's a caching mechanism in the codebase (Redis, in-memory cache patterns)
rg -n "cache|Cache|redis|Redis" --type=go | head -30Repository: QuantumNous/new-api
Length of output: 2404
Invalidate cache after updating GitHub ID.
UpdateGitHubId updates the database but fails to invalidate the user cache. Unlike other update methods in this file (lines 506, 536, 548) that call updateUserCache() or invalidateUserCache(), this function skips cache management. Since GitHubId is indexed and used for lookups via FillUserByGitHubId(), stale cache entries can cause authentication failures during migration. Add invalidateUserCache(user.Id) or refresh the cache after the database update.
🤖 Prompt for AI Agents
In `@model/user.go` around lines 602 - 608, The UpdateGitHubId method updates the
DB but doesn't clear the cached user entry; after successfully performing
DB.Model(user).Update("github_id", newGitHubId).Error return, call
invalidateUserCache(user.Id) (or updateUserCache(user)) to remove/refresh the
cached record so FillUserByGitHubId() won't read stale data; ensure this cache
invalidation runs only when the DB update succeeds and keep the function
signature unchanged.
…ers for optional fields - Change fields in UpdateCustomOAuthProviderRequest struct to use pointers for optional values, allowing for better handling of nil cases. - Update UpdateCustomOAuthProvider function to check for nil before assigning optional fields, ensuring existing values are preserved when not provided.
feat(oauth): implement custom OAuth provider
Summary by CodeRabbit