From dd8b9671c672f23f7c45d44f90dfd2696e2802db Mon Sep 17 00:00:00 2001
From: GGThed <mafiaggia@outlook.com>
Date: Tue, 11 Aug 2026 19:37:13 -0400
Subject: [PATCH] fix(authz): teams.py etait le module que la validation n
 avait pas atteint

SEC-16, de l'audit anterieur. La vague G a pose un schema a la frontiere de
matches, team_matches, tryouts et evaluations, et le HANDOFF en a tire une
regle : tout champ de formulaire passe par un schema de app/validators.py,
pas par request.form.get. teams.py ne l'avait jamais appliquee.

Deux defauts, pas un.

int(request.form.get('coach_id')) leve sur une valeur non numerique : une
soumission fabriquee etait un 500.

Et l'identifiant obtenu etait ensuite resolu sans verifier le role du compte,
dans deux des trois endroits qui le faisaient. La branche sync_staff
d'edit_team testait isinstance(user, Coach) ; son autre branche non, et
create_team non plus. Le meme fichier en desaccord avec lui-meme, sur
exactement le defaut que la vague G avait corrige dans tryouts.py -- une
soumission fabriquee pouvait nommer un joueur parmi les coachs d'une equipe.
L'identifiant vient d'un <select> rendu par le navigateur : c'est une valeur
que le client choisit.

_staff_member est la reponse unique, et OrgTeamSchema garantit que les
identifiants arrivent en entiers. Les deux tests qui epinglent le role
tombent si la verification saute : verifie par mutation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---
 app/routes/teams.py         | 118 ++++++++++++++++++++----------------
 app/validators.py           |  43 +++++++++++++
 tests/test_authorization.py | 101 ++++++++++++++++++++++++++++++
 3 files changed, 211 insertions(+), 51 deletions(-)

diff --git a/app/routes/teams.py b/app/routes/teams.py
index a909437..b876d89 100644
--- a/app/routes/teams.py
+++ b/app/routes/teams.py
@@ -8,9 +8,11 @@ from datetime import datetime
 from flask import Blueprint, flash, jsonify, redirect, render_template, request, url_for
 from flask_babel import gettext as _
 from flask_login import current_user, login_required
+from marshmallow import ValidationError
 
 from app.api import json_endpoint
 from app.extensions import db
