refactor(authz): un seul point de verite pour les autorisations d equipe
ARCH-002. La question "a quelles equipes ce coach est-il rattache ?" etait
posee a huit endroits, de cinq facons differentes, et trois d entre elles
donnaient une mauvaise reponse en production.
Le motif fautif, present tel quel dans six routes :
OrgTeam.query.filter_by(coach_id=user.id).first()
Il repond au plus une equipe, et seulement par la colonne heritee. Deux
pannes en decoulaient, silencieuses -- les pages s affichaient, vides :
- un coach rattache uniquement par la relation many-to-many n avait
aucune equipe, donc aucun joueur, aucun contrat, aucune note d equipe,
aucun match a venir sur son tableau de bord ;
- un coach de deux equipes n en voyait qu une. Le formulaire de contrat
lui proposait la moitie de son effectif, alors que la route POST
acceptait l autre moitie.
app/permissions.py devient le module ou la question se pose une fois :
coach_org_teams, manager_org_teams, attached_org_teams, visible_org_teams,
can_manage_org_team, org_team_player_ids, coach_player_ids,
can_manage_player_contract, coach_tryouts, coach_manages_tryout. Toutes
lisent les deux rattachements et toutes les equipes.
Le meme ecart existait dans le modele : Coach.get_visible_tryouts ne lisait
que la relation m2m -- calendrier vide pour un coach rattache par la
colonne -- et can_manage_this_tryout ignorait la colonne pour l equipe
cible. Les deux delegent desormais.
Corrections de portee, au passage
- one_on_one lisait org_team.coach_id : un joueur dont l equipe declare
ses coachs par la relation etait informe qu il n avait pas de coach, et
le formulaire de demande restait ferme. Passe par get_coaches(), qui
retombe deja sur la colonne heritee.
- notes_dashboard conditionnait les notes personnelles du coach a
l existence d une equipe : un coach sans equipe ne voyait pas ses
propres notes.
- can_manage_team_match reformulait can_manage_this_org_team ; la
reformulation avait derive. Elle appelle maintenant la regle.
Limite assumee : le panneau de notes d equipe reste ecrit pour une seule
equipe et affiche donc la premiere. La resolution est corrigee, la mise en
page multi-equipes ne l est pas -- c est un choix produit, pas un bug.
14 tests ajoutes. Cinq echouent sur le code d avant, verifie en remettant
les routes et le modele a leur etat precedent.
ARCH-001 fera disparaitre la colonne heritee ; cela demande une migration
de donnees, donc Alembic. D ici la, ce module est ce qui rend la
duplication inoffensive.
Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
+260
-31
@@ -1,28 +1,40 @@
|
||||
"""Shared access-control rules.
|
||||
"""Shared access-control rules — the single point of truth (ARCH-002).
|
||||
|
||||
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.
|
||||
Authorisation used to live inline in eight route modules, and the same
|
||||
question could get a different answer depending on which URL you reached.
|
||||
This module now holds the rules themselves; routes and models call it.
|
||||
|
||||
An important subtlety this module hides from callers: a coach can be
|
||||
attached to a team two different ways.
|
||||
The subtlety it hides from callers: a coach or a manager 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.
|
||||
Both are still populated. Reading only ``coach_id`` — which most of
|
||||
users.py did — silently locked out every coach who was not the first one on
|
||||
their team, and reading ``.first()`` on top of it locked a coach out of
|
||||
every team but one. Both defects were live in production. Every function
|
||||
here considers both attachment routes and every team, so the fix applies
|
||||
once instead of at each call site.
|
||||
|
||||
ARCH-001 will collapse the two columns into one for good; that needs a data
|
||||
migration, so until then this module is what makes the duplication
|
||||
harmless.
|
||||
|
||||
Functions take the acting user explicitly rather than reading
|
||||
``current_user``: it keeps them callable from models, from the Discord bot,
|
||||
and from tests without a request context.
|
||||
"""
|
||||
|
||||
from app.extensions import db
|
||||
|
||||
|
||||
def coach_org_team_ids(coach):
|
||||
"""IDs of the organisation teams a coach is attached to.
|
||||
# ---------------------------------------------------------------------------
|
||||
# Team attachment
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def coach_org_teams(coach):
|
||||
"""Organisation teams a coach is attached to, ordered by name.
|
||||
|
||||
Considers the many-to-many relationship *and* the legacy column, so the
|
||||
second coach of a team is not treated as belonging to nothing.
|
||||
@@ -31,17 +43,148 @@ def coach_org_team_ids(coach):
|
||||
coach: The user to inspect.
|
||||
|
||||
Returns:
|
||||
list[int]: Team IDs, possibly empty.
|
||||
list[OrgTeam]: Possibly empty.
|
||||
"""
|
||||
from app.models import OrgTeam
|
||||
|
||||
teams = OrgTeam.query.filter(
|
||||
return OrgTeam.query.filter(
|
||||
db.or_(
|
||||
OrgTeam.coaches.any(id=coach.id),
|
||||
OrgTeam.coach_id == coach.id,
|
||||
)
|
||||
).all()
|
||||
return [team.id for team in teams]
|
||||
).order_by(OrgTeam.name).all()
|
||||
|
||||
|
||||
def coach_org_team_ids(coach):
|
||||
"""IDs of the organisation teams a coach is attached to.
|
||||
|
||||
Args:
|
||||
coach: The user to inspect.
|
||||
|
||||
Returns:
|
||||
list[int]: Team IDs, possibly empty.
|
||||
"""
|
||||
return [team.id for team in coach_org_teams(coach)]
|
||||
|
||||
|
||||
def manager_org_teams(manager):
|
||||
"""Organisation teams a manager is attached to, ordered by name.
|
||||
|
||||
Symmetric to :func:`coach_org_teams`: ``manager_id`` and ``managers``
|
||||
carry the same duplication.
|
||||
|
||||
Args:
|
||||
manager: The user to inspect.
|
||||
|
||||
Returns:
|
||||
list[OrgTeam]: Possibly empty.
|
||||
"""
|
||||
from app.models import OrgTeam
|
||||
|
||||
return OrgTeam.query.filter(
|
||||
db.or_(
|
||||
OrgTeam.managers.any(id=manager.id),
|
||||
OrgTeam.manager_id == manager.id,
|
||||
)
|
||||
).order_by(OrgTeam.name).all()
|
||||
|
||||
|
||||
def attached_org_teams(user):
|
||||
"""Teams the user is personally attached to, whatever their role.
|
||||
|
||||
A president is attached to none in particular — they administer them
|
||||
all — so this returns an empty list for them. Callers that want "the
|
||||
teams to display" want :func:`visible_org_teams` instead.
|
||||
|
||||
Args:
|
||||
user: The acting user.
|
||||
|
||||
Returns:
|
||||
list[OrgTeam]: Possibly empty.
|
||||
"""
|
||||
from app.models import Coach, Manager, Player
|
||||
|
||||
if isinstance(user, Coach):
|
||||
return coach_org_teams(user)
|
||||
if isinstance(user, Manager):
|
||||
return manager_org_teams(user)
|
||||
if isinstance(user, Player):
|
||||
return user.get_org_teams()
|
||||
return []
|
||||
|
||||
|
||||
def visible_org_teams(user):
|
||||
"""Organisation teams the user may see, ordered by name.
|
||||
|
||||
A president sees every team; a coach or a manager sees the ones they
|
||||
are attached to; a player sees the ones they play on; anyone else sees
|
||||
none.
|
||||
|
||||
Args:
|
||||
user: The acting user.
|
||||
|
||||
Returns:
|
||||
list[OrgTeam]: Possibly empty.
|
||||
"""
|
||||
from app.models import Admin, OrgTeam
|
||||
|
||||
if isinstance(user, Admin):
|
||||
return OrgTeam.query.order_by(OrgTeam.name).all()
|
||||
return attached_org_teams(user)
|
||||
|
||||
|
||||
def can_manage_org_team(user, org_team):
|
||||
"""Whether the user may administer this organisation team.
|
||||
|
||||
Delegates to the polymorphic model method, which is the role-level
|
||||
rule; this wrapper exists so route code has one name to call and never
|
||||
has to know which subclass it is holding.
|
||||
|
||||
Args:
|
||||
user: The acting user.
|
||||
org_team: The team concerned.
|
||||
|
||||
Returns:
|
||||
bool
|
||||
"""
|
||||
return bool(org_team) and user.can_manage_this_org_team(org_team)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Reach over players
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def org_team_player_ids(team_ids):
|
||||
"""IDs of the players placed on any of these teams.
|
||||
|
||||
Args:
|
||||
team_ids: Team primary keys.
|
||||
|
||||
Returns:
|
||||
list[int]: Player IDs, possibly empty, without duplicates.
|
||||
"""
|
||||
from app.models import TeamPlayer
|
||||
|
||||
if not team_ids:
|
||||
return []
|
||||
rows = TeamPlayer.query.filter(TeamPlayer.org_team_id.in_(team_ids)).all()
|
||||
return list({row.player_id for row in rows})
|
||||
|
||||
|
||||
def coach_player_ids(coach):
|
||||
"""IDs of the players on *all* of a coach's teams.
|
||||
|
||||
The routes this replaces looked at one team — the first row matching
|
||||
the legacy column — so a coach of two teams could act on half of their
|
||||
squad and no more.
|
||||
|
||||
Args:
|
||||
coach: The acting coach.
|
||||
|
||||
Returns:
|
||||
list[int]: Player IDs, possibly empty.
|
||||
"""
|
||||
return org_team_player_ids(coach_org_team_ids(coach))
|
||||
|
||||
|
||||
def coach_can_access_player(coach, player_id):
|
||||
@@ -59,9 +202,7 @@ def coach_can_access_player(coach, player_id):
|
||||
Returns:
|
||||
bool
|
||||
"""
|
||||
from app.models import (
|
||||
Match, MatchParticipant, TeamPlayer, Tryout, TryoutRegistration,
|
||||
)
|
||||
from app.models import Match, MatchParticipant, TeamPlayer, TryoutRegistration
|
||||
|
||||
if not player_id:
|
||||
return False
|
||||
@@ -75,16 +216,7 @@ def coach_can_access_player(coach, player_id):
|
||||
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()]
|
||||
tryout_ids = coach_tryout_ids(coach, team_ids=team_ids)
|
||||
if not tryout_ids:
|
||||
return False
|
||||
|
||||
@@ -100,3 +232,100 @@ def coach_can_access_player(coach, player_id):
|
||||
Match.tryout_id.in_(tryout_ids),
|
||||
).first()
|
||||
return plays_a_match is not None
|
||||
|
||||
|
||||
def can_manage_player_contract(user, player_id):
|
||||
"""Whether the user may upload or replace a contract for this player.
|
||||
|
||||
Presidents and managers may do so for anyone. A coach may do so for the
|
||||
players on their teams — the working relationship a contract implies.
|
||||
|
||||
Args:
|
||||
user: The acting user.
|
||||
player_id: Primary key of the player concerned.
|
||||
|
||||
Returns:
|
||||
bool
|
||||
"""
|
||||
from app.models import Admin, Coach, Manager
|
||||
|
||||
if not player_id:
|
||||
return False
|
||||
if isinstance(user, (Admin, Manager)):
|
||||
return True
|
||||
if isinstance(user, Coach):
|
||||
return player_id in coach_player_ids(user)
|
||||
return False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tryouts
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def coach_tryouts(coach, team_ids=None):
|
||||
"""Tryouts a coach manages, ordered by date.
|
||||
|
||||
A coach reaches a tryout through any of the three routes the model
|
||||
supports: it targets one of their teams, they are named on the
|
||||
many-to-many relationship, or the deprecated ``coach_id`` points at
|
||||
them.
|
||||
|
||||
Args:
|
||||
coach: The acting coach.
|
||||
team_ids: Pre-computed team IDs, to avoid querying twice when the
|
||||
caller already has them.
|
||||
|
||||
Returns:
|
||||
list[Tryout]: Possibly empty.
|
||||
"""
|
||||
from app.models import Tryout
|
||||
|
||||
if team_ids is None:
|
||||
team_ids = coach_org_team_ids(coach)
|
||||
|
||||
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))
|
||||
|
||||
return Tryout.query.filter(db.or_(*conditions)).order_by(Tryout.date).all()
|
||||
|
||||
|
||||
def coach_tryout_ids(coach, team_ids=None):
|
||||
"""IDs of the tryouts a coach manages.
|
||||
|
||||
Args:
|
||||
coach: The acting coach.
|
||||
team_ids: Pre-computed team IDs, see :func:`coach_tryouts`.
|
||||
|
||||
Returns:
|
||||
list[int]: Possibly empty.
|
||||
"""
|
||||
return [tryout.id for tryout in coach_tryouts(coach, team_ids=team_ids)]
|
||||
|
||||
|
||||
def coach_manages_tryout(coach, tryout):
|
||||
"""Whether a coach manages this particular tryout.
|
||||
|
||||
Same three routes as :func:`coach_tryouts`, asked about one row.
|
||||
Written as a membership test rather than a query so that a tryout not
|
||||
yet flushed to the database still answers correctly.
|
||||
|
||||
Args:
|
||||
coach: The acting coach.
|
||||
tryout: The tryout concerned.
|
||||
|
||||
Returns:
|
||||
bool
|
||||
"""
|
||||
if tryout is None:
|
||||
return False
|
||||
if tryout.coach_id == coach.id:
|
||||
return True
|
||||
if any(c.id == coach.id for c in tryout.coaches):
|
||||
return True
|
||||
if tryout.target_org_team_id:
|
||||
return tryout.target_org_team_id in coach_org_team_ids(coach)
|
||||
return False
|
||||
|
||||
Reference in New Issue
Block a user