quarkus icon indicating copy to clipboard operation
quarkus copied to clipboard

Unable to Overwrite Response using Security-JPA & RestEasy-Classic

Open BrainShit opened this issue 3 years ago • 1 comments

Describe the bug

I'm currently unable to overwrite the Response with a CustomFormAuthenticationMechanism when using security-jpa with quarkus using RestEasy-Classic. I did not test ReastEasy-Reactive.

The last known version this was possible was using 2.7.5.Final.

If there's anything else you need let me know!

Here's my Implementation of the HttpAuthenticationMechanism

@Alternative
@Priority(1)
@ApplicationScoped
public class CustomFormAuthenticationMechanism implements HttpAuthenticationMechanism {

    private static final Logger LOGGER = LoggerFactory.getLogger(CustomFormAuthenticationMechanism.class);

    @Inject
    FormAuthenticationMechanism delegate;

    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        return delegate.authenticate(context, identityProviderManager);
    }

    @Override
    public Uni<ChallengeData> getChallenge(RoutingContext context) {
        Uni<ChallengeData> rValue = delegate.getChallenge(context);
        return rValue.onItem().transform(challengeData -> {
            if (challengeData.status == Response.Status.FOUND.getStatusCode()) {
                return new ChallengeData(Response.Status.UNAUTHORIZED.getStatusCode(), challengeData.headerName, challengeData.headerContent);
            }
            return challengeData;
        });
    }

    @Override
    public Set<Class<? extends AuthenticationRequest>> getCredentialTypes() {
        return delegate.getCredentialTypes();
    }

    @Override
    public HttpCredentialTransport getCredentialTransport() {
        return delegate.getCredentialTransport();
    }

    @Override
    public Uni<HttpCredentialTransport> getCredentialTransport(RoutingContext context) {
        return delegate.getCredentialTransport(context);
    }

    @Override
    public Uni<Boolean> sendChallenge(RoutingContext context) {
        return getChallenge(context).map(new ChallengeSender(context));
    }
}

Expected behavior

Changed Challenge Data with changed Response should be returned resulting in a 401 Response

Actual behavior

Default Response (302) is returned

How to Reproduce?

Reproducer: https://github.com/BrainShit/custom-formauth-reproducer

  1. Start the Application
  2. curl the security endpoint with user "admin" and a wrong password

Output of uname -a or ver

No response

Output of java -version

11.0.8 2020-07-14

GraalVM version (if different from Java)

No response

Quarkus version or git rev

2.11.2.Final

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

BrainShit avatar Aug 08 '22 12:08 BrainShit

/cc @sberyozkin

quarkus-bot[bot] avatar Aug 08 '22 12:08 quarkus-bot[bot]

Is there any update on this? As this is preventing me from updating further than 2.7.5

BrainShit avatar Sep 09 '22 06:09 BrainShit

Hello, I can reproduce it. I'll have a look and let you know.

michalvavrik avatar Sep 12 '22 18:09 michalvavrik

Hi @sberyozkin , when you are using custom HttpAuthenticationMechanism and call one of:

  • io.quarkus.vertx.http.runtime.security.FormAuthenticationMechanism
  • io.quarkus.smallrye.jwt.runtime.auth.JWTAuthMechanism
  • io.quarkus.oidc.runtime.AbstractOidcAuthenticationMechanism
  • io.quarkus.security.webauthn.WebAuthnAuthenticationMechanism
  • io.quarkus.vertx.http.runtime.security.BasicAuthenticationMechanism
  • io.quarkus.vertx.http.runtime.security.MtlsAuthenticationMechanism
  • has path specific mechanism

from inside of your methods (see issue description above, the very same is done in docs here https://quarkus.io/guides/security-customization#dealing-with-more-than-one-httpauthenticationmechanism), then custom auth mechanism method Uni<ChallengeData> getChallenge(RoutingContext context) won't ever get called as they set context.put(HttpAuthenticationMechanism.class.getName(), this); with themselves. You can find the problematic line here https://github.com/quarkusio/quarkus/blob/647dfdc13ffc76a65f2980a120ec0878cf07bfd6/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/security/HttpAuthenticator.java#L140

I understand that your intention here https://github.com/quarkusio/quarkus/pull/16446 was to do right challenge, not all the challenges. I also realize @BrainShit can do this

    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        var authenticated = delegate.authenticate(context, identityProviderManager);
        context.put(HttpAuthenticationMechanism.class.getName(), this);
        return authenticated;
    }

or

    @Override
    public Uni<SecurityIdentity> authenticate(RoutingContext context, IdentityProviderManager identityProviderManager) {
        var authenticated = delegate.authenticate(context, identityProviderManager);
        context.put(HttpAuthenticationMechanism.class.getName(), null);
        return authenticated;
    }

and reproducer is fixed!

From here, these are just my opinions, not facts:

  • it should be better documented, the docs shows very similar example to the issue reproducer and from docs example, you would expect getChallenge to get called
  • setting context.put(HttpAuthenticationMechanism.class.getName(), this); looks like a workaround to me. Are we really expecting custom auth mechanisms to put themselves into the context? (I mean, there should be contract that says do this and xyz happens)

I want to fix the issue (unless you think there is none :-)), but I need to hear what is expected behavior - can you specify algorithm how correct getChallenge is selected?

Thanks a lot!

michalvavrik avatar Sep 12 '22 21:09 michalvavrik

@sberyozkin I re-read docs now and following paragraph

In some cases such a default logic of selecting the challenge is exactly what is required by a given application, but sometimes it may not meet the requirements. In such cases (or indeed in other similar cases where you'd like to change the order in which the mechanisms are asked to handle the current authentication or challenge request), you can create a custom mechanism and choose which mechanism should create a challenge, for example:

suggests to use an example that won't work as I described above (please see my previous comment), because JwtAuthMechanism sets itself into RouteContext and thus CustomAwareJWTAuthMechanism#getChallenge is not called.

I'd prefer to hear from you once you have time for this (no hurry) as it's not explained why 6 mechanisms I listed above are preferred to a custom mechanism. I read the PRs that submitted these changes and it's not clear, but I can run a few tests to find out.

michalvavrik avatar Sep 13 '22 20:09 michalvavrik

https://github.com/quarkusio/quarkus/issues/22206 https://github.com/quarkusio/quarkus/pull/22483 https://github.com/quarkusio/quarkus/issues/22404 gave me an idea, I'll try to figure it out on my own.

Originally HttpAuthenticationMechanism.getCredentialTransport was only used to fail when more than one auth mechanism with the same cred transport is used. Now it is also used to find the best candidate for a challenge (for ex, if basic and oidc/web-app are used and basic fails then it is guaranteed basic will return the challenge, even without the priority based sorting). It is also now used for a path-specific authentication: https://quarkus.io/guides/security#path-specific-authentication-mechanism.

HttpAuthenticationMechanism.getCredentialTransport is only called for path specific mechanism. Maybe an easy fix is to just add the mechanism into context when it's part of the path auth mechanism.

michalvavrik avatar Sep 13 '22 21:09 michalvavrik

@michalvavrik Apologies, seeing/reading your analysis only now as I've noticed your PR...

sberyozkin avatar Sep 14 '22 10:09 sberyozkin

np

michalvavrik avatar Sep 14 '22 10:09 michalvavrik