Skip to content

People can link their Slack and GitHub accounts - #763

Open
guidovizoso wants to merge 102 commits into
mainfrom
guido/github-plugin-spec
Open

guidovizoso wants to merge 102 commits into
mainfrom
guido/github-plugin-spec

Conversation

@guidovizoso

Copy link
Copy Markdown
Collaborator

What this changes

People can link the accounts they use elsewhere to their OpenBot user, starting with Slack and GitHub. This is the groundwork for the GitHub integration (alerts such as "my PR's CI failed", and reviews): those need to know which OpenBot person a GitHub or Slack user is.

  • Identity links (provider-neutral). New identity_links and identity_link_challenges tables (migration 0053_identity_links, only adds tables). An identity is {provider, realm, subject}. Each identity belongs to at most one person, and each person has at most one link per provider and realm. Removing a person retires their links and revokes the stored tokens.

  • Settings → Connected accounts → Linked accounts. One card per account type: the linked account with Disconnect, or a "Not linked" card with Connect. A one-time notice after linking (?linked=github | github-taken | failed).

  • Slack, through OpenTag. Connect issues a one-time code (10 minutes). The person sends link <code> to the OpenTag app in a direct message. A code posted in a channel, with or without a mention, is cancelled and the person is told to make a new one. Reachability codes are still checked first and behave as before.

  • GitHub, through a GitHub App's user sign-in. POST /api/identity/github/connect returns the authorize URL with a sealed, 10-minute state. GET /api/identity/github/callback:

    • requires the same signed-in session as the person in the state, so a link started by one person can't attach another person's GitHub account (login CSRF);
    • exchanges the code and reads the numeric GitHub id;
    • stores the tokens encrypted as a connector credential and links;
    • writes identity.linked, then redirects back.

    A failed re-link leaves the existing link and token untouched. An account already linked to someone else gets its own notice.

  • Config. GITHUB_APP_CLIENT_ID + GITHUB_APP_CLIENT_SECRET (both or neither). The server refuses to start with them set and no public URL (OPENBOT_PUBLIC_URL or BETTER_AUTH_URL). Each linking option is offered only when its return path is actually wired.

  • Docs. docs/configuration.md and .env.example.

Nothing reads the links yet. The GitHub plugin itself (Bot tools, alerts, reviews posted after approval) is the next milestone.

Where it runs

  • New state that outlives a request? Postgres only: identity_links, identity_link_challenges (codes stored as sha256 hashes) and the encrypted credentials row. The GitHub OAuth state is sealed with the deployment key and carries the person and issue time, so it needs no server-side storage.
  • What happens on the second replica? Nothing different. The connect request and the callback can land on different processes: the callback opens the sealed state with the shared key and reads everything else from Postgres. A Slack code issued on one replica is redeemed on another through the hashed row.
  • Anything serialised?
    • Linking takes advisory locks per person and per identity, and is backed by unique indexes on (provider, realm, subject), (user_id, provider, realm) and the credential id.
    • Redeeming a code deletes it in the same transaction, conditional on it not having expired, so a code works once.
    • Issuing a code upserts on a unique index, so there is one live code per person and provider.
  • Anything fanned out to a browser? No.
  • New listener, port, or schedule? No. The callback is a route on the existing API (/api/identity/github/callback), mounted behind requireUser.

Boundary and audit

  • No new Bot acting calls. Linking is the person's own action in Settings and goes through the session.
  • Audit rows: identity.linked, identity.unlinked, identity.link_retired (written in the retirement transaction). Refused or failed GitHub callbacks write a structured log line with a reason or stage, and never any codes, tokens, state or account ids. They don't write an audit row, because nothing changed.
  • Nothing is trusted from the client. The GitHub identity comes from GitHub's API with the token the server exchanged itself. The Slack identity comes from the OpenTag-signed sender.

Changelog

  • CHANGELOG.md, under Unreleased: "People can link their Slack and GitHub accounts".

