Fix "username already exists" errors when linking#157
Fix "username already exists" errors when linking#157Germ-99 wants to merge 5 commits intoEchoTools:mainfrom
Conversation
* Update score validation to include subscore checks in statistics queue * Update leaderboard record write to include metadata mapping * Fix statistics storage with operator tags for leaderboard operations
There was a problem hiding this comment.
Pull Request Overview
This PR fixes "username already exists" errors when linking Discord accounts to game headsets by implementing conflict resolution logic. The solution detects when a username conflict occurs during authentication and attempts to recover by either linking the Discord ID to an existing unlinked account or identifying when the account is already properly linked.
Key changes:
- Adds intelligent conflict detection and recovery for username collisions during account linking
- Implements three recovery paths: already-linked accounts, unlinked accounts, and genuine conflicts
- Introduces error type checking to distinguish AlreadyExists errors from other authentication failures
| // Link the account to the current discord user | ||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "conflicting_user_id": conflictingUserID, | ||
| "username": username, | ||
| }).Info("Attempting to recover by linking custom_id to conflicting account") | ||
|
|
||
| if err := d.linkCustomID(ctx, logger, conflictingUserID, discordID); err != nil { | ||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "conflicting_user_id": conflictingUserID, | ||
| "error": err, | ||
| }).Error("Failed to link custom ID during conflict recovery") | ||
| return "", fmt.Errorf("failed to recover account: %w", err) | ||
| } | ||
|
|
||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "user_id": conflictingUserID, | ||
| "username": username, | ||
| }).Info("Recovery successful: custom_id linked to conflicting account") | ||
|
|
||
| return conflictingUserID, nil |
There was a problem hiding this comment.
The automatic linking of custom ID to a conflicting account (lines 100-122) occurs without explicit user verification. This could be a security concern if:
- An attacker creates an account with a victim's Discord username before the victim links
- The victim then tries to link, and the system automatically associates their Discord ID with the attacker's account
Consider adding additional verification (e.g., checking account creation date, requiring explicit user confirmation, or validating that the account has minimal activity) before automatically linking to prevent potential account takeover scenarios.
There was a problem hiding this comment.
discord usernames are unique
| // Also check the error message string for the AlreadyExists code | ||
| // Good for cases where the error is wrapped or stringified | ||
| errMsg := err.Error() | ||
| return strings.Contains(errMsg, "code = AlreadyExists") || strings.Contains(errMsg, "AlreadyExists") |
There was a problem hiding this comment.
The string matching in isAlreadyExistsError checks for both "code = AlreadyExists" and "AlreadyExists" separately. The second check is redundant since any string containing "code = AlreadyExists" will also match "AlreadyExists". Consider simplifying to just check for "AlreadyExists" if the goal is to catch both formatted and unformatted error messages.
| return strings.Contains(errMsg, "code = AlreadyExists") || strings.Contains(errMsg, "AlreadyExists") | |
| return strings.Contains(errMsg, "AlreadyExists") |
| if conflictingCustomID != "" { | ||
| if conflictingCustomID == discordID { | ||
| // Return the existing user ID - they're already linked. | ||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "user_id": conflictingUserID, | ||
| "username": username, | ||
| }).Info("Recovery successful: user already linked with matching custom_id") | ||
| return conflictingUserID, nil | ||
| } | ||
|
|
||
| // This is for cases where a different user owns the conflicting account. | ||
| // This cannot be automatically resolved. | ||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "conflicting_user": conflictingCustomID, | ||
| "username": username, | ||
| }).Warn("Account conflict: different discord user owns the username") | ||
| return "", errors.New( | ||
| "Username is already linked to a different discord account. " + | ||
| "Reference: CONFLICT_DIFFERENT_OWNER", | ||
| ) | ||
| } | ||
|
|
||
| // Link the account to the current discord user | ||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "conflicting_user_id": conflictingUserID, | ||
| "username": username, | ||
| }).Info("Attempting to recover by linking custom_id to conflicting account") | ||
|
|
||
| if err := d.linkCustomID(ctx, logger, conflictingUserID, discordID); err != nil { | ||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "conflicting_user_id": conflictingUserID, | ||
| "error": err, | ||
| }).Error("Failed to link custom ID during conflict recovery") | ||
| return "", fmt.Errorf("failed to recover account: %w", err) | ||
| } | ||
|
|
||
| logger.WithFields(map[string]interface{}{ | ||
| "discord_id": discordID, | ||
| "user_id": conflictingUserID, | ||
| "username": username, | ||
| }).Info("Recovery successful: custom_id linked to conflicting account") | ||
|
|
||
| return conflictingUserID, nil |
There was a problem hiding this comment.
There's a potential race condition between checking the conflicting account's custom_id (line 76) and linking it (line 107). Another process could link a different discord ID to this account between these operations. Consider using a transaction or atomic operation if the underlying Nakama API supports it, or at least re-verify the custom_id is still empty after the link operation succeeds.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Andrew Bates <a@sprock.io>
- Simplify isAlreadyExistsError to remove redundant string check - Add comprehensive security documentation for automatic linking - Document race condition risks and mitigations - Fix test compilation issue with operatorFromTag/operatorFromStatField Co-authored-by: thesprockee <962164+thesprockee@users.noreply.github.com>
Signed-off-by: Andrew Bates <a@sprock.io>
2 participants
Fixes: #146