Improve: Make deny_filter robust and add regression test
All checks were successful
continuous-integration/drone/push Build is passing
All checks were successful
continuous-integration/drone/push Build is passing
- Change deny_filter from [id: {:in, []}] to expr(false)
- Add regression test to ensure deny-filter matches 0 records
This commit is contained in:
parent
05cbd833bc
commit
dd4b88f0b7
2 changed files with 48 additions and 3 deletions
|
|
@ -179,10 +179,11 @@ defmodule Mv.Authorization.Checks.HasPermission do
|
||||||
|
|
||||||
# Helper function to return a filter that never matches (deny all records)
|
# Helper function to return a filter that never matches (deny all records)
|
||||||
# Used when authorization should be denied (fail-closed)
|
# Used when authorization should be denied (fail-closed)
|
||||||
# Returns [id: {:in, []}] which means "id IN []" - never matches (correct deny-all)
|
#
|
||||||
# NOTE: [id: {:not, {:in, []}}] would be "NOT (id IN [])" = true for all IDs (allow-all) - WRONG!
|
# Using `expr(false)` avoids depending on the primary key being named `:id`.
|
||||||
|
# This is more robust than [id: {:in, []}] which assumes the primary key is `:id`.
|
||||||
defp deny_filter do
|
defp deny_filter do
|
||||||
[id: {:in, []}]
|
expr(false)
|
||||||
end
|
end
|
||||||
|
|
||||||
# Helper to extract action type from authorizer
|
# Helper to extract action type from authorizer
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,44 @@
|
||||||
|
defmodule Mv.Authorization.Checks.HasPermissionFailClosedTest do
|
||||||
|
@moduledoc """
|
||||||
|
Regression tests to ensure deny-filter behavior is fail-closed (matches no records).
|
||||||
|
|
||||||
|
These tests verify that when HasPermission.auto_filter returns a deny-filter
|
||||||
|
(e.g., when actor is nil or no permission is found), the filter actually
|
||||||
|
matches zero records in the database.
|
||||||
|
|
||||||
|
This prevents regressions like the previous bug where [id: {:not, {:in, []}}]
|
||||||
|
was used, which logically evaluates to "NOT (id IN [])" = true for all IDs,
|
||||||
|
effectively allowing all records instead of denying them.
|
||||||
|
"""
|
||||||
|
use Mv.DataCase, async: true
|
||||||
|
|
||||||
|
alias Mv.Authorization.Checks.HasPermission
|
||||||
|
|
||||||
|
import Mv.Fixtures
|
||||||
|
|
||||||
|
test "auto_filter deny-filter matches no records (regression for NOT IN [] allow-all bug)" do
|
||||||
|
# Arrange: create some members in DB
|
||||||
|
_m1 = member_fixture()
|
||||||
|
_m2 = member_fixture()
|
||||||
|
|
||||||
|
# Build a minimal authorizer with a stable action type (:read)
|
||||||
|
authorizer = %Ash.Policy.Authorizer{
|
||||||
|
resource: Mv.Membership.Member,
|
||||||
|
action: %{type: :read}
|
||||||
|
}
|
||||||
|
|
||||||
|
# Act: missing actor must yield a deny-all filter (fail-closed)
|
||||||
|
deny_filter = HasPermission.auto_filter(nil, authorizer, [])
|
||||||
|
|
||||||
|
# Apply the returned filter to a real DB query (no authorization involved)
|
||||||
|
query =
|
||||||
|
Mv.Membership.Member
|
||||||
|
|> Ash.Query.new()
|
||||||
|
|> Ash.Query.filter_input(deny_filter)
|
||||||
|
|
||||||
|
{:ok, results} = Ash.read(query, domain: Mv.Membership, authorize?: false)
|
||||||
|
|
||||||
|
# Assert: deny-filter must match nothing
|
||||||
|
assert results == []
|
||||||
|
end
|
||||||
|
end
|
||||||
Loading…
Add table
Add a link
Reference in a new issue