quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.56k stars 2.62k forks source link

Custom Exception in SecurityIdentityAugmentor #39155

Open PierreAdam opened 6 months ago

PierreAdam commented 6 months ago

Describe the bug

I'm currently making an implementation of SecurityIdentityAugmentor that extracts data from the headers and use it to validate that the user currently logged has access to the ressource. If something goes wrong in analyzing the header value such as a wrong value, I want to throw a custom exception ResourceNotFoundException and send a 404 not found since the ressource being asked is not valid.

It kinda looks like this

@ApplicationScoped
public class AccountRoleAugmentor implements SecurityIdentityAugmentor {

    @Override
    public Uni<SecurityIdentity> augment(@NonNull final SecurityIdentity securityIdentity,
                                         @NonNull final AuthenticationRequestContext context) {
        // Get your user from DB, add roles etc.

        if (whatever) {
            throw new ResourceNotFoundException();
        }

        // ...
    }
}
public class ResourceNotFoundException extends RuntimeException {
    // All default exception constructors
}
@Provider
public class ResourceNotFoundExceptionMapper implements ExceptionMapper<ResourceNotFoundException> {

    @Override
    public Response toResponse(final ResourceNotFoundException exception) {
        // ResourceNotFoundExceptionMapper.logger.error("Resource not found from ExceptionMapper", exception);
        return Response.status(Response.Status.NOT_FOUND).build();
    }
}

It looks like the exception is always causing a 500 internal server error instead of going through the ExceptionMapper.

For information, when I'm trying to throw the same exception from a controller, it's working just fine and I get my 404 without any issues.

Expected behavior

I would except my custom exception to behave in the same way as exceptions such as io.quarkus.security.ForbiddenException but with my custom exception.

Actual behavior

An error 500 is always sent to the client despite the ExceptionMapper.

How to Reproduce?

  1. Create a SecurityIdentityAugmentor
  2. Create a custom exception with the proper ExceptionMapper
  3. Throw a custom exception
  4. See an error 500 instead of going through the ExceptionMapper

Output of uname -a or ver

Windows

Output of java -version

OpenJDK 21

Quarkus version or git rev

3.8.1

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

Maven 3.9.5

Additional information

No response

quarkus-bot[bot] commented 6 months ago

/cc @sberyozkin (security)

sberyozkin commented 6 months ago

@PierreAdam I can suggest a couple of things to try. First of all, by default, the security chain runs before the JAX-RS chain, so throwing the exception will bypass the JAX-RS exception mapping. So try to disable the proactive authentication: https://quarkus.io/guides/security-proactive-authentication. And also, instead of throwing the exception directly, do return Uni.createFrom().failure(new ResourceNotFoundException()); instead.

CC @michalvavrik

PierreAdam commented 6 months ago

Hello @sberyozkin, thank you for your quick answer ! I did some tests and it seems like return Uni.createFrom().failure(new ResourceNotFoundException()); is sadly not working when quarkus.http.auth.proactive=true.

However, if I disable the proactive authentication quarkus.http.auth.proactive=false, it is indeed working. Both by throwing or by using the Uni.createFrom().failure(...). It is however a bit weird that this behavior would change depending on the proactive authentication being enabled or now.

Also noteworthy, it seems that the behavior after disabling the proactive authentication when throwing an exception is also working when calling context.runBlocking(() -> ...)

I think this kind of half solves the issue but forcing the proactive authentication off for only this purpose seems a bit weird to me.

michalvavrik commented 6 months ago

+1 to everything @sberyozkin suggested, that's a right way to achieve your goals, just make sure no security

Hello @sberyozkin, thank you for your quick answer ! I did some tests and it seems like return Uni.createFrom().failure(new ResourceNotFoundException()); is sadly not working.

with disabled proactive auth and no HTTP perms defined should work

It is however a bit weird that this behavior would change depending on the proactive authentication being enabled or now.

that's implementation detail :-)

Also noteworthy, it seems that the behavior after disabling the proactive authentication when throwing an exception is also working when calling context.runBlocking(() -> ...)

I don't get this comment, can you elaborate what you mean?

I would except my custom exception to behave in the same way as exceptions such as io.quarkus.security.ForbiddenException but with my custom exception.

We were being careful and we only propagate security exceptions because there could be quite a noise. Not just your exception would be propagated, but any other exception as well and RESTEasy Reactive started just to invoke exception mappers. I'm not sure it's a good idea.

I think this kind of half solves the issue but forcing the proactive authentication off for only this purpose seems a bit weird to me.

I'm honestly not sure we should solve this, but it is possible. Anyway, you have plenty of other options, like you can use HTTP security policy and just end with 404 or throw that exception and define Vert.x failure handler.

michalvavrik commented 6 months ago

BTW there are security and performance concerns if we enable this for other exceptions as there will be more processing that does not belong to RR

michalvavrik commented 6 months ago

One else thing again

It is however a bit weird that this behavior would change depending on the proactive authentication being enabled or now.

I think it is a good think that we run security before anything else and if there is an issue for which we don't know that we need more processing, we end request promptly

michalvavrik commented 6 months ago

