From f68aacdea2f3b7dc9cb7f9368412e551cd4e2806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Buszkiewicz?= Date: Fri, 19 Dec 2025 16:08:21 +0100 Subject: [PATCH] fix: predicate functions should respect action grouping (#50) --- lib/permit.ex | 31 +++- lib/permit/context.ex | 5 +- test/permit/actions_grouping_test.exs | 217 ++++++++++++++++++++++++++ test/permit/permit_test.exs | 8 +- 4 files changed, 251 insertions(+), 10 deletions(-) create mode 100644 test/permit/actions_grouping_test.exs diff --git a/lib/permit.ex b/lib/permit.ex index 9f0b7df..869e334 100644 --- a/lib/permit.ex +++ b/lib/permit.ex @@ -144,7 +144,7 @@ defmodule Permit do do: raise("Unable to create permit authorization for nil role/user") def can(who) do - Permit.can(who, unquote(permissions_module)) + Permit.can(who, unquote(permissions_module), __MODULE__) end @impl Permit @@ -177,11 +177,22 @@ defmodule Permit do @doc false def can(who, permissions_module) do + can(who, permissions_module, nil) + end + + @doc false + def can(who, permissions_module, authorization_module) do who |> SubjectMapping.subjects() |> Stream.map(&permissions_module.can/1) |> Enum.reduce(&Permissions.concatenate(&1, &2)) - |> then(&%Permit.Context{subject: (is_struct(who) && who) || nil, permissions: &1}) + |> then( + &%Permit.Context{ + subject: (is_struct(who) && who) || nil, + permissions: &1, + authorization_module: authorization_module + } + ) end @doc """ @@ -204,12 +215,24 @@ defmodule Permit do def verify_record( %{ permissions: permissions, - subject: subject + subject: subject, + authorization_module: authorization_module } = _authorization, action, resource_or_module ) do - Permissions.granted?(permissions, action, resource_or_module, subject) + # If authorization_module is nil (backward compatibility), fall back to direct check + if is_nil(authorization_module) do + Permissions.granted?(permissions, action, resource_or_module, subject) + else + actions_module = authorization_module.actions_module() + + verify_fn = fn check_action -> + Permissions.granted?(permissions, check_action, resource_or_module, subject) + end + + Permit.Actions.verify_transitively!(actions_module, action, verify_fn) + end end defp add_predicate_name(atom), diff --git a/lib/permit/context.ex b/lib/permit/context.ex index 8111c8c..4a7bc14 100644 --- a/lib/permit/context.ex +++ b/lib/permit/context.ex @@ -4,10 +4,11 @@ defmodule Permit.Context do alias Permit.Types alias Permit.Permissions - defstruct permissions: Permissions.new(), subject: nil + defstruct permissions: Permissions.new(), subject: nil, authorization_module: nil @type t :: %Permit.Context{ permissions: Permissions.t(), - subject: Types.subject() | nil + subject: Types.subject() | nil, + authorization_module: module() | nil } end diff --git a/test/permit/actions_grouping_test.exs b/test/permit/actions_grouping_test.exs new file mode 100644 index 0000000..783d70c --- /dev/null +++ b/test/permit/actions_grouping_test.exs @@ -0,0 +1,217 @@ +defmodule Permit.ActionsGroupingTest do + @moduledoc """ + Tests for transitive action grouping verification. + + When an action depends on other permissions (e.g., show: [:read]), + the authorization check should verify if the required permissions are granted. + """ + use ExUnit.Case, async: true + + defmodule Article do + defstruct [:id, :author_id, :title] + end + + defmodule User do + defstruct [:id, :role] + end + + defmodule TestActions do + use Permit.Actions + + @impl Permit.Actions + def grouping_schema do + Map.merge(crud_grouping(), %{ + # Phoenix-style actions that depend on CRUD permissions + index: [:read], + show: [:read], + edit: [:update], + new: [:create] + }) + end + end + + defmodule TestPermissions do + use Permit.Permissions, actions_module: TestActions + + def can(%User{role: :admin}) do + permit() + |> all(Article) + end + + def can(%User{id: user_id}) do + permit() + |> read(Article) + |> update(Article, author_id: user_id) + end + + def can(_), do: permit() + end + + defmodule TestAuthorization do + use Permit, permissions_module: TestPermissions + end + + describe "transitive action grouping" do + test "show? returns true when user has :read permission and show: [:read]" do + user = %User{id: 1} + article = %Article{id: 1, author_id: 1, title: "Test"} + + # User has :read permission, show action requires :read + assert TestAuthorization.can(user) |> TestAuthorization.show?(article) + end + + test "index? returns true when user has :read permission and index: [:read]" do + user = %User{id: 1} + + # User has :read permission, index action requires :read + assert TestAuthorization.can(user) |> TestAuthorization.index?(Article) + end + + test "edit? returns true when user has :update permission on their own article and edit: [:update]" do + user = %User{id: 1} + article = %Article{id: 1, author_id: 1, title: "Test"} + + # User has :update permission on their own articles, edit action requires :update + assert TestAuthorization.can(user) |> TestAuthorization.edit?(article) + end + + test "edit? returns false when user tries to edit someone else's article" do + user = %User{id: 1} + article = %Article{id: 2, author_id: 2, title: "Test"} + + # User doesn't have :update permission on other's articles + refute TestAuthorization.can(user) |> TestAuthorization.edit?(article) + end + + test "new? returns false when user only has :read permission and new: [:create]" do + user = %User{id: 1} + + # User only has :read permission, new action requires :create + refute TestAuthorization.can(user) |> TestAuthorization.new?(Article) + end + + test "admin has all permissions including derived actions" do + admin = %User{role: :admin} + article = %Article{id: 1, author_id: 2, title: "Test"} + + # Admin has all permissions + assert TestAuthorization.can(admin) |> TestAuthorization.show?(article) + assert TestAuthorization.can(admin) |> TestAuthorization.index?(Article) + assert TestAuthorization.can(admin) |> TestAuthorization.edit?(article) + assert TestAuthorization.can(admin) |> TestAuthorization.new?(Article) + end + + test "direct permission check still works (read?)" do + user = %User{id: 1} + article = %Article{id: 1, author_id: 1, title: "Test"} + + # Direct :read permission check + assert TestAuthorization.can(user) |> TestAuthorization.read?(article) + end + + test "direct permission check for unpermitted action (delete?)" do + user = %User{id: 1} + article = %Article{id: 1, author_id: 1, title: "Test"} + + # User doesn't have :delete permission + refute TestAuthorization.can(user) |> TestAuthorization.delete?(article) + end + end + + describe "nested action grouping" do + defmodule NestedActions do + use Permit.Actions + + @impl Permit.Actions + def grouping_schema do + %{ + base: [], + level1: [:base], + level2: [:level1], + level3: [:level2] + } + end + end + + defmodule NestedPermissions do + use Permit.Permissions, actions_module: NestedActions + + def can(:user_with_base) do + permit() + |> base(Article) + end + + def can(_), do: permit() + end + + defmodule NestedAuthorization do + use Permit, permissions_module: NestedPermissions + end + + test "nested action dependencies are resolved transitively" do + # User has :base permission + # level3 requires level2, which requires level1, which requires base + assert NestedAuthorization.can(:user_with_base) + |> NestedAuthorization.do?(:level3, Article) + end + end + + describe "dependency on multiple actions" do + defmodule MultipleActions do + use Permit.Actions + + @impl Permit.Actions + def grouping_schema do + %{ + a: [], + b: [], + c: [], + d: [:a, :b, :c] + } + end + end + + defmodule MultiplePermissions do + use Permit.Permissions, actions_module: MultipleActions + + def can(:user_with_a) do + permit() + |> a(Article) + end + + def can(:user_with_a_b_c) do + permit() + |> a(Article) + |> b(Article) + |> c(Article) + end + + def can(:user_with_d) do + permit() + |> d(Article) + end + + def can(_), do: permit() + end + + defmodule MultipleAuthorization do + use Permit, permissions_module: MultiplePermissions + end + + test "user with a permission cannot perform d action" do + refute MultipleAuthorization.can(:user_with_a) |> MultipleAuthorization.d?(Article) + end + + test "user with a, b, and c permission can perform d action" do + assert MultipleAuthorization.can(:user_with_a_b_c) |> MultipleAuthorization.d?(Article) + end + + test "user with d permission can perform d action" do + assert MultipleAuthorization.can(:user_with_d) |> MultipleAuthorization.d?(Article) + end + + test "user with d permission cannot perform a action" do + refute MultipleAuthorization.can(:user_with_d) |> MultipleAuthorization.a?(Article) + end + end +end diff --git a/test/permit/permit_test.exs b/test/permit/permit_test.exs index 9f7adcf..0198c62 100644 --- a/test/permit/permit_test.exs +++ b/test/permit/permit_test.exs @@ -16,10 +16,10 @@ defmodule Permit.PermitTest do @impl Permit.Actions def grouping_schema do %{ - a: [:create], - b: [:read], - c: [:delete], - d: [:update] + a: [], + b: [], + c: [], + d: [] } end end