diff --git a/README.md b/README.md index 6fff8c0..01036bd 100644 --- a/README.md +++ b/README.md @@ -101,8 +101,11 @@ Ce qui **n'est pas** fait, pour que personne ne s'y fie : - **Les comptes créés par le formulaire d'inscription sont actifs immédiatement** : il n'y a pas d'étape de validation par le staff. Décision de produit en attente, voir `docs/roles-and-permissions.md`. -- **L'identité Discord** transite encore par un champ caché du formulaire - d'inscription : elle n'est pas prouvée par le passage OAuth2. +- **L'unicité Discord n'est pas encore garantie par PostgreSQL.** L'identité + OAuth reste désormais côté serveur, le profil ne peut plus réécrire le + snowflake et l'application refuse les nouvelles collisions. Les doublons + historiques doivent être relevés puis corrigés avant la contrainte + `UNIQUE` (`schema_report.py --check-discord-identities`). `docs/security-checklist.md` détaille la liste avant mise en production. diff --git a/app/routes/auth.py b/app/routes/auth.py index 0bff41d..d74a68d 100644 --- a/app/routes/auth.py +++ b/app/routes/auth.py @@ -21,10 +21,14 @@ from app.extensions import check_password, db, hash_password, limiter from app.i18n import LOCALE_SESSION_KEY from app.logging_config import log_auth_event from app.models import ESPORT_GAMES, Player, User -from app.validators import LoginSchema, RegisterSchema +from app.validators import LoginSchema, RegisterSchema, validate_discord_user_id #: Session key holding the pending OAuth2 anti-forgery token. DISCORD_STATE_KEY = 'discord_oauth_state' +#: Whether the OAuth result should create a registration draft or relink the +#: signed-in account. Kept server-side and covered by the same signed session +#: as the anti-forgery state. +DISCORD_PURPOSE_KEY = 'discord_oauth_purpose' # Failed-attempt tracking. The tally is kept for the audit trail and for the # cool-off marker below; it no longer refuses a correct password (SEC-018). @@ -354,6 +358,15 @@ def register(): form_data = dict(request.form) form_data['games'] = request.form.getlist('games') + # Once Discord has authenticated the identity, neither its display + # name nor its snowflake is input data anymore. Remove any client + # copies before validation as well as before persistence: otherwise a + # forged, malformed hidden value can still make the verified flow fail. + discord_oauth = session.get('discord_oauth') or {} + if discord_oauth.get('id'): + form_data.pop('discord_username', None) + form_data.pop('discord_user_id', None) + refusal = check_registration_challenge(request.form) if refusal is not None: # Logged, because this is the only place abuse of the sign-up @@ -380,8 +393,18 @@ def register(): full_name = validated['full_name'] phone = validated.get('phone') selected_games = validated.get('games', []) - discord_username = validated.get('discord_username') - discord_user_id = validated.get('discord_user_id') + # The OAuth identity is server-side state. It used to be copied into + # hidden inputs and read back from request.form, which let anyone + # replace the verified Discord account before submitting (SEC-AUTH-005). + # A manual registration may still provide a display name, but never a + # Discord snowflake: that identifier is an authentication factor for + # bot reactions and must come from Discord itself. + discord_user_id = discord_oauth.get('id') + if discord_user_id: + discord_user_id = str(discord_user_id) + discord_username = ( + discord_oauth.get('username') if discord_user_id else validated.get('discord_username') + ) league_os_profile = validated.get('league_os_profile') if User.query.filter_by(username=username).first(): @@ -392,6 +415,13 @@ def register(): flash(_('Email already registered.'), 'danger') return _rerender_registration(form_data) + # The database constraint belongs to DB-002, after production has + # been backed up and deduplicated. Refuse new duplicates now instead + # of leaving the critical impersonation path open until then. + if discord_user_id and User.query.filter_by(discord_user_id=discord_user_id).first(): + flash(_('This Discord account is already linked to another account.'), 'danger') + return _rerender_registration(form_data) + hashed_password = hash_password(password) user = Player( username=username, @@ -451,11 +481,15 @@ def discord_login(): Returns: Response: Redirect to Discord authorization URL. """ + purpose = 'profile' if current_user.is_authenticated else 'registration' + session[DISCORD_PURPOSE_KEY] = purpose + return_endpoint = 'users.edit_profile' if purpose == 'profile' else 'auth.register' + # DISCORD_REDIRECT_URI is checked too: quoting it when unset used to # raise inside the query builder rather than report a configuration error. if not DISCORD_CLIENT_ID or not DISCORD_REDIRECT_URI: flash(_('Discord OAuth2 is not configured.'), 'danger') - return redirect(url_for('auth.register')) + return redirect(url_for(return_endpoint)) # Anti-forgery token, required by RFC 6749 §10.12. Without it, an # attacker could have the victim's browser consume an authorization code @@ -479,16 +513,24 @@ def discord_login(): def discord_callback(): """Handle the OAuth2 callback from Discord. - Exchanges the authorization code for an access token, then fetches - the user's profile (/users/@me) and connections (/users/@me/connections). - Results are stored in the session and the user is redirected back to - the registration form where fields will be pre-filled. + Exchanges the authorization code for an access token, then fetches the + user's profile. During registration, connected game accounts are also + loaded into server-side draft state. For a signed-in profile relink, the + verified identity is written directly without passing through a form. Returns: - Response: Redirect to registration page. + Response: Redirect to the registration form or profile editor. """ # The state is consumed whatever happens next: a token is single-use, and # leaving it in the session would allow a replay. + purpose = session.pop(DISCORD_PURPOSE_KEY, 'registration') + if purpose == 'profile' and current_user.is_authenticated: + return_endpoint = 'users.edit_profile' + elif purpose == 'profile': + return_endpoint = 'auth.login' + else: + return_endpoint = 'auth.register' + expected_state = session.pop(DISCORD_STATE_KEY, None) received_state = request.args.get('state', '') @@ -500,12 +542,12 @@ def discord_callback(): ), 'danger', ) - return redirect(url_for('auth.register')) + return redirect(url_for(return_endpoint)) code = request.args.get('code') if not code: flash(_('Discord authorization failed. No code received.'), 'danger') - return redirect(url_for('auth.register')) + return redirect(url_for(return_endpoint)) # Exchange the authorization code for an access token token_data = { @@ -529,11 +571,11 @@ def discord_callback(): access_token = token_json.get('access_token') except requests.RequestException: flash(_('Failed to connect to Discord. Please try again.'), 'danger') - return redirect(url_for('auth.register')) + return redirect(url_for(return_endpoint)) if not access_token: flash(_('Failed to obtain Discord access token.'), 'danger') - return redirect(url_for('auth.register')) + return redirect(url_for(return_endpoint)) auth_headers = {'Authorization': f'Bearer {access_token}'} @@ -548,7 +590,43 @@ def discord_callback(): user_data = user_response.json() except requests.RequestException: flash(_('Failed to fetch Discord user profile.'), 'danger') - return redirect(url_for('auth.register')) + return redirect(url_for(return_endpoint)) + + discord_user_id = user_data.get('id') + try: + if not discord_user_id: + raise ValidationError('missing Discord user id') + discord_user_id = str(discord_user_id) + validate_discord_user_id(discord_user_id) + except ValidationError: + flash(_('Failed to fetch Discord user profile.'), 'danger') + return redirect(url_for(return_endpoint)) + + if purpose == 'profile': + # If the session expired while Discord was open, do not turn a profile + # relink into registration state for an anonymous browser. + if not current_user.is_authenticated: + flash(_('Please log in to connect your Discord account.'), 'danger') + return redirect(url_for('auth.login')) + + clash = User.query.filter( + User.discord_user_id == discord_user_id, + User.id != current_user.id, + ).first() + if clash: + flash(_('This Discord account is already linked to another account.'), 'danger') + return redirect(url_for('users.edit_profile')) + + current_user.discord_user_id = discord_user_id + current_user.discord_username = user_data.get('username') or None + db.session.commit() + log_auth_event( + 'account.discord_linked', + username=current_user.username, + user_id=current_user.id, + ) + flash(_('Discord account connected!'), 'success') + return redirect(url_for('users.edit_profile')) # Fetch the user's connected gaming accounts connections = [] @@ -587,7 +665,7 @@ def discord_callback(): # Store in session for the registration form to use session['discord_oauth'] = { - 'id': user_data.get('id'), + 'id': discord_user_id, 'username': user_data.get('username'), 'avatar': user_data.get('avatar'), 'gamertag_suggestions': gamertag_suggestions, diff --git a/app/routes/users/accounts.py b/app/routes/users/accounts.py index 7942525..ef39cd8 100644 --- a/app/routes/users/accounts.py +++ b/app/routes/users/accounts.py @@ -111,6 +111,16 @@ def edit_user(user_id): flash(_('Email already in use by another account.'), 'danger') return _rerender() + discord_clash = None + if discord_user_id: + discord_clash = User.query.filter( + User.discord_user_id == discord_user_id, + User.id != user.id, + ).first() + if discord_clash: + flash(_('This Discord account is already linked to another account.'), 'danger') + return _rerender() + role_changed = user.role != role previous_role = user.role diff --git a/app/routes/users/profile.py b/app/routes/users/profile.py index 2898431..39cfeda 100644 --- a/app/routes/users/profile.py +++ b/app/routes/users/profile.py @@ -76,7 +76,6 @@ def edit_profile(): phone = validated.get('phone') selected_games = validated.get('games', []) discord_username = validated.get('discord_username') - discord_user_id = validated.get('discord_user_id') league_os_profile = validated.get('league_os_profile') if username != current_user.username and User.query.filter_by(username=username).first(): @@ -105,7 +104,6 @@ def edit_profile(): current_user.phone = phone current_user.games = ','.join(selected_games) if selected_games else None current_user.discord_username = discord_username or None - current_user.discord_user_id = discord_user_id or None current_user.league_os_profile = league_os_profile or None update_user_gamertags(current_user, selected_games) diff --git a/app/supporting_scripts/schema_report.py b/app/supporting_scripts/schema_report.py index 7942786..d9952f2 100644 --- a/app/supporting_scripts/schema_report.py +++ b/app/supporting_scripts/schema_report.py @@ -29,10 +29,13 @@ Usage # Also look for the seeded admin/password account (SEC-003) python app/supporting_scripts/schema_report.py --check-seed-accounts + # Find Discord identities that must be reconciled before UNIQUE (SEC-012) + python app/supporting_scripts/schema_report.py --check-discord-identities + Exit codes ---------- 0 the live schema matches the models - 1 drift found — the report says what + 1 drift or requested data risk found — the report says what 2 could not connect or read the catalogue Reading the output @@ -315,6 +318,42 @@ def find_seed_accounts(engine): return results +def find_duplicate_discord_identities(engine): + """Discord snowflakes claimed by more than one account (SEC-012). + + New links are now refused in application code, but existing production + rows predate that guard. These groups must be reconciled before Alembic + can add the database-level UNIQUE constraint. + + Returns: + list[tuple]: (discord_user_id, comma-separated usernames, count). + """ + from sqlalchemy import text + + with engine.connect() as connection: + rows = connection.execute( + text( + 'SELECT discord_user_id, COUNT(*) AS account_count ' + 'FROM users ' + "WHERE discord_user_id IS NOT NULL AND discord_user_id <> '' " + 'GROUP BY discord_user_id HAVING COUNT(*) > 1 ' + 'ORDER BY discord_user_id' + ) + ).fetchall() + + duplicates = [] + for discord_user_id, account_count in rows: + usernames = connection.execute( + text( + 'SELECT username FROM users ' + 'WHERE discord_user_id = :discord_user_id ORDER BY username' + ), + {'discord_user_id': discord_user_id}, + ).scalars() + duplicates.append((discord_user_id, ', '.join(usernames), account_count)) + return duplicates + + def main(argv=None): parser = argparse.ArgumentParser(description=__doc__.split('\n')[0]) parser.add_argument( @@ -327,6 +366,11 @@ def main(argv=None): action='store_true', help='Also look for the admin/password account seeded by clear_db.py (SEC-003).', ) + parser.add_argument( + '--check-discord-identities', + action='store_true', + help='Find duplicate Discord IDs that block the SEC-012 UNIQUE constraint.', + ) args = parser.parse_args(argv) if not args.url: @@ -375,9 +419,24 @@ def main(argv=None): ) print(f' {username} ({role}): {verdict}') + duplicate_discord_identities = [] + if args.check_discord_identities: + print('\n' + '=' * 78) + print('Duplicate Discord identities (SEC-012)') + print('=' * 78) + try: + duplicate_discord_identities = find_duplicate_discord_identities(engine) + except SQLAlchemyError as exc: + print(f'Could not check: {exc}') + else: + if not duplicate_discord_identities: + print('No Discord identity is shared by multiple accounts.') + for discord_user_id, usernames, account_count in duplicate_discord_identities: + print(f' {discord_user_id}: {account_count} accounts ({usernames})') + blocking = sum(1 for f in findings if f.severity == BLOCKING) print(f'\n{len(findings)} finding(s), {blocking} blocking.') - return 1 if findings else 0 + return 1 if findings or duplicate_discord_identities else 0 if __name__ == '__main__': # pragma: no cover diff --git a/app/templates/pages/edit_profile.html b/app/templates/pages/edit_profile.html index c9977e7..656970c 100644 --- a/app/templates/pages/edit_profile.html +++ b/app/templates/pages/edit_profile.html @@ -79,14 +79,13 @@