Skip to content

Add player auth migration API#893

Merged
tudddorrr merged 1 commit intodevelopfrom
player-auth-migration
Mar 22, 2026
Merged

Add player auth migration API#893
tudddorrr merged 1 commit intodevelopfrom
player-auth-migration

Conversation

@tudddorrr
Copy link
Copy Markdown
Collaborator

No description provided.

@tudddorrr tudddorrr added the enhancement New feature or request label Mar 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.29%. Comparing base (1eccc6c) to head (46ef570).
⚠️ Report is 2 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #893      +/-   ##
===========================================
- Coverage    97.30%   97.29%   -0.01%     
===========================================
  Files          402      403       +1     
  Lines         6450     6481      +31     
  Branches       841      844       +3     
===========================================
+ Hits          6276     6306      +30     
  Misses          90       90              
- Partials        84       85       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@claude
Copy link
Copy Markdown

claude Bot commented Mar 21, 2026

PR Review: Add player auth migration API

Overall this is a clean implementation with good test coverage. One functional bug and one missing validation found.


Missing validation that the migrated alias is a Talo alias (Major)

loadAliasWithAuth only checks that the alias belongs to a player in the game and that player.auth exists - it does not validate alias.service === PlayerAliasService.TALO. In games with merged players (a player who merged a Talo account with a Steam account), calling this endpoint with the Steam alias ID would:

  1. Pass auth (player has PlayerAuth)
  2. Modify the Steam alias service and identifier to the new target
  3. Remove PlayerAuth from the player, leaving the Talo alias without credentials

A guard should be added in migrate.ts after the password check:

if (alias.service !== PlayerAliasService.TALO) {
  return ctx.throw(400, {
    message: 'Only Talo aliases can be migrated',
    errorCode: 'INVALID_MIGRATION_TARGET',
  })
}

trimmedService computed late and inconsistently (Minor)

trimmedService is assigned at line 75 inside the transaction, but the TALO guard at line 56 already uses service.trim(). Computing trimmedService once before the transaction and reusing it in the TALO guard would be cleaner.


Test Coverage

No test covers passing a non-Talo alias (e.g. a merged player sending a Steam alias ID). Worth adding given the above.

@tudddorrr tudddorrr force-pushed the player-auth-migration branch from c66d970 to 46ef570 Compare March 21, 2026 18:25
@tudddorrr tudddorrr merged commit 8d6d836 into develop Mar 22, 2026
10 checks passed
@tudddorrr tudddorrr deleted the player-auth-migration branch March 22, 2026 11:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant