We created a small extensions for rbac policies
GitHub
GitHub - traveltechdeluxe/ash-rbac
Contribute to traveltechdeluxe/ash-rbac development by creating an account on GitHub.
104 Replies
Awesome! ๐
Does this generate, under the hood, a policies code block, meaning it still uses the policies mechanism from Ash?
Will it always check for the
roles
field in a resource for its role or is there any way to configure that (for example, for my users, I normally have a roles
and organization_roles
fields that mean different things).It is just generating normal
policy
and field_policy
blocks. I was thinking of making the role getter function
configurable, but as of now it expects an actor with a roles attribute
We also combine this with normal policy blocks to add filter policies on resources that need them, as the extension is not handling that case at the moment. Tests are the best docs atm
As relationships work a bit differently, e.g. not supported by field_policies. But you can specify that a user can only use an action if it is accessed from a relationshipSuper cool man, this seems to have the potential to make my projects policies way simpler, amazing work ๐
Thanks, field policies really brought everything together.
Would you be interested in a PR adding support for custom checkers?
I was able to change the code to allow it like this:
I can't figure out how to make the bypass part take 2 arguments instead of one to allow something like this:
Sure, if you open the PR we can check what is needed for the bypass and stuff
also not sure if changing the whole module is the right approach, as the semantics of the check shouldn't change. Would it be enough for you use case if you were able to pass a function that takes the actor and returns the roles?
Sure, I thought about passing a module just to try to make less verbose, but I guess something like this:
Is fine too.
What would be a good key name for it? Or should it not even be a keyword and just the function directly like this:
could be just a function if it is an argument i think. Could you pleaseopen an issue on the repo. I think that is the better place for this discussion
we probably also need a way to configure this globaly if the actor looks different in general, having to specify it on every resource would become tedious very quickly
So, another unrelated question about the lib (not sure if I should ask that here or in the repo since that is not a PR). Does AshRbac works alongside a normal
policies
block?
For example, let's say I have this policies:
I was under the impression that this would be equivalent to this:
But from what I can tell from my testings the rbac rule is simply ignored. Is that not supported right now?As of now it is creating policies like this
As of now I'm not completely sure what the best way is to make this composable. I think allowing custom conditions shouldn't be to hard and might solve this problem.
In your example your read check would forbid it if the user was not confirmed but otherwise would authrize it. the block created from rbac would not lead to a definitive answer, therefore the authorization from your policy would allow the query to go through
The default for the rbac extension is to deny acccess and explicilty set conditions where an action is allowed. so to make it play well together you need to either add policies that deny certain stuff or use the same condition as the rbac role creates.
By custom conditions you mean add the conditions directly to rbac code block? Would that mean that we would be able to add custom conditions inside the
role
or actions
blocks?
I guess that would alleviate the problem, but seems like something that would create a lot of code duplication if I have to set the same rules for all roles (like my confirmed example since that applies to all roles).
@Zach Daniel I'm not familiar with the policies code, but is there any technical issue that makes it hard to be composable (meaning that I can split my policies into multiple policies
code blocks inside a resource)?you can already add conditions to the actions like this:
It would be possible to also allow a third element in the tuple with custom checks. e.g.
which would result in
Maybe composable was not the right word. Policies follow certain rules, if you follow them you can put your policies into different blocks. If I'm not mistaken:
all policy block with a matching conditions are executed.
a block executes all checks until an answer is found (:authorized | :forbidden) or all checks are done (:unknown)
afterwards the result of policy blocks is checked
if any returned :forbidden the request is stopped
if all are :unknown the request is also stopped
if one of them :authorized and the rest is :unknown the request is processed
the extension relies on the default being forbidden and explicitly authorizes if a condition matches.
if you add another policies like your read policy, that policy essentially bypasses the extensions policies because their policy conditions are not matching.
Now that I think about, you could possible already pass your
actor_attribute_equals(:confirmed?, false}
as condition.
That way the :user would only be allowed to execute the read actor is confirmed
I have to ask chatgpt to summarize this thread and create docs from it afterwards ๐คฃ
In this case I think I probably only have to allow an array of conditions. I think this is more in line with allow list philosophy. That way you can define explicit conditions for when a user should be allowed to do something.Policies compose just fine, but itโs important to know the main things: policies apply in order (important to account for bypasses), and all policies that apply to a request must pass.
(Except for bypasses)
So the essentially question is where does rbac put the policies it adds to your resource.
As long as it puts them at the end, then there should be no issue ๐
And all dsl entities from fragments also get put at the end, so if you have a fragment for policies you should put rbac and your policies in the same fragment.
It prepends bypasses and appends policies.
But mixing allow-list policies together with deny-list style policies leads to unwanted results
Hmโฆit shouldnโt
Every policy has to pass always no matter what
So why does that lead to unwanted results?
Ok, in the example above the policy with the forbid_if also has an authorize_if always in there. That one overrides all the other allow-list style policies. But it's only the authorize if in there that is the problem
that should not override any other policies
no policy will ever override another policy
the only thing capable of doing that is a
bypass
higher up in the policy list
in this example the second one is never looked at if the user has not the role
:user
and the first policy doesn't care about the role and only forbids in case of some_check
therefore the request is authorized no matter the role of the userif that's actually happening then it is a massive bug ๐
All policies that apply have to result in an authorized status
no the logic is sound
the second policy just isn't applied because of the condition
you said "therefore the request is authorized no matter the role of the user"
that is not what those policies say
yes because the first policy returns :authorized and the second one does not apply
if
has_role(:user)
is true, the second policy must pass
oh
well if its just authorize_if always()
then yes, you're right
sorry
a policy authorize_if always()
has no way of failing
so is effectively a noop if it applies or not
The reason its probably working when you only use the rbac
extension is because we require that at least one policy appliesif no policy applies it is denied right
Gotcha, so I think what your rbac extension needs to do is
I can not recall at the moment what it was, but I did this at first and had similar problems too.
gotcha. Well in the short term the library should probably express that it can't actually be mixed w/ your own policies on any action w/ rbac applied currently
you can, but can only add other allow-list style policies
but yeah, that should be in the docs
it doesn't matter what kind of policies they are
if any other policy applies, then they've gotten past the "at least one policy must apply" rule
a policy that contains only
authorize_if always()
can never forbid a request
So it actually only works if you have no other policies that applyyes, but if your policy is explicitly allowing something than that is what you want and it is ok if it applies
sorry, but I don't think so. Let me show you what I mean
if that first policy applies, it will override the rbac policy
which means you can really easily accidentally poke holes in your authorization
ok, i got what you mean. What i was thinking about was something with a condition more specific than the one from the extension
Yeah, if you are like narrowing it down then it works, but that's only kind of by chance
i.e if you only want users w/ a given role to be able to work against
foo_is_active
, you're actually making any rbac policies about that role ignored
based on the rbac stuff, I think what you actually want is for the role to be the condition, and the allowed actions to be the checks
i.e
how would I add an filter to this?
what kind of filter?
expr(id == ^actor(:id)) for example
is that something the
rbac
does?not at the moment
I just think filters were where I had my first problem and i started moving it over to conditions
if you do
Those two policies would result in
:foo
role being able to call the read action, and automatically applying the expr(id == ^actor)
filter.
note: If building them in a transformer you may need to reference the built in Expr
check modulebut if you leave of the has_role(:foo) from the second policy you would ave the same problem as before, right?
๐ค sorry, what exactly should the behavior be?
should all users have the filter applied? or only
has_role(:foo)
users?only has_role
this also leads to narrowing
I don't see how
The first policy says "any user w/
has_role(:foo)
can only call the read action"The second policy says "any user w/ `has_role(:foo), calling the read action, can only read themselves"
I'm just saying that there is no way around narrowing the condition
Well, that's just because its the example you gave
are you talking about the
rbac
extension adding this filter?
or you as a user of the rbac extension adding the filter?if the rbac extensio would do this
you as a user would still need to narrow the condition to add the filter
Not necessarily
if you as a user just did this:
then both policies would apply
only if the user has_role(:foo) right?
Nope
well
the first one applies if the user
has_role(:foo)
, yes
the second one applies if action(:read)
oh, lol
I see what you're sayingthe circle closes xD
you're right it has to be
So what you'd need to do to do this right is group all roles for each action
but the filter would still authorize? or is filter == :unkown
the filter authorizes, but applies the filter
yes but it would apply if no role matches also
So what you'd do is, for each action, list all roles that can do it
and add forbid_if always at the end
you don't technically need
forbid_if always()
at the end
its impliedbut the filter would still authorize if none of the roles match
Nah, this time for real lol
if
action(:read)
then both apply this time
so if you don't have an allowed role, the policy is forbidden
and the filter will be appliedyes, but if the actor acutally has_role(:baz)
then its forbidden
why?
because the first policy applies (because
action(:read)
)
and you don't have one of those rolesok, now I think i understand where my problem in understanding was. I thought if there was no matching check it would result in :unknown and the other policies would either :authorize or :forbid and if all policies returned :unkown it would default to :forbidden
nope, every policy that applies must always result in
:authorized
๐Thanks for clearing things up for me. gotta fix this tomorrow
Did you do any performance tests? I've started using this and sadly found out that somehow it made app really unresponsive and pinned my CPU to 100% and while in normal conditions LiveView took max of 1 second to send a response, now it took 35 seconds. I've started
:observer
to check what's happening and this is what I've seen(screenshot attached)
I've added
to like 6 resources, which are in a relationship. Sadly I don't have time to debug or test it further but will try if I will have some free-ish time.
๐ค
Something seems strange there
well, not "seems", something is very strange there. It probably isn't @barnabasj's fault, but something in core doing something strange.
Can you show me the result of
Ash.Policy.Info.policies(Resource)
of the resource w/ those roles? @MyrmyrI also felt, that since I started using field policies my tests were executing slower. But I haven't investigated yet.
Interesting
It might actually be the way they are modeled with conditions, now that I think about it
but with a single one that shouldn't be enough to cause problems...
I just published 0.2.0 with the refactor
oh, nice ๐ Maybe you can try that out @Myrmyr
there are only conditions in certain cases. But I didn't feel like it was faster afterwards.
only if you use the condition for a relationship for example
okay, so probably won't help then. How strange
I mean, I don't see what would cause the issue from a policies perspective, since its basically just generating the policies you would have hand written before
have you benchmarked field_policies in general?
um...no ๐
So maybe its actually just field policies
FYI I'm working on providing
Ash.Policy.Info.policies
but have to bleep some details so might take a whileI'd bet it is
here is a way to test this out
@barnabasj can you make it so that
[:*]
doesn't add any field policies?
or what does it do for :*
currently?I feel like this extension adding a lot of field policies, probably one for every field, is triggering this behavior
what i"m doing is going over every field and generate one policy each, I think I saw that you were doing that for
:*
somewhere anyway.. roles with the :*
field are added to every policiy as authorize_if check.Well, what we do for
:*
is generate one field policy for all of those fields
since you can do field_policy [:list, :of, :fields]
even still, the idea that it could take 35 seconds is pretty wildok, let me try and combine those then.
but it would be good to confirm in some way that the cause is field policies, and then I can benchmark them in core and figure out whats going on
Pastebin
[ %Ash.Policy.Policy{ condition: [{Checks.IsSiteAdminActor, [ac...
Pastebin.com is the number one paste tool since 2002. Pastebin is a website where you can store text online for a set period of time.
Pastebin
[ %Ash.Policy.FieldPolicy{ fields: [:organization_id], condi...
Pastebin.com is the number one paste tool since 2002. Pastebin is a website where you can store text online for a set period of time.
you can also have multiple field policies for a given field, and those can have a condition. So what I might suggest is doing this:
12 field policies if I counted correctly
no, wait
35 seconds happens only if I use this extension in like 7 resources that are in a relationship. If I only add this to the one I've printed policies for above it takes a bit longer(like 2-3 seconds).
ah, okay. Even 2-3 seconds is pretty wildly long but that makes sense
I'll try and combine the field policies into less policies and see if that helps
both of my examples above were wrong ๐
in what way? I'm currenly looking at my role check and see if it can be done in a more performant way. Right now the check is probably not the fastest and because it is executed a lot it might also be the problem.
I mean I didn't see numbers as high as yours but the implementation is definetively more on the it's nice to read, then on the performance side of things
without changes:
this is the result:
If I find a bit of time, but sadly that probably will take a while, I will try to recreate this issue in a separate app.
Thanks, I'll try and optimize as best I can in the meantime.
interesting
So your tests show that its quite fast
We may need to wait for more instances of this to show up, or a usable reproduction in that case ๐ฆ
I created a pr that should result in a lot less policies, maybe you can try it out and it helps already https://github.com/traveltechdeluxe/ash-rbac/pull/11
It took me some time, but I finally was able to reserve some time to finish my PR for adding support for other roles fields. there is the PR @barnabasj https://github.com/traveltechdeluxe/ash-rbac/pull/15
The only thing that I believe is missing there is adding support for
bypass
.I just released a new version with your changes
Hey @barnabasj I noticed that when I use ash_rbac, id fields from relationships return ForbiddenField. For example, if resource A has a relationship with resource B,
b_id
attribute will be returned with ForbiddenField even if I use fields [:*]
.
Removing the rbac rules and using this one instead works fine:
Is this a bug that should be reported or an I just doing something wrong?
Looking at the policy generated by rbac, seems like it filters all the :*_id
attributes when generating the policy where field_policy :*
doesn'tSounds like a bug to me, I'll take a look today
I publish a new version that I think should fix it, if not I have to dig a little deeper.
Yep, that fixed it, thanks a lot!