Add @PermissionChecker annotation that allows to create permission checker methods on CDI beans

[michalvavrik]

Adds @PermissionChecker annotation to make it easier define checkers for the @PermissionsAllowed permissions.

[michalvavrik]

@FroMage @sberyozkin please review (I cannot request review in this project)

[FroMage]

This looks great, much more intuitive to explain.

My only reserve is the stringly-typed name, but unfortunately there's no way to reference methods in annotations, short of creating a type. We can improve on this later if required.

[michalvavrik]

Sure, agreed @FroMage . And thanks for the review.

[sberyozkin]

@michalvavrik

QuakrusPermission that allows to avoid SecurityIdentityAugmentor which is most vocalized downside of current approach and which also makes API hard to understand to users.

I think you are a little bit too hard on what you created earlier. Permission.implies allows for the most complex checks that one can not perform with shortcuts. And I think expecting users to analyze the current security identity in the augmentor and decide what permissions it possesses is not a bad idea at all. Adding permission checkers manually in the augmentor is complex but this is a rather advanced case. May be we should rework the doc examples a bit...

[sberyozkin]

@michalvavrik I mean, what you propose here is super great, just saying that it is not all that bad with what we already have :-)

[michalvavrik]

I've fixed PR title, description, resolved threads and canceled my previous comments because they are not relevant anymore. All that is left is the @PermissionChecker annotation.

[sberyozkin]

Hi Michal, @michalvavrik, IMHO PermissionChecker which you are introducing here is a game changer, and I also would like to have it available in Quarkus asap, I just feel we need to try to look a bit slower at the supporting pieces

[michalvavrik]

Hi Michal, @michalvavrik, IMHO PermissionChecker which you are introducing here is a game changer, and I also would like to have it available in Quarkus asap, I just feel we need to try to look a bit slower at the supporting pieces

I understand there will be 1.5 week break now, then we can continue discussion when you are back.

I'll continue preparing implementation of the @PermissionChecker based on what I have shown before. I believe that implementation of the permission checker as I plan it goes in the right direction. If it turns it isn't, well, that's life.

There is enough time to make it to 3.16.

[sberyozkin]

Michal, I propose rethink the whole idea of requiring adding checkers in the the augmentor, if users really want it they can do it, otherwise let's try to avoid it, see a proposed Quarkus enhancement, I'd suggest try it first before proceeding with the other work, because if it works it can impact how you decide to implement PermissionChecker

Enjoy the weekend, thanks

[michalvavrik]

Michal, I propose rethink the whole idea of requiring adding checkers in the the augmentor, if users really want it they can do it, otherwise let's try to avoid it, see a proposed Quarkus enhancement, I'd suggest try it first before proceeding with the other work, because if it works it can impact how you decide to implement PermissionChecker

I have already changed as I accepted your suggestions that only @PermissionChecker should be introduced:

• The QuarkusPermission now has package-private constructor
• is internal builtin class dedicated for @PermissionChecker only, users cannot use it
• the augmentor is there to support the QuarkusPermission. I don't know how else to do that without the augmentor (well, of course it is possible, everything is possible, but the augmentor is only logical optional unless I am missing something)
• I don't want users to create or use the augmentor, it's internal concept how Quarkus will add permission checkers created by @PermissionChecker
• it has permission name in the constructor as every other permission

If you want entirely avoid QuarkusPermission, I can generate it or I can do more adjustments to avoid class if that is required as well, but I think it is smart to use what we already have. And custom permissions work, we have loads of tests so it is safer to reuse existing concept.

[michalvavrik]

see a proposed Quarkus enhancement, I'd suggest try it first before proceeding with the other work

TBH I don't know what it means. I think I read quarkusio/quarkus#43238 differently than you do, which is the reason for misunderstanding.

I don't think we need to solve it now. It is easier to iterate over some PR that proposes changes.

[sberyozkin]

Hi Michal, @michalvavrik

I don't want users to create or use the augmentor

This is close to what quarkusio/quarkus#43717 is about too: I think the fact we ask users to add permission checkers in the augmentor for @PermissionAllowed is indeed not good and is avoidable. But I guess it can be pursued independently.

As far as this PR is concerned, it is a clean introduction of @PermissionChecker only. I'd just like to avoid it be treated as a tool for avoiding the augmentation, it is about making it super easy to assign permission checkers to @PermissionAllowed.

For @PermissionAllowed-only cases, the augmentation is unavoidable (unless it is OIDC token scope) because the user must decide what permissions the identity must possess in order to pass the permission based control. What is entirely avoidable though is the requirement for users to also write permission checkers.

Given the above, what is really optimized with @PermissionChecker is the fact that users don't even have to decide what permissions the identity must possess. The possessed permissions are implicit, they are encoded in the request parameters, and the already authenticated identity, which the custom checker will assert. Please keep in mind though that the custom checkers which accept none of the request parameters can't assert anything.

