ACLs' priority sometimes does the opposite of what it says it does
When you're editing an ACL the priority field says:
Higher priority ACL rules will override lower priority rules
It appears that the relevant code is at civicrm/CRM/ACL/BAO/ACL::loadPermittedIDs()
and this code orders by Priority ascending.
- If the ACL grants(or denies) access to a particular group(/object) then it processes every ACL in turn, meaning, it works as described because the last one will have the last say; higher priority means higher priority.
- But if it grants(or denies) access to all groups(/objects) then it stops at the first one, with a
break
. So the lower priority takes it.
So for rules that refer to "all" groups(/other entities), priority is reversed(!)
I'm not sure about the code in that loop
- if it's an Allow all groups(/objects) rule for the current permission type (Edit/View), then we do an odd loop to append the keys of allGroups to ids which seems odd: why not do
$ids = array_keys($allGroups);
and save on duplicates/time/code? - if it's a Deny all groups(/objects) rule, then there's an
array_diff()
but if you remove all then surely that's just clearer as$ids = [];
? - Finally, if it's an ACL that affects all groups(/objects) but the first ACL doesn't match on permission type (e.g. we're looking for Edit perm and the first one found grants View), then there's a
break
. This is the cause of the priority reversal, but it also 'breaks'😆 other potential configurations. e.g.- there's no point in having 2 rules about 'all' groups(/objects) - e.g. one for edit and one for view - the one with the lowest priority prevents Civi ever considering the 2nd. If that's because edit also grants view (does it?) then we ought to stop people making such rules in the UI.
- rules like: Grant edit to all groups, followed (higher priority) by a deny group X won't be honoured.
I think the correct thing to do here is just get rid of that break
.
PIng @seamuslee who authored a recent relevant commit