Since upgrading to 4.1.1 when I clicked on Administration -> Backup and clicked on “Create Backup,” It shows a white screen and states “Not authorized!”
I did lots of ACL testing with that new fix before the release and it looked good. However, just hit me what is likely happening here. For example, in Administration->ACL, if you place the Demographics ACO in both the addonly and write return ACL bad things will happen. Which makes total sense, because return $gacl_object->acl_return_value() will try to return two values (I am not sure what is actually does, but whatever it returns, it breaks the code. So, an aco should only ever be in one ACL of the same group; if it is in more than one return value acl of the same group, then bad things will happen.
That will break things where doing acl_check and seeing if equivalent to addonly or write etc.
That being said, I have not been able to reproduce why the backup script is breaking, which uses:
if (!acl_check('admin', 'super')) die(xl('Not authorized','','','!'));
The question here is that does it make sense to revert the 4.1.1 release back to the original acl_check function until this is sorted out (for example, could make the Administration->ACL script not allow ACO’s in more than one group return value and issue warnings if currently the case.
Did some testing and turns out that the $gacl_object->acl_return_value() function only return one return value and it chooses the most recently added one. So, if I were to add the demographics ACO to write, then to addonly, this means a user would not be able to edit the demographics (only has addonly privileges).
And this actually gets much worse. Because a user can be added into multiple groups, so there is simply no way to control which return value is given back if the user has different return values in different groups.
I think we need to revert to old acl function and rethink this. I think the way to go is not to request the return value from the gacl function $gacl_object->acl_return_value() when looking for a specific return value since this does not work. We need to collect all the permitted return values and then see if one of them is the one we want (such as write, addonly etc.). This may require a small bit of code mod in gacl itself, but I think this may be the way we need to go(could potentially modify/rename the gacl acl_return_value function a bit by adding a parameter for the return value to check for and then to simply return a true/false; note would require modifying codebase to place the specific return value in the function parameter rather than test equality of it).