Add Discord login link flow and bootstrap auth toggle#110
Conversation
|
Warning Rate limit exceeded
⌛ 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. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR introduces Discord admin dashboard access via a new Changes
Sequence Diagram(s)sequenceDiagram
actor User as User (Discord)
participant Bot as Discord Bot
participant API as Backend API
participant DB as Person CRM Database
participant Redis as Session Store
User->>Bot: /login command
Bot->>Bot: Check user has admin role
Bot->>API: POST /auth/discord/links
API->>DB: resolve_admin_identity(discord_user_id)
DB-->>API: Return active person record
alt Identity Checks Disabled (DISCORD_LINK_REQUIRE_OIDC_IDENTITY_CHECKS=false)
API->>Redis: Create/save Discord-backed session
API->>API: Emit audit event (via_discord_link=true)
else Identity Checks Enabled (default)
API->>API: Validate OIDC group membership
API->>API: Validate email claim
API->>DB: Verify linked Discord user
end
API-->>Bot: Return one-time link_url + expires_in_seconds
Bot->>Bot: Audit login (success/denied/error)
Bot-->>User: Ephemeral response with link (or error)
sequenceDiagram
actor User as User (Dashboard)
participant API as Backend API
participant AuthStore as Redis Session Store
participant Audit as Audit Logger
User->>API: Authenticate (OIDC callback or Discord deep-link)
API->>AuthStore: Save session with actor_provider
alt Source is OIDC
AuthStore->>AuthStore: actor_provider="admin_sso"
else Source is Discord
AuthStore->>AuthStore: actor_provider="discord"
end
Audit->>Audit: Record login with actor_provider, is_admin, groups
User->>API: GET /auth/me
API->>AuthStore: Load session
AuthStore-->>API: Return session (with actor_provider)
API-->>User: Return auth response (includes actor_provider)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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
Adds a Discord /login flow for minting one-time dashboard links and introduces a bootstrap toggle to optionally relax Discord-link OIDC identity checks during admin SSO onboarding.
Changes:
- Add Discord bot
/loginslash command + unit tests to mint one-time dashboard login links viaPOST /auth/discord/links. - Update auth/admin defaults (
OIDC_ADMIN_GROUPS,DISCORD_ADMIN_ROLES) and addDISCORD_LINK_REQUIRE_OIDC_IDENTITY_CHECKS(defaulttrue) to control Discord-link identity enforcement. - Update API/worker/bot documentation and expand unit test coverage for the new auth behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_worker_config.py | Adds assertions for updated default admin group/role settings. |
| tests/unit/test_backend_api.py | Adds tests covering Discord-link bootstrap behavior when identity checks are disabled. |
| tests/unit/test_admin_login_cog.py | Introduces unit tests for the new Discord /login cog behavior. |
| apps/worker/src/five08/worker/config.py | Updates defaults and adds discord_link_require_oidc_identity_checks. |
| apps/worker/README.md | Documents Discord deep-link identity policy and bootstrap toggle. |
| apps/discord_bot/src/five08/discord_bot/cogs/admin_login.py | Implements /login command that calls backend link-mint endpoint and returns ephemeral link. |
| apps/discord_bot/README.md | Documents the new /login slash command. |
| apps/api/src/five08/backend/api.py | Adds bootstrap toggle behavior to Discord-link callback + redirect handlers and audit metadata. |
| apps/api/README.md | Documents Discord deep-link identity policy and bootstrap toggle. |
| .env.example | Updates defaults and adds the new DISCORD_LINK_REQUIRE_OIDC_IDENTITY_CHECKS variable. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return JSONResponse({"error": "link_not_found"}, status_code=404) | ||
|
|
||
| _, session = await _current_session(request) | ||
| if session is not None: |
There was a problem hiding this comment.
In bootstrap mode (DISCORD_LINK_REQUIRE_OIDC_IDENTITY_CHECKS=false), this handler will short-circuit when a session already exists, delete the link token, and redirect without forcing a fresh OIDC login. That means (a) a non-admin session won’t be upgraded to an admin session via the link, and (b) any existing session that gets the token can burn the one-time link (DoS) without proving identity. Consider always redirecting to /auth/login?...discord_link_token=... when the flag is disabled (or at least when session.is_admin is false), so the deep link reliably results in an admin session being minted via the callback path.
| if session is not None: | |
| if session is not None: | |
| # In bootstrap mode (identity checks disabled), always route through the | |
| # OIDC login flow so the deep link can mint/upgrade an admin session via | |
| # the callback path, without burning the one-time token here. | |
| if not settings.discord_link_require_oidc_identity_checks: | |
| login_query = urlencode( | |
| {"next": grant.next_path, "discord_link_token": token} | |
| ) | |
| return RedirectResponse( | |
| url=f"/auth/login?{login_query}", status_code=302 | |
| ) |
| "is_admin": is_admin, | ||
| "groups": groups, | ||
| "via_discord_link": bool(pending.discord_link_token), | ||
| "discord_link_identity_checks_enforced": ( | ||
| enforce_discord_link_identity_checks | ||
| ), | ||
| }, |
There was a problem hiding this comment.
discord_link_identity_checks_enforced is being added to the login audit metadata even when via_discord_link is false. This can make the audit stream harder to interpret since the flag only applies to the Discord-link path. Consider only including this metadata key when pending.discord_link_token is set (or nesting it under a discord_link metadata object).
…rd-admin # Conflicts: # tests/unit/test_worker_config.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
apps/worker/README.md:100
- The bootstrap-mode description here is inaccurate/incomplete. When
DISCORD_LINK_REQUIRE_OIDC_IDENTITY_CHECKS=false,/auth/discord/link/{token}can create a Discord-backed admin session directly (no OIDC roundtrip), not just “skip checks after successful OIDC authentication”. Please update this bullet to match the actual handler behavior (and align with the API README wording).
- `GET /health`: Redis/Postgres/worker health check.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Temporary bootstrap switch: when false, Discord deep-link logins skip OIDC | ||
| # admin-group and OIDC-email-to-Discord-link checks after successful OIDC auth. |
There was a problem hiding this comment.
This comment suggests bootstrap mode only skips checks “after successful OIDC auth”, but the API implementation also supports creating a Discord-backed session directly from the deep link when DISCORD_LINK_REQUIRE_OIDC_IDENTITY_CHECKS=false (no OIDC roundtrip). Please adjust the wording so operators don’t misconfigure expectations about whether OIDC is required in bootstrap mode.
| # Temporary bootstrap switch: when false, Discord deep-link logins skip OIDC | |
| # admin-group and OIDC-email-to-Discord-link checks after successful OIDC auth. | |
| # Temporary bootstrap switch: when false, Discord deep-link logins do not require | |
| # an OIDC roundtrip and skip OIDC-based admin-group and email-to-Discord-link checks. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/discord_bot/README.md (1)
21-26: Consider adding a Prerequisites section to match the style of/create-mailbox.The
/logincommand relies onAPI_SHARED_SECRETbeing configured (as shown in the backend request headers), but this prerequisite isn't explicitly documented. For consistency with the/create-mailboxcommand documentation (lines 28-34), consider adding a Prerequisites line similar to:- Prerequisites: `API_SHARED_SECRET` must be configured (configured in env; command will fail if missing).Additionally, you might consider specifying the expiry duration (e.g., "Returns an ephemeral one-time URL that expires in X seconds") or referencing the
DISCORD_LINK_TTL_SECONDSconfiguration setting for clarity.📝 Example documentation enhancement
- `/login` - Description: Generate a one-time admin dashboard login link. + - Prerequisites: `API_SHARED_SECRET` must be configured (configured in env; command will fail if missing). - Required role: any role listed in `DISCORD_ADMIN_ROLES` (`Admin,Owner` by default). - Behavior: - Calls backend `POST /auth/discord/links` using `API_SHARED_SECRET`. - - Returns an ephemeral one-time URL with expiry. + - Returns an ephemeral one-time URL that expires after the configured `DISCORD_LINK_TTL_SECONDS`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/discord_bot/README.md` around lines 21 - 26, Add a "Prerequisites" line to the `/login` command docs stating that API_SHARED_SECRET must be configured in the environment (the backend call in `/login` uses this header), and also mention the link expiry by referencing DISCORD_LINK_TTL_SECONDS (e.g., "Returns an ephemeral one-time URL that expires per DISCORD_LINK_TTL_SECONDS"); update the `/login` section where the command is described to mirror the style of `/create-mailbox` and include these two items..env.example (1)
64-64: Consider quoting value containing spaces.The value
authentik Adminscontains a space. While most.envparsers handle this correctly, quoting the value would make the format more explicit and avoid potential parsing issues with certain tools.-OIDC_ADMIN_GROUPS=authentik Admins +OIDC_ADMIN_GROUPS="authentik Admins"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 64, The OIDC_ADMIN_GROUPS example contains an unquoted value with a space; update the .env.example so the OIDC_ADMIN_GROUPS variable is quoted (e.g. OIDC_ADMIN_GROUPS="authentik Admins") to make parsing explicit and avoid issues with tools that split on whitespace—edit the OIDC_ADMIN_GROUPS line to wrap the value in quotes while preserving the example semantics.tests/unit/test_backend_api.py (1)
866-893: Consider adding actor_provider to the session fixture.The existing
test_auth_logout_writes_logout_auditconstructs anAuthSessionwithoutactor_provider(lines 869-877). This works ifAuthSessionhas a default value, but it doesn't exercise the logout audit path for Discord-authenticated sessions.Consider adding a complementary test case for logout with
actor_provider=ActorProvider.DISCORD.valueto verify the actor_subject is correctly set to the session's subject (not email) for Discord sessions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_backend_api.py` around lines 866 - 893, Update the test_auth_logout_writes_logout_audit scenario to also cover Discord-authenticated sessions by creating a complementary test that constructs an api.AuthSession with actor_provider set to api.ActorProvider.DISCORD.value (or add actor_provider to the existing session fixture) and assert that insert_audit_event's payload.actor_subject equals the session.subject (not email); keep the same setup (patching _current_session, _auth_store_from_app, and insert_audit_event) and assert response.status_code, audit_payload.action == "auth.logout", audit_payload.result == api.AuditResult.SUCCESS, and audit_payload.actor_subject == session.subject for the Discord case.tests/unit/test_admin_login_cog.py (1)
90-103: Consider asserting the message content for consistency.Other tests in this file (lines 69, 87) verify the message content (e.g.,
"not allowed","not configured"). This test only verifies thatsend_messagewas called but doesn't check what was sent. For consistency and to ensure correct user feedback:♻️ Suggested enhancement
with patch.object(cog, "_create_login_link", new=AsyncMock()) as mock_create: await cog.login.callback(cog, mock_interaction) mock_interaction.response.send_message.assert_awaited_once() + sent_message = mock_interaction.response.send_message.call_args.args[0] + assert "not allowed" in sent_message or "permission" in sent_message.lower() mock_create.assert_not_awaited()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_admin_login_cog.py` around lines 90 - 103, The test currently checks that cog.login.callback triggered mock_interaction.response.send_message but not the message content; update test_login_command_uses_configured_admin_roles to assert the actual message sent (use mock_interaction.response.send_message.assert_awaited_once_with(...) or equivalent) and verify it matches the same "not allowed" string used by the other tests (keep mock_create.assert_not_awaited()). Target the call on mock_interaction.response.send_message after invoking cog.login.callback to ensure the user-facing feedback is asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.env.example:
- Line 64: The OIDC_ADMIN_GROUPS example contains an unquoted value with a
space; update the .env.example so the OIDC_ADMIN_GROUPS variable is quoted (e.g.
OIDC_ADMIN_GROUPS="authentik Admins") to make parsing explicit and avoid issues
with tools that split on whitespace—edit the OIDC_ADMIN_GROUPS line to wrap the
value in quotes while preserving the example semantics.
In `@apps/discord_bot/README.md`:
- Around line 21-26: Add a "Prerequisites" line to the `/login` command docs
stating that API_SHARED_SECRET must be configured in the environment (the
backend call in `/login` uses this header), and also mention the link expiry by
referencing DISCORD_LINK_TTL_SECONDS (e.g., "Returns an ephemeral one-time URL
that expires per DISCORD_LINK_TTL_SECONDS"); update the `/login` section where
the command is described to mirror the style of `/create-mailbox` and include
these two items.
In `@tests/unit/test_admin_login_cog.py`:
- Around line 90-103: The test currently checks that cog.login.callback
triggered mock_interaction.response.send_message but not the message content;
update test_login_command_uses_configured_admin_roles to assert the actual
message sent (use
mock_interaction.response.send_message.assert_awaited_once_with(...) or
equivalent) and verify it matches the same "not allowed" string used by the
other tests (keep mock_create.assert_not_awaited()). Target the call on
mock_interaction.response.send_message after invoking cog.login.callback to
ensure the user-facing feedback is asserted.
In `@tests/unit/test_backend_api.py`:
- Around line 866-893: Update the test_auth_logout_writes_logout_audit scenario
to also cover Discord-authenticated sessions by creating a complementary test
that constructs an api.AuthSession with actor_provider set to
api.ActorProvider.DISCORD.value (or add actor_provider to the existing session
fixture) and assert that insert_audit_event's payload.actor_subject equals the
session.subject (not email); keep the same setup (patching _current_session,
_auth_store_from_app, and insert_audit_event) and assert response.status_code,
audit_payload.action == "auth.logout", audit_payload.result ==
api.AuditResult.SUCCESS, and audit_payload.actor_subject == session.subject for
the Discord case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ed16846-901a-48fe-8d63-4945a2067756
📒 Files selected for processing (12)
.env.exampleapps/api/README.mdapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/auth.pyapps/discord_bot/README.mdapps/discord_bot/src/five08/discord_bot/cogs/admin_login.pyapps/discord_bot/src/five08/discord_bot/config.pyapps/worker/README.mdapps/worker/src/five08/worker/config.pytests/unit/test_admin_login_cog.pytests/unit/test_backend_api.pytests/unit/test_worker_config.py
Description
Adds a new Discord
/loginslash command that mints one-time dashboard links viaPOST /auth/discord/linksand returns them ephemerally.Updates auth defaults to
OIDC_ADMIN_GROUPS=authentik AdminsandDISCORD_ADMIN_ROLES=Admin,Owner.Introduces
DISCORD_LINK_REQUIRE_OIDC_IDENTITY_CHECKS(defaulttrue) so Discord deep-link logins can optionally skip OIDC admin-group and OIDC-email linkage checks during SSO bootstrap.Updates API/worker/bot docs and adds unit tests for the new cog, settings defaults, and Discord-link bootstrap auth behavior.
Related Issue
N/A
How Has This Been Tested?
uv run pytest -q tests/unit/test_backend_api.py tests/unit/test_worker_config.py tests/unit/test_admin_login_cog.pySummary by CodeRabbit
New Features
/loginDiscord command to generate one-time admin dashboard login links for quick access to the admin dashboard.Documentation