Merged
Conversation
Contributor
Reviewer's GuideImplements Discord SSO end-to-end by adding backend OAuth2 endpoints and models, wiring them to JWT issuance and login restrictions, and updating the frontend auth/register flows and router to initiate Discord login, handle the callback, and persist tokens, plus database migration and config/env wiring. Sequence diagram for Discord SSO login and registrationsequenceDiagram
actor User
participant Browser
participant LoginView
participant RegisterView
participant DiscordCallbackView
participant FrontendAuthApi
participant BackendAuthRouter
participant DiscordOAuth
participant Database
User->>LoginView: Click Se_connecter_avec_Discord
LoginView->>FrontendAuthApi: getDiscordAuthUrl()
FrontendAuthApi->>BackendAuthRouter: GET /auth/discord/authorize
BackendAuthRouter->>BackendAuthRouter: Generate state
BackendAuthRouter->>BackendAuthRouter: Store state in discord_oauth_states
BackendAuthRouter-->>FrontendAuthApi: authorization_url
FrontendAuthApi-->>LoginView: authorization_url
LoginView->>Browser: window.location.href = authorization_url
Browser->>DiscordOAuth: Open Discord authorize URL
User->>DiscordOAuth: Authorize application
DiscordOAuth-->>Browser: Redirect to DISCORD_REDIRECT_URI with code,state
Browser->>DiscordCallbackView: Load /auth/discord/callback?code&state
DiscordCallbackView->>DiscordCallbackView: Extract code,state
DiscordCallbackView->>FrontendAuthApi: discordCallback(code,state)
FrontendAuthApi->>BackendAuthRouter: POST /auth/discord/callback
BackendAuthRouter->>BackendAuthRouter: Validate state in discord_oauth_states
BackendAuthRouter->>DiscordOAuth: POST /oauth2/token (code)
DiscordOAuth-->>BackendAuthRouter: access_token
BackendAuthRouter->>DiscordOAuth: GET /users/@me
DiscordOAuth-->>BackendAuthRouter: Discord user profile
BackendAuthRouter->>Database: SELECT user WHERE discord_id
Database-->>BackendAuthRouter: user or null
alt No user by discord_id
BackendAuthRouter->>Database: SELECT user WHERE email
Database-->>BackendAuthRouter: user or null
alt Existing user by email
BackendAuthRouter->>Database: UPDATE user SET discord_id,discord,updated_at
else No existing user
BackendAuthRouter->>Database: INSERT new user (email,password=None,nickname,discord_id,discord,roles)
end
else Existing user by discord_id
BackendAuthRouter->>Database: UPDATE user.discord if changed
end
BackendAuthRouter->>BackendAuthRouter: create_access_token(user.id,roles)
BackendAuthRouter->>BackendAuthRouter: create_refresh_token(user.id)
BackendAuthRouter-->>FrontendAuthApi: TokenResponse(access_token)
BackendAuthRouter->>Browser: Set refresh_token cookie
FrontendAuthApi-->>DiscordCallbackView: access_token
DiscordCallbackView->>Browser: Store access_token in localStorage
DiscordCallbackView->>FrontendAuthApi: fetchUser()
FrontendAuthApi->>BackendAuthRouter: GET /auth/me
BackendAuthRouter-->>FrontendAuthApi: User data
FrontendAuthApi-->>DiscordCallbackView: User data
DiscordCallbackView->>Browser: Redirect to original or home route
Note over RegisterView,FrontendAuthApi: RegisterView follows same authorize step via getDiscordAuthUrl() and redirect to Discord
Entity relationship diagram for updated User entity with Discord fieldserDiagram
USER {
int id PK
string email
string roles
string password
string password_request_token
datetime password_request_expired_at
string nickname
string firstname
string lastname
string phone
string status
boolean sim_dcs
boolean sim_other
boolean need_presentation
int cadet_flights
string discord
string discord_id
string forum
datetime created_at
datetime updated_at
}
%% Constraints
USER ||..|| USER : email_unique
USER ||..|| USER : nickname_unique
USER ||..|| USER : discord_id_unique
Class diagram for backend auth and frontend auth store changesclassDiagram
class Settings {
+str API_DCSSERVERBOT_URL
+str API_TEAMSPEAK_URL
+str APP_URL
+str UPLOAD_DIR
+str DISCORD_CLIENT_ID
+str DISCORD_CLIENT_SECRET
+str DISCORD_REDIRECT_URI
+str RECAPTCHA_SECRET_KEY
}
class User {
+int id
+str email
+str roles
+str password
+str password_request_token
+datetime password_request_expired_at
+str nickname
+str firstname
+str lastname
+str phone
+str status
+bool sim_dcs
+bool sim_other
+bool need_presentation
+int cadet_flights
+str discord
+str discord_id
+str forum
+datetime created_at
+datetime updated_at
+list~str~ get_roles_list()
}
class DiscordAuthUrlResponse {
+str authorization_url
}
class DiscordCallbackRequest {
+str code
+str state
}
class AuthRouter {
+discord_authorize()
+discord_callback(data,response,db)
}
class CacheModule {
+TTLCache teamspeak_cache
+TTLCache dcsbot_cache
+TTLCache discord_oauth_states
}
class FrontendAuthApi {
+Promise~void~ register(email,nickname,password)
+Promise~void~ login(email,password)
+Promise~void~ logout()
+Promise~void~ resetPassword(email)
+Promise~void~ confirmResetPassword(token,password)
+Promise~DiscordAuthUrlResponse~ getDiscordAuthUrl()
+Promise~TokenResponse~ discordCallback(code,state)
}
class AuthStore {
+string token
+User user
+boolean isAuthenticated()
+boolean isAdmin()
+boolean isMember()
+Promise~void~ login(email,password)
+Promise~void~ register(email,nickname,password)
+Promise~void~ loginWithDiscord(code,state)
+Promise~void~ logout()
+Promise~void~ fetchUser()
}
Settings <.. AuthRouter : uses
User <.. AuthRouter : queries_and_updates
DiscordAuthUrlResponse <.. AuthRouter : returns
DiscordCallbackRequest <.. AuthRouter : consumes
CacheModule <.. AuthRouter : uses_discord_oauth_states
FrontendAuthApi <.. AuthStore : uses
AuthStore <.. DiscordCallbackView : uses
class DiscordCallbackView {
+mounted()
}
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 security issues, 4 other issues, and left some high level feedback:
Security issues:
- Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications. (link)
- Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications. (link)
General comments:
- The in-memory
discord_oauth_statescache only works reliably with a single process/worker; if you ever scale to multiple workers/instances, consider a shared store (e.g. Redis) or another mechanism for CSRF state tracking. - In
discord_callback, you rely onDISCORD_CLIENT_ID/SECRETbeing set but only the authorize endpoint checks this; adding an explicit configuration check at the start of the callback handler would give clearer errors and avoid unnecessary external calls when SSO is misconfigured.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The in-memory `discord_oauth_states` cache only works reliably with a single process/worker; if you ever scale to multiple workers/instances, consider a shared store (e.g. Redis) or another mechanism for CSRF state tracking.
- In `discord_callback`, you rely on `DISCORD_CLIENT_ID/SECRET` being set but only the authorize endpoint checks this; adding an explicit configuration check at the start of the callback handler would give clearer errors and avoid unnecessary external calls when SSO is misconfigured.
## Individual Comments
### Comment 1
<location path="backend/app/api/auth.py" line_range="189-194" />
<code_context>
+ return DiscordAuthUrlResponse(authorization_url=f"{DISCORD_AUTHORIZE_URL}?{params}")
+
+
+@router.post("/discord/callback", response_model=TokenResponse)
+async def discord_callback(data: DiscordCallbackRequest, response: Response, db: AsyncSession = Depends(get_db)):
+ # Validate state (CSRF protection)
+ if data.state not in discord_oauth_states:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Also validate Discord OAuth configuration (client secret/redirect URI) before attempting token exchange.
The authorize endpoint correctly checks `DISCORD_CLIENT_ID` and returns 501 when Discord SSO is not configured, but the callback endpoint still calls Discord even if `DISCORD_CLIENT_SECRET` or `DISCORD_REDIRECT_URI` are missing/misconfigured, which will show up as a generic 400 to the client. Please add a similar config validation here (e.g., both client ID and client secret non-empty, redirect URI set) and return a clear 5xx for server misconfiguration instead of treating it as a user error.
```suggestion
@router.post("/discord/callback", response_model=TokenResponse)
async def discord_callback(data: DiscordCallbackRequest, response: Response, db: AsyncSession = Depends(get_db)):
# Validate Discord OAuth configuration before attempting token exchange
if (
not settings.DISCORD_CLIENT_ID
or not settings.DISCORD_CLIENT_SECRET
or not settings.DISCORD_REDIRECT_URI
):
raise HTTPException(
status_code=status.HTTP_501_NOT_IMPLEMENTED,
detail="Discord SSO is not configured",
)
# Validate state (CSRF protection)
if data.state not in discord_oauth_states:
raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid or expired state parameter")
del discord_oauth_states[data.state]
```
</issue_to_address>
### Comment 2
<location path="backend/alembic/versions/3142ebd16337_discord.py" line_range="31-37" />
<code_context>
+ # ### end Alembic commands ###
+
+
+def downgrade() -> None:
+ # ### commands auto generated by Alembic - please adjust! ###
+ op.drop_constraint('discord_id_idx', 'user', type_='unique')
+ op.alter_column('user', 'password',
+ existing_type=sa.VARCHAR(length=255),
+ nullable=False)
+ op.drop_column('user', 'discord_id')
+ # ### end Alembic commands ###
</code_context>
<issue_to_address>
**issue:** Downgrade may fail if `password` contains NULLs when restoring non-nullable constraint.
In the downgrade, `user.password` is set back to `nullable=False`, but by then there may already be Discord-created users with `password = NULL`. Running this downgrade on such a database will fail because of the NOT NULL constraint. If downgrade reliability matters, consider either (a) backfilling or blocking downgrade when NULL passwords exist, or (b) not re-tightening the constraint and documenting that behavior instead.
</issue_to_address>
### Comment 3
<location path="frontend/src/views/DiscordCallbackView.vue" line_range="15-17" />
<code_context>
+const error = ref('')
+
+onMounted(async () => {
+ const code = route.query.code as string
+ const state = route.query.state as string
+
+ if (!code || !state) {
</code_context>
<issue_to_address>
**suggestion:** Handle the possibility of `code`/`state` being arrays from the query string.
Vue Router `route.query` values can be `string | string[] | null | undefined`, so casting directly to `string` assumes they’re never arrays. Consider normalizing first (e.g. `const code = Array.isArray(route.query.code) ? route.query.code[0] : route.query.code`) before validation to avoid subtle issues if an array is ever passed.
```suggestion
onMounted(async () => {
const rawCode = route.query.code
const rawState = route.query.state
const code = Array.isArray(rawCode) ? rawCode[0] : rawCode ?? undefined
const state = Array.isArray(rawState) ? rawState[0] : rawState ?? undefined
```
</issue_to_address>
### Comment 4
<location path="backend/tests/integration/api/test_auth.py" line_range="157-128" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_discord_callback_links_existing_user_by_email(client: AsyncClient, db_session: AsyncSession):
+ # GIVEN
+ user = UserFactory(email="discord@veaf.org", nickname="ExistingPilot")
+ db_session.add(user)
+ await db_session.flush()
+
+ state = "test-state-link"
+ discord_oauth_states[state] = True
+
+ original_id = settings.DISCORD_CLIENT_ID
+ original_secret = settings.DISCORD_CLIENT_SECRET
+ settings.DISCORD_CLIENT_ID = "test-client-id"
+ settings.DISCORD_CLIENT_SECRET = "test-secret"
+
+ mock_client = AsyncMock()
</code_context>
<issue_to_address>
**suggestion (testing):** Avoid mutating global settings directly in tests; use a fixture/monkeypatch or a try/finally to ensure proper cleanup.
The Discord tests (`test_discord_authorize_returns_url`, `test_discord_authorize_disabled_without_config`, and the callback tests) directly overwrite `settings.DISCORD_CLIENT_ID`/`DISCORD_CLIENT_SECRET` and then reset them manually. If an assertion fails before the reset, later tests may run with incorrect settings. Please either use `monkeypatch.setattr` on `settings` or wrap the mutation in a `try/finally` so cleanup always happens and tests remain isolated.
Suggested implementation:
```python
@pytest.mark.asyncio
async def test_discord_callback_links_existing_user_by_email(
client: AsyncClient,
db_session: AsyncSession,
monkeypatch: pytest.MonkeyPatch,
):
```
```python
monkeypatch.setattr(settings, "DISCORD_CLIENT_ID", "test-client-id", raising=False)
monkeypatch.setattr(settings, "DISCORD_CLIENT_SECRET", "test-secret", raising=False)
```
The same pattern should be applied to the other Discord-related tests that currently mutate `settings.DISCORD_CLIENT_ID` / `settings.DISCORD_CLIENT_SECRET` directly:
1. Update each test signature to accept `monkeypatch: pytest.MonkeyPatch`.
2. Replace manual saving/restoring of `DISCORD_CLIENT_ID`/`DISCORD_CLIENT_SECRET` with `monkeypatch.setattr(settings, "DISCORD_CLIENT_ID", "...")` and `monkeypatch.setattr(settings, "DISCORD_CLIENT_SECRET", "...")`.
3. Remove any manual reset code (`settings.DISCORD_CLIENT_ID = original_id`, etc.) at the end of those tests.
This will ensure all tests remain isolated and configuration is always restored even if assertions fail.
</issue_to_address>
### Comment 5
<location path="backend/tests/integration/api/test_auth.py" line_range="187" />
<code_context>
123456789012345678
</code_context>
<issue_to_address>
**security (discord-client-id):** Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
*Source: gitleaks*
</issue_to_address>
### Comment 6
<location path="backend/tests/integration/api/test_auth.py" line_range="197" />
<code_context>
123456789012345678
</code_context>
<issue_to_address>
**security (discord-client-id):** Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
*Source: gitleaks*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+31
to
+37
| def downgrade() -> None: | ||
| # ### commands auto generated by Alembic - please adjust! ### | ||
| op.drop_constraint('discord_id_idx', 'user', type_='unique') | ||
| op.alter_column('user', 'password', | ||
| existing_type=sa.VARCHAR(length=255), | ||
| nullable=False) | ||
| op.drop_column('user', 'discord_id') |
Contributor
There was a problem hiding this comment.
issue: Downgrade may fail if password contains NULLs when restoring non-nullable constraint.
In the downgrade, user.password is set back to nullable=False, but by then there may already be Discord-created users with password = NULL. Running this downgrade on such a database will fail because of the NOT NULL constraint. If downgrade reliability matters, consider either (a) backfilling or blocking downgrade when NULL passwords exist, or (b) not re-tightening the constraint and documenting that behavior instead.
Comment on lines
+15
to
+17
| onMounted(async () => { | ||
| const code = route.query.code as string | ||
| const state = route.query.state as string |
Contributor
There was a problem hiding this comment.
suggestion: Handle the possibility of code/state being arrays from the query string.
Vue Router route.query values can be string | string[] | null | undefined, so casting directly to string assumes they’re never arrays. Consider normalizing first (e.g. const code = Array.isArray(route.query.code) ? route.query.code[0] : route.query.code) before validation to avoid subtle issues if an array is ever passed.
Suggested change
| onMounted(async () => { | |
| const code = route.query.code as string | |
| const state = route.query.state as string | |
| onMounted(async () => { | |
| const rawCode = route.query.code | |
| const rawState = route.query.state | |
| const code = Array.isArray(rawCode) ? rawCode[0] : rawCode ?? undefined | |
| const state = Array.isArray(rawState) ? rawState[0] : rawState ?? undefined |
| original_id = settings.DISCORD_CLIENT_ID | ||
| original_secret = settings.DISCORD_CLIENT_SECRET | ||
| settings.DISCORD_CLIENT_ID = "test-client-id" | ||
| settings.DISCORD_CLIENT_SECRET = "test-secret" |
Contributor
There was a problem hiding this comment.
suggestion (testing): Avoid mutating global settings directly in tests; use a fixture/monkeypatch or a try/finally to ensure proper cleanup.
The Discord tests (test_discord_authorize_returns_url, test_discord_authorize_disabled_without_config, and the callback tests) directly overwrite settings.DISCORD_CLIENT_ID/DISCORD_CLIENT_SECRET and then reset them manually. If an assertion fails before the reset, later tests may run with incorrect settings. Please either use monkeypatch.setattr on settings or wrap the mutation in a try/finally so cleanup always happens and tests remain isolated.
Suggested implementation:
@pytest.mark.asyncio
async def test_discord_callback_links_existing_user_by_email(
client: AsyncClient,
db_session: AsyncSession,
monkeypatch: pytest.MonkeyPatch,
): monkeypatch.setattr(settings, "DISCORD_CLIENT_ID", "test-client-id", raising=False)
monkeypatch.setattr(settings, "DISCORD_CLIENT_SECRET", "test-secret", raising=False)The same pattern should be applied to the other Discord-related tests that currently mutate settings.DISCORD_CLIENT_ID / settings.DISCORD_CLIENT_SECRET directly:
- Update each test signature to accept
monkeypatch: pytest.MonkeyPatch. - Replace manual saving/restoring of
DISCORD_CLIENT_ID/DISCORD_CLIENT_SECRETwithmonkeypatch.setattr(settings, "DISCORD_CLIENT_ID", "...")andmonkeypatch.setattr(settings, "DISCORD_CLIENT_SECRET", "..."). - Remove any manual reset code (
settings.DISCORD_CLIENT_ID = original_id, etc.) at the end of those tests.
This will ensure all tests remain isolated and configuration is always restored even if assertions fail.
| # THEN | ||
| assert response.status_code == 200 | ||
| await db_session.refresh(user) | ||
| assert user.discord_id == "123456789012345678" |
Contributor
There was a problem hiding this comment.
security (discord-client-id): Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
Source: gitleaks
| @pytest.mark.asyncio | ||
| async def test_discord_callback_logs_in_existing_linked_user(client: AsyncClient, db_session: AsyncSession): | ||
| # GIVEN | ||
| user = UserFactory(email="linked@veaf.org", nickname="LinkedPilot", discord_id="123456789012345678") |
Contributor
There was a problem hiding this comment.
security (discord-client-id): Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
Source: gitleaks
| 3. Set the environment variables in `backend/.env`: | ||
|
|
||
| ```env | ||
| DISCORD_CLIENT_ID=123456789012345678 |
Contributor
There was a problem hiding this comment.
security (discord-client-id): Identified a Discord client ID, which may lead to unauthorized integrations and data exposure in Discord applications.
Source: gitleaks
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add Discord OAuth2 single sign-on support across backend and frontend, including user model updates, configuration, and login flow integration.
New Features:
Bug Fixes:
Enhancements:
Build:
Tests: