Use final SecurityIdentity augmented with all the augmentors in @PermissionChecker methods

[michalvavrik]

• closes: PermissionChecker method sometimes receives unaugmented SecurityIdentity even though augmentor did run successfully #44990
• changes in the IdentityMock class ensure that added tests actualy verify the issue
• fixes the issue when augmentors order were not deterministic (when users use default augmentor priority), which resulted in a fact that SecurityIdentity passed as a method argument of a @PermissionChecker wasn't always augmented with all the augmentors

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 8f44d0d.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[sberyozkin]

Thanks @michalvavrik , will have a look tomorrow, finding it difficult to review on the phone

[michalvavrik]

Thanks @michalvavrik , will have a look tomorrow, finding it difficult to review on the phone

I can only imagine :-D We are in no hurry.

[sberyozkin]

Hi @michalvavrik The fact that we have to modify this class in a couple of places to do some special processing around the internal security augmentor implies, IMHO, that may be that original solution has to be revisited. Recall we discussed it.

Would it simplify things, if, instead of having the special internal QuarkusPermission and the special augmentor, we'd make this QuarkusPermission be an actual custom permission that will be passed to possessed permissions ? I believe the reason we have the current solution is that it was meant to make it easier for users to correctly write custom permission checkers, but we now have addPermission(...) identity builder methods...

We don't have to do it now as the bug has to be fixed, and it may require more discussion, but at least I'd ask for //Revisit comments

[sberyozkin]

@michalvavrik I can open an enhancement or housekeeping issue for us to discuss. QuarkusPermissionSecurityIdentityAugmentor plus current QuarkusPermission combination is sound, but I hope we can avoid doing QuarkusPermissionSecurityIdentityAugmentor specific processing...
Thanks

[michalvavrik]

I don't mind rewriting this at all, I'll do it if you insist. IMO will not gain anything. What you see is in this PR is internal Quarkus Security code and following operations needs to be done in one way or another:

• we need to augment security identity (hence the augmentor)
• we need add permission checker in that augmentor

We can drop QuarkusPermissionSecurityIdentityAugmentor and QuarkusPermission and call it differently, but nothing will change from the perspective of what the code does. This PR could had been avoided 100 % if the SecurityIdentity is taken from Arc, but that has following downsides:

• it requires more operations (call Arc, whatver it does inside and the identity is proxy itself)
• I think it could be a blocking operation at some scenarios, we can improve that to avoid it (because obviously we will have the identity for authorization, but ATM that can be a case)

QuarkusPermission be an actual custom permission that will be passed to possessed permissions ?

I think QuarkusPermission is actual required permission and if it is passed to a possessed permission, you need:

• add new possessed permission in augmentor (that we already do)
• get the identity from somewhere (probably arc, see above my comments)

So I respect that changes in this PR can look bad, we can avoid such exceptions if it needs to be done.