Update SecurityIdentity to list owned Permissions and allow simpler permission checks

[sberyozkin]

The main purpose of this PR is to make it possible to simplify the way @PermissionAllowed are enforced by default at the Quarkus level.

At the Quarkus level, when no (recently introduced) @PermissionChecker is used, the only way for users to have @PermissionAllowed checks enforced is basically do these checks themselves by registering a custom SecurityidentityAugmentor and adding a custom permission checker function:

For example (from the Quarkus docs):

@ApplicationScoped
public class PermissionsIdentityAugmentor implements SecurityIdentityAugmentor {

    @Override
    public Uni<SecurityIdentity> augment(SecurityIdentity identity, AuthenticationRequestContext context) {
    //...
        Permission possessedPermission = new MediaLibraryPermission("media-library",
                new String[] { "read", "write", "list"}); 
        return QuarkusSecurityIdentity.builder(identity)
                .addPermissionChecker(new Function<Permission, Uni<Boolean>>() { 
                    @Override
                    public Uni<Boolean> apply(Permission requiredPermission) {
                        boolean accessGranted = possessedPermission.implies(requiredPermission);
                        return Uni.createFrom().item(accessGranted);
                    }
                })
                .build();
       };
}

Where the users need to correctly write the permission checker function making sure it is permission which is meant to be associated with the identity is used to call implies , not the required one... And there is no way to check on SecurityIdentity which permissions it owns.

@FroMage and @michalvavrik worked out a plan to make it easier to implement such functions, but IMHO users should be totally shielded from having to write such checkers. We do not ask users to manually write role checks, and we should not ask them to do it for permissions. They can do if they really want to, but it should be avoidable.

The above code should look like this:

