Avoid requiring custom permission checkers for `@PermissionAllowed` to work.

[sberyozkin]

Description

Right now, for @PermissionAllowed to work, users must add custom permission checker Functions in the custom security identity augmentor. For example:

public class Service { 
   @PermissionAllowed("read:all")
   @GET 
   public String readAll() {...}
}

And, now, users should add permission checkers, this code from OidcIdentityProvider which converts scope claim to permissions shows it well:

static void addTokenScopesAsPermissions(QuarkusSecurityIdentity.Builder builder, Collection<String> scopes) {
        if (!scopes.isEmpty()) {
            builder.addPermissionChecker(new Function<Permission, Uni<Boolean>>() {

                private final Permission[] permissions = transformScopesToPermissions(scopes);

                @Override
                // For example, `new StringPermission("readAll")` is a required permission
                public Uni<Boolean> apply(Permission requiredPermission) {
                    // Iterate over each possessed scope permission
                    for (Permission possessedPermission : permissions) {
                        if (possessedPermission.implies(requiredPermission)) {
                            // access granted
                            return Uni.createFrom().item(Boolean.TRUE);
                        }
                    }
                    // access denied
                    return Uni.createFrom().item(Boolean.FALSE);
                }
            });
        }
    }

I recall, Stuart, myself, Michal, all looked at it and it looked totally fine, but after the recent discussions it just struck me, why do we require users do it ?

Let's compare it with RBAC:

public class Service { 
   @RolesAllowed("admin")
   @GET 
   public String readAll() {...}
}

and this is all the users have to do in case of OIDC token containing this role. Or they can add it:

// in fact it is not even needed for `scope`, it is enough to to `quarkus.oidc.token.roles-path=scope`, just typing it as an example
static void addTokenScopesAsRoles(QuarkusSecurityIdentity.Builder builder, Collection<String> scopes) {
        if (!scopes.isEmpty()) {
            for (String s : scopes) {
                 builder.addRole(s);
            }
        }
    }

Quarkus security does the rest: it matches roles itself.

IMHO, exactly the same should happen for permissions.

Implementation ideas

• SecurityIdentity should return an unmodifiable set of possessed Permissions, Set<Permission> getPermissions().

• QuarkusSecurityIdentity.Builder should have addPermission(String), addPermission(Permission) : these are the only methods users may have to call to associate a permission with the identity

• If existing SecurityIdentity permission checkers do not allow a given required permission, QuarkusSecurity iterates over identity.getPermissions() and checks which one implies required permission.

[quarkus-bot[bot]]

/cc @pedroigor (bearer-token)

[michalvavrik]

I agree with everything this issue mentions except with this point:

SecurityIdentity should return an unmodifiable set of possessed Permissions, Set<Permission> getPermissions().

I don't understand why should we add to API method that users are not supposed to use. Right now we have:

https://github.com/quarkusio/quarkus-security/blob/a7b8541f94cea706910a18a5faab1e090bca20c3/src/main/java/io/quarkus/security/identity/SecurityIdentity.java#L121

