SEC-1140: ace masks are not being compared as bitmasks. #1388
Comments
|
Ben Alex said: The reason it operates in this manner is ACL inheritance, and in particular blocking. Consider a patient records system, where there are a large number of MedicalFiles. Each MedicalFile is a sub-file of another MedicalFile. Individual doctors, nurses, patients, auditors and IT administrators are granted various “ACRUD” (admin, create, read, update, delete) access to the different MedicalFiles. Every MedicalFile has ACL permissions inherit from another MedicalFile or PermissionHolder. The PermissionHolder is where we assign system-wide permissions to all MedicalFiles that inherit from it, including “R” for those with ROLE_AUDITOR and “AR” for ROLE_APP_ADMIN. Over time the records system grew, and it was decided to implement geographic administrators. In expectation of this eventuality, the architect ensured PermissionHolders existed for each major geographic region and MedicalFiles were placed under the region that initially created them. The permission for “AR” granted to ROLE_APP_ADMIN at the root PermissionHolder was then blocked by these geographic PermissionHolders, as and when each new region appointed their own local administrator. The relevant local administrators were then given “AR” permission for the geographic PermissionHolder and therefore all MedicalFiles under it. Where this gets complicated is there remains a need for global audit. Auditing is not geographically based in this particular organisation. So the “R” granted to auditors at the root PermissionHolder should continue to flow down, even through the geographic PermissionHolders that block the “AR” bit mask for administrators. The current ACL system works by matching on an entire integer, and not the individual bits within the integer. As such, a block for “AR” will only block “AR” permissions granted above. This simplifies understanding, as you can identify the precise permission you are blocking and not unexpectedly impact other permissions at the same time. Having said that, I do not recommend people use blocking unless they really need to. The additional conceptual weight it imposes needs to have proper grounding in a use case. The rationale for this design was also to allow more aggressive database retrieval operations. Specifically, consider we know we need “read” or “admin” access to invoke a particular method that works with a particular ACL-managed object. The current design requires the developer to specify three permissions are legal: “R”, “A” and “R_A” (the latter allowing people who have both admin and read to use the method). This allows the JDBC query to theoretically retrieve only those ACEs that contain those three integers, as we have no interest in other integers in computing the effective mask. If on the other hand we just said, “R” and “A” are legal, we’d need to compute every possible integer value that would have those particular bits active. This would naturally result in an impractically long SQL query and as such we lose the optimisation potential. I understand the design does make it more difficult for simple use cases. However, working at an individual bit level would complicate blocking operations and dismiss database optimisation possibilities. Let’s not forget if the main problem is the need to specify each permission combination, you can easily address this via a simple Java method. We probably should add some support for doing this out of the box. |
|
Jay Bose said: One possible solution is a MaskComparisionStrategy interface, and the current exact match could be provided in a DefaultMaskComparisionStrategy. |
|
Taylor Mathewson said: As to the database optimisation capabilities, don't most db's support bitwise operators? I'm familiar with it in mysql, postgre, mssql and oracle (bitand only, but that should be sufficient here). |
|
Joshua DeWald said: @ben - I thought the entire reason for using an integer as the Permission was so that it could be easily masked. If it's just being compared anyhow, why limit to 32 (unless that limit has been removed). It's not really a "mask" anymore so much as a pure enumeration... which is also something that people have wanted for a while (effectively, permissions as property level). Much of the documentation implies that it is a bitmask... but it's really not... |
|
Luke Taylor said: It should now be possible to plug in custom logic following the changes for SEC-1166. |
|
Luke Taylor said: Closing, as you can now plug in custom logic to make the permission-granting decision. |
|
This issue supersedes #740 |
Brian Ewins(Migrated from SEC-1140) said:
Granting permissions with bitmasks with more than one bit set does not have the expected result, eg:
```
MutableAcl acl = (MutableAcl) mutableAclService.readAclById(objectIdentity);
Permission perm = BasePermission.buildFromMask(BasePermission.CREATE.getMask() | BasePermission.DELETE.getMask());
acl.insertAce(0, perm, sid, true);
assert(acl.isGranted(new Permission[]{BasePermission.CREATE}, new Sid[] {sid}, false));
```
As well as the forum link above, this gets mentioned other times in the forum:
http://forum.springsource.org/showthread.php?p=235305
http://forum.springsource.org/showthread.php?t=68655
and as one of the many issues in SEC-479. I’m raising this to split this one issue into a separate bug.
each time its come up, the outcome is that people are reimplementing AclImpl, BasicLookupStrategy etc. In our case we’re migrating data from a different acl system, and this results in us having to split up every individual permission in the database, (since otherwise the CumulativePermission loaded never matches). It does seem to defeat the purpose of these things being bitmasks, as well as meaning we need to store 32x as many rows.
The text was updated successfully, but these errors were encountered: