diff --git a/app/routes/teams.py b/app/routes/teams.py index 62011f5..cbc65e1 100644 --- a/app/routes/teams.py +++ b/app/routes/teams.py @@ -223,12 +223,30 @@ def edit_team(team_id): @teams_bp.route('//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 diff --git a/tests/test_authorization.py b/tests/test_authorization.py index a865114..1120def 100644 --- a/tests/test_authorization.py +++ b/tests/test_authorization.py @@ -414,3 +414,71 @@ class TestCorsPolicy: ) assert response.headers.get('Access-Control-Allow-Origin') == 'https://trusted.test' + + +class TestTeamDeletionGuard: + """SEC-AUTHZ-006 — the one team operation guarded differently. + + `delete_team` used the global `can_manage_teams()` while the other nine + team operations use `can_manage_this_org_team(team)`. The constat asked + for a straight swap; a straight swap would have handed every coach the + power to delete the team they coach, because `Coach` answers False to + the first and True to the second. It now requires both, which keeps + today's answers and narrows automatically if the per-object rule is ever + tightened. + + These tests are what stops the "simplification" from being reapplied. + """ + + @pytest.fixture + def org_team(self, app, make_user): + from app.models import OrgTeam + + admin_id = make_user('admin') + coach_id = make_user('coach') + with app.app_context(): + team = OrgTeam(name='Varsity', created_by=admin_id, coach_id=coach_id) + db.session.add(team) + db.session.commit() + return team.id, coach_id + + def test_a_coach_cannot_delete_the_team_they_coach(self, app, client, org_team, login): + """The regression a straight swap would have introduced.""" + from app.models import OrgTeam + + team_id, coach_id = org_team + with app.app_context(): + username = db.session.get(User, coach_id).username + login(username) + + response = client.post(f'/teams/{team_id}/delete', follow_redirects=False) + + assert response.status_code < 500 + with app.app_context(): + assert db.session.get(OrgTeam, team_id) is not None, ( + 'a coach deleted an organisation team, its notes and its match history' + ) + + def test_a_manager_still_can(self, app, client, org_team, as_role): + """The premise. Without it the test above passes against a route + that refuses everyone.""" + from app.models import OrgTeam + + team_id, _coach_id = org_team + as_role('manager') + + client.post(f'/teams/{team_id}/delete', follow_redirects=False) + + with app.app_context(): + assert db.session.get(OrgTeam, team_id) is None + + def test_a_player_cannot(self, app, client, org_team, as_role): + from app.models import OrgTeam + + team_id, _coach_id = org_team + as_role('player') + + client.post(f'/teams/{team_id}/delete', follow_redirects=False) + + with app.app_context(): + assert db.session.get(OrgTeam, team_id) is not None