If you want entirely avoid QuarkusPermission

Not at all, as long as it is treated as an implementation support tool, for now, at least, it is totally fine. My earlier comment was about the idea that if we can help users avoid manually augmenting with permission checkers for all cases (quarkusio/quarkus#43717), then introducing QuarkusPermission and asking user to use them just to avoid such checkers does not sound great IMHO.

Thanks again, indeed, lets do @PermissionChecker and then I propose to look at quarkusio/quarkus#43717.

Please continue enjoying the weekend

[sberyozkin]

@michalvavrik What can become a bit inconsistent is this:

• with PermissionAllowed/PermissionChecker we don't expect the identity to possess any explicit Permissions, only request parameters are enough (and again, this is why it is important the custom checkers accept some of these params or security identity)
• with PermissionAllowed and custom Permission which accepts the same request parameters and uses them to implement implies - we require users to augment even if (hopefully) we fix Avoid requiring custom permission checkers for @PermissionAllowed to work. quarkus#43717. But really, there are exactly the same mechanisms which are only implemented differently... This is why I thought of getting rid of the requirement to augment custom checkers first. But anyway, let's discuss it in a week or so

Thanks

[sberyozkin]

@michalvavrik I've just realized we need to prevent more thanks a single occurrence of @PermissionChecker("something"), but I guess it is a Quarkus level concern

[michalvavrik]

@michalvavrik I've just realized we need to prevent more thanks a single occurrence of @PermissionChecker("something"), but I guess it is a Quarkus level concern

Yes, I think nothing can be done here, the annotation itself is not repeatable and I already have validation in place.

I am moving forward every day, so when you are back from the conference you will get a PR to review. We need to get this merged and release API though.

[michalvavrik]

@sberyozkin I think we can change API in the future based on user feedback. Single occurrence of the @PermissionChecker("something") probably makes sense, but for example this javadoc says that feature is for CDI bean methods, but during implementation I realized how easy would it be for static methods. Now, CDI beans are much more practical if you need to inject other CDI beans etc. but if someone just needs to check the identity static method would be enough. Let's improve in the future.

I'll make sure there is validation test for what you mentioned.

[michalvavrik]

There is one thing I keep thinking about @sberyozkin @FroMage. It's very easy to change and we can revise this, but I am not sure what should I propose in the PR because I have to write tests for this and don't want to waste time on something that we can discard right now. So:

IIRC in the Spring user can use @PreAuthorize even for unauthenticated users. Do we allow @PermissionCheckers for anonymous (unauthenticated) users as well?

@Singleton
public class SecurityBean {

    @Inject
    RoutingContext routingContext;

    @PermissionChecker("read")
    boolean canRead(SecurityIdentity securityIdentity) {
        if (securityIdentity.isAnonymous()) {
            var header = routingContext.request().getHeader("secondary-authorization");
            return "you-can-trust-me".equals(header);
        }
        return true; // whatever business logic goes there
    }

}

So, we cannot allow it for situations when canRead has other method parameters than the securityIdentity because currently (and I suggest NOT to change it) we only allow to postpone security check to CDI interceptors if user is authenticated. But for scenarios like above, why not?

The reason why I didn't do it [version I am testing now doesn't allow that], I am honestly not sure whether it is practical and allowing to run this only for authenticated users means less processing and forcing people to thinking about security architecture (like who they authenticate and how).

[FroMage]

I don't think it makes sense to assign permissions to non-logged-in users. We can always relax this later if someone comes up with a compelling use-case.

[michalvavrik]

I don't think it makes sense to assign permissions to non-logged-in users. We can always relax this later if someone comes up with a compelling use-case.

good, I feel much more comfortable this way, thanks for the feedback

[michalvavrik]

btw I have finished impl. and now I need more tests and docs, we should get this in. @cescoffier please review or merge when you get a chance, we will need this API early next week in the Quarkus

• quarkusio/quarkus@main...michalvavrik:quarkus:feature/permissions-allowed-user-exp-improvements

[michalvavrik]

Opened quarkusio/quarkus#43846 that implements this API change.

[gsmet]

Any of you worried about the potential conflicts with traditional permissions? That's the first thing that came to my mind when having a look at this proposal but maybe I'm being too cautious.

I'm not sure another annotation makes sense but I wonder if there should be a marker (maybe speak()) to make it clear that it's a method and not your regular permission.

I'm just pushing this out there to see what you think about it. Again, I might be too cautious. The specific case I have in mind is someone having a permission that somehow gets overridden when the method is created or the opposite.

[michalvavrik]

Any of you worried about the potential conflicts with traditional permissions?

@PermissionsAllowed specifies required permissions, this PR nor API makes no changes in regard of this annotation.

