diff --git a/README.md b/README.md index a5ca447..e93c560 100644 --- a/README.md +++ b/README.md @@ -336,6 +336,17 @@ Implement all [matchers' functions](https://casbin.org/docs/function): - [ ] ipMatch - [ ] globMatch +## Testing + +### Using with Ecto.Adapters.SQL.Sandbox + +If you're using Casbin-Ex with Ecto and need to wrap operations in database transactions during testing, see our guide on [Testing with Ecto.Adapters.SQL.Sandbox and Transactions](guides/sandbox_testing.md). + +Key points: +- Use `Ecto.Adapters.SQL.Sandbox.mode(Repo, {:shared, self()})` for tests with transactions +- Allow the EnforcerServer process to access your test's database connection +- See the guide for complete examples and best practices + ## License This project is licensed under the [Apache 2.0 license](LICENSE). diff --git a/guides/sandbox_testing.md b/guides/sandbox_testing.md new file mode 100644 index 0000000..b31f983 --- /dev/null +++ b/guides/sandbox_testing.md @@ -0,0 +1,169 @@ +# Testing with Ecto.Adapters.SQL.Sandbox and Transactions + +This guide explains how to use Casbin-Ex with `Ecto.Adapters.SQL.Sandbox` when you need to wrap Casbin operations in database transactions. + +## The Problem + +When using `Ecto.Adapters.SQL.Sandbox` in checkout mode (the default), database connections are restricted to the process that checked them out. The Casbin `EnforcerServer` runs in a separate process, which causes issues when: + +1. Your test wraps Casbin operations in a `Repo.transaction` +2. The transaction locks the connection to the test process +3. The `EnforcerServer` process cannot access the locked connection, even with `Sandbox.allow/3` + +This results in the error: +``` +** (DBConnection.ConnectionError) could not checkout the connection owned by #PID<...> +``` + +## Solution: Use Shared Mode + +The recommended solution is to use Ecto's **shared mode** for tests that need to call Casbin within transactions: + +```elixir +defmodule MyApp.RolesTest do + use MyApp.DataCase + use MyApp.CasbinCase + + setup do + # Check out a connection + :ok = Ecto.Adapters.SQL.Sandbox.checkout(MyApp.Repo) + + # Enable shared mode - this allows the EnforcerServer to use the connection + Ecto.Adapters.SQL.Sandbox.mode(MyApp.Repo, {:shared, self()}) + + # Allow the EnforcerServer process to access the connection + case Registry.lookup(Acx.EnforcerRegistry, "my_enforcer") do + [{enforcer_pid, _}] -> + Ecto.Adapters.SQL.Sandbox.allow(MyApp.Repo, self(), enforcer_pid) + [] -> + :ok + end + + :ok + end + + test "create role with permissions in transaction" do + permissions = [ + %{resource: "orgs", action: "read"}, + %{resource: "users", action: "read"} + ] + + # This now works because of shared mode + assert {:ok, :created} = + MyApp.Repo.transaction(fn -> + Enum.each(permissions, fn %{resource: resource, action: action} -> + case Acx.EnforcerServer.add_policy("my_enforcer", {:p, ["analyst", resource, action]}) do + :ok -> :ok + {:error, reason} -> MyApp.Repo.rollback(reason) + end + end) + + {:ok, :created} + end) + end +end +``` + +## Important Notes + +### About Shared Mode + +- **Shared mode** allows multiple processes to access the same database connection +- All tests in a module using shared mode will share the connection +- This may reduce test isolation compared to the default checkout mode +- It's still safe because each test gets a clean transaction that's rolled back + +### When to Use Shared Mode + +Use shared mode when you need to: +- Wrap Casbin operations in application-level transactions +- Test rollback behavior with Casbin +- Test atomic operations that involve both Casbin and other database changes + +### Alternative Approaches + +If you don't need transactions in your tests, you can: + +1. **Avoid wrapping in transactions**: Call Casbin operations directly without `Repo.transaction` +2. **Test transactions separately**: Test transaction logic separately from Casbin operations +3. **Use async: false**: Use `async: false` to run tests serially with shared connections + +## Example Test Module + +Here's a complete example of a test module using shared mode: + +```elixir +defmodule MyApp.Authorization.RolesTest do + use MyApp.DataCase, async: false # async: false for shared mode + + alias MyApp.Repo + alias MyApp.Authorization.Roles + + setup do + # Set up sandbox in shared mode + :ok = Ecto.Adapters.SQL.Sandbox.checkout(Repo) + Ecto.Adapters.SQL.Sandbox.mode(Repo, {:shared, self()}) + + # Allow EnforcerServer to access the connection + case Registry.lookup(Acx.EnforcerRegistry, "my_enforcer") do + [{pid, _}] -> Ecto.Adapters.SQL.Sandbox.allow(Repo, self(), pid) + [] -> :ok + end + + :ok + end + + test "creates role with permissions atomically" do + permissions = [ + %{resource: "users", action: "read"}, + %{resource: "users", action: "write"} + ] + + # This transaction includes Casbin operations + result = Repo.transaction(fn -> + # Create database record + {:ok, role} = Roles.create_role_record("admin") + + # Add Casbin policies + Enum.each(permissions, fn perm -> + case Acx.EnforcerServer.add_policy( + "my_enforcer", + {:p, ["admin", perm.resource, perm.action]} + ) do + :ok -> :ok + {:error, reason} -> Repo.rollback(reason) + end + end) + + {:ok, role} + end) + + assert {:ok, _role} = result + end + + test "rolls back Casbin operations on error" do + result = Repo.transaction(fn -> + # Add a policy + :ok = Acx.EnforcerServer.add_policy( + "my_enforcer", + {:p, ["temp_role", "resource", "action"]} + ) + + # Simulate an error + Repo.rollback(:simulated_error) + end) + + assert {:error, :simulated_error} = result + + # Verify the policy was not persisted + policies = Acx.EnforcerServer.list_policies("my_enforcer", %{sub: "temp_role"}) + assert policies == [] + end +end +``` + +## Further Reading + +- [Ecto.Adapters.SQL.Sandbox Documentation](https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.Sandbox.html) +- [Ecto.Adapters.SQL.Sandbox Shared Mode](https://hexdocs.pm/ecto_sql/Ecto.Adapters.SQL.Sandbox.html#module-shared-mode) +- [Testing with Ecto](https://hexdocs.pm/ecto/testing-with-ecto.html) diff --git a/lib/acx/persist/ecto_adapter.ex b/lib/acx/persist/ecto_adapter.ex index e5fc1d4..140b256 100644 --- a/lib/acx/persist/ecto_adapter.ex +++ b/lib/acx/persist/ecto_adapter.ex @@ -2,11 +2,45 @@ defmodule Acx.Persist.EctoAdapter do @moduledoc """ This module defines an adapter for persisting the list of policies to a database. + + ## Ecto.Adapters.SQL.Sandbox Compatibility + + When using this adapter with `Ecto.Adapters.SQL.Sandbox` in tests, especially + with nested transactions, you need to ensure proper connection handling. + + ### Recommended: Use Shared Mode + + In your test setup, use shared mode for tests that wrap Casbin operations in transactions: + + setup do + :ok = Ecto.Adapters.SQL.Sandbox.checkout(MyApp.Repo) + Ecto.Adapters.SQL.Sandbox.mode(MyApp.Repo, {:shared, self()}) + :ok + end + + This allows the EnforcerServer process to access the database connection + during transactions. Note that this means all tests in the module will + share the same connection, which may affect test isolation. + + ### Alternative: Avoid Transactions in Tests + + If you need better test isolation, consider structuring your tests to avoid + wrapping Casbin operations in explicit transactions, or handle rollback differently. + + ### Advanced: Dynamic Repo (Limited Use) + + For advanced use cases, you can configure the adapter with a function that + returns the repo, though this alone doesn't solve the transaction isolation issue: + + # In your application setup + adapter = EctoAdapter.new(fn -> MyApp.Repo end) + + See `Ecto.Adapters.SQL.Sandbox` documentation for more details on connection handling. """ import Ecto.Changeset use Ecto.Schema - defstruct repo: nil + defstruct repo: nil, get_dynamic_repo: nil defmodule CasbinRule do @moduledoc """ @@ -109,8 +143,38 @@ defmodule Acx.Persist.EctoAdapter do end end - def new(repo) do - %__MODULE__{repo: repo} + @doc """ + Creates a new EctoAdapter with the given repo. + + ## Parameters + - `repo`: An Ecto.Repo module or a function that returns one. + + ## Examples + # Static repo (standard usage) + adapter = EctoAdapter.new(MyApp.Repo) + + # Dynamic repo (for Sandbox testing with transactions) + adapter = EctoAdapter.new(fn -> Ecto.Repo.get_dynamic_repo() || MyApp.Repo end) + """ + def new(repo) when is_atom(repo) do + %__MODULE__{repo: repo, get_dynamic_repo: nil} + end + + def new(repo_fn) when is_function(repo_fn, 0) do + %__MODULE__{repo: nil, get_dynamic_repo: repo_fn} + end + + @doc """ + Gets the repo to use for the current operation. + If get_dynamic_repo is set, calls it to get the dynamic repo. + Otherwise returns the static repo. + """ + def get_repo(%__MODULE__{get_dynamic_repo: get_fn}) when is_function(get_fn, 0) do + get_fn.() + end + + def get_repo(%__MODULE__{repo: repo}) when is_atom(repo) do + repo end defimpl Acx.Persist.PersistAdapter, for: Acx.Persist.EctoAdapter do @@ -124,13 +188,15 @@ defmodule Acx.Persist.EctoAdapter do ...> {:error, "repo is not set"} """ @spec load_policies(EctoAdapter.t()) :: [Model.Policy.t()] - def load_policies(%Acx.Persist.EctoAdapter{repo: nil}) do + def load_policies(%Acx.Persist.EctoAdapter{repo: nil, get_dynamic_repo: nil}) do {:error, "repo is not set"} end def load_policies(adapter) do + repo = Acx.Persist.EctoAdapter.get_repo(adapter) + policies = - adapter.repo.all(CasbinRule) + repo.all(CasbinRule) |> Enum.map(&CasbinRule.changeset_to_list(&1)) {:ok, policies} @@ -156,15 +222,16 @@ defmodule Acx.Persist.EctoAdapter do ...> {:error, "repo is not set"} """ @spec load_filtered_policy(EctoAdapter.t(), map()) :: {:ok, [list()]} | {:error, String.t()} - def load_filtered_policy(%Acx.Persist.EctoAdapter{repo: nil}, _filter) do + def load_filtered_policy(%Acx.Persist.EctoAdapter{repo: nil, get_dynamic_repo: nil}, _filter) do {:error, "repo is not set"} end def load_filtered_policy(adapter, filter) when is_map(filter) do + repo = Acx.Persist.EctoAdapter.get_repo(adapter) query = build_filtered_query(filter) policies = - adapter.repo.all(query) + repo.all(query) |> Enum.map(&CasbinRule.changeset_to_list(&1)) {:ok, policies} @@ -224,14 +291,12 @@ defmodule Acx.Persist.EctoAdapter do ...> {:p, ["user", "file", "read"]}) ...> {:error, "repo is not set"} """ - def add_policy(%Acx.Persist.EctoAdapter{repo: nil}, _) do + def add_policy(%Acx.Persist.EctoAdapter{repo: nil, get_dynamic_repo: nil}, _) do {:error, "repo is not set"} end - def add_policy( - %Acx.Persist.EctoAdapter{repo: repo} = adapter, - {_key, _attrs} = policy - ) do + def add_policy(adapter, {_key, _attrs} = policy) do + repo = Acx.Persist.EctoAdapter.get_repo(adapter) changeset = CasbinRule.create_changeset(policy) case repo.insert(changeset) do @@ -254,14 +319,12 @@ defmodule Acx.Persist.EctoAdapter do ...> {:p, ["user", "file", "read"]}) ...> {:error, "repo is not set"} """ - def remove_policy(%Acx.Persist.EctoAdapter{repo: nil}, _) do + def remove_policy(%Acx.Persist.EctoAdapter{repo: nil, get_dynamic_repo: nil}, _) do {:error, "repo is not set"} end - def remove_policy( - %Acx.Persist.EctoAdapter{repo: repo} = adapter, - {_key, _attr} = policy - ) do + def remove_policy(adapter, {_key, _attr} = policy) do + repo = Acx.Persist.EctoAdapter.get_repo(adapter) f = CasbinRule.changeset_to_queryable(policy) case repo.delete_all(f) do @@ -270,12 +333,8 @@ defmodule Acx.Persist.EctoAdapter do end end - def remove_filtered_policy( - %Acx.Persist.EctoAdapter{repo: repo} = adapter, - key, - idx, - attrs - ) do + def remove_filtered_policy(adapter, key, idx, attrs) do + repo = Acx.Persist.EctoAdapter.get_repo(adapter) f = CasbinRule.changeset_to_queryable({key, attrs}, idx) case repo.delete_all(f) do @@ -296,14 +355,12 @@ defmodule Acx.Persist.EctoAdapter do ...> []) ...> {:error, "repo is not set"} """ - def save_policies(%Acx.Persist.EctoAdapter{repo: nil}, _) do + def save_policies(%Acx.Persist.EctoAdapter{repo: nil, get_dynamic_repo: nil}, _) do {:error, "repo is not set"} end - def save_policies( - %Acx.Persist.EctoAdapter{repo: repo} = adapter, - policies - ) do + def save_policies(adapter, policies) do + repo = Acx.Persist.EctoAdapter.get_repo(adapter) repo.transaction(fn -> insert_policies(repo, adapter, policies) end) end diff --git a/test/persist/ecto_sandbox_transaction_test.exs b/test/persist/ecto_sandbox_transaction_test.exs new file mode 100644 index 0000000..01112a0 --- /dev/null +++ b/test/persist/ecto_sandbox_transaction_test.exs @@ -0,0 +1,125 @@ +defmodule Acx.Persist.EctoSandboxTransactionTest do + @moduledoc """ + This test module demonstrates how to use Casbin with Ecto.Adapters.SQL.Sandbox + when wrapping operations in transactions. + + These tests are marked as skip by default because they require a real database + connection. To run them, set up a test database and remove the @moduletag :skip. + """ + use ExUnit.Case, async: false + + @moduletag :skip + + alias Acx.Enforcer + alias Acx.EnforcerServer + alias Acx.Persist.EctoAdapter + + # NOTE: Replace MyApp.Repo with your actual Repo module + # @repo MyApp.Repo + # @enforcer_name "test_enforcer" + + setup do + # IMPORTANT: This setup demonstrates the correct pattern for using + # Casbin with SQL.Sandbox when operations are wrapped in transactions + + # Step 1: Check out a connection from the sandbox + # :ok = Ecto.Adapters.SQL.Sandbox.checkout(@repo) + + # Step 2: Enable shared mode - this allows the EnforcerServer process + # to access the connection checked out by the test process + # Ecto.Adapters.SQL.Sandbox.mode(@repo, {:shared, self()}) + + # Step 3: Start an enforcer with the EctoAdapter + # cfile = "../data/rbac.conf" |> Path.expand(__DIR__) + # {:ok, _pid} = EnforcerServer.start_link(@enforcer_name, cfile) + # adapter = EctoAdapter.new(@repo) + # :ok = EnforcerServer.set_persist_adapter(@enforcer_name, adapter) + + # Step 4: Allow the EnforcerServer to access the connection + # case Registry.lookup(Acx.EnforcerRegistry, @enforcer_name) do + # [{pid, _}] -> Ecto.Adapters.SQL.Sandbox.allow(@repo, self(), pid) + # [] -> :ok + # end + + # on_exit(fn -> + # if Process.whereis(@enforcer_name), do: GenServer.stop(@enforcer_name) + # end) + + :ok + end + + @tag :skip + test "add policy within a transaction succeeds" do + # This test demonstrates adding Casbin policies within a database transaction. + # With shared mode enabled in setup, this works correctly. + + # result = @repo.transaction(fn -> + # # Add multiple policies atomically + # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["alice", "data1", "read"]}) + # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["alice", "data1", "write"]}) + # + # {:ok, :success} + # end) + # + # assert {:ok, :success} = result + # + # # Verify policies were added + # policies = EnforcerServer.list_policies(@enforcer_name, %{sub: "alice"}) + # assert length(policies) == 2 + end + + @tag :skip + test "transaction rollback also rolls back Casbin policies" do + # This test demonstrates that Casbin policy changes are rolled back + # when the containing transaction is rolled back. + + # result = @repo.transaction(fn -> + # # Add a policy + # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["bob", "data2", "read"]}) + # + # # Simulate a failure that causes rollback + # @repo.rollback(:simulated_error) + # end) + # + # assert {:error, :simulated_error} = result + # + # # Verify the policy was NOT persisted + # policies = EnforcerServer.list_policies(@enforcer_name, %{sub: "bob"}) + # assert policies == [] + end + + @tag :skip + test "mixed database and casbin operations in transaction" do + # This test demonstrates using both regular database operations + # and Casbin operations within the same transaction. + + # result = @repo.transaction(fn -> + # # Insert a user record (example - replace with your schema) + # # {:ok, user} = @repo.insert(%User{name: "charlie"}) + # + # # Add corresponding Casbin policies + # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["charlie", "data3", "read"]}) + # :ok = EnforcerServer.add_policy(@enforcer_name, {:p, ["charlie", "data3", "write"]}) + # + # {:ok, :complete} + # end) + # + # assert {:ok, :complete} = result + end + + @tag :skip + test "without shared mode, transaction operations fail" do + # This test demonstrates what happens WITHOUT shared mode. + # Uncomment to see the error (but don't check this in as passing). + + # First, disable shared mode + # Ecto.Adapters.SQL.Sandbox.mode(@repo, :manual) + # + # # Now try to add a policy in a transaction - this will fail + # assert_raise DBConnection.ConnectionError, fn -> + # @repo.transaction(fn -> + # EnforcerServer.add_policy(@enforcer_name, {:p, ["dave", "data4", "read"]}) + # end) + # end + end +end