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]>
586 lines
20 KiB
Python
586 lines
20 KiB
Python
"""Access control regression tests.
|
|
|
|
Two kinds of test live here.
|
|
|
|
Passing tests pin down behaviour that is currently correct, so that the
|
|
architecture work in wave D — unifying the two coach/team models — cannot
|
|
quietly break it.
|
|
|
|
Tests marked xfail(strict=True) describe behaviour the audit found missing.
|
|
They fail today by design and will start passing when the matching finding
|
|
is fixed; strict mode then turns the unexpected pass into a failure, which
|
|
is the signal to remove the marker. They are executable documentation of
|
|
the gap, not a wish list.
|
|
"""
|
|
|
|
import pytest
|
|
|
|
from app.extensions import db
|
|
from app.models import User
|
|
|
|
#: Routes that must never answer to an unauthenticated caller.
|
|
PROTECTED_ROUTES = [
|
|
'/users',
|
|
'/users/create',
|
|
'/users/profile',
|
|
'/users/contracts',
|
|
'/tryouts',
|
|
'/teams',
|
|
'/evaluations',
|
|
'/matches/calendar',
|
|
'/team-matches',
|
|
]
|
|
|
|
#: Admin-only user management surface.
|
|
ADMIN_ONLY_ROUTES = [
|
|
'/users',
|
|
'/users/create',
|
|
]
|
|
|
|
|
|
def _redirected(response):
|
|
return response.status_code in (301, 302)
|
|
|
|
|
|
def _username(app, user_id):
|
|
with app.app_context():
|
|
return db.session.get(User, user_id).username
|
|
|
|
|
|
class TestAnonymousAccess:
|
|
@pytest.mark.parametrize('route', PROTECTED_ROUTES)
|
|
def test_anonymous_is_sent_to_login(self, client, route):
|
|
response = client.get(route, follow_redirects=False)
|
|
assert _redirected(response), f'{route} answered an anonymous caller'
|
|
assert '/auth/login' in response.headers.get('Location', '')
|
|
|
|
|
|
class TestVerticalAccess:
|
|
@pytest.mark.parametrize('route', ADMIN_ONLY_ROUTES)
|
|
@pytest.mark.parametrize('role', ['player', 'coach', 'manager', 'scout'])
|
|
def test_only_admin_reaches_user_management(self, client, as_role, role, route):
|
|
as_role(role)
|
|
response = client.get(route, follow_redirects=False)
|
|
assert _redirected(response), f'{role} reached {route}, which is meant to be admin-only'
|
|
|
|
def test_admin_reaches_user_management(self, client, as_role):
|
|
as_role('admin')
|
|
assert client.get('/users').status_code == 200
|
|
|
|
def test_player_cannot_list_evaluations(self, client, as_role):
|
|
as_role('player')
|
|
assert _redirected(client.get('/evaluations', follow_redirects=False))
|
|
|
|
def test_scout_cannot_list_teams(self, client, as_role):
|
|
as_role('scout')
|
|
assert _redirected(client.get('/teams', follow_redirects=False))
|
|
|
|
def test_non_player_cannot_request_one_on_one(self, client, as_role):
|
|
as_role('coach')
|
|
assert _redirected(client.get('/users/one-on-one', follow_redirects=False))
|
|
|
|
def test_non_coach_cannot_reach_notes_dashboard(self, client, as_role):
|
|
as_role('manager')
|
|
assert _redirected(client.get('/users/notes-dashboard', follow_redirects=False))
|
|
|
|
def test_player_cannot_delete_another_user(self, app, client, as_role, make_user):
|
|
victim_id = make_user('player')
|
|
as_role('player')
|
|
|
|
response = client.post(f'/users/{victim_id}/delete', follow_redirects=False)
|
|
assert _redirected(response)
|
|
|
|
with app.app_context():
|
|
assert db.session.get(User, victim_id) is not None, 'the user was deleted'
|
|
|
|
|
|
class TestHorizontalAccess:
|
|
def test_player_cannot_delete_another_players_availability(
|
|
self, app, client, as_role, make_user
|
|
):
|
|
from datetime import time
|
|
|
|
from app.models import PlayerDisponibility
|
|
|
|
owner_id = make_user('player')
|
|
with app.app_context():
|
|
slot = PlayerDisponibility(
|
|
player_id=owner_id,
|
|
day_of_week=1,
|
|
start_time=time(10, 0),
|
|
end_time=time(10, 30),
|
|
)
|
|
db.session.add(slot)
|
|
db.session.commit()
|
|
slot_id = slot.id
|
|
|
|
as_role('player')
|
|
response = client.post(f'/users/disponibilities/{slot_id}/delete')
|
|
|
|
assert response.status_code == 403
|
|
with app.app_context():
|
|
assert db.session.get(PlayerDisponibility, slot_id) is not None
|
|
|
|
|
|
class TestNestedResourceOwnership:
|
|
"""SEC-AUTHZ-002 — routes taking both a parent and a child id have to
|
|
check that the child actually belongs to the parent. Authorising only
|
|
the parent lets a manager of tryout A reach a team of tryout B."""
|
|
|
|
@staticmethod
|
|
def _make_tryout_with_team(app, owner_id, title):
|
|
from datetime import date
|
|
|
|
from app.models import Team, Tryout
|
|
|
|
with app.app_context():
|
|
tryout = Tryout(
|
|
title=title,
|
|
game='Valorant',
|
|
date=date(2030, 1, 1),
|
|
created_by=owner_id,
|
|
status='upcoming',
|
|
)
|
|
db.session.add(tryout)
|
|
db.session.flush()
|
|
team = Team(tryout_id=tryout.id, name=f'{title} squad', created_by=owner_id)
|
|
db.session.add(team)
|
|
db.session.commit()
|
|
return tryout.id, team.id
|
|
|
|
def test_cannot_add_a_player_to_a_team_of_another_tryout(self, app, client, as_role, make_user):
|
|
from app.models import TeamMember, TryoutRegistration
|
|
|
|
other_admin = make_user('admin')
|
|
_, foreign_team_id = self._make_tryout_with_team(app, other_admin, 'Foreign')
|
|
|
|
manager_id = as_role('manager')
|
|
own_tryout_id, _ = self._make_tryout_with_team(app, manager_id, 'Mine')
|
|
|
|
player_id = make_user('player')
|
|
with app.app_context():
|
|
db.session.add(TryoutRegistration(tryout_id=own_tryout_id, player_id=player_id))
|
|
db.session.commit()
|
|
|
|
response = client.post(
|
|
f'/tryouts/{own_tryout_id}/team/{foreign_team_id}/add',
|
|
data={'player_id': player_id},
|
|
follow_redirects=False,
|
|
)
|
|
|
|
assert response.status_code == 404, 'a team belonging to another tryout was accepted'
|
|
with app.app_context():
|
|
assert TeamMember.query.filter_by(team_id=foreign_team_id).count() == 0
|
|
|
|
def test_cannot_add_a_player_who_is_not_registered(self, app, client, as_role, make_user):
|
|
from app.models import TeamMember
|
|
|
|
manager_id = as_role('manager')
|
|
tryout_id, team_id = self._make_tryout_with_team(app, manager_id, 'Mine')
|
|
outsider_id = make_user('player')
|
|
|
|
client.post(
|
|
f'/tryouts/{tryout_id}/team/{team_id}/add',
|
|
data={'player_id': outsider_id},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
assert TeamMember.query.filter_by(team_id=team_id).count() == 0
|
|
|
|
def test_a_registered_player_can_still_be_added(self, app, client, as_role, make_user):
|
|
"""Guard against over-correcting: the normal path must keep working."""
|
|
from app.models import TeamMember, TryoutRegistration
|
|
|
|
manager_id = as_role('manager')
|
|
tryout_id, team_id = self._make_tryout_with_team(app, manager_id, 'Mine')
|
|
player_id = make_user('player')
|
|
|
|
with app.app_context():
|
|
db.session.add(TryoutRegistration(tryout_id=tryout_id, player_id=player_id))
|
|
db.session.commit()
|
|
|
|
client.post(
|
|
f'/tryouts/{tryout_id}/team/{team_id}/add',
|
|
data={'player_id': player_id, 'position': 'Duelist'},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
member = TeamMember.query.filter_by(team_id=team_id).one()
|
|
assert member.player_id == player_id
|
|
assert member.position == 'Duelist'
|
|
|
|
def test_a_non_numeric_player_id_does_not_crash(self, app, client, as_role):
|
|
"""int(player_id) on raw form input used to raise, i.e. a 500."""
|
|
manager_id = as_role('manager')
|
|
tryout_id, team_id = self._make_tryout_with_team(app, manager_id, 'Mine')
|
|
|
|
response = client.post(
|
|
f'/tryouts/{tryout_id}/team/{team_id}/add',
|
|
data={'player_id': 'not-a-number'},
|
|
follow_redirects=False,
|
|
)
|
|
assert response.status_code < 500
|
|
|
|
|
|
class TestInputValidation:
|
|
"""SEC-AUTHZ-001 — regression guard.
|
|
|
|
CreateUserSchema, EditUserSchema and EditProfileSchema used to be
|
|
imported at users.py:23-26 and never called: each name appeared exactly
|
|
once in the file, on its import line. The three routes read request.form
|
|
directly, so no password policy and no username format rule applied
|
|
anywhere in user management. These tests fail if that ever comes back.
|
|
"""
|
|
|
|
def test_edit_profile_rejects_html_in_username(self, app, client, as_role):
|
|
user_id = as_role('player')
|
|
payload = '<img src=x onerror=alert(1)>'
|
|
|
|
client.post(
|
|
'/users/profile/edit',
|
|
data={
|
|
'username': payload,
|
|
'full_name': 'Legit Name',
|
|
'email': '[email protected]',
|
|
},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
assert db.session.get(User, user_id).username != payload
|
|
|
|
def test_edit_profile_enforces_the_password_policy(self, app, client, as_role):
|
|
user_id = as_role('player')
|
|
with app.app_context():
|
|
before = db.session.get(User, user_id).password_hash
|
|
|
|
client.post(
|
|
'/users/profile/edit',
|
|
data={
|
|
'username': _username(app, user_id),
|
|
'full_name': 'Legit Name',
|
|
'email': '[email protected]',
|
|
'password': 'a',
|
|
},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
assert db.session.get(User, user_id).password_hash == before, (
|
|
'a one-character password was accepted'
|
|
)
|
|
|
|
def test_create_user_enforces_the_password_policy(self, app, client, as_role):
|
|
as_role('admin')
|
|
|
|
client.post(
|
|
'/users/create',
|
|
data={
|
|
'username': 'weakling',
|
|
'email': '[email protected]',
|
|
'password': 'a',
|
|
'full_name': 'Weak Account',
|
|
'role': 'admin',
|
|
},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
created = User.query.filter_by(username='weakling').first()
|
|
assert created is None, 'an admin account was created with password "a"'
|
|
|
|
def test_edit_user_rejects_a_duplicate_email(self, app, client, as_role, make_user):
|
|
other_id = make_user('player')
|
|
target_id = make_user('player')
|
|
as_role('admin')
|
|
|
|
with app.app_context():
|
|
taken = db.session.get(User, other_id).email
|
|
|
|
response = client.post(
|
|
f'/users/{target_id}/edit',
|
|
data={
|
|
'full_name': 'Target',
|
|
'email': taken,
|
|
'role': 'player',
|
|
},
|
|
follow_redirects=False,
|
|
)
|
|
|
|
assert response.status_code < 500, 'duplicate email produced a server error'
|
|
|
|
|
|
class TestAdminSafety:
|
|
def test_the_last_admin_cannot_demote_itself(self, app, client, as_role):
|
|
admin_id = as_role('admin')
|
|
|
|
client.post(
|
|
f'/users/{admin_id}/edit',
|
|
data={
|
|
'full_name': 'Admin',
|
|
'email': '[email protected]',
|
|
'role': 'player',
|
|
},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
assert db.session.get(User, admin_id).role == 'admin', (
|
|
'the only administrator demoted itself; no interface can undo this'
|
|
)
|
|
|
|
def test_an_admin_cannot_change_its_own_role_even_with_others_present(
|
|
self, app, client, as_role, make_user
|
|
):
|
|
"""Self-demotion is refused on its own, not only when last."""
|
|
make_user('admin')
|
|
admin_id = as_role('admin')
|
|
|
|
client.post(
|
|
f'/users/{admin_id}/edit',
|
|
data={
|
|
'full_name': 'Admin',
|
|
'email': '[email protected]',
|
|
'role': 'player',
|
|
},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
assert db.session.get(User, admin_id).role == 'admin'
|
|
|
|
def test_another_admin_can_still_be_demoted(self, app, client, as_role, make_user):
|
|
"""Guard against over-correcting: the legitimate path must work."""
|
|
other_id = make_user('admin')
|
|
as_role('admin')
|
|
|
|
client.post(
|
|
f'/users/{other_id}/edit',
|
|
data={
|
|
'full_name': 'Other',
|
|
'email': '[email protected]',
|
|
'role': 'coach',
|
|
'is_active_account': 'on',
|
|
},
|
|
follow_redirects=True,
|
|
)
|
|
|
|
with app.app_context():
|
|
assert db.session.get(User, other_id).role == 'coach'
|
|
|
|
|
|
class TestCsrf:
|
|
def test_state_changing_post_without_a_token_is_rejected(self, app_with_csrf):
|
|
"""CSRFProtect is global. This pins that down so a future
|
|
@csrf.exempt cannot slip in unnoticed."""
|
|
client = app_with_csrf.test_client()
|
|
response = client.post(
|
|
'/auth/login',
|
|
data={
|
|
'username': 'someone',
|
|
'password': 'Password123',
|
|
},
|
|
)
|
|
assert response.status_code == 400
|
|
|
|
|
|
class TestCorsPolicy:
|
|
"""SEC-WEB-003 — with no origins configured, flask-cors defaulted to '*'
|
|
and, credentials being allowed, echoed back the caller's Origin."""
|
|
|
|
def test_no_cors_headers_without_explicit_configuration(self, client):
|
|
response = client.get('/auth/login', headers={'Origin': 'https://evil.test'})
|
|
assert 'Access-Control-Allow-Origin' not in response.headers
|
|
assert 'Access-Control-Allow-Credentials' not in response.headers
|
|
|
|
def test_configured_origins_are_still_honoured(self, app_with_csrf):
|
|
from app.app import create_app
|
|
|
|
application = create_app(
|
|
{
|
|
'SECRET_KEY': 'test',
|
|
'SQLALCHEMY_DATABASE_URI': 'sqlite:///:memory:',
|
|
'TESTING': True,
|
|
'FORCE_HTTPS': False,
|
|
'ENABLE_DISCORD_BOT': False,
|
|
'AUTO_CREATE_TABLES': False,
|
|
'CORS_ALLOWED_ORIGINS': 'https://trusted.test',
|
|
}
|
|
)
|
|
response = application.test_client().get(
|
|
'/auth/login', headers={'Origin': 'https://trusted.test'}
|
|
)
|
|
|
|
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
|
|
|
|
|
|
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
|