Support for AuthenticationRequest enrichment / agumentation

[renegrob]

Description

With https://github.com/quarkusio/quarkus-security/pull/14 there is a way to pass arbitrary attributes to the IdentityProvider. However if you want to pass additional attributes to the IdentityProvider you still need to override (and extend(!)) the corresponding HttpAuthenticationMechanism. To do so you might need to override the entire authenticate()method and duplicate its code. While I agree that the IdentityProvider should generally not be aware of the HTTP request and extracting attributes from the request should be the responsibility of the HttpAuthenticationMechanism, I think it shouldn't be necessary to duplicate the HttpAuthenticationMechanism to achieve that. I suggest to do this with an Augmentor similar to SecurityIdentityAugmentor. While QuarkusIdentityProviderManagerImpl would be the perfect place in the call chain to do the augmentation, we don't have access to the Vertx request there. Also the IdentityProviderManager should work for non-http requests too.

Implementation ideas

One idea would be to have an HttpAwareIdentityProvidermanager which is just a thin wrapper around QuarkusIdentityProviderManagerImpl:

public interface HttpAwareIdentityProvidermanager {

    Uni<SecurityIdentity> authenticate(RoutingContext context, AuthenticationRequest request);

    SecurityIdentity authenticateBlocking(RoutingContext context, AuthenticationRequest request);
}

or something more generic like ContextAwareIdentityProviderManager:

public interface ContextAwareIdentityProviderManager<C> {

    Uni<SecurityIdentity> authenticate(C context, AuthenticationRequest request);

    SecurityIdentity authenticateBlocking(C context, AuthenticationRequest request);
}

The AuthenticationRequestAugmentor interface could look like this:

public interface AuthenticationRequestAugmentor<T extends AuthenticationRequest> {

    default int priority() {
        return 0;
    }

    Class<T> getRequestType();

    Uni<T> augment(T request);
}

Then the interface of HttpAuthenticationMechanism would have to be changed from:

    Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager);

to

    Uni<SecurityIdentity> authenticate(RoutingContext context, HttpAwareIdentityProvidermanager identityProviderManager);

-> This would be a breaking change but I don't see any other way to achieve this

[sberyozkin]

Hi @renegrob

Thanks for this proposal. My understanding the main requirement is to pass RoutingContext - as opposed to a general augmentation support for the security request context. I wonder if it can be achieved by updating existing Quarkus HttpAuthenticationMechanisms to set a well-known attribute representing RoutingContext on AuthenticationRequest you introduced ?

thanks

[renegrob]

Hi @sberyozkin From my point of view, the IdentityProvider should not extract HTTP request attributes, else the whole separation of concerns of HttpAuthenticationMechanism and IdentityProvider gets diluted. The only thing HttpAuthenticationMechanism mostly does is extracting the relevant information from the HTTP request. My idea is to extract things like "tenant", "login-domain" etc. in the AuthenticationRequestAugmentor and then provide them as attributes to the IdentityProvider. Like this you can for example control in the if the "tenant" is extracted from a cooke, the url or a header without having multple IdentityProvider implementations or having an extra configuration and logic for it. However maybe it is over-engineered...

[sberyozkin]

@renegrob Sure. If we look at the IdentityProviders shipped with Quarkus - they are either aware or not ware of HTTP. OidcIdentityProvider for ex - it is reused between 2 auth mechanisms (I can imagine we could've avoided introducing it).

So it really comes down to the custom identity providers - ok - I can see you may want to keep your custom provider be used not only with HTTP - makes sense.

But I'm not sure that it will be significantly cheaper to write this augmentor for example twice (say for HTTP and Kafka messages) or have simple interposing mechanisms which can do the same :-). Implementation wise it will be one more (but also breaking) option.

I don't know, lets see what Stuart @stuartwdouglas thinks :-)

thanks

[sberyozkin]

By the way - now that I've mentioned Kafka - AFAIK we don't have any non-HTTP mechanism support (we likely should) - so for now at least, IdentityProviders are indirectly tied to HTTP.

[sberyozkin]

@renegrob I think your idea is interesting - the concrete issue was that a user was looking forward to having RoutingContext injected - so I'll do a separate PR to pass it as an attribute - however having an IdentityProvider portable across different authentication mechanism transports (even if we don't have it yet) is worth supporting IMHO. Perhaps doing it for 2.0 is the best opportunity since it is a breaking change

[stuartwdouglas]

I think the existing mechanisms we have should just include it as an attribute.

[renegrob]

@sberyozkin I had this already in mind when I added the attributes to the AuthenticationRequest. This is not related to the issue with the RoutingContext injection. It is more about customizing the HttpAuthenticationMechanisms without duplicating code. But maybe there's a better way to achieve this or it is something that seldom needs to be done. Since I'm working on a 2FA server project, I have probably a different view than the average user.

[sberyozkin]

@renegrob Sure, I see your point - you started discussing it in the context of the issue opened a few days back so I referred to it - there RoutingContext is needed to do HTTP specific extraction of the request attributes in order to figure out what to do next. In meantime your earlier enhancement already allows to pass it as an attribute but as you referred above - you'd rather your IdentityProviders not dealing with RoutingContext directly - with this concrete proposal making it possibly easier to augment the request while keeping the providers HTTP neutral. So for now I'll take care of updating the existing mechanisms to pass RC around while here we can continue the discussion about this proposal