+from app.forms import flash_validation_errors, form_payload
 from app.models import (
     Admin,
     Coach,
@@ -27,6 +29,7 @@ from app.models import (
     User,
 )
 from app.permissions import visible_org_teams
+from app.validators import OrgTeamSchema
 
 teams_bp = Blueprint('teams', __name__, url_prefix='/teams')
 
@@ -116,6 +119,29 @@ def my_teams():
     return render_template('pages/my_teams.html', team_data=team_data, now=now)
 
 
+def _staff_member(user_id, expected_class):
+    """The user behind an id, only if they hold the role being assigned.
+
+    Returns None for a missing id, an unknown id, or an account of the wrong
+    role. That last case is the point (SEC-16): the id comes from a `<select>`
+    the browser rendered, so it is a value the client chooses, and nothing
+    checked it in two of the three places that used it. A forged submission
+    could therefore list a player among a team's coaches — the same defect
+    wave G fixed in `tryouts.py`, left standing here.
+
+    Args:
+        user_id: Already an int or None, thanks to OrgTeamSchema.
+        expected_class: Coach or Manager.
+
+    Returns:
+        User | None: The account, when it is of the expected role.
+    """
+    if not user_id:
+        return None
+    user = db.session.get(User, user_id)
+    return user if isinstance(user, expected_class) else None
+
+
 @teams_bp.route('/create', methods=['POST'])
 @login_required
 def create_team():
@@ -124,36 +150,33 @@ def create_team():
         flash(_('You do not have permission to create teams.'), 'danger')
         return redirect(url_for('teams.list_teams'))
 
-    name = request.form.get('name')
-    coach_id = request.form.get('coach_id')
-    manager_id = request.form.get('manager_id')
-
-    if not name:
-        flash(_('Team name is required.'), 'danger')
+    try:
+        data = OrgTeamSchema().load(form_payload(list_fields=('coach_ids', 'manager_ids')))
+    except ValidationError as err:
+        flash_validation_errors(err)
         return redirect(url_for('teams.list_teams'))
 
-    existing = OrgTeam.query.filter_by(name=name).first()
-    if existing:
+    name = data['name']
+    if OrgTeam.query.filter_by(name=name).first():
         flash(_('Team "%(name)s" already exists.', name=name), 'danger')
         return redirect(url_for('teams.list_teams'))
 
+    coach = _staff_member(data['coach_id'], Coach)
+    manager = _staff_member(data['manager_id'], Manager)
+
     team = OrgTeam(
         name=name,
-        coach_id=int(coach_id) if coach_id else None,
-        manager_id=int(manager_id) if manager_id else None,
+        coach_id=coach.id if coach else None,
+        manager_id=manager.id if manager else None,
         created_by=current_user.id,
     )
     db.session.add(team)
     db.session.flush()
 
-    if coach_id:
-        coach_user = User.query.get(int(coach_id))
-        if coach_user:
-            team.coaches.append(coach_user)
-    if manager_id:
-        manager_user = User.query.get(int(manager_id))
-        if manager_user:
-            team.managers.append(manager_user)
+    if coach:
+        team.coaches.append(coach)
+    if manager:
+        team.managers.append(manager)
 
     db.session.commit()
     flash(_('Team "%(name)s" created successfully!', name=name), 'success')
@@ -169,52 +192,45 @@ def edit_team(team_id):
         flash(_('You do not have permission to edit this team.'), 'danger')
         return redirect(url_for('teams.list_teams'))
 
-    name = request.form.get('name')
-    coach_id = request.form.get('coach_id')
-    manager_id = request.form.get('manager_id')
-
-    if not name:
-        flash(_('Team name is required.'), 'danger')
+    try:
+        data = OrgTeamSchema().load(form_payload(list_fields=('coach_ids', 'manager_ids')))
+    except ValidationError as err:
+        flash_validation_errors(err)
         return redirect(url_for('teams.list_teams'))
 
-    existing = OrgTeam.query.filter(OrgTeam.name == name, OrgTeam.id != team_id).first()
-    if existing:
+    name = data['name']
+    if OrgTeam.query.filter(OrgTeam.name == name, OrgTeam.id != team_id).first():
         flash(_('Team "%(name)s" already exists.', name=name), 'danger')
         return redirect(url_for('teams.list_teams'))
 
-    if request.form.get('sync_staff') == '1':
-        coach_ids = request.form.getlist('coach_ids')
-        manager_ids = request.form.getlist('manager_ids')
+    team.name = name
 
-        team.coaches = []
-        for cid in coach_ids:
-            if cid and cid.strip():
-                coach_user = User.query.get(int(cid))
-                if coach_user and isinstance(coach_user, Coach):
-                    team.coaches.append(coach_user)
+    if data['sync_staff'] == '1':
+        team.coaches = [
+            user for user in (_staff_member(cid, Coach) for cid in data['coach_ids']) if user
+        ]
         coach_list = team.coaches.all()
         team.coach_id = coach_list[0].id if coach_list else None
 
-        team.managers = []
-        for mid in manager_ids:
-            if mid and mid.strip():
-                manager_user = User.query.get(int(mid))
-                if manager_user and isinstance(manager_user, Manager):
-                    team.managers.append(manager_user)
+        team.managers = [
+            user for user in (_staff_member(mid, Manager) for mid in data['manager_ids']) if user
+        ]
         manager_list = team.managers.all()
         team.manager_id = manager_list[0].id if manager_list else None
     else:
-        team.coach_id = int(coach_id) if coach_id else None
-        team.manager_id = int(manager_id) if manager_id else None
+        # This branch never checked the role, while the one above did — the
+        # same file disagreeing with itself (SEC-16). _staff_member is the
+        # single answer now.
+        coach = _staff_member(data['coach_id'], Coach)
+        manager = _staff_member(data['manager_id'], Manager)
 
-        if coach_id:
-            coach_user = User.query.get(int(coach_id))
-            if coach_user and not team.coaches.filter_by(id=coach_user.id).first():
-                team.coaches.append(coach_user)
-        if manager_id:
-            manager_user = User.query.get(int(manager_id))
-            if manager_user and not team.managers.filter_by(id=manager_user.id).first():
-                team.managers.append(manager_user)
+        team.coach_id = coach.id if coach else None
+        team.manager_id = manager.id if manager else None
+
+        if coach and not team.coaches.filter_by(id=coach.id).first():
+            team.coaches.append(coach)
+        if manager and not team.managers.filter_by(id=manager.id).first():
+            team.managers.append(manager)
 
     db.session.commit()
     flash(_('Team "%(name)s" updated successfully!', name=name), 'success')
diff --git a/app/validators.py b/app/validators.py
index af2818d..277006f 100644
--- a/app/validators.py
+++ b/app/validators.py
@@ -716,6 +716,49 @@ class TryoutSchema(StripMixin):
             )
 
 
