Refactor and expand authentication system with account handling#22
Refactor and expand authentication system with account handling#22
Conversation
- Added `AuthFlow`, `AuthRegistry`, and `ConnState` for managing user authentication states. - Introduced `AccountRepository` with in-memory and YAML-based implementations for account handling. - Integrated `PasswordHasher` for secure password hashing. - Updated GameEngine to handle authentication flow, guest logins, and player reconnections. - Modified tests to handle guest accounts.
- Simplified `ConnState` by replacing unnecessary data classes with `data object`s. - Adjusted `AuthFlow` to properly trim input and improve password handling. - Updated `YamlAccountRepository` file writing to include `StandardOpenOption.WRITE` for compatibility. - Reordered test events in `GameEngineIntegrationTest` for better timing control. - Removed redundant state initialization in `AuthRegistry` and cleanup in `GameEngine`.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a752dce9e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| accounts.create(account) | ||
| } catch (e: Exception) { | ||
| outbound.send(OutboundEvent.SendError(sessionId, "Account creation failed.")) | ||
| renderMenu(sessionId) | ||
| return |
There was a problem hiding this comment.
If accounts.create(account) throws (I/O error, concurrent write, etc.), the code returns to the menu but leaves the newly created playerRecord on disk. Because handleSignupUsername rejects any name found by playersRepo.findByName, the user can never retry signup for that username even though no account exists. Consider rolling back the player record or creating the account first so a failed account write doesn’t permanently reserve the name.
Useful? React with 👍 / 👎.
| try { | ||
| return playersRepo.create(name, worldStartRoom, now) | ||
| } catch (_: Exception) { | ||
| // Try again with a new guest name. | ||
| } |
There was a problem hiding this comment.
createGuestRecord retries forever on any exception. If playersRepo.create fails due to a persistent problem (e.g., disk full or unreadable data dir), this loop never exits, so the login handler never returns and the engine tick can stall. It should bound retries and surface a failure instead of swallowing all exceptions indefinitely.
Useful? React with 👍 / 👎.
- Introduced `delete` method in `PlayerRepository` and implemented it in YAML and in-memory repositories. - Enhanced `AuthFlow` for more robust guest account creation with retry logic and failure handling. - Added cleanup for failed account creation to avoid leftover reserved names.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43c0a2c21c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| players.attachExisting(sessionId, record) | ||
| authRegistry.set(sessionId, Authed(record.id)) | ||
| postAuth(sessionId) |
There was a problem hiding this comment.
Because guest logins are immediately marked Authed and sent into the normal command router, a guest can still run the name <existing> command, which PlayerRegistry.rename treats as a login to any offline persisted player record. That means anyone can bypass the new password-based auth by logging in as guest and renaming to an existing character. Consider disabling name-based logins or requiring an account-bound session before allowing name to attach to an existing record.
Useful? React with 👍 / 👎.
|
@codex Look good? |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dd2f7aa88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // If a persisted player with this name exists, treat as login (unless it belongs to someone online) | ||
| val existingRecord: PlayerRecord? = repo.findByName(newName) | ||
| val sameRecord = existingRecord?.id == ps.playerId | ||
| if (existingRecord != null && !sameRecord && !ps.accountBound) return RenameResult.Taken | ||
|
|
There was a problem hiding this comment.
The new account-gated login-by-name path allows any account-bound session to attach to an existing player record, even when the record’s id does not match the logged-in account’s player. In rename, the guard only blocks non‑account sessions (!ps.accountBound) and then immediately binds existingRecord regardless of ownership, so an authenticated user can rename to another offline player’s name and take over that character. This breaks the account/character isolation introduced in this commit. Consider checking that existingRecord.id == ps.playerId (or an account→player mapping) before allowing the bind, and otherwise return Taken.
Useful? React with 👍 / 👎.
jnoecker
force-pushed
the
features/login
branch
from
7dd2f7a to
bc93bee
Compare
There was a problem hiding this comment.
💡 Codex Review
AmbonMUD/src/main/kotlin/dev/ambon/engine/PlayerRegistry.kt
Lines 152 to 156 in bc93bee
Keep account linkage in sync on rename
The name command still runs for authenticated players, and rename can create a new PlayerRecord and then rebind ps.playerId. However, the account table remains linked to the old playerId (login uses account.playerId), so after a rename the next login loads the old record and the new one becomes orphaned. To avoid this, either disallow renaming for account‑bound sessions or update the account record when ps.playerId changes.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| override fun findByUsernameLower(usernameLower: String): AccountRecord? { | ||
| val key = usernameLower.trim().lowercase() | ||
| if (key.isEmpty()) return null | ||
| val path = pathFor(key) | ||
| val af = readAccountFile(path) ?: return null |
There was a problem hiding this comment.
Because findByUsernameLower uses the raw (trimmed/lowercased) username to build a filename, a login attempt with a username like ../players/123 can escape data/accounts and try to parse arbitrary *.yaml files. If any such file exists but isn’t an AccountFile, readAccountFile throws and the exception bubbles up through handleLoginPassword, which can terminate the engine. This is reachable because login usernames aren’t validated, so either reject non‑[A-Za-z0-9_] at login or sanitize the key before calling pathFor.
Useful? React with 👍 / 👎.
- Added `isValidKey` validation to `YamlAccountRepository` for ensuring usernames use only letters, digits, or underscores. - Updated `AuthFlow` to reject invalid usernames during login. - Adjusted `PlayerRegistry` to support renaming for account-bound sessions, preserving player IDs. - Enhanced tests to cover new validation logic and account-bound rename behavior.
|
Too much unsupervised AI and overall too big and complicated. |
1 participant
Summary
ConnState,AuthFlow, etc.) for improved clarity and efficiency.AuthRegistry,AuthFlow, andAccountRepository(with in-memory and YAML-based implementations).PasswordHasher.GameEngineto manage the authentication flow, including guest logins and reconnections.GameEngineIntegrationTestto reorder events for better timing control.