Possible ACL Bug

tmccormi wrote on Tuesday, June 22, 2010:

This report was sent to me by Fei Lung of EMRTech.  I applied the fix to a test related to MU #22, but it breaks other parts of OpenEMR - opinions?
-Tony

I believe there is a bug in the acl_check() function (library/acl.inc) which currently allows anyone to write/add, even those with addonly permission.
On line 119 in library/acl.inc:
      return $gacl_object->acl_check($section, $value, $section_aro_value, $user);
returns true/false, but many places in the code reference the return value as a string, ie 'write', 'addonly'.  I believe the correct return for acl_check() should be:
      $x = $gacl_object->acl_return_value($section, $value, $section_aro_value, $user);
      return $x['return_value'];
which returns the actual permission (write/addonly) and not true/false.  The comments for the function indicate that the actual permission should be returned, as well as the case of non-existence of gacl, the permission is returned.

bradymiller wrote on Wednesday, June 23, 2010:

hey,
This is a good catch.
On a quick look through gacl docs and code, it appears acl_return_value() will return the return value or NULL (if none), so following I think would work better:

return $gacl_object->acl_return_value($section, $value, $section_aro_value, $user);

note the acl_query() function in gacl would return the array with an element of return_value.

However, a nasty bug will be unmasked here. For example, what happens when a user is given access to both an aco of something with ‘write’ and ‘addonly’. It appears gacl returns the most recently created/modified one, however this is obviously not gonna work. The acl_query script in gacl ( gacl/gacl.class.php ) will need to be modified to instead give the return value of highest access (ie. if have access to both write and addonly of an aco, then need to send back write).

-brady

bradymiller wrote on Wednesday, June 23, 2010:

The question is why is everything getting authorized. For some reason:

$isauth = (acl_check('acct', 'bill') == 'write')

is true. I guess comparing a boolean to a string is not a good idea…

-brady

tmccormi wrote on Wednesday, June 23, 2010:

OK, then this should be an official bug, I guess :-)   I’ll post it and refer to this thread…

-Tony

tmccormi wrote on Wednesday, June 23, 2010:

Bug posted, ID: 3020259
Here: https://sourceforge.net/tracker/?func=detail&aid=3020259&group_id=60081&atid=493001