btw I'd expect that inside augmentor the security exception is thrown. you can also throw security exception, define exception mapper and if exception reason matches, you return 404, if not, you delegate to standard exception mapper?

PierreAdam commented 6 months ago

Also noteworthy, it seems that the behavior after disabling the proactive authentication when throwing an exception is also working when calling context.runBlocking(() -> ...)

I don't get this comment, can you elaborate what you mean?

I just wanted to point out that with quarkus.http.auth.proactive=false you can do :

    public Uni<SecurityIdentity> augment(@NonNull final SecurityIdentity securityIdentity,
                                         @NonNull final AuthenticationRequestContext context) {
        return Uni.createFrom().failure(new ResourceNotFoundException());
        // OR
        throw new ResourceNotFoundException();
        // OR
        return context.runBlocking(() -> {
            throw new ResourceNotFoundException();
        });
    }

I think it is a good think that we run security before anything else and if there is an issue for which we don't know that we need more processing, we end request promptly

You are probably right on this part. I also tried having own exception extending SecurityException to see if it would change the behavior but it does not.

btw I'd expect that inside augmentor the security exception is thrown. you can also throw security exception, define exception mapper and if exception reason matches, you return 404, if not, you delegate to standard exception mapper?

This is actually an excellent point ! That kinda fully solve my problem using a trick of course. However, wouldn't it be nice to not have to "Override" ForbiddenExceptionMapper or UnauthorizedExceptionMapper and instead have an other ExceptionMapper for all the other exceptions that would extend SecurityException ?

PierreAdam commented 6 months ago

After some tests, the most elegant solution is as follow :

public class ResourceNotFoundException extends AuthenticationCompletionException {
    // Standard constructor
}
@Provider
public class CustomAuthenticationCompletionExceptionMapper implements ExceptionMapper<AuthenticationCompletionException> {

    private final AuthenticationCompletionExceptionMapper originalMapper;

    public CustomAuthenticationCompletionExceptionMapper() {
        this.originalMapper = new AuthenticationCompletionExceptionMapper();
    }

    @Override
    public Response toResponse(final AuthenticationCompletionException exception) {
        if (exception instanceof ResourceNotFoundException) {
            return Response.status(Response.Status.NOT_FOUND).build();
        }

        return this.originalMapper.toResponse(exception);
    }
}

Even with quarkus.http.auth.proactive=true this is working. However, I would really suggest that as a final user we have a specific exception such as AuthenticationUserException that we can extend in order to produce this behavior instead of having to override one of the pre-existing exceptions.

In the SecurityIdentifyAugmentor then, a simple throw new ResourceNotFoundException(); anywhere is able to be properly thrown.

So I think it would be more of an improvement to be able to have an exception that would be "user based" to allow this sort of things 😄

michalvavrik commented 6 months ago

This is actually an excellent point ! That kinda fully solve my problem using a trick of course. However, wouldn't it be nice to not have to "Override" ForbiddenExceptionMapper or UnauthorizedExceptionMapper and instead have an other ExceptionMapper for all the other exceptions that would extend SecurityException ?

Alternatively, you can have CustomException implements io.quarkus.security.AuthenticationException and define exception mapper just for CustomException. Would that help (and work? it has been while since I looked into that)?

You are probably right on this part. I also tried having own exception extending SecurityException to see if it would change the behavior but it does not.

understood

I just wanted to point out that with quarkus.http.auth.proactive=false you can do

got it, but as you know, blocking is a bad thing :-) I'd go with option number 1.

So I think it would be more of an improvement to be able to have an exception that would be "user based" to allow this sort of things 😄

let's see how my suggestion with the io.quarkus.security.AuthenticationException go and if it works for you, we can document it.

sberyozkin commented 6 months ago

@PierreAdam Hi, as I said, the security chain runs earlier by default, the Quarkus Security flow is Quarkus specific and it runs before JAX-RS by default, for the JAX-RS exception mapping to be effective one just needs to delay the security check until it is known the security is in fact required at a given JAX-RS resource, this is what disabling the proactive authentication is about, it does not reduce the security properties of the application.

I believe now that you have it working, we can close this issue, right ?

michalvavrik commented 6 months ago

I believe now that you have it working, we can close this issue, right ?

hey @sberyozkin , I believe that having documented CustomException implements io.quarkus.security.AuthenticationException => custom exception mapper is valuable. It's much better than to answer questions or having opened issues and what this issue describes is valid scenario. Let me wait for confirmation it worked and I'll use this issue as documentation one (sometimes this week). I'll add a test if it can be done easily. Thanks

PierreAdam commented 6 months ago

@michalvavrik seems like you're absolutely right about AuthenticationException. This is even better !
The last version is as follow

public class CustomException extends RuntimeException implements AuthenticationException {
    // Constructor etc.
}
@Provider
public class CustomExceptionMapper implements ExceptionMapper<CustomException> {

    @Override
    public Response toResponse(final CustomExceptionexception) {
        // Process your exception here.
        return Response.status(Response.Status.NOT_FOUND).build();
    }
}

Thank you very much for the information absolutely worth documenting in my mind 😄 Not closing the ticket just cause there is documentation update to do but for me it's fully solved. Have a great day guys !