Add AuthenticationRequest attributes to AuthenticationRequestContext so that users can access RoutingContext

[michalvavrik]

We add RoutingContext to authentication request attributes

https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpSecurityUtils.java#L14

which makes it possible for users to access RoutingContext when proactive auth is enabled. There are issues opened related to RoutingContext in augmentor

• currentVertxRequest.getCurrent in a SecurityIdentityAugmentor became null since 3.2.9 with GraphQL quarkus#37457
• Add RoutingContext to SecurityIdentity for mTLS authentication quarkus#28326
• we have seen other requests to make RoutingContext accessible from the SecurityIdentityAugmentor like in Support for AuthenticationRequest enrichment / agumentation quarkus#16861
• (headers -> RoutingContext) Reactive RestClient used within the Security layer of Quarkus times out and doesn't propagate headers in reactive HeadersFactory quarkus#37945

I've tried to explore passing the context via duplicated context local data quarkusio/quarkus#37795 but from comments you can see there are performance arguments against it. We already have this map with the RoutingContext, so now I propose to add authentication attributes to the authentication request context.

[michalvavrik]

@sberyozkin I can't request review from you, can you have a look please?

[michalvavrik]

@stuartwdouglas not sure if you wanna have a look and have a time?

[michalvavrik]

Point here is that we already have a map with the context

https://github.com/quarkusio/quarkus/blob/4d07a919719e7bb50ecbc76501eb9b55559c711f/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/QuarkusIdentityProviderManagerImpl.java#L122

and users are trying to active CDI request context too early, so why don't give them chance to use the RoutingContext? We just pass the existing map without any modifications down, that is it.

[michalvavrik]

I've changed my original proposal after talking to @sberyozkin . My original proposal was that we should add to SecurityIdentityAugmentor new method that will have authentication request attributes we already have (no creating of objects) and for those who don't need to the attributes, they could still use the original method:

default Uni<SecurityIdentity> augment(SecurityIdentity identity, AuthenticationRequestContext context, Map<String, Object> attributes) {
        return augment(identity, context);
    }

and I'd drop deprecation from my first commit, but Sergey raised the point that changing interface can be avoided by creating new authentication request context for every authentication request, so I accepted his proposal (it's normal we have different preferences :-), I liked mine).

I'll adjust PR description, title and maybe we can move on this.

[sberyozkin]

Hi @michalvavrik Thanks, sure, I'd still appreciate @stuartwdouglas commenting, only Stuart can merge it anyway.

Maybe your original proposal will win, today, now that I think about it after seeing both proposals, it looks OK, my only concern there is that the deprecated method will likely stay forever. But like you said, no context objects will be created during the augmentation - though an extra object will be allocated only if the augmentors are registered, which will not always be the case.

Can you please create a Quarkus branch based on the current PR for @stuartwdouglas to have a look ? I'll have no problems with your original PR code restored if this is what would be preferred.
Please look into it in early 2024 :-) and sorry if the current PR will end up to be reverted to your original idea in the end.

Thanks

[sberyozkin]

@stuartwdouglas Please comment

[michalvavrik]

Hi,

this is how Quarkus changes will look like (I'll write docs when there is final version of this PR) with current version of this PR: quarkusio/quarkus@3b741aa

Part that I consider strange is that during authentication (inside io.quarkus.security.identity.IdentityProvider#authenticate) same authentication attributes will be available both from AuthenticationRequest and from AuthenticationRequestContext.

Here is my preference:

• Quarkus Security changes (without deprecation) d822e01
• Quarkus changes quarkusio/quarkus@82714ad

[sberyozkin]

Thanks @michalvavrik.
Unless an object creation has any real measurable impact, I'd go with this option, not because I suggested it, but because AuthenticationRequestContext seems like a good enough container for all things related to the request context (more or less the same a RoutingContext itself), even though right now, it is only positioned as a blocking context provider.
Otherwise, lets indeed do the original approach, I'll support it too.
Let @stuartwdouglas decide 👍

Thanks

[sberyozkin]

@michalvavrik

Part that I consider strange is that during authentication (inside io.quarkus.security.identity.IdentityProvider#authenticate) same authentication attributes will be available both from AuthenticationRequest and from AuthenticationRequestContext.

Hmm, yeah, that is not quite cool. I suppose it favors your original PR then 👍

[michalvavrik]

Let's hear third opinion, I think both proposals are acceptable. It can wait for Stuart. Thanks for your comments.

[sberyozkin]

I'm now leaning toward your original proposal because having an ambiguity in the way these properties can be accepted in my proposal is not good, thanks. Hope Stuart will agree

[michalvavrik]

I'm now leaning toward your original proposal because having an ambiguity in the way these properties can be accepted in my proposal is not good, thanks. Hope Stuart will agree

in that case I switched to my original proposal but without deprecation, let's move on this one

[michalvavrik]

Thank you for review. I've pinged @gsmet to make release when he finds a time.