diff --git a/apps/api/plane/app/views/base.py b/apps/api/plane/app/views/base.py index 2a2ed0ccc7..f94943c858 100644 --- a/apps/api/plane/app/views/base.py +++ b/apps/api/plane/app/views/base.py @@ -52,12 +52,15 @@ class TimezoneMixin: # is authenticated but not authorized at all. _MIXIN_PROVIDED_ACTIONS = frozenset({"list", "retrieve", "create", "update", "partial_update", "destroy"}) -# Permission classes that establish identity but authorize nothing: they say -# who is calling, never what they may touch. A viewset carrying only these has +# Permission classes that authorize nothing: neither says anything about what +# the caller may touch. `IsAuthenticated` only establishes that there is a +# caller; `AllowAny` does not even do that. A viewset carrying only these has # delegated all of its authorization to per-method checks, so a mixin-served -# action has none. Tested for membership rather than comparing against the -# default, so that a weaker declaration than the default — `[AllowAny]`, or an -# empty list — is not mistaken for a deliberate, restrictive one. +# action has none. +# +# Membership-tested rather than compared against the default, so a declaration +# that is *weaker* than the default — `[AllowAny]`, or an empty list — is not +# mistaken for a deliberate restrictive one. _NON_AUTHORIZING_PERMISSIONS = frozenset({IsAuthenticated, AllowAny}) @@ -142,9 +145,11 @@ class BaseViewSet(TimezoneMixin, ReadReplicaControlMixin, ModelViewSet, BasePagi return owner is not None and not owner.__module__.startswith("rest_framework") def initial(self, request, *args, **kwargs): - # Runs after authentication and permission checks, so an anonymous - # caller still gets 401 rather than having the route's existence - # confirmed or denied first. + # Deliberately after super(), which runs authentication and the + # permission classes. Whatever they would have rejected is still + # rejected first and with their own status — an unauthenticated caller + # gets 401 from IsAuthenticated rather than learning from a 405 that the + # route exists. super().initial(request, *args, **kwargs) if not self._resolved_action_is_authorized(): diff --git a/apps/api/plane/tests/unit/views/test_routed_action_authorization.py b/apps/api/plane/tests/unit/views/test_routed_action_authorization.py index 417c90a204..c4ce6afc4b 100644 --- a/apps/api/plane/tests/unit/views/test_routed_action_authorization.py +++ b/apps/api/plane/tests/unit/views/test_routed_action_authorization.py @@ -212,7 +212,13 @@ def _other_surface_fall_throughs(): duplicated BaseViewSet that does not have it. """ from django.urls import get_resolver - from rest_framework.permissions import IsAuthenticated + + # Imported rather than restated. An earlier version of this helper listed + # only `(IsAuthenticated,)` and `()` as non-authorizing, which silently + # treated `[AllowAny]` as a deliberate restrictive declaration — on + # plane.space, the one surface where AllowAny is routine. Sharing the guard's + # own definition keeps the two from drifting apart again. + from plane.app.views.base import _NON_AUTHORIZING_PERMISSIONS found = set() @@ -244,7 +250,7 @@ def _other_surface_fall_throughs(): if action == "create" and owner(viewset, "perform_create") is not None: continue declared = tuple(getattr(viewset, "permission_classes", ()) or ()) - if declared not in ((IsAuthenticated,), ()): + if any(permission not in _NON_AUTHORIZING_PERMISSIONS for permission in declared): continue found.add((viewset.__module__, viewset.__name__, verb, action)) @@ -310,7 +316,8 @@ def test_guard_is_actually_wired_into_request_handling(): through on.""" def partial_update(self, request, *args, **kwargs): # pragma: no cover - ... + + pass class Guarded(BaseViewSet): def update(self, request, *args, **kwargs): @@ -346,15 +353,15 @@ def test_guard_recognises_the_patterns_it_must_not_reject(): class OwnImplementation(BaseViewSet): def partial_update(self, request): # pragma: no cover - never called - ... + pass class RidesCreateMixin(BaseViewSet): def perform_create(self, serializer): # pragma: no cover - never called - ... + pass class TransitivelyAuthorizesPatch(BaseViewSet): def update(self, request): # pragma: no cover - never called - ... + pass class HasRealPermissionClass(BaseViewSet): permission_classes = [object]