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 <[email protected]>
This commit is contained in:
GGThed
+67
-51
@@ -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')
|
||||
|
||||
@@ -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(
|
||||
|
||||
Reference in New Issue
Block a user