Consider refactoring some of the PermissionChecker support code

[sberyozkin]

Description

QuarkusSecurityPermissionAugmentor is an internal SecurityIdentityAugmentor, which works with another internal class, QuarkusPermission to implement permission checks. It requires Quarkus Security be aware of QuarkusSecurityPermissionAugmentor.

The current solution was provided with the best intent to help users to write custom permission checkers which were required if no PermissionChecker was available. However, with QuarkusSecurityIdentity.Builder now providing several addPermission shortcuts leading to the automatic allocation of permission checker functions, it is not obvious QuarkusPermission should remain in the current form.

The proposal here is to drop QuarkusSecurityPermissionAugmentor, and re-implement QuarkusPermission for it to have its implies effective...

It is not a very imprtant issue to address, I only hope that making Quarkus Security be unaware of QuarkusSecurityPermissionAugmentor may help going forward

Implementation ideas

No response

[michalvavrik]

I provided some feedback on this PR here #45012 (comment). TL;DR;:

• you can't avoid augmenting the identity because that is how possessed permission gets in (so you need augmentor or some code that basically augments identity)
• you will need to do possessedPermission.implies(requiredPermission) however all the logic whether permission is granted is inside requiredPermission and there is nothing that possesedPermission requires. Because the possessedPermission doesn't contain any requirements other than that checker method inside requiredPermission
• you will need to find a reliable mechanism to get SecurityIdentity from ARc proxy in non-blocking manner (it can be done, just requires additional work)

I think this issue is about this line of code https://github.com/michalvavrik/quarkus/blob/8f44d0d8d2caa733ca6f191974d2bd9ac6024407/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/QuarkusIdentityProviderManagerImpl.java#L210, which is result of a fact that we do io.quarkus.security.identity.SecurityIdentity#checkPermission so if you do a permission check on inside identity but you require a different identity, you need to propagate it somehow. I preferred the instanceof because it is exception, it is the fastest way to to get the right identity and noone else needs it.

[michalvavrik]

It is not a very imprtant issue to address, I only hope that making Quarkus Security be unaware of QuarkusSecurityPermissionAugmentor may help going forward

I don't want it to looks like I am against it, I have already mentioned that if you want it, I'll address this issue. But if I can provide my personal opinion, then: QuarkusSecurityPermissionAugmentor is part of the Quarkus Security and it's internal code, so this tight coupling doesn't bother me too much.

I am happy to follow whatever is decided in this issue though.

[michalvavrik]

That said, I absolutely understand why you opened this, I am sure there is more people who share your opinion.

[sberyozkin]

Hey @michalvavrik Both of us will decide by agreeing if something needs to be done or not :-). You are welcome to challenge it.

Sure, I just recall, I had something in mind during the earlier discussions which I thought at a time, would avoid having to deal with the internal augmentation, something along the lines of SecurityIdentity#getPermissions() (this method is not available yet) and then linking them with QuarkusPermission#implies which would call the checker method - I don't recall, I'll keep this issue open for a bit longer and try to remind myself what exactly I had in mind... and then we'll decide if it is worth keeping it open or not

Thanks

[michalvavrik]

Yeah, it was hard to explain my thoughts without a loads of text, I'll come back to this when I have more time to think about it. Let's keep it open.