Before this PR, you had 3 options how to authorize required permissions:

• add SecurityIdentity permission checker that did possessedPermission.implies(requiredPermission)
• map roles to permissions and Quarkus added for you ^^^
• token claims were mapped to ^^^

What this PR adds is another way to add this permission checker to the SecurityIdentity.

The specific case I have in mind is someone having a permission that somehow gets overridden when the method is created or the opposite.

I made a "trick" that prohibits that. I changed build time generation of required permissions so that @PermissionsAllowed("read") that is matched to @PermissionChecker("read") doesn't have permission name read. Instead, the name is generated permissions class name. We will never be able to avoid situation when user adds itself some custom checker that says grant access to every permissions which would include @PermissionsAllowed("read") but that comes from customizable security. We cannot exclude Quarkus users completely.

[michalvavrik]

@gsmet I re-read your comment again and I am not sure I have answered your question, feel free to ask again or let me know what I misunderstood.

[gsmet]

Yeah I don't think you have answered my question.

My point is that it might be easy to get confused between permissions and permission checkers during the life of an application if there's no marker to differentiate them.

Again I'm not saying I'm right, I'm just pointing to a potential issue.

[michalvavrik]

My point is that it might be easy to get confused between permissions and permission checkers during the life of an application if there's no marker to differentiate them.

Ah, sorry about that, now I get what you are saying. I don't think it is possible to entirely avoid this. It is only possible if signatures are matching because the @PermissionChecker must return boolean, but for example CRUD delete methods that accepts ID can have matching method signature.

In my eyes, it is user failure, but if we can do something to help users, I am open to changes.

I'm not sure another annotation makes sense but I wonder if there should be a marker (maybe speak()) to make it clear that it's a method and not your regular permission.

If you look to my tests in the quarkusio/quarkus#43846 you will see that you can easily combine possessed permissions and permission checkers and IMHO that how it should be. This new annotation shouldn't reduce @PermissionsAllowed capabilities (it's repeatable annotation, you can switch AND/OR with inclusive flag). If we start treat @PermissionsAllowed("read") differently just because it is checked by this @PermissionChecker("read"), it means we need to differentiate between them. My personal preference is not to do that.

[sberyozkin]

Hi @gsmet

My point is that it might be easy to get confused between permissions and permission checkers during the life of an application if there's no marker to differentiate them.

Can you please type some code to show what you are concerned about ? Say start with code fragment 1 and then show the code fragment 2 highlighting the possible issue ?

@PermissionAllowed("speak") is like @RolesAllowed("speak") in a sense that both only state the authorization restrictions that must be met before the method is allowed to be invoked, both are set on the secured method.

@PermissionChecker is about marking a CDI bean method which will be used to assert with a boolean response if the security identity possesses a speak permission, and in this case the fact that the identity possesses the expected permission can be deduced by checking JAX-RS request parameters, as opposed to a more difficult mechanism which also plan to simplify.

This is essentially our way of avoiding PerAuthorize like solutions with expression languages, etc... This PR will also let users avoid having to deal directly with Java Permission classes where it can make sense...

The use case which we discussed with one of the users is like this: assert that the authenticated user in project A has a write permission, in Project B - read permission. Project id is a query param. They did it with Spring Preauthorize, then succeeded with the current default mechanism requiring adding permission checkers to the identity.

But now, they will just do

class JaxrsResource {
    @GET
    @PermissionAllowed("read")
    String read(@QueryParam("project") String projectId) {}
}

class MyPanacheBean {
   
   @Inject SecurityIdentity identity;

   @PermissionChecker("read")
    boolean canRead(String projectId) {
           Set<String> projectPermissions = getUserProjectPermissions(
identity.getPrincipal().getName(), projectId);

           return projectPermissions.contains("read");
    }
}

etc.

Hope it clarifies a few things, but please type some code to make it clearer what the concern is

[michalvavrik]

@sberyozkin IIUC what @gsmet is worried about is that users will do this (metacode):

@Path("product")
public class ProductResource {
 
 @PermissionChecker("delete-product")
 @Path("{id}")
 @GET
  public boolean delete(@PathParam("id") Long productId, SecurityContext securityContext) {
     return Product.deleteById(productId);
  }

  @PermissionsAllowed("delete-product")
  boolean canDelete(Long productId, SecurityContext securityContext) {
       var user =  User.findByName(securityContext.getPrincipal().getName());
       var product = Product.getById(productId);
       return userOwnsProduct(user, product);
  }
}

so because they share signature and both returns boolean, they can be confused if user made mistake and didn't bother to test security. What we can do about that:

• validate that @PermissionChecker is not used together with app entry point, that is ExecutionModelAnnotationsAllowedBuildItem ?

