Repository navigation
fix(profile): only offer account linking to local sign-ins - #1261
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:
|
A linked session merges both platforms' perms, so a donor on one platform kept the other platform's trial perms after that trial ended, since clearNonDonor skips donor sessions. Linked sessions now record the strategies that granted trial perms in `trialStrategies`, and clearNonDonor also clears sessions listing the ending strategy there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
@Mygod one more thought: since each review round turns up another edge case in merging perms across linked sessions (trial expiry, donor status, webhooks), we could still go the simpler way and only allow linking from a local account, i.e. disable the link button for users signed in with Discord or Telegram. That would drop the cross-platform session merging entirely. Happy to go either way, which do you prefer? I'd rather not make this overcomplicated :) |
|
The linking flow permits unauthorized identity attachment through legacy Telegram callbacks, and linked trial-only scanner/webhook permissions can survive expiry. Both cases were reproduced with database-free checks despite the new tests passing. Full review comments:
|
Agreed with making this less complicated. I personally don't need this feature so as long as you don't introduce any vulnerabilities I'm fine. |
Linking from a Discord or Telegram session merged both platforms' perms into one session, which trial expiry and donor checks keyed on a single strategy cannot clear correctly. Only local sign-ins link now, and the profile hides the link buttons for other sessions; a callback for them signs in to that account like any other login. This drops the linkedStrategies and trialStrategies session tracking. Linking also has to be bound to the session that started it: the legacy Telegram widget's signed callback is not, so only the Telegram OAuth flow links, and the Discord strategy now checks the OAuth state. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fabio1988
changed the title
|
@Mygod simplified in 5ff94c9, linking now only works from a local sign-in:
Updated the PR title/description to match. |
|
The patch introduces a reproducible trial-permission expiry regression and overly broad database locking during account merges. Build, lint, formatting, and 30 focused tests passed, but live OAuth/MySQL integration was not exercised. Full review comments:
|
Linking Telegram to a Discord sign-in (or the reverse) fails, and making it work means merging two platforms' perms into one session. Hide the link buttons for Discord and Telegram sessions instead and leave the server's linking for local accounts as it is on main. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fabio1988
changed the title
|
@Mygod thanks, I've cut this down to the original problem in bd4bb8b: the link buttons are now hidden for Discord and Telegram sign-ins, and all server changes are reverted, so local account linking is exactly as on
Title and description are updated to match. |
## [1.57.1](v1.57.0...v1.57.1) (2026-11-02) ### Bug Fixes * **profile:** only offer account linking to local sign-ins ([#1261](#1261)) ([f01a1a2](f01a1a2))
|
🎉 This PR is included in version 1.57.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Problem
The profile's Link Discord / Link Telegram buttons are shown to every signed-in user, but the server only links accounts for local sign-ins. Linking Telegram to a Discord sign-in, or Discord to a Telegram sign-in, fails.
Making that work would mean merging both platforms' perms into one session, which trial expiry and donor checks (keyed on a single strategy) can't clear correctly. See the review discussion below.
Fix
Client only (
src/features/profile/LinkAccounts.jsx):Local account linking on the server is unchanged from
main.Testing
yarn testpasses (132/132), eslint and prettier are clean,yarn buildsucceeds.🤖 Generated with Claude Code