diff --git a/CHANGELOG.MD b/CHANGELOG.MD index 3b72825..306f121 100644 --- a/CHANGELOG.MD +++ b/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: diff --git a/README.md b/README.md index 5ca2408..0597f88 100644 --- a/README.md +++ b/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/).* diff --git a/django_woah/authorization/conditions.py b/django_woah/authorization/conditions.py index 402d65b..7a87712 100644 --- a/django_woah/authorization/conditions.py +++ b/django_woah/authorization/conditions.py @@ -101,6 +101,13 @@ 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 @@ -108,23 +115,13 @@ 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__}" diff --git a/django_woah/authorization/indirect_perms.py b/django_woah/authorization/indirect_perms.py index e4e8807..b964d0b 100644 --- a/django_woah/authorization/indirect_perms.py +++ b/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) diff --git a/django_woah/authorization/knowledge_base.py b/django_woah/authorization/knowledge_base.py index d6ed7d8..30fc5f8 100644 --- a/django_woah/authorization/knowledge_base.py +++ b/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) diff --git a/django_woah/authorization/solver.py b/django_woah/authorization/solver.py index c9e377f..0c165e5 100644 --- a/django_woah/authorization/solver.py +++ b/django_woah/authorization/solver.py @@ -264,25 +264,10 @@ 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: @@ -290,7 +275,7 @@ def get_actors_q(self, context: Context) -> Optional[Q]: 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 .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) diff --git a/django_woah/drf/views.py b/django_woah/drf/views.py index f45c0c3..5d2d93f 100644 --- a/django_woah/drf/views.py +++ b/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) diff --git a/django_woah/models.py b/django_woah/models.py index c9acc4f..e8cdc28 100644 --- a/django_woah/models.py +++ b/django_woah/models.py @@ -182,7 +182,7 @@ class Meta: def __str__(self): target = "*" if not self.object_id else f"{self.content_type, self.object_id}" - return f"<{self.perm}>, target:<{target}>, for:<{self.user_group}>" + return f"<{self.perm}>, resource:<{target}>, user_group:<{self.user_group}>" def full_clean(self, *args, **kwargs): resource_data = [self.content_type, self.object_id] @@ -196,10 +196,29 @@ def full_clean(self, *args, **kwargs): "Both a resource and a non_model_resource_id may not be specified." ) + if ( + not self.resource + and not self.non_model_resource_id + and not settings.WOAH_DANGEROUS_ALLOW_UNSPECIFIED_RESOURCE_PERMS + ): + print( + "For model-wide AssignedPerms (object_id=None), consider assigning the perm to the" + "resource's owner model instead, and use IndirectPerms to indirectly give the perm on the" + "matching resources.\n" + "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.\n" + "!!! Do not mess with the `WOAH_DANGEROUS_ALLOW_UNSPECIFIED_RESOURCE_PERMS` setting, it only " + "temporarily exists to facilitate easier testing !!!" + ) + raise ValidationError( + "Either a resource or a non model resource id must be specified." + ) + try: self.owner except AssignedPerm.owner.RelatedObjectDoesNotExist: - self.owner = self.user_group.owner + if not self.resource: + self.owner = self.user_group.owner return super().full_clean(*args, **kwargs) @@ -251,6 +270,9 @@ def clean(self): self.root_user_group = root_user_group + def __str__(self): + return f"user:<{self.user}>, user_group:<{self.user_group}>, is_outside_collaborator:<{self.is_outside_collaborator}>" + def get_root_user_group(owner) -> UserGroup: return UserGroup.objects.get( @@ -361,11 +383,9 @@ def assign_perm(perm, to_user, on_account, for_resource=None) -> AssignedPerm: kwargs = { "user_group": user_group, "perm": perm, + "owner": on_account, } if for_resource: kwargs["resource"] = for_resource - return AssignedPerm.objects.create( - user_group=get_single_user_user_group(related_to_user=to_user, owned_by_account=on_account), - perm=perm, - ) + return AssignedPerm.objects.create(**kwargs) diff --git a/examples/issue_tracker/base_app/test_authorizations.py b/examples/issue_tracker/base_app/test_authorizations.py index f608d0e..db321c7 100644 --- a/examples/issue_tracker/base_app/test_authorizations.py +++ b/examples/issue_tracker/base_app/test_authorizations.py @@ -136,6 +136,8 @@ def test_add_assigned_perm_when_assigned_perm_is_given_on_user_group( "resource": f"http://testserver/api/user_groups/{root_org_user_group.id}", } + assert created_assigned_perm.owner == organization + # def test_add_assigned_perm_as_root_account_owner( # api_client, account, organization, unrelated_account diff --git a/examples/issue_tracker/issue_tracker/settings.py b/examples/issue_tracker/issue_tracker/settings.py index 1c75d42..9497f01 100644 --- a/examples/issue_tracker/issue_tracker/settings.py +++ b/examples/issue_tracker/issue_tracker/settings.py @@ -9,6 +9,7 @@ For the full list of settings and their values, see https://docs.djangoproject.com/en/4.2/ref/settings/ """ +import sys import os @@ -154,3 +155,6 @@ # "DEFAULT_FILTER_BACKENDS": ("django_filters.rest_framework.DjangoFilterBackend",), "TEST_REQUEST_DEFAULT_FORMAT": "json", } + + +WOAH_DANGEROUS_ALLOW_UNSPECIFIED_RESOURCE_PERMS = True # Don't use this in production!!! This is to facilitate the testing of the example app. diff --git a/pyproject.toml b/pyproject.toml index 93ae976..96c21dd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,7 +1,7 @@ [tool.poetry] name = "django-woah" -version = "0.4.2" -description = "A package intended to aid developers in implementing authorization for Django apps." +version = "0.5.0" +description = "A useful package for implementing authorization in your Django apps." authors = ["Bogdan Petrea "] maintainers = ["Bogdan Petrea "] readme = "README.md"