+class OrgTeamSchema(StripMixin):
+    """An organisation team, created or edited (SEC-16).
+
+    `teams.py` was the one route module wave G's validation pass did not
+    reach, and it still read every field through `request.form.get` and
+    converted with a bare `int()`. Two consequences, both reachable from a
+    hand-made POST by anyone allowed to manage teams:
+
+    - `int('abc')` raises, so a non-numeric `coach_id` was a 500;
+    - `int('-1')` was accepted, and the id was then looked up without ever
+      checking what role the account had.
+
+    The second is the same defect wave G found in `tryouts.py`, where a
+    forged submission could name a *player* as coach. Here it survived in
+    two of the three branches of the same file: `edit_team`'s `sync_staff`
+    path checks `isinstance(user, Coach)`, its other path does not, and
+    `create_team` does not either. Role checking belongs with the lookup,
+    not with the schema, so it lives in `_staff_member` in the route — but
+    the ids have to survive the trip as integers first.
+    """
+
+    name = fields.String(
+        required=True,
+        validate=validate.Length(min=1, max=100, error=_l('Team name is required.')),
+        error_messages={'required': _l('Team name is required.')},
+    )
+    coach_id = fields.Integer(
+        allow_none=True,
+        load_default=None,
+        validate=validate.Range(min=1),
+        error_messages={'invalid': _l('Invalid coach selection.')},
+    )
+    manager_id = fields.Integer(
+        allow_none=True,
+        load_default=None,
+        validate=validate.Range(min=1),
+        error_messages={'invalid': _l('Invalid manager selection.')},
+    )
+    coach_ids = fields.List(fields.Integer(validate=validate.Range(min=1)), load_default=list)
+    manager_ids = fields.List(fields.Integer(validate=validate.Range(min=1)), load_default=list)
+    sync_staff = fields.String(allow_none=True, load_default=None)
+
+
 def score_field():
     """One evaluation criterion: 1 to 10, or not scored at all."""
     return fields.Integer(
diff --git a/tests/test_authorization.py b/tests/test_authorization.py
index 1120def..3b9fcd3 100644
--- a/tests/test_authorization.py
+++ b/tests/test_authorization.py
@@ -482,3 +482,104 @@ class TestTeamDeletionGuard:
 
         with app.app_context():
             assert db.session.get(OrgTeam, team_id) is not None
+
+
+class TestTeamStaffAssignment:
+    """SEC-16 — teams.py was the one route module wave G's validation pass
+    did not reach, and it showed in two ways.
+
+    `int(request.form.get('coach_id'))` raised on anything non-numeric, so a
+    hand-made POST was a 500. And the id it produced was looked up without
+    checking the account's role in two of the three places that used it:
+    `edit_team`'s sync_staff branch tested `isinstance(user, Coach)`, its
+    other branch did not, and `create_team` did not either. The same file
+    disagreeing with itself — and the same defect wave G fixed in tryouts.py.
+    """
+
+    def test_a_non_numeric_id_is_refused_not_a_500(self, app, client, as_role):
+        from app.models import OrgTeam
+
+        as_role('manager')
+
+        response = client.post(
+            '/teams/create',
+            data={'name': 'Varsity', 'coach_id': 'not-a-number'},
+            follow_redirects=False,
+        )
+
+        assert response.status_code < 500, 'a forged field must not be a server error'
+        with app.app_context():
+            assert OrgTeam.query.filter_by(name='Varsity').first() is None
+
+    def test_a_player_cannot_be_named_coach_of_a_team(self, app, client, as_role, make_user):
+        """The one that matters. The id comes from a <select> the browser
+        rendered, so it is a value the client chooses."""
+        from app.models import OrgTeam
+
+        player_id = make_user('player')
+        as_role('manager')
+
+        client.post(
+            '/teams/create',
+            data={'name': 'Varsity', 'coach_id': str(player_id)},
+            follow_redirects=False,
+        )
+
+        with app.app_context():
+            team = OrgTeam.query.filter_by(name='Varsity').first()
+            assert team is not None, 'the team should still be created, without the bad staff'
+            assert team.coach_id is None
+            assert team.coaches.all() == []
+
+    def test_a_real_coach_is_still_assigned(self, app, client, as_role, make_user):
+        """The premise. Without it the test above passes against a route that
+        assigns nobody at all."""
+        from app.models import OrgTeam
+
+        coach_id = make_user('coach')
+        as_role('manager')
+
+        client.post(
+            '/teams/create',
+            data={'name': 'Varsity', 'coach_id': str(coach_id)},
+            follow_redirects=False,
+        )
+
+        with app.app_context():
+            team = OrgTeam.query.filter_by(name='Varsity').first()
+            assert team.coach_id == coach_id
+            assert [user.id for user in team.coaches.all()] == [coach_id]
+
+    def test_editing_cannot_slip_a_player_in_either(self, app, client, as_role, make_user):
+        """The branch that had no role check at all."""
+        from app.models import OrgTeam
+
+        coach_id = make_user('coach')
+        player_id = make_user('player')
+        as_role('manager')
+
+        client.post('/teams/create', data={'name': 'Varsity', 'coach_id': str(coach_id)})
+        with app.app_context():
+            team_id = OrgTeam.query.filter_by(name='Varsity').first().id
+
+        client.post(
+            f'/teams/{team_id}/edit',
+            data={'name': 'Varsity', 'coach_id': str(player_id)},
+            follow_redirects=False,
+        )
+
+        with app.app_context():
+            team = db.session.get(OrgTeam, team_id)
+            assert player_id not in [user.id for user in team.coaches.all()]
+            assert team.coach_id != player_id
+
+    def test_a_team_still_needs_a_name(self, app, client, as_role):
+        from app.models import OrgTeam
+
+        as_role('manager')
+
+        response = client.post('/teams/create', data={'name': ''}, follow_redirects=False)
+
+        assert response.status_code < 500
+        with app.app_context():
+            assert OrgTeam.query.count() == 0