How it was checked

  • Server: 4,505 tests, including database integration tests for the store, the GitHub callback (conflict, wrong-session and reconnect cases) and the migration.
  • App: 1,274 tests. Both typechecks, the app build and Biome are clean.
  • Reviewed with an unbiased multi-agent review (two full rounds and two focused rounds). The findings that are left are written down as follow-ups. The most important: Reachability link <uuid> codes are still accepted in shared channels (this predates the PR), and a GitHub token from a failed connect isn't revoked at GitHub.
  • Tried by hand against a real GitHub App: connect, the redirect back, the cancel and forged-state paths, and the notices.

A chat-started link was confirmed by whoever was signed in when they
opened its URL, so someone could start a link from their own Slack,
send the URL to a victim, and have their Slack identity attached to the
victim's OpenBot user once the victim confirmed.

OpenBot now issues the code to the signed-in person instead
(issueChallenge), and the link is made when that code arrives from their
chat account (redeemChallenge). The code is a random UUID so a chat
handler can accept `link <uuid>`, matching the OpenTag Reachability
pairing; only its sha256 is stored, it lasts ten minutes, issuing again
replaces the person's previous code, and a refused redeem (wrong
provider, identity already linked elsewhere) leaves the code unused.
The browser confirmation routes and their origin check are removed; no
route issues codes until a chat consumer exists.

Migration 0052 had not shipped, so it is regenerated rather than
followed by 0053.

Removed or renamed symbols and their call sites (git grep over server,
app and tests, excluding docs/superpowers plans):
- beginChallenge, peekChallenge, confirmChallenge, completeChallenge:
  store.ts, routes.ts, identity-routes and identity-challenges tests;
  all removed or rewritten.
- completionHint -> instruction: providers.ts, routes.ts,
  identity-providers test.
- trustedAppOrigins: app.ts only; Learning mount restored to main's
  inline origins.
- /challenges/peek, /challenges/confirm: routes.ts and the app's
  queries/mutations (removed in the following commit); the only
  remaining mention is the test pinning that they 404.
The server no longer confirms chat-started links (codes are issued to
the signed-in person instead), so /settings/link, its link-state helper
and the challenge query and confirm mutation go. The route tree is
regenerated and matches main's.

Call sites of removed symbols: challengeQueryOptions, LinkChallenge,
identityKeys.challenge, confirmChallengeMutationOptions and the
link-state exports were used only by settings/link.tsx and
link-state.test.ts, both deleted.
Removing a person retired their plugin connections but left the tokens on
their identity links live. retireIdentityLinks revokes each linked
credential and marks the link needs_reconnect with a null credential_id,
keeping the rows so the outside account is still refused, not treated as a guest.

