feat(auth): add multi-provider account binding#73
Conversation
Introduce user auth identities, explicit bind/unbind flows, and provider-priority profile aggregation so multiple Casdoor login methods can resolve to one local account safely.
📝 WalkthroughWalkthroughIntroduces multi-provider account binding with a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as Auth Handler
participant Service as User Service
participant DB as Database
participant OAuth as OAuth Provider
rect rgba(100, 150, 255, 0.5)
Note over Client,OAuth: Initiate Binding (StartBindAuth)
Client->>Handler: POST /api/auth/bind/start?provider=github&redirectUrl=...
Handler->>Handler: Generate signed bindState (HMAC-SHA256)
Handler->>OAuth: Build login URL with signed state
Handler-->>Client: 302 Redirect to OAuth login
end
rect rgba(100, 200, 100, 0.5)
Note over Client,OAuth: OAuth Callback
OAuth-->>Client: Redirect to /api/auth/bind/callback?code=...&state=signed
Client->>Handler: GET /api/auth/bind/callback?code=...&state=...
Handler->>Handler: Decode & validate signed state (expiry, session)
Handler->>OAuth: Exchange code for token
OAuth-->>Handler: Access token & user info
Handler->>Handler: Extract provider identity claims
end
rect rgba(200, 150, 100, 0.5)
Note over Handler,DB: Bind Identity to User
Handler->>Service: BindIdentityToUser(userID, claims)
Service->>DB: Check for existing identity
Service->>DB: Insert new UserAuthIdentity
Service->>Service: Select primary by provider rank
Service->>DB: Update is_primary flags
Service->>Service: Refresh user profile from ranked identities
Service->>DB: Update user fields (email, avatar, etc.)
Service-->>Handler: Success
Handler-->>Client: 302 Redirect with success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans multiple architectural layers (models, handlers, service, migration) with significant new logic including HMAC-based state signing/validation, provider-ranked primary identity selection, identity-to-profile aggregation, and complex backfill migration with conditional key derivation from legacy fields. The handler and service implementations require careful review of security constraints, transaction boundaries, and provider ranking determinism. 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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain modules listed in go.work or their selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
server/internal/handlers/handlers_test.go (1)
22-34: Cache the RSA key to avoid generating one per test call.
signHandlersTestJWTis invoked from several test cases (TestBindCallbackRejectsProviderMismatch,TestBindCallbackSuccess,TestBindCallbackRejectsIdentityAlreadyBound, often twice per test for current/bound tokens).rsa.GenerateKey(rand.Reader, 2048)is ~100–300ms each, which adds up quickly on CI. Since the signature is not asserted against a JWKS in these tests (claims are just parsed), a single shared key is sufficient.♻️ Suggested change
+var ( + handlersTestRSAKeyOnce sync.Once + handlersTestRSAKey *rsa.PrivateKey +) + func signHandlersTestJWT(t *testing.T, claims jwt.MapClaims) string { t.Helper() - key, err := rsa.GenerateKey(rand.Reader, 2048) - if err != nil { - t.Fatalf("generate rsa key: %v", err) - } + handlersTestRSAKeyOnce.Do(func() { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("generate rsa key: %v", err) + } + handlersTestRSAKey = key + }) token := jwt.NewWithClaims(jwt.SigningMethodRS256, claims) - tokenString, err := token.SignedString(key) + tokenString, err := token.SignedString(handlersTestRSAKey) if err != nil { t.Fatalf("sign jwt: %v", err) } return tokenString }(Don't forget to add
"sync"to the imports.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/handlers/handlers_test.go` around lines 22 - 34, The test helper signHandlersTestJWT currently generates a new RSA key on every call which is slow; change it to use a package-level cached *rsa.PrivateKey initialized once (e.g. via sync.Once or a sync.Mutex-protected variable) so subsequent calls reuse the same key, add "sync" to imports, and update signHandlersTestJWT to use that cached key instead of calling rsa.GenerateKey each invocation (keep function name signHandlersTestJWT and the existing error handling).docs/proposals/MULTI_PROVIDER_ACCOUNT_BINDING_DESIGN.md (1)
41-41: Markdown heading levels inconsistent for subsections.Subsections like
## 3.1,## 3.2,## 4.1,## 4.2,## 4.3,## 5-derived## 5.1/## 6.1/etc., and## 7.1/## 7.2/## 8.1-## 8.4/## 9-children/## 10.1-## 10.7/## 11.1-## 11.3/## 12.1-## 12.4are written at H2 instead of H3. As written they appear at the same outline level as their parent sections (e.g.,## 4.1 现有 users 表定位调整is rendered as a sibling of## 4. 数据模型设计), which makes the TOC and document structure misleading.Recommend bumping these subsection headings to
###so they nest under the H2 parent.Also applies to: 81-81, 100-100, 129-129, 175-175, 193-193, 243-243, 255-255, 267-267, 291-291, 311-311, 328-328, 358-358, 365-365, 373-373, 381-381, 389-389, 397-397, 404-404, 416-416, 420-420, 442-442, 452-452, 456-456, 464-464, 472-472
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/proposals/MULTI_PROVIDER_ACCOUNT_BINDING_DESIGN.md` at line 41, Several subsection headings are at H2 (e.g., "## 3.1 采用显式绑定", "## 4.1 现有 users 表定位调整", and the other listed headings) but should be nested under their H2 parents; change each of the indicated headings from "##" to "###" so they become H3 (e.g., update "## 3.1 采用显式绑定" → "### 3.1 采用显式绑定", "## 4.1 现有 users 表定位调整" → "### 4.1 现有 users 表定位调整", and similarly for the headings at the other reported locations) to restore correct document outline and TOC nesting.server/internal/models/models.go (1)
576-600: Note:is_primaryuniqueness is enforced only at application level.The design doc (section 4.3
#3) requires "at most oneis_primary=trueperuser_subject_id", but the model and migration enforce this only viaRefreshUserProfileFromIdentitiesrather than a partial unique index. That's a defensible choice (avoids transactional dance during primary swaps), but it does mean any code path that mutatesis_primaryoutside that single function — including the new backfill inserver/cmd/migrate/main.gowhich always writes"is_primary": true— must be careful not to create duplicates.For the backfill specifically: since each user can only contribute one identity (it short-circuits after the first row per user), the invariant holds today. Just worth a comment near the
IsPrimaryfield or nearRefreshUserProfileFromIdentitiesdocumenting that this is the sole legitimate writer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/models/models.go` around lines 576 - 600, The IsPrimary boolean on UserAuthIdentity is only protected by application logic (RefreshUserProfileFromIdentities) so add a clear comment on the IsPrimary field in models.go noting that RefreshUserProfileFromIdentities is the sole legitimate writer and that other code must not set is_primary directly; then update the migration/backfill in server/cmd/migrate/main.go to avoid writing "is_primary": true blindly (either call RefreshUserProfileFromIdentities after backfilling a user's identities or ensure the backfill only marks the first identity per user as primary) so the one-primary-per-user invariant is preserved.server/cmd/migrate/main_test.go (1)
128-147: StrengthenTestBackfillUserAuthIdentitiesassertions beyond row count.The backfill in
backfillUserAuthIdentities(server/cmd/migrate/main.go) does non-trivial work: lower-casingAuthProvider, prioritizingExternalKeyovercasdoor_universal_id/casdoor_sub/casdoor_id, and marking the rowis_primary=true. Asserting onlycount == 1means a regression that flipsis_primary, picks the wrong external_key derivation, or stores the wrong provider would still pass.♻️ Suggested additional assertions
var count int64 if err := db.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", "u1").Count(&count).Error; err != nil { t.Fatalf("count identities: %v", err) } if count != 1 { t.Fatalf("expected 1 backfilled identity, got %d", count) } + var got models.UserAuthIdentity + if err := db.Where("user_subject_id = ?", "u1").First(&got).Error; err != nil { + t.Fatalf("load identity: %v", err) + } + if got.Provider != "github" || got.ExternalKey != "casdoor:uuid-1" || !got.IsPrimary { + t.Fatalf("unexpected backfilled identity: %+v", got) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cmd/migrate/main_test.go` around lines 128 - 147, Update TestBackfillUserAuthIdentities to validate the actual backfilled row contents, not just the count: after calling backfillUserAuthIdentities, SELECT the created models.UserAuthIdentity for user_subject_id "u1" and assert that AuthProvider is lower-cased (matches strings.ToLower of the source), ExternalKey equals the original ExternalKey value (and if ExternalKey is nil test a case where it falls back to CasdoorUniversalID/CasdoorSub/CasdoorID), ProviderUserID matches ProviderUserID from the User, and IsPrimary is true; also assert there is exactly one identity with IsPrimary=true for that user to guard against duplicate primaries. Use the existing functions/structs backfillUserAuthIdentities and models.UserAuthIdentity to locate the code to change.server/cmd/migrate/main.go (2)
187-192: Optional: exposeuser-auth-identities [--dry-run]as a subcommand.Other backfills (
user-subject-ids,user-external-identities) have dedicated CLI subcommands with--dry-runsupport, which is helpful for ops to preview impact in production. SincebackfillUserAuthIdentitiesalready acceptsdryRun booland supportserrDryRunRollback, adding a subcommand here is mostly wiring.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cmd/migrate/main.go` around lines 187 - 192, Add a new "user-auth-identities" subcommand to the migrate CLI that mirrors the pattern used by "user-subject-ids" and "user-external-identities": parse a --dry-run boolean flag, call backfillUserAuthIdentities(db, dryRun), treat errDryRunRollback specially (do not treat it as a real error when dryRun is true), and log/exit with the same messaging and exit codes as the other backfill subcommands; ensure the command name, help text, and flag wiring are added to the main migrate command registration so ops can run "user-auth-identities --dry-run" to preview changes.
358-376: Defensive: gate column access withMigrator().HasColumnfor parity withbackfillUserExternalIdentities.
backfillUserExternalIdentities(lines 272–289) carefully checksHasColumnforphone,auth_provider,external_key, andprovider_user_idbefore selecting them, because they were added byensureUserIdentityColumnsand may be absent in older snapshots.backfillUserAuthIdentitiesselects all of these unconditionally. Today the call order inmain()(line 177 → 187 → 190) makes this safe, but a future refactor that runs the backfill standalone (e.g., adding a CLI subcommand) would silently fail with a SQL error. Consider mirroring the defensive pattern, or at minimum documenting the ordering invariant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/cmd/migrate/main.go` around lines 358 - 376, backfillUserAuthIdentities is selecting columns (phone, auth_provider, external_key, provider_user_id) that may not exist in older schemas; mirror the defensive pattern used in backfillUserExternalIdentities by using db.Migrator().HasColumn checks before including each optional column in the SELECT, or otherwise gate the whole backfill behind ensureUserIdentityColumns; update the Select(...) construction in backfillUserAuthIdentities to conditionally append these column names only if HasColumn reports true (refer to Migrator().HasColumn, backfillUserExternalIdentities, and ensureUserIdentityColumns to match behavior).server/internal/handlers/handlers.go (2)
647-651: Usestrconv.ParseUintinstead offmt.Sscanffor the path id.
fmt.Sscanf(c.Param("id"), "%d", &identityID)accepts inputs like"5abc"and parses to5, ignoring trailing garbage.strconv.ParseUint(c.Param("id"), 10, 64)(orgin'sc.GetUint) is stricter and idiomatic.♻️ Proposed fix
- var identityID uint - if _, err := fmt.Sscanf(c.Param("id"), "%d", &identityID); err != nil || identityID == 0 { + idVal, err := strconv.ParseUint(c.Param("id"), 10, 64) + if err != nil || idVal == 0 { c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid identity id"}) return } + identityID := uint(idVal)(Add
"strconv"to imports.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/handlers/handlers.go` around lines 647 - 651, Replace the loose fmt.Sscanf-based id parsing with a strict strconv.ParseUint call: import "strconv", call strconv.ParseUint(c.Param("id"), 10, 64), check the returned error and that the parsed value is non-zero, then cast it to uint and assign to identityID (replacing the current var identityID uint and the fmt.Sscanf check in the handler). Ensure the invalid-path branch returns the same BadRequest JSON when ParseUint fails or yields 0.
462-469: Prefer sentinel errors over string-comparison for binding outcomes.
err.Error() == "identity_already_bound"(here and on line 653 for"cannot unbind last identity") couples handlers to the exact wording from the service. A typo or wrapped error breaks the case silently and gives users a generic 500. Consider exporting sentinels fromuserpkg(e.g.userpkg.ErrIdentityAlreadyBound,userpkg.ErrCannotUnbindLastIdentity) and usingerrors.Is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/handlers/handlers.go` around lines 462 - 469, The handler currently compares error strings returned by UserModule.Service.BindIdentityToUser (and similarly for the "cannot unbind last identity" case) which is brittle; define and export sentinel errors in the user package (e.g. userpkg.ErrIdentityAlreadyBound and userpkg.ErrCannotUnbindLastIdentity), modify the service implementations to return or wrap these sentinels, and update the handler to check errors using errors.Is(err, userpkg.ErrIdentityAlreadyBound) (and errors.Is(..., userpkg.ErrCannotUnbindLastIdentity)) instead of err.Error() == "...", returning the same HTTP responses when the sentinel matches and a 500 otherwise.server/internal/user/service.go (1)
365-371: RedundantSaveon every login.When
shouldUpdate == true, line 365 saves the user. Then line 369 callsBindIdentityToUser, which (viarefreshUserProfileFromIdentitiesTx, line 686) does anothertx.Save(&user)— derived from identities and likely overwriting some of what was just saved on line 365. Functionally harmless, but it doubles the writes per login and the second save can clobber profile fields that were freshly synced from claims (e.g., thevalidEmailPtr's pick).Consider letting
refreshUserProfileFromIdentitiesTxbe the single source of truth and removing the field-by-field updates above (or vice versa).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/user/service.go` around lines 365 - 371, The code currently saves the user in the outer flow when shouldUpdate == true (s.db.Save(&user)) and then calls BindIdentityToUser which calls refreshUserProfileFromIdentitiesTx and does another tx.Save(&user), causing redundant writes and potential clobbering; pick one source-of-truth: either remove the field-by-field updates & outer s.db.Save(&user) and let refreshUserProfileFromIdentitiesTx (via BindIdentityToUser) be the single updater of profile fields, or keep the outer updates and change refreshUserProfileFromIdentitiesTx to avoid saving/overwriting those fields (e.g., skip Save there or only merge non-empty fields); update the code around shouldUpdate, s.db.Save(&user), BindIdentityToUser, and refreshUserProfileFromIdentitiesTx accordingly so only one save occurs per login and profile merging is deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/cmd/migrate/main.go`:
- Line 260: The unique index on external_key will conflict with soft-deleted
UserAuthIdentity rows; update the index creation SQL (the statement creating
idx_user_auth_identities_external_key) to be a partial index that only enforces
uniqueness for live rows (i.e., add a WHERE deleted_at IS NULL condition) so
that soft-deleted records (gorm.DeletedAt) don’t cause unique-constraint
violations when UnbindIdentity performs soft deletes.
In `@server/internal/handlers/handlers.go`:
- Around line 422-475: The handler bindAuthCallback currently returns JSON on
all error paths which exposes raw error pages to users; change each error
response to perform an HTTP redirect to the original frontend (use
state.RedirectTo if set, otherwise defaultFrontendURL + "/settings/account")
appending query params like bind=error and reason=<short_code> (e.g.
invalid_or_expired_state, code_required, auth_required, invalid_session,
binding_session_invalid, token_exchange_failed, parse_identity_failed,
provider_mismatch, identity_already_bound) so the frontend can display a
friendly message; ensure you still log the detailed error server-side (not sent
to user) and preserve distinct status codes internally but always return an HTTP
redirect to the computed redirectURL on every failure path in bindAuthCallback.
- Around line 195-207: The current sign/verify functions (signBindStatePayload,
verifyBindStatePayload) use a hardcoded fallback bindStateSecret and reuse
Casdoor secret, enabling token forgery; remove the literal default and stop
using cfg.Casdoor.Secret as the HMAC key, instead require a dedicated non-empty
BIND_STATE_SECRET (e.g., bindStateSecret) and make signBindStatePayload error or
panic/return empty when the secret is missing; update verifyBindStatePayload to
handle that case safely (reject when secret absent); add a startup validation in
InitCookieConfig that checks cfg.InternalSecret or the dedicated
BIND_STATE_SECRET is present and fail fast (return error/exit) if not, so the
service never runs with a missing bind-state secret.
- Around line 432-446: The code currently parses unverified claims via
ParseJWTClaimsFromAccessToken and then calls UserModule.Service.GetOrCreateUser
which can create users from forged tokens; change the flow to verify the access
token signature before trusting claims (use the same validator the auth
middleware uses) or avoid creation by replacing GetOrCreateUser with a
non-creating lookup such as UserModule.Service.GetUserByID(state.UserSubjectID);
specifically, after obtaining currentToken via middleware.ExtractToken, validate
the token signature and only then call GetOrCreateUser (or, preferably, call
GetUserByID with state.UserSubjectID) so that unverified claims cannot cause new
user creation and the subsequent SubjectID check is meaningful.
In `@server/internal/user/service.go`:
- Around line 414-420: The user creation (s.db.Create(&user)) and initial bind
(s.BindIdentityToUser) must be performed inside a single DB transaction to avoid
orphaned users when the bind fails; change the flow so you begin a transaction,
create the user via the transaction handle, call BindIdentityToUser using the
same transaction context/handle, and commit only if bind succeeds (treating the
"identity_already_bound" case as non-fatal), otherwise rollback and return the
bind error; finally, fetch the user (s.GetUserByID) only after a successful
commit or return the rolled-back error. Ensure ListUserIdentities/UnbindIdentity
behavior remains unchanged by always using the committed state.
- Around line 167-205: The TOCTOU bug in UserService.UnbindIdentity arises
because the identity count is read without locking, allowing two concurrent
transactions to both see count==2 and delete both identities; fix by acquiring a
FOR UPDATE lock on the user's identity rows before counting (e.g. use
tx.Clauses(clause.Locking{Strength: "UPDATE"})/tx.Where("user_subject_id = ?",
userSubjectID).Find(...)) or alternatively perform the count after the Delete
within the same transaction while holding a row lock, ensuring the check and
delete are serialized; update the logic around the tx.Where(...).Count(&count)
and tx.Delete(&identity)/refreshUserProfileFromIdentitiesTx to use the chosen
pessimistic lock so a concurrent UnbindIdentity cannot remove the last identity.
---
Nitpick comments:
In `@docs/proposals/MULTI_PROVIDER_ACCOUNT_BINDING_DESIGN.md`:
- Line 41: Several subsection headings are at H2 (e.g., "## 3.1 采用显式绑定", "## 4.1
现有 users 表定位调整", and the other listed headings) but should be nested under their
H2 parents; change each of the indicated headings from "##" to "###" so they
become H3 (e.g., update "## 3.1 采用显式绑定" → "### 3.1 采用显式绑定", "## 4.1 现有 users
表定位调整" → "### 4.1 现有 users 表定位调整", and similarly for the headings at the other
reported locations) to restore correct document outline and TOC nesting.
In `@server/cmd/migrate/main_test.go`:
- Around line 128-147: Update TestBackfillUserAuthIdentities to validate the
actual backfilled row contents, not just the count: after calling
backfillUserAuthIdentities, SELECT the created models.UserAuthIdentity for
user_subject_id "u1" and assert that AuthProvider is lower-cased (matches
strings.ToLower of the source), ExternalKey equals the original ExternalKey
value (and if ExternalKey is nil test a case where it falls back to
CasdoorUniversalID/CasdoorSub/CasdoorID), ProviderUserID matches ProviderUserID
from the User, and IsPrimary is true; also assert there is exactly one identity
with IsPrimary=true for that user to guard against duplicate primaries. Use the
existing functions/structs backfillUserAuthIdentities and
models.UserAuthIdentity to locate the code to change.
In `@server/cmd/migrate/main.go`:
- Around line 187-192: Add a new "user-auth-identities" subcommand to the
migrate CLI that mirrors the pattern used by "user-subject-ids" and
"user-external-identities": parse a --dry-run boolean flag, call
backfillUserAuthIdentities(db, dryRun), treat errDryRunRollback specially (do
not treat it as a real error when dryRun is true), and log/exit with the same
messaging and exit codes as the other backfill subcommands; ensure the command
name, help text, and flag wiring are added to the main migrate command
registration so ops can run "user-auth-identities --dry-run" to preview changes.
- Around line 358-376: backfillUserAuthIdentities is selecting columns (phone,
auth_provider, external_key, provider_user_id) that may not exist in older
schemas; mirror the defensive pattern used in backfillUserExternalIdentities by
using db.Migrator().HasColumn checks before including each optional column in
the SELECT, or otherwise gate the whole backfill behind
ensureUserIdentityColumns; update the Select(...) construction in
backfillUserAuthIdentities to conditionally append these column names only if
HasColumn reports true (refer to Migrator().HasColumn,
backfillUserExternalIdentities, and ensureUserIdentityColumns to match
behavior).
In `@server/internal/handlers/handlers_test.go`:
- Around line 22-34: The test helper signHandlersTestJWT currently generates a
new RSA key on every call which is slow; change it to use a package-level cached
*rsa.PrivateKey initialized once (e.g. via sync.Once or a sync.Mutex-protected
variable) so subsequent calls reuse the same key, add "sync" to imports, and
update signHandlersTestJWT to use that cached key instead of calling
rsa.GenerateKey each invocation (keep function name signHandlersTestJWT and the
existing error handling).
In `@server/internal/handlers/handlers.go`:
- Around line 647-651: Replace the loose fmt.Sscanf-based id parsing with a
strict strconv.ParseUint call: import "strconv", call
strconv.ParseUint(c.Param("id"), 10, 64), check the returned error and that the
parsed value is non-zero, then cast it to uint and assign to identityID
(replacing the current var identityID uint and the fmt.Sscanf check in the
handler). Ensure the invalid-path branch returns the same BadRequest JSON when
ParseUint fails or yields 0.
- Around line 462-469: The handler currently compares error strings returned by
UserModule.Service.BindIdentityToUser (and similarly for the "cannot unbind last
identity" case) which is brittle; define and export sentinel errors in the user
package (e.g. userpkg.ErrIdentityAlreadyBound and
userpkg.ErrCannotUnbindLastIdentity), modify the service implementations to
return or wrap these sentinels, and update the handler to check errors using
errors.Is(err, userpkg.ErrIdentityAlreadyBound) (and errors.Is(...,
userpkg.ErrCannotUnbindLastIdentity)) instead of err.Error() == "...", returning
the same HTTP responses when the sentinel matches and a 500 otherwise.
In `@server/internal/models/models.go`:
- Around line 576-600: The IsPrimary boolean on UserAuthIdentity is only
protected by application logic (RefreshUserProfileFromIdentities) so add a clear
comment on the IsPrimary field in models.go noting that
RefreshUserProfileFromIdentities is the sole legitimate writer and that other
code must not set is_primary directly; then update the migration/backfill in
server/cmd/migrate/main.go to avoid writing "is_primary": true blindly (either
call RefreshUserProfileFromIdentities after backfilling a user's identities or
ensure the backfill only marks the first identity per user as primary) so the
one-primary-per-user invariant is preserved.
In `@server/internal/user/service.go`:
- Around line 365-371: The code currently saves the user in the outer flow when
shouldUpdate == true (s.db.Save(&user)) and then calls BindIdentityToUser which
calls refreshUserProfileFromIdentitiesTx and does another tx.Save(&user),
causing redundant writes and potential clobbering; pick one source-of-truth:
either remove the field-by-field updates & outer s.db.Save(&user) and let
refreshUserProfileFromIdentitiesTx (via BindIdentityToUser) be the single
updater of profile fields, or keep the outer updates and change
refreshUserProfileFromIdentitiesTx to avoid saving/overwriting those fields
(e.g., skip Save there or only merge non-empty fields); update the code around
shouldUpdate, s.db.Save(&user), BindIdentityToUser, and
refreshUserProfileFromIdentitiesTx accordingly so only one save occurs per login
and profile merging is deterministic.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e013fb59-71f2-4216-a7b5-f6aa6bcc8225
📒 Files selected for processing (12)
docs/proposals/MULTI_PROVIDER_ACCOUNT_BINDING_DESIGN.mddocs/proposals/casdoor-identity-normalization/05-service-and-flow.mdserver/cmd/api/main.goserver/cmd/migrate/main.goserver/cmd/migrate/main_test.goserver/internal/handlers/handlers.goserver/internal/handlers/handlers_test.goserver/internal/handlers/registry_test.goserver/internal/models/models.goserver/internal/user/service.goserver/internal/user/service_test.gotodo/MULTI_PROVIDER_ACCOUNT_BINDING_PROGRESS.md
| updated_at timestamptz, | ||
| deleted_at timestamptz | ||
| )`, | ||
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_user_auth_identities_external_key ON user_auth_identities(external_key)`, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
ast-grep --pattern $'type UserAuthIdentity struct {
$$$
}'
rg -nP -C2 'UserAuthIdentity\b' --type=go -g '!**/*_test.go'Repository: XDfield/costrict-web
Length of output: 12394
Unique index on external_key will conflict with soft-deleted rows.
The UserAuthIdentity model includes DeletedAt gorm.DeletedAt (line 595 in server/internal/models/models.go), confirming GORM soft-delete is in use. The UnbindIdentity function performs soft deletes, leaving rows physically present with deleted_at set. Subsequent attempts to bind the same external_key will fail with a unique-constraint violation since the index does not filter out soft-deleted rows.
Make the index partial to enforce uniqueness only on live rows:
Proposed fix
- `CREATE UNIQUE INDEX IF NOT EXISTS idx_user_auth_identities_external_key ON user_auth_identities(external_key)`,
+ `CREATE UNIQUE INDEX IF NOT EXISTS idx_user_auth_identities_external_key ON user_auth_identities(external_key) WHERE deleted_at IS NULL`,📝 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.
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_user_auth_identities_external_key ON user_auth_identities(external_key)`, | |
| `CREATE UNIQUE INDEX IF NOT EXISTS idx_user_auth_identities_external_key ON user_auth_identities(external_key) WHERE deleted_at IS NULL`, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/cmd/migrate/main.go` at line 260, The unique index on external_key
will conflict with soft-deleted UserAuthIdentity rows; update the index creation
SQL (the statement creating idx_user_auth_identities_external_key) to be a
partial index that only enforces uniqueness for live rows (i.e., add a WHERE
deleted_at IS NULL condition) so that soft-deleted records (gorm.DeletedAt)
don’t cause unique-constraint violations when UnbindIdentity performs soft
deletes.
| func signBindStatePayload(payload string) string { | ||
| key := strings.TrimSpace(bindStateSecret) | ||
| if key == "" { | ||
| key = "costrict-bind-state-default" | ||
| } | ||
| h := hmac.New(sha256.New, []byte(key)) | ||
| _, _ = h.Write([]byte(payload)) | ||
| return hex.EncodeToString(h.Sum(nil)) | ||
| } | ||
|
|
||
| func verifyBindStatePayload(payload, sig string) bool { | ||
| return hmac.Equal([]byte(signBindStatePayload(payload)), []byte(sig)) | ||
| } |
There was a problem hiding this comment.
Critical: hardcoded fallback HMAC key allows bind-state forgery if no secret is configured.
If both cfg.InternalSecret and cfg.Casdoor.Secret are empty, signBindStatePayload falls back to the literal "costrict-bind-state-default" (line 198). Because that constant is in source, anyone can forge a valid bind-state token, bypassing expiry/nonce/user checks and binding arbitrary identities to arbitrary users in misconfigured deployments. The bind flow should fail closed when no key is available.
Additionally, reusing the Casdoor OAuth client secret as the HMAC key (line 82) couples two unrelated cryptographic concerns; prefer a dedicated BIND_STATE_SECRET and require it to be non-empty at boot.
🛡️ Proposed fix
func signBindStatePayload(payload string) string {
key := strings.TrimSpace(bindStateSecret)
if key == "" {
- key = "costrict-bind-state-default"
+ // Fail closed: refuse to sign without a configured secret.
+ // signBindStatePayload should never be reached if InitCookieConfig
+ // validated configuration at boot. Returning an unguessable random
+ // signature here ensures verifyBindStatePayload always rejects.
+ return ""
}
h := hmac.New(sha256.New, []byte(key))
_, _ = h.Write([]byte(payload))
return hex.EncodeToString(h.Sum(nil))
}
func verifyBindStatePayload(payload, sig string) bool {
+ if strings.TrimSpace(bindStateSecret) == "" {
+ return false
+ }
return hmac.Equal([]byte(signBindStatePayload(payload)), []byte(sig))
}And add a startup check in InitCookieConfig:
func InitCookieConfig(cfg *config.Config) {
cookieSecure = cfg.CookieSecure
bindStateSecret = cfg.InternalSecret
if strings.TrimSpace(bindStateSecret) == "" {
- bindStateSecret = cfg.Casdoor.Secret
+ // Fall back only with a clear log; ideally require a dedicated secret.
+ bindStateSecret = cfg.Casdoor.Secret
+ }
+ if strings.TrimSpace(bindStateSecret) == "" {
+ log.Fatalf("bind-state secret is required (set INTERNAL_SECRET or CASDOOR_SECRET)")
}📝 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 signBindStatePayload(payload string) string { | |
| key := strings.TrimSpace(bindStateSecret) | |
| if key == "" { | |
| key = "costrict-bind-state-default" | |
| } | |
| h := hmac.New(sha256.New, []byte(key)) | |
| _, _ = h.Write([]byte(payload)) | |
| return hex.EncodeToString(h.Sum(nil)) | |
| } | |
| func verifyBindStatePayload(payload, sig string) bool { | |
| return hmac.Equal([]byte(signBindStatePayload(payload)), []byte(sig)) | |
| } | |
| func signBindStatePayload(payload string) string { | |
| key := strings.TrimSpace(bindStateSecret) | |
| if key == "" { | |
| // Fail closed: refuse to sign without a configured secret. | |
| // signBindStatePayload should never be reached if InitCookieConfig | |
| // validated configuration at boot. Returning an unguessable random | |
| // signature here ensures verifyBindStatePayload always rejects. | |
| return "" | |
| } | |
| h := hmac.New(sha256.New, []byte(key)) | |
| _, _ = h.Write([]byte(payload)) | |
| return hex.EncodeToString(h.Sum(nil)) | |
| } | |
| func verifyBindStatePayload(payload, sig string) bool { | |
| if strings.TrimSpace(bindStateSecret) == "" { | |
| return false | |
| } | |
| return hmac.Equal([]byte(signBindStatePayload(payload)), []byte(sig)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/handlers/handlers.go` around lines 195 - 207, The current
sign/verify functions (signBindStatePayload, verifyBindStatePayload) use a
hardcoded fallback bindStateSecret and reuse Casdoor secret, enabling token
forgery; remove the literal default and stop using cfg.Casdoor.Secret as the
HMAC key, instead require a dedicated non-empty BIND_STATE_SECRET (e.g.,
bindStateSecret) and make signBindStatePayload error or panic/return empty when
the secret is missing; update verifyBindStatePayload to handle that case safely
(reject when secret absent); add a startup validation in InitCookieConfig that
checks cfg.InternalSecret or the dedicated BIND_STATE_SECRET is present and fail
fast (return error/exit) if not, so the service never runs with a missing
bind-state secret.
| func bindAuthCallback(c *gin.Context, state bindState) { | ||
| if state.ExpiresAt == 0 || time.Now().Unix() > state.ExpiresAt { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "invalid_or_expired_state"}) | ||
| return | ||
| } | ||
| code := c.Query("code") | ||
| if code == "" { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "code is required"}) | ||
| return | ||
| } | ||
| currentToken := middleware.ExtractToken(c) | ||
| if currentToken == "" || UserModule == nil || UserModule.Service == nil { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Authentication required"}) | ||
| return | ||
| } | ||
| currentClaims, err := userpkg.ParseJWTClaimsFromAccessToken(currentToken) | ||
| if err != nil { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid current session"}) | ||
| return | ||
| } | ||
| currentUser, err := UserModule.Service.GetOrCreateUser(currentClaims) | ||
| if err != nil || currentUser == nil || currentUser.SubjectID != state.UserSubjectID { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid binding session"}) | ||
| return | ||
| } | ||
|
|
||
| tokenResp, err := exchangeCodeForTokenFunc(code, state.CallbackURL) | ||
| if err != nil || tokenResp.AccessToken == "" { | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": fmt.Sprintf("Failed to exchange code for token: %v", err)}) | ||
| return | ||
| } | ||
| claims, err := userpkg.ParseJWTClaimsFromAccessToken(tokenResp.AccessToken) | ||
| if err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": "Failed to parse bound identity"}) | ||
| return | ||
| } | ||
| if state.Provider != "" && !strings.EqualFold(claims.Provider, state.Provider) { | ||
| c.JSON(http.StatusConflict, gin.H{"error": "provider_mismatch"}) | ||
| return | ||
| } | ||
| if err := UserModule.Service.BindIdentityToUser(currentUser.SubjectID, claims); err != nil { | ||
| if err.Error() == "identity_already_bound" { | ||
| c.JSON(http.StatusConflict, gin.H{"error": "identity_already_bound", "message": "该登录方式已绑定其他账号"}) | ||
| return | ||
| } | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to bind identity"}) | ||
| return | ||
| } | ||
| redirectURL := state.RedirectTo | ||
| if redirectURL == "" { | ||
| redirectURL = defaultFrontendURL + "/settings/account?bind=success" | ||
| } | ||
| c.Redirect(http.StatusFound, redirectURL) | ||
| } |
There was a problem hiding this comment.
Browser-redirect callback returns JSON on errors; users will see raw error blobs.
Every error path in bindAuthCallback (invalid_or_expired_state, code is required, Authentication required, provider_mismatch, identity_already_bound, etc.) responds with c.JSON(...). Since this endpoint is reached via a browser redirect from Casdoor, the user lands on a JSON page rather than the originating UI. On the success path you redirect to state.RedirectTo (line 474), so consider redirecting on failure too with an error=... query parameter, e.g. redirectURL + "?bind=error&reason=invalid_state", so the frontend can surface a friendly message.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/handlers/handlers.go` around lines 422 - 475, The handler
bindAuthCallback currently returns JSON on all error paths which exposes raw
error pages to users; change each error response to perform an HTTP redirect to
the original frontend (use state.RedirectTo if set, otherwise defaultFrontendURL
+ "/settings/account") appending query params like bind=error and
reason=<short_code> (e.g. invalid_or_expired_state, code_required,
auth_required, invalid_session, binding_session_invalid, token_exchange_failed,
parse_identity_failed, provider_mismatch, identity_already_bound) so the
frontend can display a friendly message; ensure you still log the detailed error
server-side (not sent to user) and preserve distinct status codes internally but
always return an HTTP redirect to the computed redirectURL on every failure path
in bindAuthCallback.
| currentToken := middleware.ExtractToken(c) | ||
| if currentToken == "" || UserModule == nil || UserModule.Service == nil { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Authentication required"}) | ||
| return | ||
| } | ||
| currentClaims, err := userpkg.ParseJWTClaimsFromAccessToken(currentToken) | ||
| if err != nil { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid current session"}) | ||
| return | ||
| } | ||
| currentUser, err := UserModule.Service.GetOrCreateUser(currentClaims) | ||
| if err != nil || currentUser == nil || currentUser.SubjectID != state.UserSubjectID { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid binding session"}) | ||
| return | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm ParseUnverifiedTokenClaims really skips signature verification
ast-grep --pattern $'func ParseUnverifiedTokenClaims($_) ($_, $_) {
$$$
}'
# Find how /api/auth/bind/callback is wired (middleware applied?)
rg -nP -C3 'auth/bind/callback|bind/callback|StartBindAuth|bindAuthCallback' --type=goRepository: XDfield/costrict-web
Length of output: 5140
🏁 Script executed:
# Check the middleware applied to the auth router group
grep -n -B 10 -A 5 'auth.GET.*bind/callback' server/cmd/api/main.go | head -50
# Also check the full auth router setup to see what middleware is applied
sed -n '210,230p' server/cmd/api/main.go
# Check if there's any middleware wrapping the auth group
sed -n '200,240p' server/cmd/api/main.goRepository: XDfield/costrict-web
Length of output: 3355
🏁 Script executed:
# Get the full bindAuthCallback implementation (starting at line 422)
sed -n '422,460p' server/internal/handlers/handlers.go
# Find GetOrCreateUser to see if it creates users
rg -n 'func.*GetOrCreateUser' --type=go -A 15Repository: XDfield/costrict-web
Length of output: 7168
🏁 Script executed:
# Get the full GetOrCreateUser function
sed -n '230,280p' server/internal/user/service.goRepository: XDfield/costrict-web
Length of output: 1843
🏁 Script executed:
# Continue reading GetOrCreateUser to see the user creation logic
sed -n '280,320p' server/internal/user/service.goRepository: XDfield/costrict-web
Length of output: 1218
🏁 Script executed:
# Continue to see user creation logic
sed -n '320,360p' server/internal/user/service.goRepository: XDfield/costrict-web
Length of output: 1696
🏁 Script executed:
# Continue to see user creation logic
sed -n '360,410p' server/internal/user/service.goRepository: XDfield/costrict-web
Length of output: 1817
Verify the access token signature before parsing and trusting claims in /api/auth/bind/callback.
The endpoint calls ParseJWTClaimsFromAccessToken() which uses unverified JWT parsing (see server/internal/authidentity/normalize.go:30—parser.ParseUnverified()), allowing a forged token to reach GetOrCreateUser(). Since GetOrCreateUser() creates new users when claims don't match existing identities, a crafted token can create phantom user records. The SubjectID equality check happens after user creation, so the side effect occurs regardless of the subsequent validation failure.
Recommended fix: Either verify the token signature using the same validator the auth middleware uses, or replace GetOrCreateUser() with GetUserByID(state.UserSubjectID) to prevent user creation from unverified claims. At minimum, ensure user lookup—not creation—occurs before trusting unverified token identifiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/handlers/handlers.go` around lines 432 - 446, The code
currently parses unverified claims via ParseJWTClaimsFromAccessToken and then
calls UserModule.Service.GetOrCreateUser which can create users from forged
tokens; change the flow to verify the access token signature before trusting
claims (use the same validator the auth middleware uses) or avoid creation by
replacing GetOrCreateUser with a non-creating lookup such as
UserModule.Service.GetUserByID(state.UserSubjectID); specifically, after
obtaining currentToken via middleware.ExtractToken, validate the token signature
and only then call GetOrCreateUser (or, preferably, call GetUserByID with
state.UserSubjectID) so that unverified claims cannot cause new user creation
and the subsequent SubjectID check is meaningful.
| func (s *UserService) UnbindIdentity(userSubjectID string, identityID uint) error { | ||
| return s.db.Transaction(func(tx *gorm.DB) error { | ||
| var identity models.UserAuthIdentity | ||
| if err := tx.Where("id = ? AND user_subject_id = ?", identityID, userSubjectID).Take(&identity).Error; err != nil { | ||
| return err | ||
| } | ||
|
|
||
| var count int64 | ||
| if err := tx.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", userSubjectID).Count(&count).Error; err != nil { | ||
| return err | ||
| } | ||
| if count <= 1 { | ||
| return fmt.Errorf("cannot unbind last identity") | ||
| } | ||
|
|
||
| wasPrimary := identity.IsPrimary | ||
| if err := tx.Delete(&identity).Error; err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if wasPrimary { | ||
| var remaining []*models.UserAuthIdentity | ||
| if err := tx.Where("user_subject_id = ?", userSubjectID).Find(&remaining).Error; err != nil { | ||
| return err | ||
| } | ||
| best := selectBestPrimary(remaining) | ||
| if best != nil { | ||
| if err := tx.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", userSubjectID).Update("is_primary", false).Error; err != nil { | ||
| return err | ||
| } | ||
| if err := tx.Model(&models.UserAuthIdentity{}).Where("id = ?", best.ID).Update("is_primary", true).Error; err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return s.refreshUserProfileFromIdentitiesTx(tx, userSubjectID) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Concurrency: TOCTOU race can leave a user with zero identities.
The count <= 1 guard (line 178) and the subsequent tx.Delete (line 183) are read/write within a transaction, but the count query is not pessimistically locked. With Postgres' default Read Committed isolation, two concurrent UnbindIdentity calls on the same user (when count == 2) can both observe count = 2, both pass the guard, and both delete their respective rows — leaving the user with 0 identities, which the invariant explicitly forbids. The user would then be unable to log in via any provider.
Lock the user's identity rows for update before counting, or count after delete inside the transaction:
🔒️ Proposed fix (pessimistic lock)
return s.db.Transaction(func(tx *gorm.DB) error {
var identity models.UserAuthIdentity
- if err := tx.Where("id = ? AND user_subject_id = ?", identityID, userSubjectID).Take(&identity).Error; err != nil {
+ if err := tx.Clauses(clause.Locking{Strength: "UPDATE"}).
+ Where("user_subject_id = ?", userSubjectID).
+ Find(&[]models.UserAuthIdentity{}).Error; err != nil {
+ return err
+ }
+ if err := tx.Where("id = ? AND user_subject_id = ?", identityID, userSubjectID).Take(&identity).Error; err != nil {
return err
}
var count int64
if err := tx.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", userSubjectID).Count(&count).Error; err != nil {
return err
}
if count <= 1 {
return fmt.Errorf("cannot unbind last identity")
}(clause is already imported in this file.)
📝 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 (s *UserService) UnbindIdentity(userSubjectID string, identityID uint) error { | |
| return s.db.Transaction(func(tx *gorm.DB) error { | |
| var identity models.UserAuthIdentity | |
| if err := tx.Where("id = ? AND user_subject_id = ?", identityID, userSubjectID).Take(&identity).Error; err != nil { | |
| return err | |
| } | |
| var count int64 | |
| if err := tx.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", userSubjectID).Count(&count).Error; err != nil { | |
| return err | |
| } | |
| if count <= 1 { | |
| return fmt.Errorf("cannot unbind last identity") | |
| } | |
| wasPrimary := identity.IsPrimary | |
| if err := tx.Delete(&identity).Error; err != nil { | |
| return err | |
| } | |
| if wasPrimary { | |
| var remaining []*models.UserAuthIdentity | |
| if err := tx.Where("user_subject_id = ?", userSubjectID).Find(&remaining).Error; err != nil { | |
| return err | |
| } | |
| best := selectBestPrimary(remaining) | |
| if best != nil { | |
| if err := tx.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", userSubjectID).Update("is_primary", false).Error; err != nil { | |
| return err | |
| } | |
| if err := tx.Model(&models.UserAuthIdentity{}).Where("id = ?", best.ID).Update("is_primary", true).Error; err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| return s.refreshUserProfileFromIdentitiesTx(tx, userSubjectID) | |
| }) | |
| } | |
| func (s *UserService) UnbindIdentity(userSubjectID string, identityID uint) error { | |
| return s.db.Transaction(func(tx *gorm.DB) error { | |
| var identity models.UserAuthIdentity | |
| if err := tx.Clauses(clause.Locking{Strength: "UPDATE"}). | |
| Where("user_subject_id = ?", userSubjectID). | |
| Find(&[]models.UserAuthIdentity{}).Error; err != nil { | |
| return err | |
| } | |
| if err := tx.Where("id = ? AND user_subject_id = ?", identityID, userSubjectID).Take(&identity).Error; err != nil { | |
| return err | |
| } | |
| var count int64 | |
| if err := tx.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", userSubjectID).Count(&count).Error; err != nil { | |
| return err | |
| } | |
| if count <= 1 { | |
| return fmt.Errorf("cannot unbind last identity") | |
| } | |
| wasPrimary := identity.IsPrimary | |
| if err := tx.Delete(&identity).Error; err != nil { | |
| return err | |
| } | |
| if wasPrimary { | |
| var remaining []*models.UserAuthIdentity | |
| if err := tx.Where("user_subject_id = ?", userSubjectID).Find(&remaining).Error; err != nil { | |
| return err | |
| } | |
| best := selectBestPrimary(remaining) | |
| if best != nil { | |
| if err := tx.Model(&models.UserAuthIdentity{}).Where("user_subject_id = ?", userSubjectID).Update("is_primary", false).Error; err != nil { | |
| return err | |
| } | |
| if err := tx.Model(&models.UserAuthIdentity{}).Where("id = ?", best.ID).Update("is_primary", true).Error; err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| return s.refreshUserProfileFromIdentitiesTx(tx, userSubjectID) | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/user/service.go` around lines 167 - 205, The TOCTOU bug in
UserService.UnbindIdentity arises because the identity count is read without
locking, allowing two concurrent transactions to both see count==2 and delete
both identities; fix by acquiring a FOR UPDATE lock on the user's identity rows
before counting (e.g. use tx.Clauses(clause.Locking{Strength:
"UPDATE"})/tx.Where("user_subject_id = ?", userSubjectID).Find(...)) or
alternatively perform the count after the Delete within the same transaction
while holding a row lock, ensuring the check and delete are serialized; update
the logic around the tx.Where(...).Count(&count) and
tx.Delete(&identity)/refreshUserProfileFromIdentitiesTx to use the chosen
pessimistic lock so a concurrent UnbindIdentity cannot remove the last identity.
| } | ||
| if err := s.BindIdentityToUser(user.SubjectID, claims); err != nil && err.Error() != "identity_already_bound" { | ||
| return nil, err | ||
| } | ||
| if refreshed, err := s.GetUserByID(user.SubjectID); err == nil { | ||
| return refreshed, nil | ||
| } |
There was a problem hiding this comment.
Orphaned user on bind failure after Create.
s.db.Create(&user) on line 396 commits immediately. If BindIdentityToUser then fails (line 415), we return an error to the caller but a users row exists with no user_auth_identities row. Subsequent logins for this user via the same external_key will succeed (GetOrCreateUser re-finds the user) and re-attempt the bind, so this is self-healing — but until then, ListUserIdentities returns empty, and UnbindIdentity would refuse with "cannot unbind last identity". Wrap user creation + initial bind in a single transaction so partial state isn't possible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/user/service.go` around lines 414 - 420, The user creation
(s.db.Create(&user)) and initial bind (s.BindIdentityToUser) must be performed
inside a single DB transaction to avoid orphaned users when the bind fails;
change the flow so you begin a transaction, create the user via the transaction
handle, call BindIdentityToUser using the same transaction context/handle, and
commit only if bind succeeds (treating the "identity_already_bound" case as
non-fatal), otherwise rollback and return the bind error; finally, fetch the
user (s.GetUserByID) only after a successful commit or return the rolled-back
error. Ensure ListUserIdentities/UnbindIdentity behavior remains unchanged by
always using the committed state.
1 participant
Summary
Testing
Summary by CodeRabbit
New Features
Documentation