Improve Security documentation

[MarcusBiel]

The security documentation should be made CRYSTAL clear in the sense that - you must never use @authenticated and
@RolesAllowed together - even better, if such combined usage of Annotations would break at compile time.

The security documentation actually says it - but it's easy to miss this part (or it's strong implications) I think:

Quarkus comes with built-in security to allow for Role-Based Access Control (RBAC) based on the common security annotations @RolesAllowed, @Denyall, @permitAll on REST endpoints and CDI beans. An example of an endpoint that makes use of both JAX-RS and Common Security annotations to describe and secure its endpoints is given in SubjectExposingResource Example. Quarkus also provides the io.quarkus.security.Authenticated annotation that will permit any authenticated user to access the resource (equivalent to @RolesAllowed("**")).

I found this as a bug in my code through integration testing. All my ADMIN endpoints where available open to any kind of authenticated user.

[sberyozkin]

@MarcusBiel Hi, I'd actually consider it a bug given that the method specific RolesAllowed does not override it.
For example, given

@PermitAll
public class Resource {
    @GET getForAll() {}

    @RolesAllowed("admin")
    @GET getForAdmin() {}
}

getForAdmin can only be invoked by admin.

It is really inconsistent if we have getAdmin open for all in

@Authenticated
public class Resource {
    @GET getForAll() {}

    @RolesAllowed("admin")
    @GET getForAdmin() {}
}

@stuartwdouglas Hi Stuart, do you agree ? IMHO we have to fix it...Thanks

[sberyozkin]

I'm not 100% sure, given this RolesAllowed text, if a method specific role like admin overrides a wildcard, but it does feel counterintuitive that the getForAdmin is totally opened in the last example.

[stuartwdouglas]

@MarcusBiel Can you provide a reproducer? I just checked and we do reject this configuration, you should get an error like:

Caused by: io.quarkus.builder.BuildException: Build failure: Build failed due to errors [error]: Build step io.quarkus.arc.deployment.ArcProcessor#registerBeans threw an exception: java.lang.IllegalStateException: Method admin of class io.quarkus.resteasy.test.security.RolesAllowedResource is annotated with multiple security annotations

[sberyozkin]

Hi Stuart, should we instead follow a method-level annotation overrides the class-level annotation approach ? It works for JAX-RS (ex, method level Produces/Consumes overrides class level Produces/Consumes). Looks like it should be safe enough given the error you are seeing (i.e, right now no users should have a class-level wildcard and a method specific non-wildcard RolesAllowed - which is a bit ambiguous since the wildcard covers all - so we just say - method level wins - I think it definitely wins in case of class level Deny vs method level RolesAllowed, so why not ?)

[stuartwdouglas]

What error are we actually seeing? Method level annotations definitely override class level ones, and this is tested, so I really need to see a reproducer to see what the actual problem is.

[MarcusBiel]

@stuartwdouglas sure, I will provide a reproducer if it helps you - but I don't think it will even be necessary. I had both annotations on class level, not method level. My assumption was - @ Authenticated says that this Resource is authenticated, and @ RolesAllowed says who actually may authenticate. Of course, the actual error is on my side - but I find those annotations rather confusing - same as @ PermitAll.

@Path("/api/admin") @Authenticated @RolesAllowed("Admin") @Produces(MediaType.APPLICATION_JSON) @Consumes(MediaType.APPLICATION_JSON) public class MyAdminResource {

[sberyozkin]

Interesting, @Authenticated @RolesAllowed("Admin") on the class level...

OK, here is how I've been thinking about Authenticated. if I have:
(1)

@Authenticated
public class Resource {
    @GET get() {}
}

then what I want here is to ensure that only the authenticated users can access this resource. True, it is equivalent to setting @PermitAll or @RolesAllowed("**") but I'd rather have no associations with RBAC at all here, as long as the token or whatever is valid the resource is open.

Now if I have
(2)

@Authenticated
public class Resource {
    @RolesAllowed("admin")
    @GET getForAdmin() {}
}

Then yes, I intuitively want to say, getForAdmin is only for the users with the admin role. Authenticated is not really needed here because for the RBAC to work the user has to be authenticated, but it does not harm having it there. I'd definitely expect the last example to be supported. Therefore, IMHO,

(3)

@Authenticated  @RolesAllowed("admin")
public class Resource {
   @GET getForAdmin() {}
}

Should work too, it is the same as (2). Having @Authenticated should not be about RBAC IMHO.

I'd only expect an error if I had

(4)

@@RolesAllowed("**")  @RolesAllowed("admin")
public class Resource {
   @GET getForAdmin() {}
}

[sberyozkin]

I think if we just drop (equivalent to @RolesAllowed("**") from Authenticated docs and just let users add this annotation whenever they feel like it will avoid all the confusions and will just work :-)

[stuartwdouglas]

Ah, I only tested duplication at the method level, looks like there is a bug in the code that detects duplication at the class level, I have opened a PR.

[khoek]

(Sorry to dig up this issue:) Where is the meaning of the wildcard notation in @RolesAllowed("**") documented? Is this Quarkus-specific or part of the standard? If the latter, would anyone be able to provide a link?