V0.5.0

CHANGELOG.MD

@@ -9,7 +9,45 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 N/A
 
 
-## [0.4.2] - 2025-07-18
+## [0.5.0] - 2025-11-07 **Critical**
+### Fixed
+- Fixed a case where `CombinedConditions` would cause false positives. **Critical**
+- Fixed the `assign_perm` helper ignoring for_resource and on_account params. **Critical**
+- Handle ObjectDoesNotExist in HasRelatedResourcePerms.
+- Fixed a case where restrict_to_perms is used in TransitiveFromRelationPerms.
+- Handle ObjectDoesNotExist in HasRootMembership.
+- Improved the experimental solver.get_actors_q and mention caveat.
+- Improved DRF View queryset attribute detection.
+
+### Changed
+- For `AssignedPerms`, temporarily require either `resource` or the `non_model_resource_id` field to be specified. **Critical**
+  - This is to avoid false positives caused by `AssignedPerms` with no object_id specified. This limitation will be removed in a future release.
+  - Workaround: For model-wide `AssignedPerms` (`object_id=None`), consider assigning the perm to the resource's owner UserModel instead, and use `IndirectPerms` to indirectly give the perm to the matching resources.
+  ```python
+  # Example
+  class SomeResourceAuthorizationScheme:
+      def get_indirect_perms(self, context: Context) -> list[IndirectPerms]:
+          return [
+              ConditionalPerms(
+                  conditions=[
+                      HasRelatedResourcePerms(
+                          relation=self.owner_relation,
+                          perms=[self.Roles.MEMBERSHIP_MANAGER],
+                      ),
+                  ],
+                  receives_perms=[self.Roles.MEMBERSHIP_MANAGER],
+              )
+          ]
+  ```
+- When creating an AssignedPerm (even via the assign_perm helper) if a resource is given, the owner must be manually specified. **Breaking Change** 
+  - This limitation is in place to prevent an authorization bypass exploit. It will be removed when a proper fix will be published in a future release.
+
+### Added
+- Added an experimental Unauthorized condition.
+- Added some small DX improvements.
+
+
+## [0.4.2] - 2025-07-18 **Critical**
 ### Fixed
 - Fixed `AuthorizationSolver.get_actors_q` not properly enforcing auth in most cases. This was caused by not separating the used contexts from the root context, which caused the wrong AssignedPerms/Memberships to propagate to third level and deeper subcontexts. A better way to handle this may be implemented in the future. **Critical**
 - Fixed a potential AttributeError in merge_qs util.
@@ -33,13 +71,13 @@ N/A
 With this release, a few more shortcomings and limitations have been knocked out.
 
 ## Added
-  - Added an `AuthorizationSolver.get_perms` method for fetching all the permissions an actor has on resource(s).
-  - Added a DRF field called `PermissionsField` which will output all the permissions the user has on the serialized resource.
-    - If it's important, do your own benchmarking, but the performance penalty I encountered is a base of ~20ms, and then about 2ms for each resource in the listing:
-      - ~160ms for the original request, with 20 resources listed;
-      - ~200ms with 30 perms to check for each of the 20 resources (with `PermissionField`); this might be a best-case scenario though...
-  - Added an experimental and subject to change `Context.knowledge_base` that basically retains information in the form of (Actor, Condition, Resource, Truth). It is mainly used for optimization purposes, especially for calling `AuthorizationSolver.get_perms`.
-  - Added an `AuthorizationScheme.get_perms_pseudo_hierarchy` method to roughly determine the hierarchy of perms/roles (higher value means less important). It is mainly used for optimization purposes, especially for calling `AuthorizationSolver.get_perms`.
+- Added an `AuthorizationSolver.get_perms` method for fetching all the permissions an actor has on resource(s).
+- Added a DRF field called `PermissionsField` which will output all the permissions the user has on the serialized resource.
+  - If it's important, do your own benchmarking, but the performance penalty I encountered is a base of ~20ms, and then about 2ms for each resource in the listing:
+    - ~160ms for the original request, with 20 resources listed;
+    - ~200ms with 30 perms to check for each of the 20 resources (with `PermissionField`); this might be a best-case scenario though...
+- Added an experimental and subject to change `Context.knowledge_base` that basically retains information in the form of (Actor, Condition, Resource, Truth). It is mainly used for optimization purposes, especially for calling `AuthorizationSolver.get_perms`.
+- Added an `AuthorizationScheme.get_perms_pseudo_hierarchy` method to roughly determine the hierarchy of perms/roles (higher value means less important). It is mainly used for optimization purposes, especially for calling `AuthorizationSolver.get_perms`.
 
 ### Changed
 - Simplified solver/scheme method names:

