diff --git a/app/apps/common/functions/permissions.py b/app/apps/common/functions/permissions.py index 560c3d7..713c9f9 100644 --- a/app/apps/common/functions/permissions.py +++ b/app/apps/common/functions/permissions.py @@ -1,36 +1,58 @@ from django.core.exceptions import PermissionDenied +from django.http import Http404 from django.shortcuts import get_object_or_404 +READ = "read" +EDIT = "edit" -def get_owned_object_or_403(klass, request, *args, owner_path="owner", **kwargs): - """Fetch an object like ``get_object_or_404`` while enforcing ownership. - Returns the object when it has no owner, or when it is owned by - ``request.user``; otherwise raises :class:`~django.core.exceptions.PermissionDenied` - (HTTP 403). This mirrors the owner check used by ``transaction_rule_edit`` - (``if obj.owner and obj.owner != request.user``) so authorization is applied - uniformly across handlers that resolve an object from a URL id. +def get_shared_object_or_error(klass, request, *, level=EDIT, via=None, **kwargs): + """Fetch an object like ``get_object_or_404`` while enforcing access control. - An object with no owner stays accessible to everyone, preserving the - existing behaviour for legacy/unowned objects. + ``SharedObjectManager`` scopes querysets to what a user may *see*, which is + not the same as what they may *change*. Views that resolve an object from a + URL id must state which of the two they need, otherwise a shared or public + object becomes writable by anyone who can see it. - ``owner_path`` is a dotted attribute path to the owning user, so nested - ownership is supported for objects owned through a relation, e.g. a rule - action owned via its parent rule:: + ``level`` selects the check applied to the governing ``SharedObject``: - get_owned_object_or_403( - TransactionRuleAction, request, id=pk, owner_path="rule.owner" + ``READ`` + The object must be visible to the user. Denial raises :class:`Http404` + so the response does not confirm that the id exists. + ``EDIT`` + The object must be owned by the user. Denial raises + :class:`~django.core.exceptions.PermissionDenied` (HTTP 403), which the + frontend surfaces as an "Access Denied" dialog. An object the user + cannot even see raises :class:`Http404` instead, so 403 never confirms + the existence of an object they were not allowed to know about. + + Objects with no owner stay accessible to everyone, preserving the existing + behaviour for legacy/unowned objects. + + ``via`` is a dotted path to the ``SharedObject`` that governs access, for + models owned through a relation, e.g. a rule action governed by its parent + rule:: + + get_shared_object_or_error( + TransactionRuleAction, request, id=pk, level=EDIT, via="rule" ) + + The path is resolved with a plain ``getattr``, so a path that does not + resolve raises ``AttributeError`` rather than silently granting access. """ - obj = get_object_or_404(klass, *args, **kwargs) + obj = get_object_or_404(klass, **kwargs) - owner = obj - for attr in owner_path.split("."): - owner = getattr(owner, attr, None) - if owner is None: - break + guard = obj + for attr in via.split(".") if via else []: + guard = getattr(guard, attr) - if owner is not None and owner != request.user: + if level not in (READ, EDIT): + raise ValueError(f"Unknown access level: {level!r}") + + if not guard.is_visible_to(request.user): + raise Http404 + + if level == EDIT and not guard.is_editable_by(request.user): raise PermissionDenied return obj diff --git a/app/apps/common/models.py b/app/apps/common/models.py index 10f79c8..f9a876f 100644 --- a/app/apps/common/models.py +++ b/app/apps/common/models.py @@ -58,13 +58,35 @@ class SharedObject(models.Model): models.Index(fields=["visibility"]), ] - def is_accessible_by(self, user): - """Check if a user can access this object""" - return ( - self.visibility == "public" - or self.owner == user - or (self.visibility == "shared" and user in self.shared_with.all()) - ) + # NOTE: these two predicates must stay in sync with the ``Q`` objects built + # by ``SharedObjectManager.get_queryset`` above. The manager filters at the + # queryset level and these check a single instance, so they cannot share an + # implementation; ``SharedObjectPredicateParityTests`` asserts they agree. + def is_visible_to(self, user): + """Whether ``user`` may read this object. + + Mirrors ``SharedObjectManager``: public objects, objects with no owner, + the owner's own objects, and objects explicitly shared with the user. + """ + if self.owner is None or self.visibility == "public": + return True + + if not user or not user.is_authenticated: + return False + + return self.owner_id == user.pk or self.shared_with.filter(pk=user.pk).exists() + + def is_editable_by(self, user): + """Whether ``user`` may mutate this object. + + Sharing grants read access only; mutation stays with the owner. Objects + with no owner remain editable by everyone, preserving the behaviour of + legacy/unowned objects. + """ + if self.owner is None: + return True + + return bool(user and user.is_authenticated and self.owner_id == user.pk) def save(self, *args, **kwargs): if not self.pk and not self.owner: diff --git a/app/apps/rules/views.py b/app/apps/rules/views.py index 72e7b97..5054c02 100644 --- a/app/apps/rules/views.py +++ b/app/apps/rules/views.py @@ -4,14 +4,19 @@ from copy import deepcopy from django.contrib import messages from django.contrib.auth.decorators import login_required +from django.core.exceptions import PermissionDenied from django.db import transaction from django.http import HttpResponse -from django.shortcuts import render, get_object_or_404, redirect -from apps.common.functions.permissions import get_owned_object_or_403 +from django.shortcuts import render, redirect from django.utils.translation import gettext_lazy as _ from django.views.decorators.http import require_http_methods from apps.common.decorators.htmx import only_htmx +from apps.common.functions.permissions import ( + EDIT, + READ, + get_shared_object_or_error, +) from apps.rules.forms import ( TransactionRuleForm, TransactionRuleActionForm, @@ -63,7 +68,9 @@ def rules_list(request): @disabled_on_demo @require_http_methods(["GET", "POST"]) def transaction_rule_toggle_activity(request, transaction_rule_id, **kwargs): - transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id) + transaction_rule = get_shared_object_or_error( + TransactionRule, request, id=transaction_rule_id, level=EDIT + ) current_active = transaction_rule.active transaction_rule.active = not current_active transaction_rule.save(update_fields=["active"]) @@ -113,17 +120,9 @@ def transaction_rule_add(request, **kwargs): @disabled_on_demo @require_http_methods(["GET", "POST"]) def transaction_rule_edit(request, transaction_rule_id): - transaction_rule = get_object_or_404(TransactionRule, id=transaction_rule_id) - - if transaction_rule.owner and transaction_rule.owner != request.user: - messages.error(request, _("Only the owner can edit this")) - - return HttpResponse( - status=204, - headers={ - "HX-Trigger": "updated, hide_offcanvas", - }, - ) + transaction_rule = get_shared_object_or_error( + TransactionRule, request, id=transaction_rule_id, level=EDIT + ) if request.method == "POST": form = TransactionRuleForm(request.POST, instance=transaction_rule) @@ -152,7 +151,9 @@ def transaction_rule_edit(request, transaction_rule_id): @disabled_on_demo @require_http_methods(["GET", "POST"]) def transaction_rule_view(request, transaction_rule_id): - transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id) + transaction_rule = get_shared_object_or_error( + TransactionRule, request, id=transaction_rule_id, level=READ + ) edit_actions = transaction_rule.transaction_actions.all() update_or_create_actions = ( @@ -176,17 +177,20 @@ def transaction_rule_view(request, transaction_rule_id): @disabled_on_demo @require_http_methods(["DELETE"]) def transaction_rule_delete(request, transaction_rule_id): - transaction_rule = get_object_or_404(TransactionRule, id=transaction_rule_id) + transaction_rule = get_shared_object_or_error( + TransactionRule, request, id=transaction_rule_id, level=READ + ) - if ( - transaction_rule.owner != request.user - and request.user in transaction_rule.shared_with.all() - ): + if transaction_rule.is_editable_by(request.user): + transaction_rule.delete() + messages.success(request, _("Rule deleted successfully")) + elif transaction_rule.shared_with.filter(pk=request.user.pk).exists(): + # Someone else's rule shared with us: we can drop our own access to it, + # but never delete it. transaction_rule.shared_with.remove(request.user) messages.success(request, _("Item no longer shared with you")) else: - transaction_rule.delete() - messages.success(request, _("Rule deleted successfully")) + raise PermissionDenied return HttpResponse( status=204, @@ -201,7 +205,9 @@ def transaction_rule_delete(request, transaction_rule_id): @disabled_on_demo @require_http_methods(["GET"]) def transaction_rule_take_ownership(request, transaction_rule_id): - transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id) + transaction_rule = get_shared_object_or_error( + TransactionRule, request, id=transaction_rule_id, level=EDIT + ) if not transaction_rule.owner: transaction_rule.owner = request.user @@ -223,17 +229,7 @@ def transaction_rule_take_ownership(request, transaction_rule_id): @disabled_on_demo @require_http_methods(["GET", "POST"]) def transaction_rule_share(request, pk): - obj = get_object_or_404(TransactionRule, id=pk) - - if obj.owner and obj.owner != request.user: - messages.error(request, _("Only the owner can edit this")) - - return HttpResponse( - status=204, - headers={ - "HX-Trigger": "updated, hide_offcanvas", - }, - ) + obj = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT) if request.method == "POST": form = SharedObjectForm(request.POST, instance=obj, user=request.user) @@ -262,7 +258,9 @@ def transaction_rule_share(request, pk): @disabled_on_demo @require_http_methods(["GET", "POST"]) def transaction_rule_action_add(request, transaction_rule_id): - transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id) + transaction_rule = get_shared_object_or_error( + TransactionRule, request, id=transaction_rule_id, level=EDIT + ) if request.method == "POST": form = TransactionRuleActionForm(request.POST, rule=transaction_rule) @@ -290,12 +288,14 @@ def transaction_rule_action_add(request, transaction_rule_id): @disabled_on_demo @require_http_methods(["GET", "POST"]) def transaction_rule_action_edit(request, transaction_rule_action_id): - transaction_rule_action = get_owned_object_or_403( - TransactionRuleAction, request, id=transaction_rule_action_id, owner_path="rule.owner" - ) - transaction_rule = get_object_or_404( - TransactionRule, id=transaction_rule_action.rule.id + transaction_rule_action = get_shared_object_or_error( + TransactionRuleAction, + request, + id=transaction_rule_action_id, + level=EDIT, + via="rule", ) + transaction_rule = transaction_rule_action.rule if request.method == "POST": form = TransactionRuleActionForm( @@ -328,8 +328,12 @@ def transaction_rule_action_edit(request, transaction_rule_action_id): @disabled_on_demo @require_http_methods(["DELETE"]) def transaction_rule_action_delete(request, transaction_rule_action_id): - transaction_rule_action = get_owned_object_or_403( - TransactionRuleAction, request, id=transaction_rule_action_id, owner_path="rule.owner" + transaction_rule_action = get_shared_object_or_error( + TransactionRuleAction, + request, + id=transaction_rule_action_id, + level=EDIT, + via="rule", ) transaction_rule_action.delete() @@ -349,7 +353,9 @@ def transaction_rule_action_delete(request, transaction_rule_action_id): @disabled_on_demo @require_http_methods(["GET", "POST"]) def update_or_create_transaction_rule_action_add(request, transaction_rule_id): - transaction_rule = get_owned_object_or_403(TransactionRule, request, id=transaction_rule_id) + transaction_rule = get_shared_object_or_error( + TransactionRule, request, id=transaction_rule_id, level=EDIT + ) if request.method == "POST": form = UpdateOrCreateTransactionRuleActionForm( @@ -381,8 +387,8 @@ def update_or_create_transaction_rule_action_add(request, transaction_rule_id): @disabled_on_demo @require_http_methods(["GET", "POST"]) def update_or_create_transaction_rule_action_edit(request, pk): - linked_action = get_owned_object_or_403( - UpdateOrCreateTransactionRuleAction, request, id=pk, owner_path="rule.owner" + linked_action = get_shared_object_or_error( + UpdateOrCreateTransactionRuleAction, request, id=pk, level=EDIT, via="rule" ) transaction_rule = linked_action.rule @@ -418,8 +424,8 @@ def update_or_create_transaction_rule_action_edit(request, pk): @disabled_on_demo @require_http_methods(["DELETE"]) def update_or_create_transaction_rule_action_delete(request, pk): - linked_action = get_owned_object_or_403( - UpdateOrCreateTransactionRuleAction, request, id=pk, owner_path="rule.owner" + linked_action = get_shared_object_or_error( + UpdateOrCreateTransactionRuleAction, request, id=pk, level=EDIT, via="rule" ) linked_action.delete() @@ -441,7 +447,7 @@ def update_or_create_transaction_rule_action_delete(request, pk): @disabled_on_demo @require_http_methods(["GET", "POST"]) def dry_run_rule_created(request, pk): - rule = get_owned_object_or_403(TransactionRule, request, id=pk) + rule = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT) logs = None results = None @@ -486,7 +492,7 @@ def dry_run_rule_created(request, pk): @disabled_on_demo @require_http_methods(["GET", "POST"]) def dry_run_rule_deleted(request, pk): - rule = get_owned_object_or_403(TransactionRule, request, id=pk) + rule = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT) logs = None results = None @@ -531,7 +537,7 @@ def dry_run_rule_deleted(request, pk): @disabled_on_demo @require_http_methods(["GET", "POST"]) def dry_run_rule_updated(request, pk): - rule = get_owned_object_or_403(TransactionRule, request, id=pk) + rule = get_shared_object_or_error(TransactionRule, request, id=pk, level=EDIT) logs = None results = None diff --git a/app/templates/rules/fragments/list.html b/app/templates/rules/fragments/list.html index 07cb0f7..998708e 100644 --- a/app/templates/rules/fragments/list.html +++ b/app/templates/rules/fragments/list.html @@ -64,14 +64,21 @@ - - {% if rule.active %}{% else %} - {% endif %} - + {% if not rule.owner or user == rule.owner %} + + {% if rule.active %}{% else %} + {% endif %} + + {% else %} + + {% if rule.active %}{% else %} + {% endif %} + + {% endif %}
{{ rule.order }}