fix(authz): une seule regle pour l'acces coach vers joueur

SEC-AUTHZ-004 et SEC-AUTHZ-005. La meme question -- ce coach peut-il agir
sur ce joueur ? -- recevait cinq reponses differentes selon la route :

  teams.py:add_player_note     verifiait l'appartenance via TeamPlayer
  users.py, 4 routes de notes  ne verifiaient rien au-dela d'isinstance
  contract.py:can_view         interrogeait la colonne heritee coach_id, et
                               traitait un team_id nul comme un joker

Consequences levees
  - tout coach pouvait ecrire une note nominative sur tout joueur du club.
    Ces notes sont visibles par le joueur concerne.
  - tout coach figurant dans OrgTeam.coach_id pouvait lire n'importe quel
    contrat sans equipe rattachee. Or upload_contract laisse team_id nul des
    que le joueur n'appartient a aucune equipe : la condition
    `not self.team_id or ...` ouvrait donc largement.
  - symetriquement, un coach rattache uniquement par la relation
    many-to-many ne voyait aucun contrat.

app/permissions.py
  Premier pas concret vers ARCH-002, sans refonte : un module unique, pas
  une couche de services. coach_org_team_ids() lit la relation m2m ET la
  colonne heritee, donc le deuxieme coach d'une equipe cesse d'etre
  invisible. coach_can_access_player() accorde l'acces si le joueur est sur
  une equipe du coach, ou inscrit a un tryout qu'il gere, ou participant a
  un match de ce tryout.

14 tests, dont deux verifient que les chemins legitimes fonctionnent
toujours : un coach note bien son propre joueur, et voit bien son contrat.

Note : la regle metier retenue -- equipe OU tryout -- est une lecture du
comportement existant, pas une decision produit. Si le club attend autre
chose, c'est desormais un seul endroit a changer.

Co-Authored-By: Claude Opus 5 <[email protected]>

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()
+            return coach_can_access_player(user, self.player_id)
-            if org_team and (not self.team_id or self.team_id == org_team.id):
-                return True
         return False
 
     def can_upload_signed(self, user):

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

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,

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))