then there are options suggested by @gsmet , IIUC they will require explicit match between @PermissionsAllowed(value = "owner", usePermissionChecker = true) and @PermissionChecker("owner"). I might have misunderstood that! Anywa, reason why this cannot be done is that @PermissionsAllowed allows following:

• one of @PermissionAllowed({ "checker1", "checker2", "possessedPermission1"})
• all of @PermissionAllowed(value = { "checker1", "checker2", "possessedPermission1"}, inclusive = true)

Also, it is one more thing that user needs to do. I think that if this confusion happens, user made mistake.

[michalvavrik]

Considering you need matching signature that returns boolean, is new proposed validation using ExecutionModelAnnotationsAllowedBuildItem sufficient response @gsmet ? Because it will only limit possible confusion to CDI beans and we do advise to prefer endpoint-validation inside docs (I think I wrote it somewhere TBH, didn't check now).

[sberyozkin]

@michalvavrik, thanks, I think users can do a lot of strange things not only with mixing up PermissionCheckers and PermissionAllowed. Besides, I don't think anyone is actually writing JAX-RS methods returning number or booleans like true or 1.

But since it is technically possible, let's tighten it a bit, but it is a Quarkus level concern, here, at the API level, we only introduce the actual concept, PermissionChecker.

I propose, at the Quarkus level, fail the build if a user puts a PermissionChecker on the JAX-RS method, do you think it is possible to do ?

[michalvavrik]

Yes @sberyozkin it is and I'll add that validation. It is not a difficult task as io.quarkus.resteasy.reactive.common.deployment.JaxrsMethodsProcessor#jaxrsMethods also produces ExecutionModelAnnotationsAllowedBuildItem. And yeah, I agree with your comment, but it's @gsmet that needs to ACK.

[michalvavrik]

We need to move on with this one, @cescoffier @gsmet do you plan to proceed with review?

[sberyozkin]

Hi @gsmet @cescoffier I'm going to merge it now, hope the above comment clarifies some concerns from Guillaume, we can still review and reopen this PR before the actual release

[michalvavrik]

IMO if anyone else wanted to review but could not find a time, they would simple write short sentence like "I'll review later". I have asked Clement to review 10 days ago, so let's merge it or find someone who is interested in review. Thank you

[cescoffier]

Hey sorry for the delay. You asked for a review just before devoxx (and then I had a F2F).

I share the concern of @gsmet. It looks rather confusing from my (remote) POV.

[michalvavrik]

Hey sorry for the delay. You asked for a review just before devoxx (and then I had a F2F).

np, in that case let's wait, thanks for the response

I share the concern of @gsmet. It looks rather confusing from my (remote) POV.

alright, we will discuss it with @gsmet in that comment thread.

[sberyozkin]

Hey @cescoffier @gsmet, I'm still not certain that, as prototyped by Michal, users will start mixing up requirements like @PermissionAllowed with validators like @PermissionChecker and putting each of them into the wrong places, especially after some dev iterations, it is not really a @PermissionChecker problem.

But in any case, it is a Quarkus level build time verification, as well as the documentation concern, we'll try to tighten a few things there to avoid some obvious errors.

@PermissionChecker is going to become one of the most important additions to Quarkus Security, lets get it in

[FroMage]

@gsmet @cescoffier I did not understand the reason for you confusion. Do you mind re-wording it so I can understand the problem?

[sberyozkin]

Hi Everyone, @gsmet, @michalvavrik @cescoffier @FroMage, IMHO the best way forward is to merge this PR, which I'm going to do now, only an interface is introduced, quarkus-security still has to be released, so the PR is easily revertable,

But it is really only at the Quarkus level, at quarkusio/quarkus#43846, where we can add some extra build time validations, and I've asked both Guillaume and Clement review it too.

So I'm merging this PR now, and either Guillaume and Clement can revert it.

Enjoy the weekend all

[sberyozkin]

@michalvavrik By the way, in addition to doing the @PermissionChecker is not attached to the JAX-RS method check, and other checks Guillaume @gsmet or Clement @cescoffier may recommend, IMHO we should also fail the build if @PermissionChecker does not have a @PermissionAllowed match, to avoid the accidental proliferation of orphaned @PermissionChecker checkers, please add a test to your Quarkus PR next week if you haven't already, thanks

[michalvavrik]

IMHO we should also fail the build if @permissionchecker does not have a @PermissionAllowed match, to avoid the accidental proliferation of orphaned @permissionchecker checkers, please add a test to your Quarkus PR next week if you haven't already, thanks

agreed, but I though about that after first Guillaume comment and added it https://github.com/quarkusio/quarkus/blob/9441bea73e8067c81657d8cf5d84a2bdbdcbc7d9/extensions/security/deployment/src/main/java/io/quarkus/security/deployment/PermissionSecurityChecks.java#L414. It was after your review Sergey so you could not see it. I'm looking forward for the release, thanks for moving discussion forward @sberyozkin .