@ApplicationScoped
public class PermissionsIdentityAugmentor implements SecurityIdentityAugmentor {

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

And Quarkus Security will do the required checks itself, by checking SecurityIdentity#getPermissions() added in this PR.

See also quarkusio/quarkus#43717

[michalvavrik]

I kinda said it all in my comments in quarkusio/quarkus#43717. I can't think of anything else to comment here.

FWIW idea to add addPermission to the QuarkusSecurityIdentity.Builder is brilliant.

[FroMage]

Seems like a good idea.

[sberyozkin]

@michalvavrik Michal, I've totally missed Steph approved, what should we do here, I'm assuming you are OK with keeping a method allowing to list current permissions. I can drop the methods for checking them, it can be done later, but indeed, letting users list posessed permissions seems useful, similarly to what users can do with getRoles, are you OK with this plan ?

[michalvavrik]

@michalvavrik Michal, I've totally missed Steph approved, what should we do here, I'm assuming you are OK with keeping a method allowing to list current permissions. I can drop the methods for checking them, it can be done later, but indeed, letting users list posessed permissions seems useful, similarly to what users can do with getRoles, are you OK with this plan ?

Honestly, I am still worried that users will consider following interchangeable:

# 1
return identity.checkPermissionBlocking("edit");
# 2
for (Permission p : identity.getPermissions()) {
            if (p.implies(new StringPermission("edit")) { return true}
}
return false;

(it's metacode, don't take it literately please). Because that is what we ourselves do in Quarkus, we mostly (if not always) use getRoles and not io.quarkus.security.identity.SecurityIdentity#hasRole. I have already proposed here something like that here.

If you said in the getPermissions javadoc that it checking of permissions must be performed with the checkPermission then I think this PR makes positive changes and should be merged.

[sberyozkin]

Sorry Michal, I did not quite get your concern about the interchangeable code above, it does look interchangeable to me or did I miss something ?

[michalvavrik]

Sorry Michal, I did not quite get your concern about the interchangeable code above, it does look interchangeable to me or did I miss something ?

I don't know, it quite hard for me to explain it because I am not sure where we disagree. I can try again, I hope I am not repeating myself and it is useful:

# 1
return identity.checkPermissionBlocking("edit");
# 2
for (Permission p : identity.getPermissions()) {
            if (p.implies(new StringPermission("edit")) { return true}
}
return false;

is not interchangeable because you can do for example this:

• https://github.com/quarkusio/quarkus/blob/453add378a658aff7b69dc4cebd06f508e32773b/extensions/keycloak-authorization/runtime/src/main/java/io/quarkus/keycloak/pep/runtime/KeycloakPolicyEnforcerAuthorizer.java#L159

If users or Quarkus or Quarkiverse extensions add these checkers, they are not java.security.Permissions. You must not use getPermissions for checking permissions because this addPermissionChecker and checkPermission exists in Quarkus Security API very long time. I'd like users to realize it, if you don't think there is a risk, I can respect that.

[sberyozkin]

Thanks @michalvavrik I'll think about your comment and reply a bit later, this PR is not essential for the next release

[sberyozkin]

H @michalvavrik,

Sorry Michal, I did not quite get your concern about the interchangeable code above, it does look interchangeable to me or did I miss something ?

I don't know, it quite hard for me to explain it because I am not sure where we disagree. I can try again, I hope I am not repeating myself and it is useful:

1

return identity.checkPermissionBlocking("edit");

2

for (Permission p : identity.getPermissions()) {
if (p.implies(new StringPermission("edit")) { return true}
}
return false;

is not interchangeable because you can do for example this:
-  https://github.com/quarkusio/quarkus/blob/453add378a658aff7b69dc4cebd06f508e32773b/extensions/keycloak->authorization/runtime/src/main/java/io/quarkus/keycloak/pep/runtime/KeycloakPolicyEnforcerAuthorizer.java#L159 

So, like I said before, permissions are as equally the properties of SecurityIdentity as the roles are, there is no difference between permissions and roles in this regard, both are just identity properties.

Just because a permission checker method exists, it does not mean we should not allow users check what permissions the identity owns. We have users like the one in #quarkusio/quarkus#50827 (reply in thread) who are trying to workaround the fact they can't get a list of owned Permissions which is not a good experience.

If users or Quarkus or Quarkiverse extensions add these checkers, they are not java.security.Permissions. You must not use getPermissions for checking permissions because this addPermissionChecker and checkPermission exists in Quarkus Security API very long time.

Your example, may or may not produce the same output, the list of permissions is just a list of permissions, we don't give any guarantees that iterating over them must produce the same result that some extension's permission checker returns, in scope of that extension checking permissions can mean an entirely different thing to what it means in scope of user's code.

So, I don't really see why users must not use permissions just because some other part of the system registers a permission checker.

if you don't think there is a risk, I can respect that.

IMHO there is no risk, but what I can do is I can add a note to highlight your point, that checking Permission directly is not guaranteed to produce the same result at the registered permission checker

[michalvavrik]

I think we are going in the circle, my opinion didn't change even after reading your comments. All I am asking you to do is to update javadoc of checkPermission which says Checks if a user holds a given permission.. If there is checkPermission which checks permissions, I'd definitely expect it checks permissions and not permissions & permission checkers. You would not expect hasRole to check anything else but roles, would you?

Anyway, I agree that risk is small, my suggestions are about clarity. So I'd suggest that you rebase this PR on the current main and if you disagree, it can go in. Just get review of committers in addition to what I said. Thanks

[michalvavrik]

ad quarkusio/quarkus#50827 - I have no idea why it would be useful, it sounds to me like users want to do custom checks using our API. If they have their own permissions, they can store it as identity attributes. I don't know in which scenario their request makes a sense.

[michalvavrik]

As everything was already said.

[sberyozkin]

@michalvavrik Sure, I know I can merge as I have an approval from @FroMage , but it is important to me we reach some consensus.

I believe you look at it from the functional point of view, why would someone want to have a look at the list of permissions if they can register a permission checker.

But this is not equivalent in terms of the user experience at the SecurityIdentity API level. All they can do with the permission checker is to do dentity.checkPermissionBlocking("edit"); while just giving a list of permissions opens up other options or allows them to avoid encapsulating the permission check, we don't know, but we don't compromize anything either.

Now, with roles, they can do both getRoles() and hasRole(String role) - so this is already not a consistent API.

Arguably, the permission checker registration should not have been added at all, as it brings the user element into a SecurityIdentity and hence a certain unpredictability you are implying, compared to a kind of equivalent hasRole(String role) with the identity implementation driving it.

So like I said, because I appreciate your concern, I'll add a relevant note

[michalvavrik]

Arguably, the permission checker registration should not have been added at all, as it brings the user element into a SecurityIdentity and hence a certain unpredictability you are implying, compared to a kind of equivalent hasRole(String role) with the identity implementation driving it.

Permission checker in terms of the SecurityIdentity class is how permissions can be granted. This worked for years, for example Keycloak Authorization is adding permission checker for identity based on permissions received from Keycloak. There is no equivalent in form of the java.security.Permission.

So what introduces link between permissions (which doesn't exist ATM) and checkPermissions is you, it is not fault of the permission checkers. Sorry, I just think that starting introducing a new method will be confusing.

There is nothing more to be said, I read your opinions and I respect them. Let's move on.

[michalvavrik]

According to API, roles needs to be resolved beforehand. But permissions checks are asynchronous, which helps us to perform non-blocking checks and you cannot do it with java.security.Permission.

[sberyozkin]

@michalvavrik Sorry, I know we kind of completed the discussion, but I missed

I think we are going in the circle, my opinion didn't change even after reading your comments. All I am asking you to do is to update javadoc of checkPermission which says Checks if a user holds a given permission.. If there is checkPermission which checks permissions, I'd definitely expect it checks permissions and not permissions & permission checkers. You would not expect hasRole to check anything else but roles, would you?

Sure, I mean, we already have the existing SecurityIdentity.checkPermission(Permission), it is not impacted by this PR at all, this PR only lets users do SecurityIdentity.checkPermission(String), that delegates to the existing SecurityIdentity.checkPermission(Permission), nothing conceptually new is introduced.

Anyway, I agree that risk is small, my suggestions are about clarity.

Risk of exactly what ? And yes, I will clarify that checkPermission works with registered permission checkers, letting users see a list of permissions does not conflict with that in any way

According to API, roles needs to be resolved beforehand. But permissions checks are asynchronous, which helps us to perform non-blocking checks and you cannot do it with java.security.Permission.

Again, you are talking about functional stuff, API needs to let users just to see a list of permissions.

In fact they can do it now, for ex, if they were mapped from scope:

((JsonWebToken)identity.getPrincipal()).getClaim("scope").split(" ") and see those scopes.

Or with the PR: identity.getPermissions()...

[michalvavrik]

Risk of exactly what ? And yes, I will clarify that checkPermission works with registered permission checkers, letting users see a list of permissions does not conflict with that in any way

Risk that users believe that checkPermissions checks getPermissions. Personally, I use hasRole and getRoles().contains interchangeably. You must not do that with permissions. If you add a note as we agreed and you say, I think all is good. It should be clear that getPermissions is for rather edge case scenarios that noone has explained to me yet. We have users asking, but do they understand how it works?

Again, you are talking about functional stuff, API needs to let users just to see a list of permissions.
In fact they can do it now, for ex, if they were mapped from scope:
((JsonWebToken)identity.getPrincipal()).getClaim("scope").split(" ") and see those scopes.
Or with the PR: identity.getPermissions()...

I don't know enough about Keycloak Authorizer to recognize you are right :-)

Just so that it is not lost in all the text: I think

• this PR is useful
• is fine to get in as long as you enhance the javadoc

Thank you for your patience.

[sberyozkin]

@michalvavrik

Risk that users believe that checkPermissions checks getPermissions

I'll make sure it is clearly stated that the former only works with registered permission checkers

It should be clear that getPermissions is for rather edge case scenarios that noone has explained to me yet

Believe it or not, for simple cases, I found it, a few times, awkward having to register a function checker that does exactly what I can do with checking a permission directly - but this point brings a functional angle. So as suggested earlier, irrespective of how a permission is actually checked, a list of the identity permissions is just one of the properties of the SecurityIdentity instance - it can be useful to iterate over them not only for the purpose of checking them, but some metrics, etc.

I'll let you know once I update the PR, appreciate your feedback.

Thanks

[michalvavrik]

Believe it or not, for simple cases, I found it, a few times, awkward having to register a function checker that does exactly what I can do with checking a permission directly - but this point brings a functional angle.

I believe you. I always look what I have done before.

So as suggested earlier, irrespective of how a permission is actually checked, a list of the identity permissions is just one of the properties of the SecurityIdentity instance - it can be useful to iterate over them not only for the purpose of checking them, but some metrics, etc.

Alright.