Call sites of retireIdentityLinks: one, the person-removal retirer passed to
createPeopleStore in server/src/index.ts, after retireConnectionsFor.
On a reconnect the callback rotated the live github-user-token credential
up front (committing the old token's revocation) and only then called
linkVerified. If the link write threw, the catch revoked the new token too,
leaving the person's link active but pointing at a revoked token while the
page said nothing was saved.

The callback no longer rotates. Each connection stores its token with
create() under its own key, `${userId}:${githubId}:<uuid>`, so it can never
collide with the live token (credentials_active_key_idx) and the old one
is not touched before the link is written. linkVerified -> writeLink
already revokes the credential the link previously named, inside the same
transaction that repoints the link (revokeQuietly on existing.credentialId
for a relink, on previous.credentialId when a different account replaces
the realm's link). One live token per person+account still holds; on any
failure the callback revokes only the token this attempt stored.

Readers of keyId / findLiveByKey checked:
- identity/store.ts claimCredential: selects the link's credential by id,
  kind "connector" and provider "github-user-token"; never by keyId.
- plugins/store.ts (two findLiveByKey calls): MCP user-connection and
  OAuth-client keys, other providers.
- credentials.ts addCredential service: admin-entered keys.
No code looks a github-user-token up by keyId; the link holds it by id.

The callback's credential deps narrow to create and revoke.
Nothing read config.githubApp.slug. Removed: GithubAppConfig.slug and its parser (server/src/config.ts), the GITHUB_APP_SLUG lines in .env.example, the slug mention in docs/configuration.md, and the slug test case (now asserts the setting is ignored).
Renumbers the identity links migration to 0053_identity_links (main added
0052_shared_brokered_accounts); the generated SQL is unchanged. Linked
accounts stays the last section on Connected accounts, after main's new
Shared by your organisation section.
// their own Slack account to its issuer, so an account code is only redeemed in a direct
// message and one posted here is cancelled (an unmentioned channel post is cancelled in
// `withoutLinkCodes`).
if (!sender.private) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When someone mentions the app in a channel, only the first link <code> match is handled (LINK.exec in answer() passes one code to link()). Any other account codes in the same message are neither cancelled nor redacted. The unmentioned (observe) path cancels every code through withoutLinkCodes.

Scenario: @OpenBot link <code-A> link <slack-account-code> is posted in a shared channel. The second code stays live and readable to everyone there, so any member can DM link <code> to the app within 10 minutes and bind their own Slack account to the person who issued the code. Suggested fix: in the non-private Slack branch, cancel every account code in the message (for example, reuse the LINKS loop from withoutLinkCodes), not only code.

@davidmckayv davidmckayv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against main at 4773ef68 (this branch already contains it; fast-forward, no conflicts). Approving, with four nits.

Security

  • GitHub: the state is sealed with AES-256-GCM under its own label from the deployment key (auth/signed-value.ts) and refused after 10 minutes or when future-dated (identity/github-oauth.ts). The callback requires the signed-in session to be the person in the state, which blocks login CSRF (identity/github-callback.ts), and is mounted behind requireUser (app.ts:708). Redirects are only to fixed /settings/connected-accounts?linked= values, so no open redirect. The code exchange has a timeout and does not follow redirects. Tokens are stored as an encrypted connector credential; logs carry the stage and error name only. A failed link revokes the new credential; an account already linked to someone else gets github-taken and changes nothing.
  • Linking: writeLink takes a per-person advisory lock plus a row lock and refuses an identity owned by someone else; re-linking revokes the old token; the 0053 unique indexes enforce one owner per identity and one link per person per realm.
  • Slack: codes are randomUUID() (122 bits), stored as sha256, 10-minute, single-use, so brute force isn't realistic. The DM check reads OpenTag's sender context, trusted only after the shared-secret check (delivery/opentag.ts:285). Reachability codes are still matched first, and identity codes must be UUID-shaped before any query (identity/store.ts:342). A code posted in a channel is cancelled, and link codes are stripped from text passed to Slack triggers.
  • Removal: retireIdentityLinks runs inside peopleStore.retireOwned, which covers admin removal (app.ts:1026) and directory deprovisioning (admin/controls.ts:533), and revokes tokens even if the plugin half fails.

Migration 0053 follows 0052 in the journal, its snapshot prevId is 0052's id, it applies cleanly to a fresh database, and drizzle-kit check reports no drift.

Tests: typecheck, lint, format:check and build pass. test:ci against a fresh PostgreSQL: this branch 6,345 pass, main 6,082 pass, with the same 13 local-environment failures on both and nothing new. No new dependencies; the only outbound hosts are github.com and api.github.com.

Nits

  1. The GitHub state isn't single-use (identity/github-oauth.ts), so it can be replayed within 10 minutes. Session binding and GitHub's single-use code make that harmless today; a one-time nonce would harden it.
  2. identity_links.user_id and identity_link_challenges.user_id have no foreign key to users (server/drizzle/0053_identity_links.sql), so cleanup relies entirely on the retire path.
  3. A missing session on the callback answers 401 JSON rather than the ?linked=failed notice, so someone whose session expired mid-flow lands on raw JSON.
  4. createApp now takes 38 positional arguments, and server/tests/self-host-banner.test.ts hard-codes index 35. An options object would stop each new service from breaking tests.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants