Repository navigation
Conversation
The profile offers "Link Discord" / "Link Telegram" to every signed-in user, but the server only linked when the signed-in row was a local account. Discord and Telegram users got a button that did nothing, and worse: - Telegram checked the row found by the Telegram id instead of the signed-in user, so linking Telegram never worked, not even for local accounts. - Linking Discord while signed in with Telegram rewrote the Telegram row's strategy to "discord" without storing a Discord id. Both clients now share linkAccount(): when a signed-in user authenticates with a platform their row has no id for, the id is attached to their row. A row that only existed for that account is merged in (gym badges keeping the higher badge, backups, nest submissions, profile data) and deleted, in one transaction, and its sessions are cleared. A local account, or one already linked to a different account of the other platform, is never merged away; the user lands on the blocked page with an explanation instead. Signing in with an account the row already has stays a plain login. Webhooks keep following the sign-in method: a Discord sign-in alerts the Discord account and a Telegram sign-in the Telegram one, so the webhook strategy selector stays reserved for local accounts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
We can either disable that button completely if it's a non-local user.. or we link it properly. I thought it's a good idea linking it, any thoughts @Mygod ? |
![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: 85267a0df0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| })} | ||
| {auth.discordId && auth.telegramId && ( | ||
| {/* Discord and Telegram sign-ins always alert the account they used */} | ||
| {auth.strategy === 'local' && auth.discordId && auth.telegramId && ( |
There was a problem hiding this comment.
For a locally authenticated user, LocalClient.authHandler never adds strategy to the object passed to done (it only updates the database row), and getServerSettings forwards that session object, so the client receives auth.strategy === ''. For exactly the users with both IDs, this new predicate is therefore always false and removes the only UI for changing webhookStrategy; serialize strategy: 'local' or gate on a value that local sessions actually contain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch, fixed in 6219114: local sessions carry no strategy, so the selector is now gated on "not a Discord or Telegram sign-in" (!['discord', 'telegram'].includes(auth.strategy)) instead of === 'local'.
| const [current, other] = await Promise.all([ | ||
| User.query().findOne({ id: userId }), | ||
| User.query().findOne({ [field]: externalId }), | ||
| ]) |
There was a problem hiding this comment.
When two link callbacks for the same signed-in row overlap (for example, two sessions authorize different Discord accounts before either callback commits), both reads here can observe the platform field as empty because they run before the transaction at line 145. Both callbacks then move/delete their respective account rows and update the same current row; the last ID wins, so one login identity is lost even though its badges, backups, and data have already been coalesced. Read, plan, and lock both rows in one query inside the transaction before mutating them.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6219114: both rows are now read in one query with FOR UPDATE inside the transaction that plans and applies the merge, so a second callback for the same user waits and then sees the platform id already set.
| ...reqUser, | ||
| ...linked, | ||
| username: reqUser.username || platformUser.username, | ||
| perms: mergePerms(reqUser.perms, platformUser.perms), |
There was a problem hiding this comment.
When a local row with no provider IDs absorbs an existing row that already has both IDs, linkAccount copies the second provider and its stored permissions at lines 139-142, but this returned session still unions only the local permissions and the provider just authenticated. The transferred provider's roles remain unavailable for the whole new session (for example, Telegram-only features are unavailable after linking Discord) even though the database row says Telegram is linked; merge the carried permission set into the serialized user as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in 6219114 by refusing that case: a row that already has both ids is a linked account of its own and is no longer merged away, so no provider id (or its permissions) is carried over anymore.
| if (!current.data && merge.data) update.data = merge.data | ||
| if (!current[otherField] && merge[otherField]) { | ||
| update[otherField] = merge[otherField] | ||
| update[`${otherPlatform}Perms`] = merge[`${otherPlatform}Perms`] |
There was a problem hiding this comment.
When a dual-provider row formed by this helper is later absorbed into a local row via the provider that was linked second, the ordinary initial provider login never populated the original provider's *Perms, while line 130 saved only the second provider's permissions. This assignment therefore carries the original provider ID with a null permission set and then deletes the source row, so future local logins cannot gain original-provider-only roles despite reporting that provider as linked; persist or reauthenticate the carried provider before transferring its ID.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same as above, resolved in 6219114: a row with both ids is refused instead of absorbed, so the carry-over path (and the null-permission transfer) is gone.
| await state.db.models.User.query() | ||
| .findOne(req.user ? { id: req.user.id } : { discordId: discordUser.id }) | ||
| .findOne({ discordId: discordUser.id }) |
There was a problem hiding this comment.
When a signed-in user reauthenticates or switches an already-linked Discord account, linkAccount has just selected the incoming Discord row at linkAccount.js:122 to return the login plan, and this branch immediately performs the identical lookup again; the Telegram path does the same. Return and reuse the fetched row so these normal auth paths do not spend a redundant model query.
AGENTS.md reference: AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6219114: linkAccount returns the row it read with the login plan and the Discord/Telegram clients reuse it instead of querying again.
- Show the webhook strategy selector to local sessions again. They carry no strategy, so gating on strategy === 'local' hid it for everyone; gate on "not a Discord or Telegram sign-in" instead. - Read and lock both rows (FOR UPDATE, one query) inside the transaction that merges them, so concurrent link callbacks for the same user cannot both link. - Refuse to merge away a row that already has both ids. It is a linked account of its own, and carrying its second id over would carry it without that provider's permissions. - Hand the row read for a plain login back to the Discord and Telegram clients instead of querying it again. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The new linking flow can retain expired trial permissions and leave webhook access unusable. Focused linking and Telegram tests, lint, and formatting passed; one adjacent Discord test failed because sandbox restrictions returned EPERM instead of ENOENT. Live OAuth/database integration and a production build were not run. Full review comments:
|
A session that linked a second platform kept only its own rmStrategy, so the trial cleanup of the linked platform never matched it and its trial perms outlived the trial. Sessions now list every strategy they hold perms from in linkedStrategies, and clearNonDonor/clearTrial match either. Linking also overwrote the computed webhook fallback with the session's null selectedWebhook. linkAccount now stores the fallback on the row when it has none and returns it with the linked fields. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
Linked trial permissions can outlive their expiry when either account contributes donor status. Focused tests and changed-file lint/format checks passed; live OAuth/database integration and production build were not run. Review comment:
|
Problem
The profile's Link Discord / Link Telegram buttons are shown to every signed-in user, but the server only linked when the signed-in row was a local account:
DiscordClient.js):if (req.user && userExists?.strategy === 'local'), so for a Telegram user the button did nothing. Worse, the fallthrough rewrote the Telegram row'sstrategytodiscordwithout storing adiscordId.TelegramClient.js): the same check ran against the row found by the Telegram id, not the signed-in user, so linking Telegram never worked, not even for local accounts.Users with both platforms ended up with two separate rows and a button that silently did nothing.
Fix
Both clients now share
server/src/services/linkAccount.js. When a signed-in user authenticates with a platform their row has no id for:FOR UPDATEinside it): gym badges are moved (where both have a badge for the same gym, the higher one is kept), backups, nest submissions and profiledataare moved, and that row's sessions are cleared.account_already_linkedmessage instead.Webhooks keep following the sign-in method. A Discord sign-in manages the Discord Poracle user and a Telegram sign-in the Telegram one. Because of that, the webhook strategy selector in the profile is now only shown to local accounts, the only sign-in that reads it.
Testing
server/test/linkAccount.test.jscovers the link/merge/refuse/login decision and the session user.linkAccountagainst MariaDB 11.4 with the real migrations: merge, badge de-duplication, moving backups and nest submissions, a plain re-login, and refusing a local account.node --test server/test/*.test.js: 144/144 pass. eslint and prettier are clean.🤖 Generated with Claude Code