diff --git a/src/apps/api/serializers/leaderboards.py b/src/apps/api/serializers/leaderboards.py index b4e244e1f..2c4fc7871 100644 --- a/src/apps/api/serializers/leaderboards.py +++ b/src/apps/api/serializers/leaderboards.py @@ -116,7 +116,7 @@ def get_submissions(self, instance): .prefetch_related( Prefetch( 'scores', - queryset=SubmissionScore.objects.select_related( + queryset=SubmissionScore.objects.filter(column__hidden=False).select_related( 'column', 'column__leaderboard', ), diff --git a/src/apps/api/serializers/submissions.py b/src/apps/api/serializers/submissions.py index 9c91737ca..eda7daa10 100644 --- a/src/apps/api/serializers/submissions.py +++ b/src/apps/api/serializers/submissions.py @@ -19,7 +19,7 @@ class SubmissionSerializer(serializers.ModelSerializer): - scores = SubmissionScoreSerializer(many=True) + scores = serializers.SerializerMethodField(read_only=True) filename = serializers.SerializerMethodField(read_only=True) owner = serializers.CharField(source='owner.username') phase_name = serializers.CharField(source='phase.name') @@ -72,6 +72,10 @@ def get_filename(self, instance): # NOTE: if submission data is None, it means it is soft deleted return "Deleted File" + def get_scores(self, instance): + scores = [score for score in instance.scores.all() if not score.column.hidden] + return SubmissionScoreSerializer(scores, many=True, context=self.context).data + def get_auto_run(self, instance): # returns this submission's competition auto_run_submissions Flag return instance.phase.competition.auto_run_submissions diff --git a/src/apps/api/tests/test_leaderboards.py b/src/apps/api/tests/test_leaderboards.py index 5455ef173..c5a899028 100644 --- a/src/apps/api/tests/test_leaderboards.py +++ b/src/apps/api/tests/test_leaderboards.py @@ -128,3 +128,95 @@ def test_anonymous_user_cannot_see_leaderboard_entries(self): self.lb.save() resp = self.get_leaderboard() assert resp.status_code == 200 + + +class HiddenColumnScoreTests(APITestCase): + """ + Column.hidden is meant to hide a score from everyone + viewing the leaderboard (e.g. so participants can't tune submissions + against it), so a hidden column's score must never appear in the + leaderboard-detail (`/api/leaderboards//`) response, for any viewer + - anonymous, logged-in, or organizer/admin alike - consistent with how + the `columns` metadata list already filters hidden columns out. + (Previously `submissions[*].scores` was built from a query with no + hidden-column filter, so a hidden column's score leaked through even + though the UI never rendered a column for it.) + """ + + def setUp(self): + self.creator = factories.UserFactory(username='hcs_creator', password='test') + self.admin = factories.UserFactory(username='hcs_admin', password='test', super_user=True) + self.normal_user = factories.UserFactory(username='hcs_normal', password='test') + self.comp = factories.CompetitionFactory(created_by=self.creator) + self.leaderboard = factories.LeaderboardFactory(hidden=False, primary_index=0) + self.phase = factories.PhaseFactory(competition=self.comp, leaderboard=self.leaderboard) + + self.visible_column = factories.ColumnFactory( + leaderboard=self.leaderboard, index=0, key='visible_col', hidden=False) + self.hidden_column = factories.ColumnFactory( + leaderboard=self.leaderboard, index=1, key='hidden_col', hidden=True) + + self.submission = factories.SubmissionFactory(phase=self.phase, leaderboard=self.leaderboard) + factories.SubmissionScoreFactory(submissions=[self.submission], column=self.visible_column) + factories.SubmissionScoreFactory(submissions=[self.submission], column=self.hidden_column) + + def get_leaderboard(self): + return self.client.get(reverse('leaderboard-detail', kwargs={'pk': self.leaderboard.id})) + + def score_column_keys(self, resp): + submissions = resp.json()['submissions'] + assert len(submissions) == 1 + return {score['column_key'] for score in submissions[0]['scores']} + + def test_anonymous_user_does_not_see_hidden_column_score(self): + """ + An anonymous (unauthenticated) GET on leaderboard-detail must not + include the hidden column's score in the submission's scores array. + Expected: only the visible column's score is returned. + """ + resp = self.get_leaderboard() + assert resp.status_code == 200 + keys = self.score_column_keys(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys + + def test_normal_authenticated_user_does_not_see_hidden_column_score(self): + """ + A logged-in user with no organizer/collaborator/admin relationship to + the competition must not see the hidden column's score via + leaderboard-detail either. Expected: only the visible column's score + is returned. + """ + self.client.force_login(self.normal_user) + resp = self.get_leaderboard() + assert resp.status_code == 200 + keys = self.score_column_keys(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys + + def test_competition_creator_does_not_see_hidden_column_score(self): + """ + For consistency with the columns metadata (which already filters out + hidden columns for everyone, organizers included), the competition + creator must not see the hidden column's score via leaderboard-detail + either. Expected: only the visible column's score is returned. + """ + self.client.force_login(self.creator) + resp = self.get_leaderboard() + assert resp.status_code == 200 + keys = self.score_column_keys(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys + + def test_superuser_does_not_see_hidden_column_score(self): + """ + Even a superuser/site-admin must not see the hidden column's score + via this endpoint, for the same consistency reason. Expected: only + the visible column's score is returned. + """ + self.client.force_login(self.admin) + resp = self.get_leaderboard() + assert resp.status_code == 200 + keys = self.score_column_keys(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys diff --git a/src/apps/api/tests/test_submissions.py b/src/apps/api/tests/test_submissions.py index 8601f0aa2..d6e4c5ad7 100644 --- a/src/apps/api/tests/test_submissions.py +++ b/src/apps/api/tests/test_submissions.py @@ -6,7 +6,7 @@ from competitions.models import Submission, CompetitionParticipant from factories import UserFactory, CompetitionFactory, PhaseFactory, CompetitionParticipantFactory, SubmissionFactory, \ - TaskFactory, OrganizationFactory, DataFactory, LeaderboardFactory + TaskFactory, OrganizationFactory, DataFactory, LeaderboardFactory, ColumnFactory, SubmissionScoreFactory from datasets.models import Data from profiles.models import Membership @@ -695,3 +695,116 @@ def test_organization_is_removed_from_soft_deleted_submission(self): self.organization_submission.refresh_from_db() assert self.organization_submission.is_soft_deleted is True assert self.organization_submission.organization is None + + +class HiddenColumnSubmissionScoreTests(APITestCase): + """ + Column.hidden is meant to hide a score from everyone + (e.g. so participants can't tune submissions against it), so a hidden + column's score must never appear in the submission-list + (`/api/submissions/?phase=`) or submission-detail + (`/api/submissions//`) responses, for any viewer - the submission's + own owner, or organizer/admin alike - consistent with how hidden columns + are already excluded from leaderboard column metadata. + (Previously SubmissionSerializer.scores serialized every SubmissionScore + row with no awareness of Column.hidden, so hidden-column values leaked + to anyone who could see the submission at all.) + + Anonymous viewers aren't covered here: SubmissionViewSet.get_queryset + returns nothing at all to them, tested separately by + test_anonymous_cannot_list_or_retrieve_submissions. + """ + + def setUp(self): + self.creator = UserFactory(username='hcss_creator', password='test') + self.superuser = UserFactory(username='hcss_admin', password='test', is_superuser=True, is_staff=True) + self.owner = UserFactory(username='hcss_owner', password='test') + self.comp = CompetitionFactory(created_by=self.creator) + self.leaderboard = LeaderboardFactory(primary_index=0) + self.phase = PhaseFactory(competition=self.comp, leaderboard=self.leaderboard) + + CompetitionParticipantFactory(user=self.owner, competition=self.comp, status=CompetitionParticipant.APPROVED) + + self.visible_column = ColumnFactory(leaderboard=self.leaderboard, index=0, key='visible_col', hidden=False) + self.hidden_column = ColumnFactory(leaderboard=self.leaderboard, index=1, key='hidden_col', hidden=True) + + # status=FINISHED and leaderboard=self.leaderboard put the submission in + # its most-exposed state (on a leaderboard, done running); what must stay + # hidden is the hidden column's score within it, not the submission itself. + self.submission = SubmissionFactory( + phase=self.phase, + owner=self.owner, + leaderboard=self.leaderboard, + status=Submission.FINISHED, + ) + SubmissionScoreFactory(submissions=[self.submission], column=self.visible_column) + SubmissionScoreFactory(submissions=[self.submission], column=self.hidden_column) + + def score_column_keys_from_list(self, resp): + body = resp.json() + submissions = body.get('results', body) + matching = [s for s in submissions if s['id'] == self.submission.id] + assert len(matching) == 1 + return {score['column_key'] for score in matching[0]['scores']} + + def score_column_keys_from_detail(self, resp): + return {score['column_key'] for score in resp.json()['scores']} + + def test_submission_owner_does_not_see_hidden_column_score_in_submission_list(self): + """ + A GET on submission-list as the submission owner, filtered to the + submission's phase, must not include the hidden column's score. + Expected: only the visible column's score is present in the + submission's scores array. + """ + self.client.force_login(self.owner) + url = reverse('submission-list') + resp = self.client.get(url, {'phase': self.phase.id}) + assert resp.status_code == 200 + keys = self.score_column_keys_from_list(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys + + def test_submission_owner_does_not_see_hidden_column_score(self): + """ + The submission owner is exactly the kind of participant an organizer + wants to keep a hidden metric from, so even the owner viewing their + own submission must not see the hidden column's score. Expected: only + the visible column's score is present. + """ + self.client.force_login(self.owner) + url = reverse('submission-detail', args=(self.submission.pk,)) + resp = self.client.get(url) + assert resp.status_code == 200 + keys = self.score_column_keys_from_detail(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys + + def test_competition_creator_does_not_see_hidden_column_score(self): + """ + For consistency with how hidden columns are excluded from leaderboard + column metadata for everyone, the competition creator must not see + the hidden column's score via submission-detail either. Expected: + only the visible column's score is present. + """ + self.client.force_login(self.creator) + url = reverse('submission-detail', args=(self.submission.pk,)) + resp = self.client.get(url) + assert resp.status_code == 200 + keys = self.score_column_keys_from_detail(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys + + def test_superuser_does_not_see_hidden_column_score(self): + """ + Even a superuser/site-admin must not see the hidden column's score + via submission-detail, for the same consistency reason. Expected: + only the visible column's score is present. + """ + self.client.force_login(self.superuser) + url = reverse('submission-detail', args=(self.submission.pk,)) + resp = self.client.get(url) + assert resp.status_code == 200 + keys = self.score_column_keys_from_detail(resp) + assert self.hidden_column.key not in keys + assert self.visible_column.key in keys