Topics

Ash Framework•2mo ago

Upgrade to 3.7.6 Broke FilterCheck

We're using Ash.Policy.FilterCheck and it seems the return type may have changed, we were returning true if all access, expr for subset access and false for no access. After upgrading, the true isn't letting any records come out. I saw the return type is either a keyword list or expression. What is the keyword list we should return? I've trued {:ok, true}, {:halt, true} and {:cont, true} but none of these let us have the previous behaviour of allowing all like before.

116 Replies

ZachDaniel•2mo ago

That should work. But it depends on how you're using the check of course What you're saying is you changed nothing and upgraded to 3.7.6 and it changed that behavior? What's happening instead? An error?

allenwymaOP•2mo ago

the tests are failing due to authorization i can verify by running the test with autohrize?: fasle in the query

ZachDaniel•2mo ago

So a forbidden error? I think I know what's happening potentially.

allenwymaOP•2mo ago

this is also failing:

def filter(actor, %{query: %Ash.Query{tenant: tenant} = _query} = _authorizer, _opts) do
    if can_read_feature?(actor, tenant, :members_notes) do
      if can_read_feature?(actor, tenant, :member_confidential_notes) do
        {:cont, expr(true)}
      else
        expr(access_level != :confidential)
      end
    else
      {:halt, expr(false)}
    end
  end

def filter(actor, %{query: %Ash.Query{tenant: tenant} = _query} = _authorizer, _opts) do
    if can_read_feature?(actor, tenant, :members_notes) do
      if can_read_feature?(actor, tenant, :member_confidential_notes) do
        {:cont, expr(true)}
      else
        expr(access_level != :confidential)
      end
    else
      {:halt, expr(false)}
    end
  end

ZachDaniel•2mo ago

We may be being too strict with the final policy check due to a recent change. I will look into it shortly Cont and halt aren't a thing In filter checks

allenwymaOP•2mo ago

the typespec is filter(actor :: term(), context(), options()) :: Keyword.t() | Ash.Expr.t() so seems returning a boolean shouldnt' be correct, either?

ZachDaniel•2mo ago

Booleans are expressions

allenwymaOP•2mo ago

https://tenor.com/bnHxa.gif

Tenor

allenwymaOP•2mo ago

cc @AngyL75

ZachDaniel•2mo ago

The policy in question

allenwymaOP•2mo ago

okay, makes sense why it was working before then 🙂

ZachDaniel•2mo ago

Does it have an access_type set?

allenwymaOP•2mo ago

how can i see that?

ZachDaniel•2mo ago

It will be in the policies

allenwymaOP•2mo ago

policy action_type([:read]) do
      authorize_if actor_attribute_equals(:super_admin, true)
      authorize_if CauseBeacon.Organizations.Role.CanReadMemberNote
    end

policy action_type([:read]) do
      authorize_if actor_attribute_equals(:super_admin, true)
      authorize_if CauseBeacon.Organizations.Role.CanReadMemberNote
    end

ZachDaniel•2mo ago

