mirror of
https://github.com/makeplane/plane.git
synced 2026-08-29 10:08:51 +02:00
fix(security): share the non-authorizing permission set with the route scan
Review caught a real divergence. The runtime guard treats
`{IsAuthenticated, AllowAny}` as non-authorizing, but the scan covering
plane.api and plane.space restated the rule as "not (IsAuthenticated,) and not
()". That silently accepted `[AllowAny]` as a deliberate restrictive
declaration, so a viewset there declaring AllowAny and falling through to a DRF
mixin would never appear in the manifest — on plane.space, the one surface where
AllowAny is routine (10+ classes use it today, all APIViews rather than
viewsets, so nothing is currently masked). The two tests also directly
contradicted each other: one asserts that shape is unauthorized while the other
skipped it.
The scan now imports `_NON_AUTHORIZING_PERMISSIONS` from the guard instead of
restating it, so the two cannot drift apart again. This is the same mistake the
guard itself had before review — "differs from the default" is not the same
question as "actually authorizes" — and restating a rule in a second place is
what let it survive in one of them.
Also: correct the comment on that set, which claimed both classes "establish
identity" when AllowAny does not; make initial()'s comment precise about what
ordering after super() actually buys (the permission classes' own rejection and
status code win, rather than a 405 disclosing that the route exists); and use
`pass` rather than a bare ellipsis for the test stub bodies.
Co-authored-by: Plane AI <noreply@plane.so>
This commit is contained in:
@@ -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():
|
||||
|
||||
@@ -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]
|
||||
|
||||
Reference in New Issue
Block a user