Ensure augmented SecurityIdentity is used in SecurityEvents and move configuration-based roles-mapping to authentication phase

[michalvavrik]

#39563 pre-step. Changes also make sense standalone.

• uses augmented identity added to the authZ failure events instead of original one (IMO original behavior was a bug, we need to use the one used for the authZ)
• if identity got augmented during authorization (HTTP Permissions can augment the identity), we add event property that signal it happened - this provides additional information for those interested in later SecurityIdentity changes
• makes identity augmentation via quarkus.http.auth.roles-mapping part of the authentication process, so that when the authenticatione event is fired, the identity is already augmented
• in order to do above-mentioned refactors authentication handler, which allows to drop synchronized method and reduce (runtime) logic inside authentication handler

[michalvavrik]

I have rebased this PR on current main and resolved merge conflicts. No additional changes.

[sberyozkin]

@michalvavrik You mention this PR is a prep for #39563, it does like another well done PR.

What I'm not sure about is that the augmentation events will be sen after HTTP policy related augmentations (HttpSecurityPolicy is mainly about authorization checks and not the augmentation) and there could be a few of these ones. But there will be nothing related to the augmentation reported when one or more SecurityIdentityAugmentor whose job is to augment are run.
So we have a single AuthenticationSuccessEvent fired when an initial identity is created (which may involve several SecurityIdentityAugmentor steps). And now we go into a fine-grained sequence of firing AuthorizationSuccessEvents with the augmentation status if HttpSecurityPolicy is used.
I understand that you want to cover every possible case where the identity can be augmented to have a very precise OTel view.
But if we put OTel aside for a second, we have a sequence of events that only cover the post authentication step, after the typical augmentation has already been done by SecurityIdentityAugmentor.

So I have a few questions, these are only meant to discuss things further before finalizing this PR:

• Do we really need to have a fine grained sequence of augmentation events related to the HTTP policy work, which requires tracing all these places in the code where it can happen ? I'm wondering how useful it can be, instead of always getting only a single authorization success event, may be I'm wrong...
• If you think we do, what can we tell to users when they ask - I see this Authorization success event when HttpSecurityPolicy has augmented the identity, but nothing is reported if my SecurityIdentityAugmentor did it ? I feel that if do go ahead with reporting the incremental identity updates, then SecurityIdentityAugmentor should be part of the picture.

Thoughts ?

Thanks

[michalvavrik]

I appreciate your time.

Do we really need to have a fine grained sequence of augmentation events related to the HTTP policy work, which requires tracing all these places in the code where it can happen ?

No. There is alternative way that I didn't mention before because I did not anticipate resistance. I simply belived that I am was adding a useful information. What can be done is to update Span on every single authorization event as long the SecurityIdentity present in the events is the correct one. It is extra processing, but WTH.

Full disclosure: by correct SecurityIdentity I mean the augmented identity because IMO current situation is a bug. We should append the latest identity which is the augmented one.

I'm wondering how useful it can be, instead of always getting only a single authorization success event, may be I'm wrong...

This PR adds one property into the events that are going to be fired anyway. I suppose that users can do what I will do and just compare previous and current state if they have a similar use case as I do. I can drop it as explained before. Apologies I did not mention it before (for reasons also explained above).

If you think we do, what can we tell to users when they ask - I see this Authorization success event when HttpSecurityPolicy has augmented the identity, but nothing is reported if my SecurityIdentityAugmentor did it ? I feel that if do go ahead with reporting the incremental identity updates, then SecurityIdentityAugmentor should be part of the picture.

@sberyozkin I propose I will drop SECURITY_IDENTITY_AUGMENTED but I'll leave the rest of this PR, namely:

• refactoring of the authorization handler which avoids synchronized block (but is done simply because I needed to move Roles Mapping, it's not arbitrary refactoring)
• moving standalone RolesMapping to the authentication phase because I believe it belongs there, there is no authorization present
  • this is important for OTel because without this, the identity augmented by RolesMapping wouldn't be present in any event if there is no authorization, only authentication
• SecurityEvent#getSecurityIdentity will return the latest identity, that is augmented one even in case of the authorization failure

Is that acceptable, please?

[michalvavrik]

I went ahead and just dropped the attribute.

[sberyozkin]

@michalvavrik Thanks, for the update,

No. There is alternative way that I didn't mention before because I did not anticipate resistance.

It is not a resistance, just an attempt to make sense of it as a reviewer, and see if something can be made more consistent, etc.

Let me have a look at your recent updates shortly

[sberyozkin]

@michalvavrik The PR in the current form looks much more straightforward and easier to understand,

SecurityEvent#getSecurityIdentity will return the latest identity, that is augmented one even in case of the authorization failure

Yes, IMHO, users won't be particularly interested to trace every incremental identity update but the end result - if the final security identity is authorized. I may prove to be wrong but then we can start fine-tuning it if some real demand arises for it.

Thanks

[sberyozkin]

@michalvavrik Can you please rebase this PR, as the last build run 2 weeks back ?

[michalvavrik]

It is not a resistance, just an attempt to make sense of it as a reviewer, and see if something can be made more consistent, etc.

No complains from my side Sergey. Thank you for the review.

[michalvavrik]

I've rebased the PR and re-checked all the changes (as I was bit short on details now), and changes still lgtm. Let's see how CI goes. Thanks

[quarkus-bot]

Status for workflow Quarkus CI

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

✅ 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.

---

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

• Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)