Regression v2.0.2.Final: RolesAllowed check skipped when endpoint method payload POJO class is private

[lwitkowski]

Describe the bug

@RolesAllowed annotation is ignored for endpoints, which methods have strongly-typed POJO as parameter, and if POJO class is inner and private.
Quarkus v2.0.1.Final is the last version where it works correctly, staring from v2.0.2.Final it works as described.

Expected behavior

REST service should return http 401/403 and endpoint method/implementation should not be called.

Actual behavior

Endpoint method/implementation is executed and response is sent to the client despite missing JWT/role.

How to Reproduce?

Given

Quarkus: v2.0.2.Final or newer
Extensions: cdi, oidc, resteasy, resteasy-jackson, security, smallrye-context-propagation
POST endpoint accepting payload as strongly-typed POJO private class, with @RolesAllowed("some-group") annotation

When

When endpoint is requested without valid JWT or when 'groups' claim does not contain required group (role)

Sample code

Minimal reproducer project: https://github.com/lwitkowski/quarkus-role-bug/
Github Actions: https://github.com/lwitkowski/quarkus-role-bug/actions/runs/1090923795

[geoand]

cc @sberyozkin

[quarkus-bot]

/cc @sberyozkin

[sberyozkin]

@lwitkowski Hi, I'm surprised it even works at the JAX-RS level given that the parameter class is private static; can you please try the latest Quarkus release ?
@stuartwdouglas - does it ring any bell to you why a RolesAllowed check is skipped for a method like this one, https://github.com/lwitkowski/quarkus-role-bug/blob/main/src/main/java/quarkus/HelloResource.java#L19 ?

[sberyozkin]

@lwitkowski The logs in your tests show:

Body:
{
    "key": 1
}

HTTP/1.1 200 OK
Content-Length: 1
Content-Type: application/json
key from payload: 0

The returned key value is 0 - which implies that something is going wrong at the deserialization level - as the value of 1 is expected. As I said I'm not sure it is a valid JAX-RS setup

[lwitkowski]

Thanks for looking into this @sberyozkin, you're right about log message showing 0 instead of 1, it was caused by package private modifier for key field, but it doesn't change anything regarding RolesAllowed check.

I've just committed some fixes: lwitkowski/quarkus-role-bug@b3d6cb4

1. Test run with the latest quarkus (2.1.2.Final) - still red: https://github.com/lwitkowski/quarkus-role-bug/actions/runs/1138800044
2. I fixed access modifier in payload DTO - it logs input key value correctly now

Regarding private static for DTO - as far as I know it doesn't work at all with jsonb (it cannot create class instance), but it works fine with jackson with all Quarkus versions, even the latest one (I'm now referring to deserialization itself, not RolesAllowed issue). I personally don't see any reason not to use private static DTOs - the more private the code the better in general :)

[sberyozkin]

@lwitkowski Thanks, sure, it is just that the 3rd party message body readers/writers need to access this private static class - Jackson-based ones can drill into it, others can't (such as jsonb ones for ex - and there is no requirement for them to be able to do so).
My guess at this stage is that the roles allowed checks are skipped now due to the visibility problems of such classes.

[lwitkowski]

@sberyozkin Yes, I also suspect some try {} catch() { //nothing to be done here } somewhere in role-checking code, but I think we both agree it would be better to return http 500 in this case, than to allow executing potentially dangerous operation and returning http 2XX?

[stuartwdouglas]

I think we should probably explicitly disallow this. AFAIK interceptors can't be applied to these methods, as they basically have the same semantics as private methods.

Even if we solve this issue it will likely just cause other problems down the road (e.g. things like @transactional likely won't work, as interceptors can't be applied).

[sberyozkin]

@stuartwdouglas Hey Stuart - can you please look into disabling it/failing the build if the body entity is private ? I've just had about a 1 day panic attack with #21571 :-), and it proved it was the same problem, if it is too hard to enforce then I can add a big warning to the docs ?