diff --git a/apps/api/plane/app/views/view/base.py b/apps/api/plane/app/views/view/base.py index bc464c4e6aa..863b165eb09 100644 --- a/apps/api/plane/app/views/view/base.py +++ b/apps/api/plane/app/views/view/base.py @@ -117,8 +117,30 @@ def partial_update(self, request, slug, pk): return Response(serializer.data, status=status.HTTP_200_OK) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) + @allow_permission(allowed_roles=[ROLE.ADMIN, ROLE.MEMBER, ROLE.GUEST], level="WORKSPACE") def retrieve(self, request, slug, pk): + # Requires workspace membership, matching list() above. This was the only + # action on this class with no check at all, and get_queryset() does not + # supply one either: it filters on workspace__slug and then + # `Q(owned_by=request.user) | Q(access=1)`. That second clause looks like + # a visibility predicate but is vacuous — `access` is in the + # serializer's read_only_fields so the API never sets it, and the model + # defaults it to 1 (Public), so every row matches. A caller with no + # membership in the workspace could therefore read any global view in it + # by id. issue_view = self.get_queryset().filter(pk=pk).first() + + # get_queryset() is scoped to the URL workspace, so None means either no + # such view or one belonging to a workspace this URL does not name. + # Serializing None yields a hollow object — every field null or empty — + # returned as 200, and enqueues a recent-visit for an entity that does + # not exist. Answer 404, as the other retrieve endpoints do. + if issue_view is None: + return Response( + {"error": "The required object does not exist."}, + status=status.HTTP_404_NOT_FOUND, + ) + serializer = IssueViewSerializer(issue_view) recent_visited_task.delay( slug=slug, diff --git a/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py b/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py new file mode 100644 index 00000000000..3f409327073 --- /dev/null +++ b/apps/api/plane/tests/contract/app/test_undecorated_action_authz_app.py @@ -0,0 +1,277 @@ +# Copyright (c) 2023-present Plane Software, Inc. and contributors +# SPDX-License-Identifier: AGPL-3.0-only +# See the LICENSE file for details. + +"""Contract tests for viewset actions whose authorization lives outside a decorator. + +These actions are *defined* on their viewset, so a scan for routes falling +through to a DRF generic mixin cannot see them, and neither carries an +``@allow_permission`` decorator. That makes them easy to misread in both +directions — one of the two turned out to be guarded after all. + +1. **Fix.** ``WorkspaceViewViewSet.retrieve`` + (``/workspaces//views//``) had no check at all, and its queryset + supplied none either: it filters on ``workspace__slug`` and then + ``Q(owned_by=request.user) | Q(access=1)``. That second clause reads like a + visibility predicate but is vacuous — ``access`` sits in the serializer's + ``read_only_fields`` so the API never sets it, and the model defaults it to + ``1`` (Public), so every row matches. A user with no membership in the + workspace could read any global view in it by id. It now requires workspace + membership, matching ``list``. + +2. **Regression coverage only, no fix.** ``IssueDetailIdentifierEndpoint`` + (``/workspaces//work-items/-/``) resolves a work item by its + human-readable sequence number, which makes it an enumeration surface. It was + reported as missing the guest restriction. **It is not.** The membership check + near the top of ``get`` is followed, after the issue is fetched, by an + explicit role-5 / ``guest_view_all_features`` / ``created_by`` check that + returns 403. That guard had no test, so the disclosure it prevents was one + refactor away from being lost silently. These tests pin it. +""" + +from uuid import uuid4 + +import pytest +from rest_framework import status +from rest_framework.test import APIClient + +from plane.db.models import ( + Issue, + IssueView, + Project, + ProjectMember, + User, + Workspace, + WorkspaceMember, +) + +WORK_ITEM_BY_IDENTIFIER_URL = "/api/workspaces/{slug}/work-items/{project_identifier}-{sequence_id}/" +WORKSPACE_VIEW_DETAIL_URL = "/api/workspaces/{slug}/views/{pk}/" + + +def _make_user(prefix): + """A saved, active user with a unique email.""" + unique_id = uuid4().hex[:8] + user = User.objects.create( + email=f"{prefix}-{unique_id}@plane.so", + username=f"{prefix}_{unique_id}", + first_name=prefix.title(), + last_name="User", + ) + user.set_password("test-password") + user.save() + return user + + +def _client_for(user): + client = APIClient() + client.force_authenticate(user=user) + return client + + +@pytest.fixture +def project(db, workspace, create_user): + """A project with guest_view_all_features at its default of False.""" + project = Project.objects.create( + name="Scoped Project", + identifier="SP", + workspace=workspace, + created_by=create_user, + ) + ProjectMember.objects.create(project=project, member=create_user, workspace=workspace, role=20) + return project + + +@pytest.fixture +def guest(db, workspace, project): + """An active project GUEST (role=5).""" + user = _make_user("guest") + WorkspaceMember.objects.create(workspace=workspace, member=user, role=5) + ProjectMember.objects.create(project=project, member=user, workspace=workspace, role=5) + return user + + +@pytest.fixture +def guest_client(guest): + return _client_for(guest) + + +def _make_issue(name, project, workspace, author): + """Create an issue with a deterministic ``created_by``. + + ``BaseModel.save`` sets ``created_by`` from the current request user, which + is anonymous under tests, so a ``created_by=`` kwarg to ``create`` is + discarded. Passing ``created_by_id`` to ``save`` sets it explicitly. + """ + issue = Issue(name=name, project=project, workspace=workspace) + issue.save(created_by_id=author.id) + return issue + + +@pytest.fixture +def own_issue(db, workspace, project, guest): + return _make_issue("Guest's own work item", project, workspace, guest) + + +@pytest.fixture +def foreign_issue(db, workspace, project, create_user): + return _make_issue("Someone else's work item", project, workspace, create_user) + + +@pytest.mark.contract +class TestWorkItemByIdentifierGuestScope: + """A restricted guest must not resolve work items they did not author. + + Pins an existing guard rather than covering a new fix. This endpoint resolves + by sequence number, so losing the guard would let a guest walk + ``PROJ-1..PROJ-N`` and read every work item's full detail — including + ``description_html`` — plus the project and issue UUIDs that every other + endpoint keys on. + """ + + @pytest.mark.django_db + def test_guest_cannot_read_foreign_work_item(self, guest_client, workspace, project, foreign_issue): + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = guest_client.get(url) + + assert response.status_code == status.HTTP_403_FORBIDDEN, ( + f"Restricted guest resolved a foreign work item: {response.status_code} {getattr(response, 'data', None)!r}" + ) + + @pytest.mark.django_db + def test_guest_can_read_their_own_work_item(self, guest_client, workspace, project, own_issue): + """Positive control: the guard must not lock a guest out of their own item.""" + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=own_issue.sequence_id, + ) + response = guest_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert str(response.data["id"]) == str(own_issue.id) + + @pytest.mark.django_db + def test_full_member_can_read_any_work_item(self, session_client, workspace, project, foreign_issue): + """Positive control: a full member is unaffected.""" + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = session_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert str(response.data["id"]) == str(foreign_issue.id) + + @pytest.mark.django_db + def test_guest_with_view_all_features_can_read_any_work_item(self, guest_client, workspace, project, foreign_issue): + """Positive control: the guard is conditional on the project setting.""" + project.guest_view_all_features = True + project.save(update_fields=["guest_view_all_features"]) + + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = guest_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + + @pytest.mark.django_db + def test_non_member_is_refused(self, workspace, project, foreign_issue): + """A user with no project membership must still be refused outright.""" + outsider = _make_user("outsider") + WorkspaceMember.objects.create(workspace=workspace, member=outsider, role=15) + + url = WORK_ITEM_BY_IDENTIFIER_URL.format( + slug=workspace.slug, + project_identifier=project.identifier, + sequence_id=foreign_issue.sequence_id, + ) + response = _client_for(outsider).get(url) + + assert response.status_code == status.HTTP_403_FORBIDDEN, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + + +@pytest.fixture +def workspace_view(db, workspace, create_user): + """A global (project-less) workspace view owned by the workspace owner.""" + view = IssueView(name="Global view", workspace=workspace, owned_by=create_user, access=1) + view.save() + return view + + +@pytest.mark.contract +class TestWorkspaceViewRetrieveRequiresMembership: + """Reading a global view must require membership of its workspace.""" + + @pytest.mark.django_db + def test_outsider_cannot_read_a_global_view(self, workspace, workspace_view): + """A user in a different workspace entirely, holding only the view id.""" + outsider = _make_user("outsider") + other_workspace = Workspace.objects.create( + name="Outsider Workspace", + owner=outsider, + slug=f"outsider-{uuid4().hex[:8]}", + ) + WorkspaceMember.objects.create(workspace=other_workspace, member=outsider, role=20) + + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=workspace_view.id) + response = _client_for(outsider).get(url) + + assert response.status_code in ( + status.HTTP_403_FORBIDDEN, + status.HTTP_404_NOT_FOUND, + ), f"Non-member read a global view: {response.status_code} {getattr(response, 'data', None)!r}" + + @pytest.mark.django_db + def test_workspace_member_can_read_a_global_view(self, session_client, workspace, workspace_view): + """Positive control: a member of the workspace still reads it.""" + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=workspace_view.id) + response = session_client.get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + assert str(response.data["id"]) == str(workspace_view.id) + + @pytest.mark.django_db + def test_missing_view_is_a_404_not_an_empty_200(self, session_client, workspace): + """A member asking for a view id that does not exist must get 404. + + get_queryset() is scoped to the URL workspace, so this also covers a real + view id belonging to a different workspace. + """ + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=uuid4()) + response = session_client.get(url) + + assert response.status_code == status.HTTP_404_NOT_FOUND, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + ) + + @pytest.mark.django_db + def test_workspace_guest_can_read_a_global_view(self, workspace, workspace_view): + """Positive control: the role set matches list(), which permits guests.""" + guest_user = _make_user("wsguest") + WorkspaceMember.objects.create(workspace=workspace, member=guest_user, role=5) + + url = WORKSPACE_VIEW_DETAIL_URL.format(slug=workspace.slug, pk=workspace_view.id) + response = _client_for(guest_user).get(url) + + assert response.status_code == status.HTTP_200_OK, ( + f"Got {response.status_code}: {getattr(response, 'data', None)!r}" + )