Like `access_type :strict'

allenwymaOP•2mo ago

i don't think we have ever set that anywhere

ZachDaniel•2mo ago

Hmm...okay

allenwymaOP•2mo ago

so then it should probably be a default :filter if it's on a read, no? 🙂

ZachDaniel•2mo ago

Yeah it defaults to filter Hmmmmmmmm What if you return expr(fragment("TRUE")) That's just to tell me where the problem is, not something I actually want you to have to do

allenwymaOP•2mo ago

still fails

ZachDaniel•2mo ago

Oh That was unexpected

allenwymaOP•2mo ago

i'm trying to turn on the debug logs for the queries config :logger, level: :debug seems this doesn't work 😄 thinking maybe i can see somethign in the logs

ZachDaniel•2mo ago

Are you capturing logs in your tests?

allenwymaOP•2mo ago

i have policy logs on, i could have sworn config :ash, :policies, show_policy_breakdowns?: true but nothing is coming up i guess thats' for access okay, found it now 😄 i mean for the debug logs 19:52:00.545 [debug] CauseBeacon.Organizations.MemberNote.read: skipped query run due to filter being false" so looks like the filter is somehow being intreted as false when i'm returning true?

ZachDaniel•2mo ago

config :ash, :policies, log_successful_policy_breakdowns: :error # Use whatever log level you'd like to use here

config :ash, :policies, log_successful_policy_breakdowns: :error # Use whatever log level you'd like to use here

add that to get more logs

allenwymaOP•2mo ago

running with this: config :ash, :policies, show_policy_breakdowns?: true, log_successful_policy_breakdowns: :error

1825b55-ccc2-416e-a558-b6545ed6c13d"]
19:55:37.197 [error] Successful authorization: CauseBeacon.Organizations.MemberNote.read


Policy Breakdown
  user: %{id: "d6d223a8-9a4b-41b2-b740-bd410fe42ebe"}

  Policy | 🌟:

    condition: action.type == :read

    authorize if: actor.super_admin == true | ✘ | ⬇    
    authorize if: checks if a user's role authorize read of a member's note | true | ✓ | 🌟

1825b55-ccc2-416e-a558-b6545ed6c13d"]
19:55:37.197 [error] Successful authorization: CauseBeacon.Organizations.MemberNote.read


Policy Breakdown
  user: %{id: "d6d223a8-9a4b-41b2-b740-bd410fe42ebe"}

  Policy | 🌟:

    condition: action.type == :read

    authorize if: actor.super_admin == true | ✘ | ⬇    
    authorize if: checks if a user's role authorize read of a member's note | true | ✓ | 🌟

true with a check?

ZachDaniel•2mo ago

That implies that its authorizing the results of the query 🤔

allenwymaOP•2mo ago

sorry and after that: 19:55:37.197 [debug] CauseBeacon.Organizations.MemberNote.read: skipped query run due to filter being false"

ZachDaniel•2mo ago

And to be clear all that you changed was that you updated ash?

allenwymaOP•2mo ago

so seems authorized and false? correct i was just doing my weekly mix deps update i've updated ash, ash_postgres ash_phoenix pretty sure the culprits are probably ash, ash_postgres, ash_sql 1-3 of those

ZachDaniel•2mo ago

Yeah, its likely ash we changed this code

allenwymaOP•2mo ago

should i explicitly set the check to filter?

ZachDaniel•2mo ago

do you mean access_type :filter?

allenwymaOP•2mo ago

yeah

ZachDaniel•2mo ago

cant hurt but probably not I've set up the same thing in tests and its working as far as I can tell 🤔

allenwymaOP•2mo ago

yeah, no use

ZachDaniel•2mo ago

Okay Do you have a few minutes to keep looking into this then? My testing shows it working So best thing here is to clone down Ash and poke around

allenwymaOP•2mo ago

so my can_read_feature can potentially be doing some querying of the database i have a special function that can check data on a user, wondering that may be an issue

ZachDaniel•2mo ago

Have you confirmed that its returning what you expect in that test? i.e true or false?

allenwymaOP•2mo ago

how can i best do that?

ZachDaniel•2mo ago

IO.inspect the return value

allenwymaOP•2mo ago

move to anotehr function and cehck the return? ok let me try try

ZachDaniel•2mo ago

If its returning the wrong thing then that makes sense, and the policies may be a red herring

allenwymaOP•2mo ago

CanReadMemberNote filter: fragment("TRUE") CanReadMemberNote filter: true seems to be right

ZachDaniel•2mo ago

kk, so that s not the issue then. alright, so next steps would be to clone down Ash

allenwymaOP•2mo ago

easy 🙂 main?

ZachDaniel•2mo ago

yep and |> IO.inspect() on 1735

%{authorizer | for_fields: opts[:for_fields]}
|> Checker.strict_check_scenarios()
|> IO.inspect() # <- add this
|> handle_strict_check_result(opts)

%{authorizer | for_fields: opts[:for_fields]}
|> Checker.strict_check_scenarios()
|> IO.inspect() # <- add this
|> handle_strict_check_result(opts)

will have a big output probably

allenwymaOP•2mo ago

uhm

ZachDaniel•2mo ago

lol

allenwymaOP•2mo ago

is hthis right? {:ash, path: "../../ash/ash", override: true},

ZachDaniel•2mo ago

yes

allenwymaOP•2mo ago

     error: undefined function transform/1 (there is no such import)
     │
 182 │     transform fn pub_sub_notification ->
     │     ^
     │
     └─ lib/cause_beacon/notifications/notification.ex:182:5: CauseBeacon.Notifications.Notification (module)

     error: undefined function transform/1 (there is no such import)
     │
 182 │     transform fn pub_sub_notification ->
     │     ^
     │
     └─ lib/cause_beacon/notifications/notification.ex:182:5: CauseBeacon.Notifications.Notification (module)

maybe i need to clean?

ZachDaniel•2mo ago

in lib/ash/policy/authorizer/authorizer.ex Did you pull latest main?

allenwymaOP•2mo ago

yeah, i just did a pull weird

ZachDaniel•2mo ago

Possibly a clean then

allenwymaOP•2mo ago

  defp strict_check_result(authorizer, opts \\ []) do
    %{authorizer | for_fields: opts[:for_fields]}
    |> Checker.strict_check_scenarios()
    |> IO.inspect(label: "strict_check_result")
    |> handle_strict_check_result(opts)
  end

  defp strict_check_result(authorizer, opts \\ []) do
    %{authorizer | for_fields: opts[:for_fields]}
    |> Checker.strict_check_scenarios()
    |> IO.inspect(label: "strict_check_result")
    |> handle_strict_check_result(opts)
  end

ZachDaniel•2mo ago

👍

allenwymaOP•2mo ago

here's what i have, still compiling i'm about to lose my mind

Generated ash_paper_trail app
==> ash_sql
Compiling 14 files (.ex)

== Compilation error in file lib/aggregate.ex ==
** (KeyError) key :related? not found
    (ash 3.4.56) expanding struct: Ash.Query.Exists.__struct__/1
    lib/aggregate.ex:274: AshSql.Aggregate.add_aggregates/6
    (elixir 1.18.4) expanding macro: Kernel.|>/2
    lib/aggregate.ex:150: AshSql.Aggregate.add_aggregates/6

Generated ash_paper_trail app
==> ash_sql
Compiling 14 files (.ex)

== Compilation error in file lib/aggregate.ex ==
** (KeyError) key :related? not found
    (ash 3.4.56) expanding struct: Ash.Query.Exists.__struct__/1
    lib/aggregate.ex:274: AshSql.Aggregate.add_aggregates/6
    (elixir 1.18.4) expanding macro: Kernel.|>/2
    lib/aggregate.ex:150: AshSql.Aggregate.add_aggregates/6

what ma i doing wrong? 😄

ZachDaniel•2mo ago

lol rm -rf _build? What commit of ash are you on? Should be 381068e

allenwymaOP•2mo ago

54437e17da344555628bdfdb425596dbca252301

ZachDaniel•2mo ago

thats not latest main

allenwymaOP•2mo ago

hmm

ZachDaniel•2mo ago

are you pulling from your fork's latest main? Do you need something like git pull upstream main?

allenwymaOP•2mo ago

🤫 this didnt happen 😄 yeah, just occrured to me my code is behind haha sorry

allenwymaOP•2mo ago

message.txt

allenwymaOP•2mo ago

oh, it's massiv, sorry

ZachDaniel•2mo ago

no thats good Okay, so, so far so good oh, wait no I don't think that is the right one?

allenwymaOP•2mo ago

how do i know which is right? it was the last one logged, so it hought it'd be right?

ZachDaniel•2mo ago

Look at the resource and action keys You can add some conditional logic in there

|> tap(fn result -> 
  if authorizer.resource == Resource and authorizer.action.name == :action do
    IO.inspect(result)
  end
end)

|> tap(fn result -> 
  if authorizer.resource == Resource and authorizer.action.name == :action do
    IO.inspect(result)
  end
end)

allenwymaOP•2mo ago

okay, thanks, let me try now 🙂

allenwymaOP•2mo ago

log.ex

allenwymaOP•2mo ago

i'm guessing the second element in the tuple being false is no bueno?

ZachDaniel•2mo ago

yep okay, interesting 🤔 mix deps.update crux Just in case I don't think that will matter

allenwymaOP•2mo ago

still failing i see def strict_check_scenarios is a case statement, shall i check each branch to see where the answer is coming from?

ZachDaniel•2mo ago

Alright, into checker.ex we go we want to know the branch we get into here:

def strict_check_scenarios(authorizer) do
  case Ash.Policy.Policy.solve(authorizer) do
    {:ok, value, authorizer} when is_boolean(value) ->
      {:ok, value, authorizer}

    {:ok, scenarios, authorizer} ->
      scenarios
      |> remove_scenarios_with_impossible_facts(authorizer)
      |> case do
        [] -> {:ok, false, authorizer}
        scenarios -> {:ok, scenarios, authorizer}
      end

    {:error, authorizer, error} ->
      {:error, authorizer, error}
  end
end

def strict_check_scenarios(authorizer) do
  case Ash.Policy.Policy.solve(authorizer) do
    {:ok, value, authorizer} when is_boolean(value) ->
      {:ok, value, authorizer}

    {:ok, scenarios, authorizer} ->
      scenarios
      |> remove_scenarios_with_impossible_facts(authorizer)
      |> case do
        [] -> {:ok, false, authorizer}
        scenarios -> {:ok, scenarios, authorizer}
      end

    {:error, authorizer, error} ->
      {:error, authorizer, error}
  end
end

Specifically the first or second one in line 66 of checker.ex

allenwymaOP•2mo ago

line 66 is the def

ZachDaniel•2mo ago

yeah, just pointing the area

allenwymaOP•2mo ago

ok

ZachDaniel•2mo ago

def strict_check_scenarios(authorizer) do
  case Ash.Policy.Policy.solve(authorizer) do
    {:ok, value, authorizer} when is_boolean(value) ->
      # here
      {:ok, value, authorizer}

    {:ok, scenarios, authorizer} ->
      scenarios
      |> remove_scenarios_with_impossible_facts(authorizer)
      |> tap(fn scenarios -> 
        # and here
      end)
      |> case do
        [] -> {:ok, false, authorizer}
        scenarios -> {:ok, scenarios, authorizer}
      end

    {:error, authorizer, error} ->
      {:error, authorizer, error}
  end
end

def strict_check_scenarios(authorizer) do
  case Ash.Policy.Policy.solve(authorizer) do
    {:ok, value, authorizer} when is_boolean(value) ->
      # here
      {:ok, value, authorizer}

    {:ok, scenarios, authorizer} ->
      scenarios
      |> remove_scenarios_with_impossible_facts(authorizer)
      |> tap(fn scenarios -> 
        # and here
      end)
      |> case do
        [] -> {:ok, false, authorizer}
        scenarios -> {:ok, scenarios, authorizer}
      end

    {:error, authorizer, error} ->
      {:error, authorizer, error}
  end
end

allenwymaOP•2mo ago

okay, i turned off the previous tap, too, for now, reduce the log or shit, i need that 😄 strict_check_scenarios boolean result: false

case Ash.Policy.Policy.solve(authorizer) do
      {:ok, value, authorizer} when is_boolean(value) ->
        IO.inspect(value, label: "strict_check_scenarios boolean result")
        {:ok, value, authorizer}

case Ash.Policy.Policy.solve(authorizer) do
      {:ok, value, authorizer} when is_boolean(value) ->
        IO.inspect(value, label: "strict_check_scenarios boolean result")
        {:ok, value, authorizer}

ZachDaniel•2mo ago

okay, into policy.ex we go

allenwymaOP•2mo ago

so the Policy.solve is false

ZachDaniel•2mo ago

line 88

allenwymaOP•2mo ago

https://tenor.com/tlFB.gif

Tenor

ZachDaniel•2mo ago

check_context = check_context(authorizer)

{expression, authorizer} =
  build_requirements_expression(authorizer, check_context)

IO.inspect(expression) # probably `false`

case expression do
  expr when is_boolean(expr) ->
    {:ok, expr, authorizer}

  expression ->
    scenario_options = scenario_options(check_context)

    expression
    |> Formula.from_expression()
    |> Crux.satisfying_scenarios(scenario_options)
    |> case do
      [] ->
        {:error, authorizer, :unsatisfiable}

      scenarios ->
        mapped_scenarios = Enum.map(scenarios, &Map.drop(&1, [true, false]))
        {:ok, Enum.uniq(mapped_scenarios), authorizer}
    end
end

check_context = check_context(authorizer)

{expression, authorizer} =
  build_requirements_expression(authorizer, check_context)

IO.inspect(expression) # probably `false`

case expression do
  expr when is_boolean(expr) ->
    {:ok, expr, authorizer}

  expression ->
    scenario_options = scenario_options(check_context)

    expression
    |> Formula.from_expression()
    |> Crux.satisfying_scenarios(scenario_options)
    |> case do
      [] ->
        {:error, authorizer, :unsatisfiable}

      scenarios ->
        mapped_scenarios = Enum.map(scenarios, &Map.drop(&1, [true, false]))
        {:ok, Enum.uniq(mapped_scenarios), authorizer}
    end
end

More importantly probably here:

  defp build_requirements_expression(authorizer, check_context) do
    {expression, authorizer} =
      authorizer.policies
      |> expression(check_context)
      |> simplify_policy_expression(check_context)
      |> expand_constants(authorizer, check_context)

    authorizer = %{authorizer | solver_statement: expression}

    {expression, authorizer}
  end

  defp build_requirements_expression(authorizer, check_context) do
    {expression, authorizer} =
      authorizer.policies
      |> expression(check_context)
      |> simplify_policy_expression(check_context)
      |> expand_constants(authorizer, check_context)

    authorizer = %{authorizer | solver_statement: expression}

    {expression, authorizer}
  end

Each line of that pipeline

allenwymaOP•2mo ago

okay, running the test blew out my buffer... ok Policy.solve expression: false

ZachDaniel•2mo ago

Try this:

defp build_requirements_expression(authorizer, check_context) do
  {expression, authorizer} =
    authorizer.policies
    |> expression(check_context)
    |> tap(fn -> end)
    |> simplify_policy_expression(check_context)
    |> tap(fn -> end)
    |> expand_constants(authorizer, check_context)

  if ... do
     IO.inspect(expression)
  end

  authorizer = %{authorizer | solver_statement: expression}

  {expression, authorizer}
end

defp build_requirements_expression(authorizer, check_context) do
  {expression, authorizer} =
    authorizer.policies
    |> expression(check_context)
    |> tap(fn -> end)
    |> simplify_policy_expression(check_context)
    |> tap(fn -> end)
    |> expand_constants(authorizer, check_context)

  if ... do
     IO.inspect(expression)
  end

  authorizer = %{authorizer | solver_statement: expression}

  {expression, authorizer}
end

allenwymaOP•2mo ago

i got it in anothe terminal, no worries

{expression, authorizer} =
      build_requirements_expression(authorizer, check_context)

    IO.inspect(expression, label: "Policy.solve expression")

{expression, authorizer} =
      build_requirements_expression(authorizer, check_context)

    IO.inspect(expression, label: "Policy.solve expression")

so that's false in teh expression Policy.solve expression: false strict_check_scenarios boolean result: false strict_check_result: {:ok, false,

allenwymaOP•2mo ago

maybe you need this?

message.ex

allenwymaOP•2mo ago

    {expression, authorizer} =
      authorizer.policies
      |> expression(check_context)
      |> IO.inspect(label: "Initial policy expression")
      |> simplify_policy_expression(check_context)
      |> IO.inspect(label: "Simplified policy expression")
      |> expand_constants(authorizer, check_context)
      |> IO.inspect(label: "Expanded constants expression")

    {expression, authorizer} =
      authorizer.policies
      |> expression(check_context)
      |> IO.inspect(label: "Initial policy expression")
      |> simplify_policy_expression(check_context)
      |> IO.inspect(label: "Simplified policy expression")
      |> expand_constants(authorizer, check_context)
      |> IO.inspect(label: "Expanded constants expression")

or maybei have the wrong logs? i was trying to cut it down to just what you need due to the setup data

ZachDaniel•2mo ago

That looks roughly right but I don't see Simplified policy expression there Just Expanded constants expression We're narrowing in on the issue You could also try pinning {:crux, "== 0.1.1"} or 0.1.0 maybe we messed something up there, not sure.

allenwymaOP•2mo ago

trying to, hard to follow here it is all sorry, i'll just send youall: it's really really tough for me to try to cut it

allenwymaOP•2mo ago

message.ex

allenwymaOP•2mo ago

okay, i'll do {:crux, "== 0.1.1"} seems cux 0.1.1 fails

ZachDaniel•2mo ago

Okay, we can simplify for now by commenting out other policies for non-read actions

allenwymaOP•2mo ago

     ** (Ash.Error.Unknown) 
     Bread Crumbs:
       > Exception raised in: CauseBeacon.Organizations.AdminMember.read
       > Exception raised in: CauseBeacon.Organizations.Organization.read
       > Exception raised in: CauseBeacon.Organizations.MemberNote.read

     Unknown Error

     * ** (FunctionClauseError) no function clause matching in CauseBeacon.Organizations.AdminMember.CanManageResource.match?/3

     ** (Ash.Error.Unknown) 
     Bread Crumbs:
       > Exception raised in: CauseBeacon.Organizations.AdminMember.read
       > Exception raised in: CauseBeacon.Organizations.Organization.read
       > Exception raised in: CauseBeacon.Organizations.MemberNote.read

     Unknown Error

     * ** (FunctionClauseError) no function clause matching in CauseBeacon.Organizations.AdminMember.CanManageResource.match?/3

ZachDaniel•2mo ago

So we can test just this one action's policy expression

allenwymaOP•2mo ago

let me try to skip the policies

ZachDaniel•2mo ago

Can you also add the conditional logic?

allenwymaOP•2mo ago

wow, it's going to be a lot to try to skip

ZachDaniel•2mo ago

like if authorizer.resource == .. The ones you sent include other action calls as far as I can tell

allenwymaOP•2mo ago

ok so i should still keep with the broken crux? cause it kills the test

ZachDaniel•2mo ago

no, use the new one 👍

allenwymaOP•2mo ago

message.ex

ZachDaniel•2mo ago

@allenwyma DMd Just going to run your app if you'll let me 😄 Okay @allenwyma I've found the culprit There is indeed a bug somewhere in our expression logic its not a scary one because it will only prevent access, not add access but it comes from having a trailing bypass

bypass actor_attribute_equals(:super_admin, true) do
  authorize_if always()
end
end

bypass actor_attribute_equals(:super_admin, true) do
  authorize_if always()
end
end

If you put that at the beginning and not the end (where I think you'd probably want it) then your policies will work.

allenwymaOP•2mo ago

😂 It’s in my app logic?

ZachDaniel•2mo ago

That is, yes there is a bug that shouldn't be preventing you

allenwymaOP•2mo ago

Yeah but still good to know. I have someone to hunt down 😂 Thanks so much for that!

ZachDaniel•2mo ago

We fixed abut around trailing bypasses, and were clearly too aggressive. I might actually just make trailing bypasses raise an error lol because it makes no sense to have a bypass that doesn't bypass anything

Jonatan Männchen•2mo ago

fix is on the way: https://github.com/ash-project/ash/pull/2404

allenwymaOP•2mo ago

yeah, i didn't examine the whole thing, but makes sense. i talked to the guy who wrote it and he also understood 🙂 i'm also thinking maybe we shouold updat the docs with an example for filtercheck and the typespec? It wasn't clear to me that a boolean is an expression. But, I think the Keywords part isnt' clear to me what keywords i can return? i can submit a PR since this is on my mind if you can help to fill in the typespec details for me

ZachDaniel•2mo ago

The keyword part is the keyword filter syntax i.e [id: 10] or [score: [greater_than: 20]]

allenwymaOP•2mo ago

oh, like siimilar to a expression sort of?

ZachDaniel•2mo ago

Yep! https://hexdocs.pm/ash/Ash.Filter.html#module-keyword-list-syntax

AngyL75•2mo ago

Thanks @Zach for the debugging... and the bypass at the end... was strange indeed 🙂 I still need to improve my coding 🙂

Ash Framework Join

The Elixir backend framework for unparalleled productivity. Declarative tools that let you stop wasting time. Use with Phoenix LiveView or build APIs in minutes for your front-end of choice.

2.1KMembers

View on Discord

Did you find this page helpful?