fix(authz): finir SEC-16, que mon propre correctif avait laisse a moitie
La vague K a pose un schema sur create_team et edit_team et a laisse cinq routes soeurs du meme fichier lire int(request.form.get(...)) : add_coach, add_manager, remove_coach, remove_manager et add_player. Un identifiant non numerique y etait un 500 dans chacune. C'est exactement la lecon que ce projet repete depuis la vague D -- corriger un motif fautif dans une seule couche le laisse dans les autres -- et cette fois c'est le correctif lui-meme qui l'a commise. Notee comme telle. Deux defauts de plus, trouves en finissant. add_player ecrivait status tel quel dans une colonne NOT NULL String(20). Et toggle_player_status lit "substitute si status == starter, sinon starter" : une valeur inconnue devenait donc starter a la premiere bascule, c'est-a-dire promouvait son porteur. Liste blanche dans TEAM_PLAYER_STATUSES. Et aucune de ces routes ne regardait is_active_account. La requete qui alimente la liste deroulante des joueurs ne le filtrait pas non plus, alors que les deux requetes juste au-dessus, coachs et gerants, le posaient -- deux lignes d'ecart, meme fichier. Un compte desactive etait donc propose et accepte, alors que is_active_account est precisement ce qui dit que la personne a quitte le club. Meme oubli dans tryouts.py. _staff_member delegue desormais a _assignable au lieu de repeter isinstance : deux fonctions du meme fichier repondant differemment a "ce compte peut-il prendre ce role" est la forme de tous les defauts qu'a eus ce module. Verifie par mutation : retirer le controle d'activite ou la liste blanche fait tomber trois tests. Co-Authored-By: Claude Opus 5 <[email protected]>
This commit is contained in:
GGThed
@@ -583,3 +583,148 @@ class TestTeamStaffAssignment:
|
||||
assert response.status_code < 500
|
||||
with app.app_context():
|
||||
assert OrgTeam.query.count() == 0
|
||||
|
||||
|
||||
class TestTeamRosterAssignment:
|
||||
"""SEC-16, the half wave K missed.
|
||||
|
||||
Wave K put a schema on `create_team` and `edit_team` and left five
|
||||
sibling routes reading `int(request.form.get(...))` — `add_coach`,
|
||||
`add_manager`, `remove_coach`, `remove_manager` and `add_player`. Each
|
||||
was a 500 on a non-numeric id. `add_player` also accepted any `status`
|
||||
string into a NOT NULL column, and none of them checked whether the
|
||||
account had been deactivated.
|
||||
|
||||
Fixing a pattern in one place and not its neighbours is the mistake this
|
||||
project keeps making. Here it was made by the fix for it.
|
||||
"""
|
||||
|
||||
@pytest.fixture
|
||||
def team(self, app, make_user):
|
||||
from app.models import OrgTeam
|
||||
|
||||
admin_id = make_user('admin')
|
||||
with app.app_context():
|
||||
org_team = OrgTeam(name='Varsity', created_by=admin_id)
|
||||
db.session.add(org_team)
|
||||
db.session.commit()
|
||||
return org_team.id
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
('path', 'field'),
|
||||
[
|
||||
('add_coach', 'coach_id'),
|
||||
('add_manager', 'manager_id'),
|
||||
('remove_coach', 'coach_id'),
|
||||
('remove_manager', 'manager_id'),
|
||||
('add_player', 'player_id'),
|
||||
],
|
||||
)
|
||||
def test_a_non_numeric_id_is_not_a_500(self, app, client, team, as_role, path, field):
|
||||
as_role('admin')
|
||||
|
||||
response = client.post(f'/teams/{team}/{path}', data={field: 'not-a-number'})
|
||||
|
||||
assert response.status_code < 500
|
||||
|
||||
def test_a_real_coach_is_added(self, app, client, team, as_role, make_user):
|
||||
"""The premise for the two tests below."""
|
||||
from app.models import OrgTeam
|
||||
|
||||
coach_id = make_user('coach')
|
||||
as_role('admin')
|
||||
|
||||
client.post(f'/teams/{team}/add_coach', data={'coach_id': str(coach_id)})
|
||||
|
||||
with app.app_context():
|
||||
org_team = db.session.get(OrgTeam, team)
|
||||
assert [c.id for c in org_team.coaches.all()] == [coach_id]
|
||||
|
||||
def test_a_player_cannot_be_added_as_coach(self, app, client, team, as_role, make_user):
|
||||
from app.models import OrgTeam
|
||||
|
||||
player_id = make_user('player')
|
||||
as_role('admin')
|
||||
|
||||
client.post(f'/teams/{team}/add_coach', data={'coach_id': str(player_id)})
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(OrgTeam, team).coaches.all() == []
|
||||
|
||||
def test_a_deactivated_coach_cannot_be_added(self, app, client, team, as_role, make_user):
|
||||
"""`is_active_account` is what says the person has left the club.
|
||||
Putting them back on a roster contradicts it."""
|
||||
from app.models import OrgTeam, User
|
||||
|
||||
coach_id = make_user('coach')
|
||||
with app.app_context():
|
||||
db.session.get(User, coach_id).is_active_account = False
|
||||
db.session.commit()
|
||||
as_role('admin')
|
||||
|
||||
client.post(f'/teams/{team}/add_coach', data={'coach_id': str(coach_id)})
|
||||
|
||||
with app.app_context():
|
||||
assert db.session.get(OrgTeam, team).coaches.all() == []
|
||||
|
||||
def test_a_deactivated_player_cannot_be_added(self, app, client, team, as_role, make_user):
|
||||
from app.models import TeamPlayer, User
|
||||
|
||||
player_id = make_user('player')
|
||||
with app.app_context():
|
||||
db.session.get(User, player_id).is_active_account = False
|
||||
db.session.commit()
|
||||
as_role('admin')
|
||||
|
||||
client.post(f'/teams/{team}/add_player', data={'player_id': str(player_id)})
|
||||
|
||||
with app.app_context():
|
||||
assert TeamPlayer.query.filter_by(org_team_id=team).count() == 0
|
||||
|
||||
def test_a_deactivated_player_is_not_offered(self, app, client, as_role, make_user):
|
||||
"""The select and the route agreed on nothing: the query had no
|
||||
is_active_account filter while the two beside it did."""
|
||||
from app.models import User
|
||||
|
||||
player_id = make_user('player', username='ghostplayer')
|
||||
with app.app_context():
|
||||
db.session.get(User, player_id).is_active_account = False
|
||||
db.session.commit()
|
||||
as_role('admin')
|
||||
|
||||
body = client.get('/teams').get_data(as_text=True)
|
||||
|
||||
assert 'ghostplayer' not in body
|
||||
|
||||
def test_an_unknown_roster_status_is_refused(self, app, client, team, as_role, make_user):
|
||||
"""`status` went into a NOT NULL String(20) unchecked, and the toggle
|
||||
reads anything that is not 'starter' as substitute — so an unknown
|
||||
value silently promoted its holder on the next press."""
|
||||
from app.models import TeamPlayer
|
||||
|
||||
player_id = make_user('player')
|
||||
as_role('admin')
|
||||
|
||||
client.post(
|
||||
f'/teams/{team}/add_player',
|
||||
data={'player_id': str(player_id), 'status': 'captain-for-life'},
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
rows = TeamPlayer.query.filter_by(org_team_id=team).all()
|
||||
assert [r.status for r in rows] != ['captain-for-life']
|
||||
|
||||
def test_a_known_status_is_kept(self, app, client, team, as_role, make_user):
|
||||
from app.models import TeamPlayer
|
||||
|
||||
player_id = make_user('player')
|
||||
as_role('admin')
|
||||
|
||||
client.post(
|
||||
f'/teams/{team}/add_player',
|
||||
data={'player_id': str(player_id), 'status': 'substitute'},
|
||||
)
|
||||
|
||||
with app.app_context():
|
||||
row = TeamPlayer.query.filter_by(org_team_id=team).one()
|
||||
assert row.status == 'substitute'
|
||||
|
||||
Reference in New Issue
Block a user