README.md

@@ -1,5 +1,5 @@
 # django-woah
-A package intended to aid developers in implementing authorization for Django apps.  
+A useful package for implementing authorization in your Django apps.
 
 *This project is developed at [Presslabs](https://www.presslabs.com/).*
 

django_woah/authorization/conditions.py

@@ -101,30 +101,27 @@ def __init__(self, *conditions: Condition, operation: OPERATIONS, **kwargs):
         self.conditions = conditions
         self.operation = operation
 
+        if self.operation == self.OPERATIONS.AND:
+            self._q_connector = Q.AND
+        elif self.operation == self.OPERATIONS.OR:
+            self._q_connector = Q.OR
+        else:
+            raise ValueError("Unexpected Condition operation")
+
         super().__init__(**kwargs)
 
     @property
     def _identity(self) -> tuple:
         return super()._identity + (self.operation, *(sorted(condition._identity for condition in self.conditions)))
 
     def get_resources_q(self, context: Context) -> Optional[Q]:
-        result = Q()
-
-        for condition in self.conditions:
-            q = condition.get_resources_q(context)
-
-            if self.operation == self.OPERATIONS.AND:
-                if q is None:
-                    return None
-
-                result &= q
-            elif self.operation == self.OPERATIONS.OR:
-                if q is not None:
-                    result |= q
-            else:
-                raise ValueError("Unexpected Condition operation")
+        if not self.conditions:
+            raise ValueError("Found CombinedCondition with no conditions")
 
-        return result
+        return merge_qs(
+            [condition.get_resources_q(context) for condition in self.conditions],
+            connector=self._q_connector,
+        )
 
     def get_assigned_perms_q(self, context: Context) -> Optional[Q]:
         qs = [
@@ -158,7 +155,7 @@ def get_memberships_q(self, context: Context) -> Optional[Q]:
 
     def verify_authorization(self, context: Context) -> bool:
         if not self.conditions:
-            return True
+            raise ValueError("Found CombinedCondition with no conditions")
 
         for condition in self.conditions:
             ok = condition.verify_authorization(context)
@@ -223,9 +220,12 @@ def __init__(self, actor, is_outside_collaborator=None, **kwargs):
     def _identity(self) -> tuple:
         return super()._identity + (self.is_outside_collaborator,)
 
-    def get_memberships_q(self, context: Context) -> Q:
+    def get_memberships_q(self, context: Context) -> Optional[Q]:
         if isinstance(context.resource, Model):
-            owner = get_object_relation(context.resource, self.relation)
+            try:
+                owner = get_object_relation(context.resource, self.relation)
+            except ObjectDoesNotExist:
+                return None
 
             q = Q(user_group=owner) if self.relation_is_user_group else Q(user_group__owner=owner)
         else:
@@ -452,7 +452,10 @@ def get_assigned_perms_q(self, context: Context) -> Optional[Q]:
     def verify_authorization(self, context: Context) -> bool:
         relation = self.relation if context.resource.pk else self.unsaved_object_relation
 
-        resource = get_object_relation(context.resource, relation)
+        try:
+            resource = get_object_relation(context.resource, relation)
+        except ObjectDoesNotExist:
+            return False
 
         if not resource:
             return False
@@ -575,6 +578,17 @@ def __repr__(self):
         return self.q.__repr__()
 
 
+class Unauthorized(Condition):
+    def _get_atoms(self, context):
+        return Atom((context.actor, context.perm, _resource_to_atom(context.resource)) + self._identity)
+
+    def get_resources_q(self, _: Context) -> None:
+        return None
+
+    def __repr__(self):
+        return Unauthorized
+
+
 def _class_fq(klass):
     return f"{klass.__module__}.{klass.__qualname__}"
 

django_woah/authorization/indirect_perms.py

@@ -107,7 +107,7 @@ def set_scheme(self, scheme):
         if not self.restrict_to_perms:
             self.restrict_to_perms = set(self.scheme.get_scheme_perms())
         else:
-            self.restrict_to_perms = self.restrict_to_perms.intersection(self.scheme.get_scheme_perms())
+            self.restrict_to_perms = self.restrict_to_perms.intersection(set(self.scheme.get_scheme_perms()))
 
         self.relation_scheme = self.scheme.get_auth_scheme_by_relation(self.relation)
 

django_woah/authorization/knowledge_base.py

@@ -25,6 +25,8 @@ class TruthBearer(Generic[T]):
     # Class is subject to change
 
     def __init__(self, *args: T, operator=OP.AND, truth=True):
+        # TODO: Maybe rename self.truth to self.boolean or something similar, since Unauthorized always returning False
+        #       sounds like a double negation and may lead to confusion.
         self.truth = truth
         self.operator = operator
         self.identity: tuple[T, ...] = tuple(args)

django_woah/authorization/solver.py

@@ -264,33 +264,18 @@ def get_model(self, resources) -> type[Model]:
 
         return model
 
-    def get_actors_q(self, context: Context) -> Optional[Q]:
-        if context.actor is not None:
-            raise ValueError("Must not specify context actor")
-
-        if not context.resource:
-            raise ValueError("Must specify context resource")
-
-        if context.assigned_perms is None:
-            raise ValueError("Must specify context assigned_perms")
-
-        if context.memberships is None:
-            raise ValueError("Must specify context memberships")
-
-        model = self.get_model(context.resource)
-        scheme = self.get_auth_scheme_for_model(model)
-
+    def _split_assigned_perms_and_memberships_by_actor_id(self, assigned_perms, memberships):
         assigned_perms_by_user_group_id = {}
 
-        for assigned_perm in context.assigned_perms:
+        for assigned_perm in assigned_perms:
             if assigned_perm.user_group_id not in assigned_perms_by_user_group_id:
                 assigned_perms_by_user_group_id[assigned_perm.user_group_id] = [assigned_perm]
             else:
                 assigned_perms_by_user_group_id[assigned_perm.user_group_id].append(assigned_perm)
 
         actors_context_data = {}
 
-        for membership in context.memberships:
+        for membership in memberships:
             actor_id = membership.user_id
 
             if not actors_context_data.get(actor_id):
@@ -306,10 +291,75 @@ def get_actors_q(self, context: Context) -> Optional[Q]:
                     assigned_perms_by_user_group_id.get(membership.user_group_id, [])
                 )
 
+        return actors_context_data
+
+    def get_actors_q(self, context: Context, restrict_to_actors: Optional[list | QuerySet] = None) -> Optional[Q]:
+        """
+        !!! WARNING !!!
+        Although this method will never return false positives, in some cases it's results will include FALSE NEGATIVES,
+        meaning actors which would pass verify_authorization(context) might not be included in the result.
+
+        Here are the known cases for which this method will return false negatives (there might be unknown cases too):
+        - Usage of conditions that access context.actor to form the .get_resources_q() result. Examples:
+            - QCondition(some_field=context.actor)
+            - Custom conditions which use context.actor to form get_resources_q, but don't fetch assigned perms or
+              memberships that match the context.actor:
+                  def get_resources_q(context): return Q(**{self.account_owner_relation: context.actor})
+
+        Therefore, it is best not to verify authorization based on it (use verify_authorization for that), but rather
+        use it for some displaying purposes where false negatives might not be a huge issue.
+
+        However, there is a workaround available:
+            1. Using the `restrict_to_actors` parameter to verify against a prefetched set of Actors (the UserModel must be
+                have the fields used in authorization prefetched, meaning for common cases the the ID is usually enough,
+                unless you are verifying against emails, usernames, countries etc...).
+
+                Do note that you may pass the entire set of potential models, meaning <UserModel>.objects.all(), but this
+                will be horribly slow, so you'll avoid this for most use cases.
+
+            <2. See the TODO below>
+        """
+
+        # TODO: Implement a different solution to avoid false negatives:
+        #       1. Add get_potential_actors_q for conditions and implement where needed. This should highly reduce
+        #          the false negatives to extreme cases, maybe no case if the method will always be implemented when
+        #          required.
+        #          The default implementation could attempt to check if the known cases match (see if context.actor was
+        #          used) and raise a NotImplementedError
+
+        if context.actor is not None:
+            raise ValueError("Must not specify context actor")
+
+        if not context.resource:
+            raise ValueError("Must specify context resource")
+
+        if context.assigned_perms is None:
+            raise ValueError("Must specify context assigned_perms")
+
+        if context.memberships is None:
+            raise ValueError("Must specify context memberships")
+
+        model = self.get_model(context.resource)
+        scheme = self.get_auth_scheme_for_model(model)
+
+        actors_context_data = self._split_assigned_perms_and_memberships_by_actor_id(
+            assigned_perms=context.assigned_perms,
+            memberships=context.memberships,
+        )
+        if restrict_to_actors is None:
+            actor_ids = actors_context_data.keys()
+        else:
+            actor_ids = [actor.id for actor in restrict_to_actors]
+
         actor_class = get_user_model()
         authorized_actors_ids = []
 
-        for actor_id, context_data in actors_context_data.items():
+        for actor_id in actor_ids:
+            context_data = actors_context_data.get(actor_id, {
+                "assigned_perms": [],
+                "memberships": []
+            })
+
             subcontext = context.subcontext()
             subcontext.actor = actor_class(id=actor_id)
             subcontext.assigned_perms = context_data["assigned_perms"]
@@ -328,10 +378,10 @@ def get_actors_q(self, context: Context) -> Optional[Q]:
 
         return Q(pk__in=authorized_actors_ids)
 
-    def get_actors_queryset(self, context: Context):
+    def get_actors_queryset(self, context: Context, restrict_to_actors: Optional[list | QuerySet] = None) -> QuerySet:
         actor_class = get_user_model()
 
-        q = self.get_actors_q(context)
+        q = self.get_actors_q(context, restrict_to_actors)
         if q is None:
             return actor_class.objects.none()
 
@@ -358,7 +408,7 @@ def get_resources_queryset(
         context: Optional[Context | CombinedContext] = None,
         base_queryset=None,
         **kwargs,
-    ):
+    ) -> QuerySet:
         if not context:
             context = self.get_context(**kwargs)
 

