diff --git a/app/models/contract.py b/app/models/contract.py index 8ce40cf..cda8fb6 100644 --- a/app/models/contract.py +++ b/app/models/contract.py @@ -28,13 +28,29 @@ class Contract(db.Model): uploader = db.relationship('User', foreign_keys=[uploaded_by_id]) def can_view(self, user): + """Whether this user may read the contract and download its files. + + Two defects used to sit in the coach branch: + + - `not self.team_id` acted as a wildcard, so any coach listed in the + legacy OrgTeam.coach_id column could read every contract with no + team attached — and upload_contract leaves team_id null whenever + the player belongs to no team. + - the lookup went through OrgTeam.coach_id only, so a coach attached + through the many-to-many relationship saw nothing at all. + + Access now follows the same rule as everywhere else: the coach and + the player must actually work together. + """ if user.id == self.player_id: return True + from app.models.user_model.admin import Admin from app.models.user_model.manager import Manager from app.models.user_model.coach import Coach from app.models.user_model.user import User - from app.models.org_team.org_team import OrgTeam + from app.permissions import coach_can_access_player + if isinstance(user, Admin): return True if isinstance(user, Manager): @@ -42,9 +58,7 @@ class Contract(db.Model): if player and player.get_org_teams(): return True if isinstance(user, Coach): - org_team = OrgTeam.query.filter_by(coach_id=user.id).first() - if org_team and (not self.team_id or self.team_id == org_team.id): - return True + return coach_can_access_player(user, self.player_id) return False def can_upload_signed(self, user): diff --git a/app/permissions.py b/app/permissions.py new file mode 100644 index 0000000..85a741b --- /dev/null +++ b/app/permissions.py @@ -0,0 +1,102 @@ +"""Shared access-control rules. + +Authorisation logic currently lives inline in eight route modules, and the +same question — "may this coach act on this player?" — is answered +differently depending on which route you reach. This module is the first +step towards a single point of truth (ARCH-002); rules move here as they +are unified, rather than in one sweeping change. + +An important subtlety this module hides from callers: a coach can be +attached to a team two different ways. + + OrgTeam.coach_id the original single-coach column + OrgTeam.coaches the many-to-many relationship added later + +Both are still populated, and route code reads one or the other with no +apparent pattern. Reading only `coach_id` — which most of users.py does — +silently locks out every coach who is not the first one on their team. +coach_org_team_ids() always considers both. +""" + +from app.extensions import db + + +def coach_org_team_ids(coach): + """IDs of the organisation teams a coach is attached to. + + Considers the many-to-many relationship *and* the legacy column, so the + second coach of a team is not treated as belonging to nothing. + + Args: + coach: The user to inspect. + + Returns: + list[int]: Team IDs, possibly empty. + """ + from app.models import OrgTeam + + teams = OrgTeam.query.filter( + db.or_( + OrgTeam.coaches.any(id=coach.id), + OrgTeam.coach_id == coach.id, + ) + ).all() + return [team.id for team in teams] + + +def coach_can_access_player(coach, player_id): + """Whether a coach may read or write information about a player. + + True when the player sits on one of the coach's teams, or takes part in + a tryout the coach manages. Anything else means the two have no working + relationship, and a note or a contract about that player is none of the + coach's business. + + Args: + coach: The acting coach. + player_id: Primary key of the player concerned. + + Returns: + bool + """ + from app.models import ( + Match, MatchParticipant, TeamPlayer, Tryout, TryoutRegistration, + ) + + if not player_id: + return False + + team_ids = coach_org_team_ids(coach) + if team_ids: + on_team = TeamPlayer.query.filter( + TeamPlayer.player_id == player_id, + TeamPlayer.org_team_id.in_(team_ids), + ).first() + if on_team: + return True + + # Tryouts the coach manages, through any of the three routes the model + # supports: target team, many-to-many, or the deprecated coach_id. + conditions = [ + Tryout.coaches.any(id=coach.id), + Tryout.coach_id == coach.id, + ] + if team_ids: + conditions.append(Tryout.target_org_team_id.in_(team_ids)) + + tryout_ids = [t.id for t in Tryout.query.filter(db.or_(*conditions)).all()] + if not tryout_ids: + return False + + registered = TryoutRegistration.query.filter( + TryoutRegistration.player_id == player_id, + TryoutRegistration.tryout_id.in_(tryout_ids), + ).first() + if registered: + return True + + plays_a_match = MatchParticipant.query.join(Match).filter( + MatchParticipant.player_id == player_id, + Match.tryout_id.in_(tryout_ids), + ).first() + return plays_a_match is not None diff --git a/app/routes/users.py b/app/routes/users.py index 3e86767..bd2c1fb 100644 --- a/app/routes/users.py +++ b/app/routes/users.py @@ -25,6 +25,7 @@ from app.validators import ( UploadContractSchema, ) from app.logging_config import log_auth_event +from app.permissions import coach_can_access_player import requests ALLOWED_CONTRACT_EXTENSIONS = {'pdf'} @@ -1225,6 +1226,10 @@ def manage_personal_notes(): flash('Can only add notes for players.', 'danger') return redirect(url_for('users.notes_dashboard')) + if not coach_can_access_player(current_user, player_id): + flash('You can only write notes about players you work with.', 'danger') + return redirect(url_for('users.notes_dashboard')) + note = PersonalNote( player_id=player_id, coach_id=current_user.id, @@ -1263,6 +1268,10 @@ def add_personal_note(): flash('Can only add notes for players.', 'danger') return redirect(url_for('users.notes_dashboard')) + if not coach_can_access_player(current_user, player_id): + flash('You can only write notes about players you work with.', 'danger') + return redirect(url_for('users.notes_dashboard')) + note = PersonalNote( player_id=player_id, coach_id=current_user.id, @@ -1304,6 +1313,10 @@ def add_note_from_tryout(tryout_id): flash('Player and content are required.', 'danger') return redirect(url_for('users.add_note_from_tryout', tryout_id=tryout_id)) + if not coach_can_access_player(current_user, player_id): + flash('You can only write notes about players you work with.', 'danger') + return redirect(url_for('users.add_note_from_tryout', tryout_id=tryout_id)) + note = PersonalNote( player_id=player_id, coach_id=current_user.id, @@ -1351,6 +1364,10 @@ def add_note_from_match(match_id): flash('Player and content are required.', 'danger') return redirect(url_for('users.add_note_from_match', match_id=match_id)) + if not coach_can_access_player(current_user, player_id): + flash('You can only write notes about players you work with.', 'danger') + return redirect(url_for('users.add_note_from_match', match_id=match_id)) + note = PersonalNote( player_id=player_id, coach_id=current_user.id, diff --git a/tests/test_permissions.py b/tests/test_permissions.py new file mode 100644 index 0000000..df8360a --- /dev/null +++ b/tests/test_permissions.py @@ -0,0 +1,238 @@ +"""Coach ↔ player access rules. + +SEC-AUTHZ-004 and SEC-AUTHZ-005. The same question — "may this coach act on +this player?" — was answered five different ways across the codebase: + + teams.py:add_player_note checked TeamPlayer membership + users.py, four note routes checked nothing beyond isinstance(Coach) + contract.py:can_view checked the legacy coach_id column, and + treated a null team_id as a wildcard + +app/permissions.py now holds one rule, and it reads both the many-to-many +relationship and the legacy column — so the second coach of a team is no +longer invisible to it (ARCH-002). +""" + +from datetime import date + +import pytest + +from app.extensions import db +from app.models import ( + Contract, OrgTeam, PersonalNote, TeamPlayer, Tryout, TryoutRegistration, +) +from app.permissions import coach_can_access_player, coach_org_team_ids + + +@pytest.fixture +def team_factory(app): + def _make(name, *, coach_id=None, legacy_coach_id=None, player_ids=()): + with app.app_context(): + from app.models import User + + team = OrgTeam(name=name, created_by=coach_id or legacy_coach_id, + coach_id=legacy_coach_id) + db.session.add(team) + db.session.flush() + if coach_id: + team.coaches.append(db.session.get(User, coach_id)) + for player_id in player_ids: + db.session.add(TeamPlayer(player_id=player_id, org_team_id=team.id)) + db.session.commit() + return team.id + + return _make + + +class TestTeamResolution: + def test_a_coach_attached_by_the_relationship_is_found( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + team_id = team_factory('Varsity', coach_id=coach_id) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert coach_org_team_ids(coach) == [team_id] + + def test_a_coach_attached_by_the_legacy_column_is_found( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + team_id = team_factory('JV', legacy_coach_id=coach_id) + + with app.app_context(): + from app.models import User + coach = db.session.get(User, coach_id) + assert coach_org_team_ids(coach) == [team_id] + + def test_an_unattached_coach_has_no_team(self, app, make_user): + coach_id = make_user('coach') + with app.app_context(): + from app.models import User + assert coach_org_team_ids(db.session.get(User, coach_id)) == [] + + +class TestPlayerAccess: + def test_a_coach_reaches_a_player_on_their_team(self, app, make_user, team_factory): + coach_id = make_user('coach') + player_id = make_user('player') + team_factory('Varsity', coach_id=coach_id, player_ids=[player_id]) + + with app.app_context(): + from app.models import User + assert coach_can_access_player(db.session.get(User, coach_id), player_id) + + def test_a_second_coach_of_the_team_also_reaches_the_player( + self, app, make_user, team_factory + ): + """The case that used to fail everywhere users.py looked at coach_id.""" + first_coach = make_user('coach') + second_coach = make_user('coach') + player_id = make_user('player') + team_id = team_factory('Varsity', legacy_coach_id=first_coach, + player_ids=[player_id]) + + with app.app_context(): + from app.models import User + team = db.session.get(OrgTeam, team_id) + team.coaches.append(db.session.get(User, second_coach)) + db.session.commit() + + assert coach_can_access_player(db.session.get(User, second_coach), player_id) + + def test_a_coach_does_not_reach_an_unrelated_player( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + stranger_id = make_user('player') + team_factory('Varsity', coach_id=coach_id) + + with app.app_context(): + from app.models import User + assert not coach_can_access_player(db.session.get(User, coach_id), stranger_id) + + def test_a_coach_reaches_a_player_registered_in_their_tryout( + self, app, make_user + ): + coach_id = make_user('coach') + player_id = make_user('player') + + with app.app_context(): + from app.models import User + tryout = Tryout(title='Open tryout', game='Valorant', + date=date(2030, 3, 1), created_by=coach_id) + db.session.add(tryout) + db.session.flush() + tryout.coaches.append(db.session.get(User, coach_id)) + db.session.add(TryoutRegistration(tryout_id=tryout.id, player_id=player_id)) + db.session.commit() + + assert coach_can_access_player(db.session.get(User, coach_id), player_id) + + def test_a_missing_player_id_is_refused(self, app, make_user): + coach_id = make_user('coach') + with app.app_context(): + from app.models import User + assert not coach_can_access_player(db.session.get(User, coach_id), None) + + +class TestPersonalNoteRoutes: + """SEC-AUTHZ-004 — every coach could write a nominative note about + every player of the club. The notes are visible to the player.""" + + def test_a_coach_cannot_note_an_unrelated_player( + self, app, client, as_role, make_user, team_factory + ): + stranger_id = make_user('player') + coach_id = as_role('coach') + team_factory('Varsity', coach_id=coach_id) + + client.post('/users/personal-notes/manage', data={ + 'player_id': stranger_id, 'content': 'Unrelated observation', + }, follow_redirects=True) + + with app.app_context(): + assert PersonalNote.query.filter_by(player_id=stranger_id).count() == 0 + + def test_a_coach_can_still_note_their_own_player( + self, app, client, as_role, make_user, team_factory + ): + """Guard against over-correcting.""" + player_id = make_user('player') + coach_id = as_role('coach') + team_factory('Varsity', coach_id=coach_id, player_ids=[player_id]) + + client.post('/users/personal-notes/manage', data={ + 'player_id': player_id, 'content': 'Good positioning today', + }, follow_redirects=True) + + with app.app_context(): + note = PersonalNote.query.filter_by(player_id=player_id).one() + assert note.content == 'Good positioning today' + + +class TestContractVisibility: + """SEC-AUTHZ-005 — `not self.team_id` was a wildcard, and upload_contract + leaves team_id null whenever the player has no team.""" + + @staticmethod + def _contract(app, player_id, uploader_id, team_id=None): + with app.app_context(): + contract = Contract( + player_id=player_id, team_id=team_id, uploaded_by_id=uploader_id, + original_filename='c.pdf', stored_filename='uuid.pdf', + file_path='/tmp/uuid.pdf', + ) + db.session.add(contract) + db.session.commit() + return contract.id + + def test_a_teamless_contract_is_not_visible_to_every_coach( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + stranger_id = make_user('player') + admin_id = make_user('admin') + team_factory('Varsity', legacy_coach_id=coach_id) + contract_id = self._contract(app, stranger_id, admin_id, team_id=None) + + with app.app_context(): + from app.models import User + contract = db.session.get(Contract, contract_id) + assert not contract.can_view(db.session.get(User, coach_id)) + + def test_a_coach_sees_the_contract_of_their_own_player( + self, app, make_user, team_factory + ): + coach_id = make_user('coach') + player_id = make_user('player') + admin_id = make_user('admin') + team_factory('Varsity', coach_id=coach_id, player_ids=[player_id]) + contract_id = self._contract(app, player_id, admin_id) + + with app.app_context(): + from app.models import User + contract = db.session.get(Contract, contract_id) + assert contract.can_view(db.session.get(User, coach_id)) + + def test_the_player_always_sees_their_own_contract(self, app, make_user): + player_id = make_user('player') + admin_id = make_user('admin') + contract_id = self._contract(app, player_id, admin_id) + + with app.app_context(): + from app.models import User + contract = db.session.get(Contract, contract_id) + assert contract.can_view(db.session.get(User, player_id)) + + def test_an_admin_sees_every_contract(self, app, make_user): + player_id = make_user('player') + admin_id = make_user('admin') + contract_id = self._contract(app, player_id, admin_id) + + with app.app_context(): + from app.models import User + contract = db.session.get(Contract, contract_id) + assert contract.can_view(db.session.get(User, admin_id))