From 64bb7f9481a564bf2e3f946213622553e148945d Mon Sep 17 00:00:00 2001 From: Marco Acierno Date: Mon, 8 Jan 2024 23:24:26 +0000 Subject: [PATCH] Fix Submission permission model and crash on submission page (#3672) --- backend/api/submissions/schema.py | 12 +- .../api/submissions/tests/test_submission.py | 59 ++++++++- ...elds.py => test_submission_permissions.py} | 115 ++---------------- .../api/submissions/tests/test_submissions.py | 11 -- backend/api/submissions/types.py | 46 +------ backend/conftest.py | 11 ++ backend/schedule/tests/factories.py | 8 +- frontend/src/pages/submission/[id]/index.tsx | 13 +- 8 files changed, 110 insertions(+), 165 deletions(-) rename backend/api/submissions/tests/{test_submission_fields.py => test_submission_permissions.py} (69%) diff --git a/backend/api/submissions/schema.py b/backend/api/submissions/schema.py index e71c598a43..050c5e6f1f 100644 --- a/backend/api/submissions/schema.py +++ b/backend/api/submissions/schema.py @@ -1,5 +1,6 @@ import random import typing +from api.submissions.permissions import CanSeeSubmissionRestrictedFields import strawberry @@ -20,9 +21,18 @@ class SubmissionsQuery: @strawberry.field def submission(self, info, id: strawberry.ID) -> typing.Optional[Submission]: try: - return SubmissionModel.objects.get_by_hashid(id) + submission = SubmissionModel.objects.get_by_hashid(id) except SubmissionModel.DoesNotExist: return None + except IndexError: + return None + + if not CanSeeSubmissionRestrictedFields().has_permission( + source=submission, info=info + ): + return None + + return submission @strawberry.field(permission_classes=[IsAuthenticated]) def submissions( diff --git a/backend/api/submissions/tests/test_submission.py b/backend/api/submissions/tests/test_submission.py index 71b5966f62..d2b4bbc1ac 100644 --- a/backend/api/submissions/tests/test_submission.py +++ b/backend/api/submissions/tests/test_submission.py @@ -1,9 +1,11 @@ from pytest import mark from api.helpers.ids import encode_hashid +from schedule.tests.factories import ScheduleItemFactory + +pytestmark = mark.django_db -@mark.django_db def test_returns_none_when_missing(graphql_client): resp = graphql_client.query( """query SubmissionQuery($id: ID!) { @@ -18,6 +20,20 @@ def test_returns_none_when_missing(graphql_client): assert resp["data"]["submission"] is None +def test_returns_none_with_invalid_id_string(graphql_client): + resp = graphql_client.query( + """query SubmissionQuery($id: ID!) { + submission(id: $id) { + id + } + }""", + variables={"id": "invalid"}, + ) + + assert not resp.get("errors") + assert resp["data"]["submission"] is None + + def test_returns_correct_submission(graphql_client, user, submission_factory): graphql_client.force_login(user) submission = submission_factory(speaker_id=user.id) @@ -35,7 +51,6 @@ def test_returns_correct_submission(graphql_client, user, submission_factory): assert resp["data"]["submission"]["id"] == submission.hashid -@mark.django_db def test_user_can_edit_submission_if_within_cfp_time_and_is_the_owner( graphql_client, user, submission_factory ): @@ -57,12 +72,12 @@ def test_user_can_edit_submission_if_within_cfp_time_and_is_the_owner( assert response["data"]["submission"]["canEdit"] is True -@mark.django_db def test_cannot_edit_submission_if_not_the_owner( graphql_client, user, submission_factory ): graphql_client.force_login(user) submission = submission_factory(conference__active_cfp=True) + ScheduleItemFactory(submission=submission) response = graphql_client.query( """query Submission($id: ID!) { @@ -77,7 +92,6 @@ def test_cannot_edit_submission_if_not_the_owner( assert response["data"]["submission"] == {"id": submission.hashid, "canEdit": False} -@mark.django_db def test_can_edit_submission_if_cfp_is_closed(graphql_client, user, submission_factory): graphql_client.force_login(user) submission = submission_factory(speaker_id=user.id, conference__active_cfp=False) @@ -95,3 +109,40 @@ def test_can_edit_submission_if_cfp_is_closed(graphql_client, user, submission_f ) assert response["data"]["submission"]["canEdit"] is True + + +def test_cannot_see_submissions_if_restricted(graphql_client, user, submission_factory): + graphql_client.force_login(user) + submission = submission_factory(conference__active_cfp=True) + + response = graphql_client.query( + """query Submission($id: ID!) { + submission(id: $id) { + id + } + }""", + variables={"id": submission.hashid}, + ) + + assert response["data"]["submission"] is None + + +def test_can_see_submissions_while_voting_with_ticket( + graphql_client, user, submission_factory, mock_has_ticket +): + graphql_client.force_login(user) + submission = submission_factory( + conference__active_cfp=False, conference__active_voting=True + ) + mock_has_ticket(submission.conference) + + response = graphql_client.query( + """query Submission($id: ID!) { + submission(id: $id) { + id + } + }""", + variables={"id": submission.hashid}, + ) + + assert response["data"]["submission"]["id"] == submission.hashid diff --git a/backend/api/submissions/tests/test_submission_fields.py b/backend/api/submissions/tests/test_submission_permissions.py similarity index 69% rename from backend/api/submissions/tests/test_submission_fields.py rename to backend/api/submissions/tests/test_submission_permissions.py index 480ff1a15a..79c47a244d 100644 --- a/backend/api/submissions/tests/test_submission_fields.py +++ b/backend/api/submissions/tests/test_submission_permissions.py @@ -67,32 +67,13 @@ def test_voting_open_and_user_cannot_vote( ): submission = _submission(submission_factory, user) graphql_client.force_login(other_user) - can_vote_mock = mocker.patch( + mocker.patch( "api.submissions.permissions.check_if_user_can_vote", return_value=False ) data = _query(graphql_client, submission) - # ✔️ public - assert data["submission"]["title"] == submission.title.localize("en") - assert data["submission"]["slug"] == submission.slug - - # ❌ restricted - assert data["submission"]["elevatorPitch"] is None - assert data["submission"]["abstract"] is None - assert data["submission"]["topic"] is None - assert data["submission"]["type"] is None - assert data["submission"]["duration"] is None - assert data["submission"]["audienceLevel"] is None - assert data["submission"]["languages"] is None - assert data["submission"]["tags"] is None - - # ❌ private - assert data["submission"]["speakerLevel"] is None - assert data["submission"]["previousTalkVideo"] is None - assert data["submission"]["notes"] is None - - can_vote_mock.assert_called() + assert data["submission"] is None def test_voting_open_and_user_can_vote( @@ -106,11 +87,8 @@ def test_voting_open_and_user_can_vote( data = _query(graphql_client, submission) - # ✔️ public assert data["submission"]["title"] == submission.title.localize("en") assert data["submission"]["slug"] == submission.slug - - # ✔️ restricted assert data["submission"]["elevatorPitch"] == submission.elevator_pitch.localize( "en" ) @@ -136,7 +114,7 @@ def test_voting_open_and_user_can_vote( can_vote_mock.assert_called() -def test_voring_closed_and_user_is_authenticated( +def test_voting_closed_and_user_is_authenticated( graphql_client, other_user, submission_factory, user ): submission = _submission(submission_factory, user, conference__active_voting=False) @@ -144,51 +122,17 @@ def test_voring_closed_and_user_is_authenticated( data = _query(graphql_client, submission) - # ✔️ public - assert data["submission"]["title"] == submission.title.localize("en") - assert data["submission"]["slug"] == submission.slug + assert data["submission"] is None - # ❌ restricted - assert data["submission"]["elevatorPitch"] is None - assert data["submission"]["abstract"] is None - assert data["submission"]["topic"] is None - assert data["submission"]["type"] is None - assert data["submission"]["duration"] is None - assert data["submission"]["audienceLevel"] is None - assert data["submission"]["languages"] is None - assert data["submission"]["tags"] is None - # ❌ private - assert data["submission"]["speakerLevel"] is None - assert data["submission"]["previousTalkVideo"] is None - assert data["submission"]["notes"] is None - - -def test_voring_closed_and_user_is_not_authenticated( +def test_voting_closed_and_user_is_not_authenticated( graphql_client, submission_factory, user ): submission = _submission(submission_factory, user, conference__active_voting=False) data = _query(graphql_client, submission) - # ✔️ public - assert data["submission"]["title"] == submission.title.localize("en") - assert data["submission"]["slug"] == submission.slug - - # ❌ restricted - assert data["submission"]["elevatorPitch"] is None - assert data["submission"]["abstract"] is None - assert data["submission"]["topic"] is None - assert data["submission"]["type"] is None - assert data["submission"]["duration"] is None - assert data["submission"]["audienceLevel"] is None - assert data["submission"]["languages"] is None - assert data["submission"]["tags"] is None - - # ❌ private - assert data["submission"]["speakerLevel"] is None - assert data["submission"]["previousTalkVideo"] is None - assert data["submission"]["notes"] is None + assert data["submission"] is None def test_accepted_submission_user_can_see_public_and_restricted_fields( @@ -203,11 +147,8 @@ def test_accepted_submission_user_can_see_public_and_restricted_fields( data = _query(graphql_client, submission) - # ✔️ public assert data["submission"]["title"] == submission.title.localize("en") assert data["submission"]["slug"] == submission.slug - - # ✔️ restricted assert data["submission"]["elevatorPitch"] == submission.elevator_pitch.localize( "en" ) @@ -239,11 +180,9 @@ def test_admin_user_can_see_everything( data = _query(graphql_client, submission) - # ✔️ public assert data["submission"]["title"] == submission.title.localize("en") assert data["submission"]["slug"] == submission.slug - # ✔️ restricted assert data["submission"]["elevatorPitch"] == submission.elevator_pitch.localize( "en" ) @@ -273,11 +212,9 @@ def test_submission_author_can_see_everything(graphql_client, submission_factory data = _query(graphql_client, submission) - # ✔️ public assert data["submission"]["title"] == submission.title.localize("en") assert data["submission"]["slug"] == submission.slug - # ✔️ restricted assert data["submission"]["elevatorPitch"] == submission.elevator_pitch.localize( "en" ) @@ -315,11 +252,9 @@ def test_ranked_submission_user_can_see_public_and_restricted_fields( data = _query(graphql_client, submission) - # ✔️ public assert data["submission"]["title"] == submission.title.localize("en") assert data["submission"]["slug"] == submission.slug - # ✔️ restricted assert data["submission"]["elevatorPitch"] == submission.elevator_pitch.localize( "en" ) @@ -353,24 +288,7 @@ def test_ranking_is_not_public_cannot_see_restricted_and_private_fields( data = _query(graphql_client, submission) - # ✔️ public - assert data["submission"]["title"] == submission.title.localize("en") - assert data["submission"]["slug"] == submission.slug - - # ❌ restricted - assert data["submission"]["elevatorPitch"] is None - assert data["submission"]["abstract"] is None - assert data["submission"]["topic"] is None - assert data["submission"]["type"] is None - assert data["submission"]["duration"] is None - assert data["submission"]["audienceLevel"] is None - assert data["submission"]["languages"] is None - assert data["submission"]["tags"] is None - - # ❌ private - assert data["submission"]["speakerLevel"] is None - assert data["submission"]["previousTalkVideo"] is None - assert data["submission"]["notes"] is None + assert data["submission"] is None def test_ranking_does_not_exists_cannot_see_restricted_and_private_fields( @@ -384,21 +302,4 @@ def test_ranking_does_not_exists_cannot_see_restricted_and_private_fields( data = _query(graphql_client, submission) - # ✔️ public - assert data["submission"]["title"] == submission.title.localize("en") - assert data["submission"]["slug"] == submission.slug - - # ❌ restricted - assert data["submission"]["elevatorPitch"] is None - assert data["submission"]["abstract"] is None - assert data["submission"]["topic"] is None - assert data["submission"]["type"] is None - assert data["submission"]["duration"] is None - assert data["submission"]["audienceLevel"] is None - assert data["submission"]["languages"] is None - assert data["submission"]["tags"] is None - - # ❌ private - assert data["submission"]["speakerLevel"] is None - assert data["submission"]["previousTalkVideo"] is None - assert data["submission"]["notes"] is None + assert data["submission"] is None diff --git a/backend/api/submissions/tests/test_submissions.py b/backend/api/submissions/tests/test_submissions.py index 79b6e6770c..2aad20f839 100644 --- a/backend/api/submissions/tests/test_submissions.py +++ b/backend/api/submissions/tests/test_submissions.py @@ -3,17 +3,6 @@ pytestmark = pytest.mark.django_db -@pytest.fixture -def mock_has_ticket(requests_mock, settings): - def wrapper(conference): - requests_mock.post( - f"{settings.PRETIX_API}organizers/{conference.pretix_organizer_id}/events/{conference.pretix_event_id}/tickets/attendee-has-ticket/", - json={"user_has_admission_ticket": True}, - ) - - return wrapper - - def test_returns_submissions_paginated(graphql_client, user, submission_factory): graphql_client.force_login(user) diff --git a/backend/api/submissions/types.py b/backend/api/submissions/types.py index 60394f448d..6433c8a714 100644 --- a/backend/api/submissions/types.py +++ b/backend/api/submissions/types.py @@ -18,18 +18,6 @@ from api.schedule.types import ScheduleItem -def restricted_field() -> StrawberryField: - """Field that can only be seen by admin, the submitter or who has the ticket - until voting is not closed, after it will be public""" - - def resolver(self, info: Info): - if CanSeeSubmissionRestrictedFields().has_permission(self, info): - return getattr(self, info.python_name) - return None - - return strawberry.field(resolver=resolver) - - def private_field() -> StrawberryField: """Field that can only be seen by admin and the submitter""" @@ -82,16 +70,12 @@ class Submission: speaker_level: Optional[str] = private_field() previous_talk_video: Optional[str] = private_field() short_social_summary: Optional[str] = private_field() - topic: Optional[ - Annotated["Topic", strawberry.lazy("api.conferences.types")] - ] = restricted_field() - type: Optional[SubmissionType] = restricted_field() - duration: Optional[ - Annotated["Duration", strawberry.lazy("api.conferences.types")] - ] = restricted_field() + topic: Optional[Annotated["Topic", strawberry.lazy("api.conferences.types")]] + type: Optional[SubmissionType] + duration: Optional[Annotated["Duration", strawberry.lazy("api.conferences.types")]] audience_level: Optional[ Annotated["AudienceLevel", strawberry.lazy("api.conferences.types")] - ] = restricted_field() + ] notes: Optional[str] = private_field() @strawberry.field @@ -102,22 +86,14 @@ def schedule_items( @strawberry.field def multilingual_elevator_pitch(self, info: Info) -> Optional[MultiLingualString]: - if not CanSeeSubmissionRestrictedFields().has_permission(self, info): - return None return MultiLingualString.create(self.elevator_pitch) @strawberry.field def multilingual_abstract(self, info: Info) -> Optional[MultiLingualString]: - if not CanSeeSubmissionRestrictedFields().has_permission(self, info): - return None - return MultiLingualString.create(self.abstract) @strawberry.field def multilingual_title(self, info: Info) -> Optional[MultiLingualString]: - if not CanSeeSubmissionRestrictedFields().has_permission(self, info): - return None - return MultiLingualString.create(self.title) @strawberry.field @@ -126,16 +102,10 @@ def title(self, language: str) -> str: @strawberry.field() def elevator_pitch(self, language: str, info: Info) -> Optional[str]: - if not CanSeeSubmissionRestrictedFields().has_permission(self, info): - return None - return self.elevator_pitch.localize(language) @strawberry.field() def abstract(self, language: str, info: Info) -> Optional[str]: - if not CanSeeSubmissionRestrictedFields().has_permission(self, info): - return None - return self.abstract.localize(language) @strawberry.field @@ -176,15 +146,11 @@ def my_vote(self, info) -> Optional[VoteType]: @strawberry.field def languages(self, info) -> Optional[List[Language]]: - if CanSeeSubmissionRestrictedFields().has_permission(self, info): - return self.languages.all() - return None + return self.languages.all() @strawberry.field def tags(self, info) -> Optional[List[SubmissionTag]]: - if CanSeeSubmissionRestrictedFields().has_permission(self, info): - return self.tags.all() - return None + return self.tags.all() @strawberry.type diff --git a/backend/conftest.py b/backend/conftest.py index cd09afd502..10feead201 100644 --- a/backend/conftest.py +++ b/backend/conftest.py @@ -118,3 +118,14 @@ def wrapper(filename: str = "test.jpg"): @pytest.fixture def locale(): return lambda code: Locale.objects.get_or_create(language_code=code)[0] + + +@pytest.fixture +def mock_has_ticket(requests_mock, settings): + def wrapper(conference): + requests_mock.post( + f"{settings.PRETIX_API}organizers/{conference.pretix_organizer_id}/events/{conference.pretix_event_id}/tickets/attendee-has-ticket/", + json={"user_has_admission_ticket": True}, + ) + + return wrapper diff --git a/backend/schedule/tests/factories.py b/backend/schedule/tests/factories.py index 966b904660..461dd76ae8 100644 --- a/backend/schedule/tests/factories.py +++ b/backend/schedule/tests/factories.py @@ -54,8 +54,14 @@ class ScheduleItemFactory(DjangoModelFactory): @classmethod def _create(cls, model_class, *args, **kwargs): - _type = kwargs.get("type", None) + if "submission" in kwargs and "type" not in kwargs: + kwargs["type"] = ( + ScheduleItem.TYPES.talk + if kwargs["submission"].type.name == "talk" + else ScheduleItem.TYPES.training + ) + _type = kwargs.get("type", None) if _type == ScheduleItem.TYPES.custom: kwargs.pop("submission", None) diff --git a/frontend/src/pages/submission/[id]/index.tsx b/frontend/src/pages/submission/[id]/index.tsx index 024df77257..4f15af2340 100644 --- a/frontend/src/pages/submission/[id]/index.tsx +++ b/frontend/src/pages/submission/[id]/index.tsx @@ -15,6 +15,7 @@ import { createHref } from "~/components/link"; import { ScheduleEventDetail } from "~/components/schedule-event-detail"; import { prefetchSharedQueries } from "~/helpers/prefetch"; import { useCurrentLanguage } from "~/locale/context"; +import NotFoundPage from "~/pages/404"; import { getType } from "~/pages/event/[slug]"; import { queryIsVotingClosed, @@ -54,6 +55,10 @@ export const SubmissionPage = () => { const otherLanguage = viewInLanguage === "it" ? "en" : "it"; + if (!italianSubmission && !englishSubmission) { + return ; + } + return ( { const client = getApolloClient(null, req.cookies); - await Promise.all([ + const [_, englishSubmission, italianSubmission] = await Promise.all([ prefetchSharedQueries(client, locale), queryIsVotingClosed(client, { conference: process.env.conferenceCode, @@ -155,6 +160,12 @@ export const getServerSideProps: GetServerSideProps = async ({ }), ]); + if (!englishSubmission && !italianSubmission) { + return { + notFound: true, + }; + } + return addApolloState( client, {