which we cannot remove (both because it would be super breaking and I think it's great to have these checkers). Permissions added with the QuarkusSecurityIdentity.Builder should have addPermission(String), addPermission(Permission) will be transformed to the checkers internally. If we have only SecurityIdentity#checkPermission, we know that checkPermission returns correct result. But if we have SecurityIdentity#getPermissions then users will think that they can check permissions instead of calling checkPermission and they will miss some permission checks.

That is, we cannot say that SecurityIdentity#checkPermissions is simply a method that checks SecurityIdentity#getPermissions because it is not true, there are also functions on QuarkusSecurityIdentity that checks permissions - https://github.com/quarkusio/quarkus/blob/main/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/QuarkusSecurityIdentity.java#L218

[michalvavrik]

due to concerns I have raised in previous comment, I'll implement it differently. I am going to have a look.

[michalvavrik]

@sberyozkin now Quarkus OIDC extension has:

quarkus.oidc.roles.role-claim-path
quarkus.oidc.roles.role-claim-separator
quarkus.oidc.roles.source

would it make sense to introduce:

quarkus.oidc.permissions.permission-claim-path
quarkus.oidc.permissions.permission-claim-separator
quarkus.oidc.permissions.source

?

[sberyozkin]

@michalvavrik We may have some misunderstanding here

I don't understand why should we add to API method that users are not supposed to use

They won't, Quarkus Security will use them... I'm not sure sure what do you mean. Are users supposed to use SecurityIdentity.getRoles or not ?

I'm also not sure what your last comment about OIDC is about, sorry...

[sberyozkin]

You link to SecurityIdentity#checkPermission is about users writing the code, while I'd like users not to write any checker code at all, all the users should be worried about is associating permissions that the identity is supposed to own with this identity

[sberyozkin]

there are also functions on QuarkusSecurityIdentity that checks permissions - https://github.com/quarkusio/quarkus/blob/main/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/QuarkusSecurityIdentity.java#L218

This is exactly what we should've avoided doing, it is just only now that it is becoming obvious to me...It is too complex, there should be no dev experience difference in the way roles and permissions are checked.

[michalvavrik]

@sberyozkin I agree with all of that. I just don't want to add getPermissions method if we can avoid that. I think it will be impractical to keep possessed permissions as additional concept as they need to integrate with existing permission checkers. Let me open PR tomorrow and it will show you how I understand the problem. If it turns out you see it differently, I'll put it to draft and rework. In my mind, this issue is about improving QuarkusSecurityIdentity.Builder, which should be very simple.

I'm also not sure what your last comment about OIDC is about, sorry...

Ok, let's that one be. I just thought that users can map roles from token claims, we could provide same feature for permissions. but that is not for this issue and let's not discuss it here.

[sberyozkin]

@michalvavrik

I think it will be impractical to keep possessed permissions as additional concept as they need to integrate with existing permission checkers.

Why exactly :-) ? Why are we differentiating between roles and permissions as far as their association with the identity is concerned ? How do I know as a user what permission the current identity has ?

[sberyozkin]

In my mind, this issue is about improving QuarkusSecurityIdentity.Builder, which should be very simple.

So how will it look if the users can't do what is proposed in https://github.com/quarkusio/quarkus-security/pull/57,

@ApplicationScoped
public class PermissionsIdentityAugmentor implements SecurityIdentityAugmentor {

    @Override
    public Uni<SecurityIdentity> augment(SecurityIdentity identity, AuthenticationRequestContext context) {
        return QuarkusSecurityIdentity.builder(identity)
               // no need to write functions
                .addPermission(new MediaLibraryPermission("media-library", new String[] { "read", "write", "list"});)
                .build();
       };
}

?

[michalvavrik]

@michalvavrik

I think it will be impractical to keep possessed permissions as additional concept as they need to integrate with existing permission checkers.

Why exactly :-) ? Why are we differentiating between roles and permissions ? How do I know as a user what permission the current identity has ?

Because I am worried that it will be confusing and we gain nothing, users don't need to get permissions. I am worried about this method:

https://github.com/quarkusio/quarkus-security/blob/8eb9e15216408419e580049c3a4f034a3c980fb4/src/main/java/io/quarkus/security/identity/SecurityIdentity.java#L121

If you add permission getter, it means that anyone can get these permissions. So how will you explain that getPermissions contains but a subset of permissions? Other part will be permission checkers as they exist today. Because we cannot remove this method:

https://github.com/quarkusio/quarkus/blob/main/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/QuarkusSecurityIdentity.java#L218

Or do you want to remove it? I realize you wrote that we will run "permission checkers" and then "permissions" when checkPermission is called, but are you sure it will be clear to users?

[michalvavrik]

In my mind, this issue is about improving QuarkusSecurityIdentity.Builder, which should be very simple.

So how will it look if the users can't do what is proposed in quarkusio/quarkus-security#57,

@ApplicationScoped
public class PermissionsIdentityAugmentor implements SecurityIdentityAugmentor {

