spring-security icon indicating copy to clipboard operation
spring-security copied to clipboard

ACL change Permission#getMask() type from "int" to "long"

Open belovaf opened this issue 5 years ago • 3 comments

Expected Behavior

Permission#getMask() returns long.

Current Behavior

Permission#getMask() returns int.

Context In our project we have very complex security restrictions. We have a lot of tiny permissions (1 bit for each) which we compose to a single mask permission. We reached a limit of int size (32 bits). I think java.util.BitSet mask will be perfect in our case, but it will be breaking change. But if we change a mask type to a long it will be a completely backward compatible change. And I think that 64 bits will be enough for us.

Will you accept PR from me with this change?

belovaf avatar Nov 27 '20 23:11 belovaf

Thanks for the suggestion, @belovaf. Unfortunately, it would not be backward compatible. An application that is doing:

int mask = permission.getMask();

would no longer compile since the type is being narrowed from long to int.

Given that, we wouldn't be able to accept this change.

jzheaux avatar Nov 30 '20 21:11 jzheaux

Thank you for such a fast response!

And can we do something like this (choose either getBitSetMask or getLongMask):

interface Permission extends Serializable {
    int getMask();

    default BitSet getBitSetMask() {
        return BitSet.valueOf(new long[]{getMask()});
    }

    default long getLongMask() {
        return getMask();
    }
}

And then we can change BasicLookupStrategy implementation to use new default method. In this case we don't brake clients code and enable mask bigger than 32 bits.

getBitSetMask looks much more flexible, but we should accept dynamic length mask patterns. getLongMask is much easier to implement.

Can you give your feedback about this implementation?

belovaf avatar Dec 01 '20 12:12 belovaf

Given this breaks passivity, we should consider it for 6.0.x or close as invalid.

rwinch avatar Jun 07 '22 18:06 rwinch

@rwinch Is there any plan to increase the mask size? We reached the limit and we can't add any more permissions due to that.

dimasmessias avatar Jun 20 '23 09:06 dimasmessias