fix(authz): aligner la suppression d equipe, sans elargir l acces
SEC-AUTHZ-006. delete_team etait la seule des dix operations d'equipe gardee par la capacite globale can_manage_teams() ; les neuf autres passent par can_manage_this_org_team(team). Le constat avait raison sur l'incoherence et tort sur le correctif. Appliquer sa recommandation telle quelle -- remplacer par la verification par objet -- ELARGIT l'acces. Coach repond False a la capacite globale et True pour ses propres equipes : la substitution donnait donc a chaque coach le pouvoir de supprimer l'equipe qu'il entraine, avec ses notes d'equipe et son historique de matchs. Le constat raisonnait sur Manager, ou les deux repondent True, et a manque le role ou elles divergent. Les deux sont donc exigees. Le comportement d'aujourd'hui est preserve a l'identique (administrateurs et gerants oui, coachs non) et la dette que le constat visait est bien fermee : le jour ou Manager.can_manage_this_org_team sera resserre -- ce qui est souhaitable -- la suppression se resserrera avec lui au lieu de rester la seule porte ouverte. C'est la quatrieme recommandation d'audit qu'il faut corriger avant de l'appliquer. Le test qui l'epingle echoue si quelqu'un refait la simplification : verifie par mutation. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
+21
-3
@@ -223,12 +223,30 @@ def edit_team(team_id):
|
||||
@teams_bp.route('/<int:team_id>/delete', methods=['POST'])
|
||||
@login_required
|
||||
def delete_team(team_id):
|
||||
"""Delete an organization team."""
|
||||
if not current_user.can_manage_teams():
|
||||
"""Delete an organization team.
|
||||
|
||||
Two checks, not one, and not the one the audit recommended (SEC-AUTHZ-006).
|
||||
|
||||
The constat was right about the inconsistency: this was the only team
|
||||
operation guarded by the global `can_manage_teams()` while the other
|
||||
nine use `can_manage_this_org_team(team)`. It was wrong about the fix.
|
||||
Simply swapping to the per-object check **widens** access — `Coach`
|
||||
returns False for the global capability and True for its own teams, so
|
||||
the swap would hand every coach the power to delete the team they coach,
|
||||
along with its notes and its match history. The constat reasoned about
|
||||
`Manager`, where both return True, and missed the role where they differ.
|
||||
|
||||
Requiring both preserves today's behaviour exactly (admins and managers
|
||||
yes, coaches no) and still closes the debt the constat was about: the
|
||||
day `Manager.can_manage_this_org_team` is narrowed — which it should be —
|
||||
deletion narrows with it instead of staying the one way in.
|
||||
"""
|
||||
team = OrgTeam.query.get_or_404(team_id)
|
||||
|
||||
if not (current_user.can_manage_teams() and current_user.can_manage_this_org_team(team)):
|
||||
flash(_('You do not have permission to delete teams.'), 'danger')
|
||||
return redirect(url_for('teams.list_teams'))
|
||||
|
||||
team = OrgTeam.query.get_or_404(team_id)
|
||||
name = team.name
|
||||
|
||||
# One transaction. This used to commit three times, so a failure at the
|
||||
|
||||
Reference in New Issue
Block a user