    @Override
    public Uni<SecurityIdentity> augment(SecurityIdentity identity, AuthenticationRequestContext context) {
        return QuarkusSecurityIdentity.builder(identity)
               // no need to write functions
                .addPermission(new MediaLibraryPermission("media-library", new String[] { "read", "write", "list"});)
                .build();
       };
}

?

You could achieve same by adding same addPermission method, but you turn it internally into a permission checker. Anyway, I can see you took some steps in https://github.com/quarkusio/quarkus-security/pull/57 so let's forget that plan (my plan). We can discuss your proposal instead. I understand intentions.

[michalvavrik]

I reviewed https://github.com/quarkusio/quarkus-security/pull/57, let's go with your plan and continue discussion there. It doesn't need to be my way. Thanks for this effort.

[sberyozkin]

Michal, we don't have to forget your plan, but discuss pros and cons of both plans and find a compromise, I appreciate your concerns. In fact I'm thinking our plans are not that far away :-)

You could achieve same by adding same addPermission method, but you turn it internally into a permission checker.

Aha, we are actually not thinking that differently, see the code above :-)

So, https://github.com/quarkusio/quarkus-security/pull/57 does not have to be a prerequisite...

SecurityIdentity.getPermissions() returns a complete set of permissions the identity owns/possesses, it lets users to find out, without even using @PermissionAllowed, or when multiple @PermissionAllowed are used, which permissions exactly the current identity has, exactly what they can do with roles.

It does not conflict with SecurityIdentity.checkPermission because that method is about checking if the security identity has one of the possessed permissions which will imply the passed in permission, and SecurityIdentityAugmentor supporting such requests with permission checkers.

Would you like to start with what you suggested and we can consider https://github.com/quarkusio/quarkus-security/pull/57 later ? The only concern here is like I said earlier, users would still not know, when interacting with SecurityIdentity what permissions it has... but it is not a show stopper for now...

[sberyozkin]

My problem in general with the SecurityIdentity API that we offer a simple get / boolean check access to roles (which can be seen as a high level grouping of permissions) while require users write manual checking code to manage permissions, and there is no any association between the identity and the possessed permissions - only an indirect association at the checker level, but in any case, I'm OK with starting from only addPermission at the builder level... and review possible direct association later Apologies I did not get your idea immediately, thanks

[sberyozkin]

@michalvavrik I can do it myself to save you some time if you'd like and ask you to review ? Oh please complete it if you have already started looking into it, thanks for your help

[michalvavrik]

It does not conflict with SecurityIdentity.checkPermission because that method is about checking if the security identity has one of the possessed permissions which will imply the passed in permission, and SecurityIdentityAugmentor supporting such requests with permission checkers.

Alright, if that is documented in quarkusio/quarkus-security#57 I think we can go with it. I just think we should clarify their relation because if we didn't start from here, from the existing state, I'd tell you that checkPermission should test getPermission. If it is going to also check permission checkers (or whatever users want) we need to tell users always check permissions with SecurityIdentity#checkPermission and not getPermissions

@michalvavrik I can do it myself to save you some time if you'd like and ask you to review ?

Yeah, let's go for it, I will be busy till Friday and then I have loads other Quarkus issues planned. Thanks, I think addPermission method should be added in 3.17 and docs should reflect that so that we have better user story (not sure if it is right term?) to present.

[sberyozkin]

@michalvavrik SecurityIdentity#getPermissions will be clearly documented that they only return a set of possessed permissions, there will be no conflict whatsoever with checkPermission, but we can put https://github.com/quarkusio/quarkus-security/pull/57 on hold for now, let me move it to the Draft

I think addPermission method should be added in 3.17 and docs should reflect that so that we have better user story (not sure if it is right term?) to present

Sure, let me look into it and I'll ask you to review... IMHO users will be happy they won't have to register checkers :-)

[michalvavrik]

@sberyozkin by now I think I understand and I am fine with the plan. Thanks for pushing this forward.

https://github.com/quarkusio/quarkus-security/pull/57 on hold for now, let me move it to the Draft

IMO that is not necessary. But it can take long enough to get reviews and Quarkus Security API release, so let's start with the addPermission method.