Uh oh!
There was an error while loading. Please reload this page.
[codex] Fix Discord dashboard login links - #287
Conversation
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR implements a two-stage Discord deep-link authentication flow for dashboard login. It adds role-based identity fallback for local/dev environments, splits link consumption into a GET confirmation page and POST consume endpoint, extends data models to carry Discord roles and display names, and includes comprehensive test coverage and documentation updates. ChangesDiscord Deep-Link Login with Local Dev Support
Sequence Diagram(s)sequenceDiagram
participant Bot as Discord Bot
participant CreateHandler as CreateHandler
participant RedisStore as Redis
participant GetHandler as GetHandler
participant Browser as Browser
participant ConsumeHandler as ConsumeHandler
participant Verifier as Verifier
Bot->>CreateHandler: POST discord_user_id, roles, display_name
CreateHandler->>CreateHandler: Resolve user (not found in prod)
alt Local mode + roles provided
CreateHandler->>CreateHandler: Synthesize DiscordAdminIdentity
end
CreateHandler->>RedisStore: Save DiscordLinkGrant
CreateHandler-->>Bot: Return one-time token
Bot-->>Browser: Display token link
Browser->>GetHandler: GET /auth/discord/link/{token}
GetHandler->>RedisStore: Fetch DiscordLinkGrant
GetHandler->>GetHandler: Check Accept header
alt Accept: text/html
GetHandler-->>Browser: HTML form page (no-store)
else Accept: application/json
GetHandler-->>Browser: JSON response
end
Browser->>ConsumeHandler: POST /auth/discord/link/{token}/consume
ConsumeHandler->>RedisStore: Fetch and consume grant
ConsumeHandler->>Verifier: Resolve dashboard identity
alt Identity not found + local mode
ConsumeHandler->>ConsumeHandler: Use stored display_name & roles
end
ConsumeHandler->>ConsumeHandler: Create AuthSession
ConsumeHandler->>ConsumeHandler: Set auth session cookie
ConsumeHandler-->>Browser: 302 Redirect to dashboard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes the Discord dashboard one-time login link being burned by link previews/scanners by splitting the flow into a non-consuming GET interstitial and a POST /consume endpoint, replaces raw JSON errors with a friendly HTML page for expired/used links, adds a local/dev/test-only fallback that authorizes via the bot-provided Discord role context when the local people cache lacks a CRM-linked row, and clarifies the dev launcher banner so it no longer implies the worker has an API port.
Changes:
- Split
/auth/discord/link/{token}into a no-store HTML interstitial (GET) plus a newPOST /auth/discord/link/{token}/consumethat performs token consumption, session creation, and cookie set; surfaces a friendly HTML page on missing/expired tokens with JSON kept forAccept: application/json. - Extend
DiscordLinkGrantand the create-link API/Discord cog payload withdiscord_display_nameanddiscord_roles, and add a_dev_discord_link_identity_from_rolesfallback gated by_dashboard_dev_sensitive_access_enabled()for local/dev/test only. - Update READMEs and the
dev_mux.pybanner; add unit tests covering the GET interstitial, friendly 404 HTML (and JSON via Accept), POST consume, and the local role-based fallback for create + consume.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/api/src/five08/backend/api.py | Splits link handler into non-consuming GET + POST consume, adds dev-only role fallback identity, threads new payload fields through grant save, registers new POST route. |
| apps/api/src/five08/backend/auth.py | Adds discord_roles and discord_display_name to DiscordLinkGrant with safe deserialization. |
| apps/api/src/five08/backend/dashboard.py | New discord_link_continue_html interstitial (auto-POST + manual button) and discord_link_unavailable_html friendly page. |
| apps/discord_bot/src/five08/discord_bot/cogs/admin_login.py | Sends invoking user's display name and Discord role names when creating the link. |
| scripts/dev_mux.py | Banner clarifies Web/API + bot health listeners and notes worker has no HTTP listener. |
| tests/unit/test_backend_api.py | Adds tests for local role fallback (create + consume), GET non-consumption, friendly/JSON 404 responses, and updates existing tests to POST /consume. |
| tests/unit/test_admin_login_cog.py | Asserts new discord_display_name and discord_roles arguments are forwarded. |
| apps/api/README.md, apps/worker/README.md, apps/discord_bot/README.md, README.md | Documents the new GET/POST split, dev-only role fallback, and DASHBOARD_PUBLIC_BASE_URL guidance. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:09ac241519
ℹ️ 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".
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/five08/backend/api.py (1)
3992-4049:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake Discord link consumption atomic to preserve one-time semantics.
Line 3992 reads the grant and Line 4049 deletes it later, so concurrent consume requests can both succeed before deletion. Use an atomic pop (
GETDEL) path for discord links instead of separate get/delete.Suggested fix
# apps/api/src/five08/backend/auth.py+ async def pop_discord_link(self, token: str) -> DiscordLinkGrant | None:+ value = await self._pop_json(self._discord_link_key(token))+ if value is None:+ return None+ try:+ return DiscordLinkGrant(+ discord_user_id=str(value["discord_user_id"]),+ next_path=str(value["next_path"]),+ discord_roles=_to_string_list(value.get("discord_roles")),+ discord_display_name=_to_optional_str(value.get("discord_display_name")),+ )+ except Exception:+ logger.warning("Invalid discord-link payload in Redis")+ return None# apps/api/src/five08/backend/api.py- grant = await store.get_discord_link(token)+ grant = await store.pop_discord_link(token) if grant is None: ... - ...- await store.delete_discord_link(token)+ ...🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/five08/backend/api.py` around lines 3992 - 4049, The code reads the grant with store.get_discord_link and deletes it later with store.delete_discord_link, which allows race conditions where two consumers both see the grant before deletion; change this to an atomic pop operation on the store (e.g. implement and call store.pop_discord_link or store.get_and_delete_discord_link(token)) so the retrieval and removal happen in one step, then use the returned grant for the existing logic (remove the later store.delete_discord_link call). Keep the rest of the flow (identity resolution, save_session via store.save_session, and session_id handling) unchanged but ensure the new pop method returns the same grant structure or None to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/api/src/five08/backend/api.py`:
- Around line 3992-4049: The code reads the grant with store.get_discord_link
and deletes it later with store.delete_discord_link, which allows race
conditions where two consumers both see the grant before deletion; change this
to an atomic pop operation on the store (e.g. implement and call
store.pop_discord_link or store.get_and_delete_discord_link(token)) so the
retrieval and removal happen in one step, then use the returned grant for the
existing logic (remove the later store.delete_discord_link call). Keep the rest
of the flow (identity resolution, save_session via store.save_session, and
session_id handling) unchanged but ensure the new pop method returns the same
grant structure or None to preserve behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd8a82c6-377e-4ffb-8c39-bfe89e66ac54
📒 Files selected for processing (11)
README.mdapps/api/README.mdapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/auth.pyapps/api/src/five08/backend/dashboard.pyapps/discord_bot/README.mdapps/discord_bot/src/five08/discord_bot/cogs/admin_login.pyapps/worker/README.mdscripts/dev_mux.pytests/unit/test_admin_login_cog.pytests/unit/test_backend_api.py
09ac241 to
45b72deCompareUh oh!
There was an error while loading. Please reload this page.
Summary
GET /auth/discord/link/{token}renders a no-store browser page and does not consume the one-time token./auth/discord/link/{token}/consume, with a manual fallback button, so normal users continue without an extra click while Discord previews and basic link scanners do not burn the token.{"error":"link_not_found"}browser failures with a friendly expired/used-link page that tells users to run/dashboard-loginagain.peoplecache has no matching CRM-linked row; production still requires the normal CRM/people identity.WORKER_API_BASE_URLas if the worker were listening on the API port.Root Cause
The original one-time Discord dashboard link was consumed by a GET request. Link previews or security scanners could request the URL, follow the redirect without preserving the browser session cookie, and burn the token before the user opened it. Separately, local testing denied
/dashboard-loginwhen the local people cache lacked a CRM-linked row even if Discord roles were correct.Validation
uv run pytest tests/unit/test_backend_api.py tests/unit/test_admin_login_cog.py -q./scripts/lint.sh./scripts/mypy.shpython3 -m py_compile scripts/dev_mux.pySummary by CodeRabbit
Release Notes
New Features
Documentation