SEC-2342: DefaultPermissionGrantingStrategy directly compares permission mask

[spring-projects-issues]

Igor Artamonov (Migrated from SEC-2342) said:

DefaultPermissionGrantingStrategy.isGranted uses direct comparison for the permission mask:

//line :68
if ((ace.getPermission().getMask() == p.getMask()) && ace.getSid().equals(sid)) {

So, for:

• composite mask "RW" (ace mask is 3)
• when we require "R" (p mask is 1) permission

isGranted will return false.

I believe there should be & used, instead of ==

[ndouba]

+1. I agree. It is counterintuitive to call this a mask when it is being used as a flag. See PR #3708 for a proposed change. Right now we've copied and pasted the DefaultPermissionGrantingStrategy into another class to accomplish the same behaviour with &.

[rwinch]

@ndouba Thanks for the input and submitting the PR!

This is related if not a duplicate of #1388

I'm going to need some time to look into some of the history behind the issue (i.e. #1388 (comment)) . Also, (assuming we accept the change) I'm a bit hesitant to update existing code in a non-passive way. We may need to add a flag to the class to enable the functionality or create another class that performs the operation with the updates.

[ndouba]

Maybe a better alternative to my PR is to create a new interface (PermissionComparisonStrategy) which allows people to inject their own permission comparison into the DefaultPermissionGrantingStrategy bean? I can also see how some people may want to implement their own version of it (i.e. mask1 > mask2, or if mask1 == ADMINISTER || (mask1 & mask2) != 0, etc.).

EDIT: I noticed the other comment also mentioned the same strategy. Also in looking at the previous comments I disagree with the use case presented as bitwise AND operations are supported by database queries and the AclService classes are not the ones responsible for querying by bitmask. It is the responsibility of the DefaultPermissionGrantingStrategy to do this evaluation as it is designed today. Therefore, blocking would not be an issue. As I understand it, the AclService is responsible for deserializing and serializing Acl objects from and to the database into POJOs, respectively, where the Acl's object identity matches.

[kortatu]

I've done the same as @ndouba , copy the code in my PermissionGrantingStrategy but executing a bitwise and operation and comparing with 0.

[eleftherias]

Fixed by gh-5908, which allows overriding isGranted method in DefaultPermissionGrantingStrategy for adding custom permission comparison.