django_woah/drf/views.py

@@ -78,7 +78,7 @@ def get_cache_key(self, key, *args, **kwargs):
 
     @property
     def model(self) -> type[Model]:
-        if queryset := getattr(self, "queryset", None):
+        if (queryset := getattr(self, "queryset", None)) is not None:
             return queryset.model
 
         if serializer_class := self.get_serializer_class():
@@ -385,7 +385,6 @@ def get_base_context_for_get_perms(self):
             actor=root_context.contexts[0].actor, perm=None, resource=resource
         )
         context.assigned_perms = context_for_memberships.assigned_perms
-
         context.memberships = list(
             [
                 m
@@ -420,6 +419,10 @@ def get_authorization_model_object(
 
         lookup_url_kwarg = self.get_authorized_model_lookup_url_kwarg()
         if lookup_url_kwarg and self.kwargs.get(lookup_url_kwarg) is None:
+            print(
+                f"No kwarg found for given lookup_url_kwarg `{lookup_url_kwarg}`.\n"
+                f"Check if URL parameter name matches the view lookup_url_kwarg."
+            )
             return None
 
         queryset = self.authorization_model.objects.filter(
@@ -475,7 +478,11 @@ def get_unsaved_resource(self, initial_obj=None) -> Model:
         For example:
         - when reverse relations should be part of the validation, because those are not handled here.
             - using the `unsaved_object_relation` field, in the AuthorizationScheme, might help in some of these cases
-        - when certain fields fail validation (even if that case should result in a validation error eventually).
+        - when certain fields fail the serializer validation
+            - usually these cases are allowed to pass authorization so that they may fail the validation process down
+              the line and return proper validation errors instead of ambiguously failing authorization; still some
+              quirky code may possibly cause the validation not to fail... which would be considered a logic error in
+              the validation process, but still is worth mentioning here.
         """
 
         raise_exception = getattr(settings, validation_error_setting, True)