Support injection of the @BeanParam parameters into a custom permission referenced in the @PermissionAllowed security annotation

[michalvavrik]

• Closes: Support @PermissionAllowed for @BeanParam parameter #43231

This PR allows to pass into custom java.security.Permission only parameters you need from a @BeanParam. I can imagine you have different types of @BeanParams that share a header or query param etc. that you want to pass. It's alternative, but not only option. Instead of this, users could introduce level of abstraction (shared interface) they will inject into the java.security.Permission instead.

IMO what this can be convenient in very simple scenarios, like when you want to pass beanParam.field into your custom java.security.Permission.

Permission constructor parameters are newly matched automatically based on secured method parameter names.

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

This doesn't have support, so let's close this. Thanks to reviewers that spend time on this. I appreciate it.

[sberyozkin]

@michalvavrik Michal, you don't have to close just because I thought it was unnecessary, let me re-open for a bit longer, in case someone else would like to comment. I actually came here with another comment but it was already closed, so let me add it

[sberyozkin]

@michalvavrik I wonder can we avoid typing bean.paramName in @PermissionAllowed and instead check if the custom permission constructor has matching names and add those bean param named automatically to @PermissionAllowed ?

[michalvavrik]

@michalvavrik Michal, you don't have to close just because I thought it was unnecessary, let me re-open for a bit longer, in case someone else would like to comment.

I am not defeatist, I just don't have better (new) arguments :-) Let's hear from @FroMage if he manages to find a time later, I may have misunderstood this comment #43231 (comment).

I actually came here with another comment but it was already closed, so let me add it

I am ready to provide examples and answers why I implemented this. we can always close this later in case there is not an agreement, I am not worried.

[michalvavrik]

@michalvavrik I wonder can we avoid typing bean.paramName in @PermissionAllowed and instead check if the custom permission constructor has matching names and add those bean param named automatically to @PermissionAllowed ?

We can, and it is already done for not nested params (when there is no dot). I didn't implement it for cases where there is a dot. In general, only reason for params annotation attribute is there can be ambiguity (like nested fields with a same name). While bean.paramName makes it unambiguious.

[sberyozkin]

@michalvavrik, yeah sure, the last comment in the issue made it clearer for me, so this PR is alive :-).

So what do you think about this case: it is a BeanParam, but the permission constructor does not accept it, so try to deduce required bean param names ?

[michalvavrik]

So what do you think about this case: it is a BeanParam, but the permission constructor does not accept it, so try to deduce required bean param names ?

I think that's a right way to do. I can implement it. I can also rewrite docs to accent simple auto detected cases. Is it acceptable to keep what there is now, like beanParam.nested? It is useful for ambiguous cases, to explicitly declare what you want to map.

[sberyozkin]

Sure, I guess it is a small amount of code to support beanName.paramName expressions, but indeed, it is worth highlighting how nicely it can be auto-mapped, if it proves possible to implement

[michalvavrik]

Sure, I guess it is a small amount of code to support beanName.paramName expressions, but indeed, it is worth highlighting how nicely it can be auto-mapped, if it proves possible to implement

I didn't realize it at first, but how it works ATM is explained here https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/PermissionsAllowed.java#L68C10-L68C11 TL;DR; autodetected are parameters based on data type. We can also add autodetection based on construct <-> secured method parameter name matches, I can implement that, but it can't be done by default otherwise it will be a breaking change. I'll have deeper look in next 2 days and see what I can do to make it easy.

[FroMage]

LGTM, but docs need a fix.

[FroMage]

Once again a permission where this. members are never used. I don't understand this model.

[michalvavrik]

The model comes from the https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/security/Permission.html#implies(java.security.Permission). In Quarkus terms, the identity has permission checker that can check required permissions. Here it is:

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

So technically, you only need one required permission and one checker. You can do requiredPermission.implies(null) or whatever, but it feels very hacky, so I always expect users will declare both requiredPermission and possessedPermission and do: possessedPermission.implies(requiredPermission).

I expect this can be simplified with your @RequiredPermission or @PermissionChecker or whatever later.

BTW this particular MyPermission is here to test that bean parameters were passed correctly, I don't think it needs to use this for that purpose. I wouldn't write this to the docs.

[FroMage]

I can understand implies as it's documented, which is about figuring out if, given an already granted permission, another one is implied by it. For example, given that the current user has the admin permission, is the read permission implied? Probably, yes.

But this is not what we're describing here with MyPermission. I see no relation.

SecurityIdentity.checkPermission(Permission) I also understand: does the current user have the right to do X? Fine. That makes sense.

But again, this does not look like what MyPermission.implies is doing. Is this something related to the current PR, or the general security model in Quarkus?

[michalvavrik]

But this is not what we're describing here with MyPermission. I see no relation.

ad describing: it's a test, not docs; but sure, I'll answer

SecurityIdentity.checkPermission(Permission) I also understand: does the current user have the right to do X? Fine. That makes sense.

When you specify @PermissionsAllowed(permission=CustomPermission) then CustomPermission instance is created and passed to the SecurityIdentity.checkPermission(Permission) because it is fact checkPermission(requiredPermission).

It depends on how user writes SecurityIdentityAugmentor whether this is used. I am using one and the same permission in the all tests for making it easy, but you can also do this (meta code ...):

@Override
public Uni<SecurityIdentity> augment(SecurityIdentity identity, AuthenticationRequestContext context) {
    return QuarkusSecurityIdentity.builder(identity).addPermissionChecker(requiredPermission -> {
        if (requiredPermission.implies(new StringPermission("read"))) {
            return Uni.createFrom().item(Boolean.TRUE);
        }
        return Uni.createFrom().item(Boolean.FALSE);
    }).build();
}

and this augmentor part can be simplified with #43238.

I cannot simplify this model in this PR.

[FroMage]

Oh, so wait, users have to implement a SecurityIdentityAugmentor to make this work?

[michalvavrik]

It is one option. There are other ways to add permission checker on your security identity. For example we map token claims as StringPermission. Or you can remap SecurityIdentity roles like this:

map identity role 'user' to String permission 'read'
quarkus.http.auth.policy.role-policy2.permissions.user=read  
quarkus.http.auth.permission.roles2.paths=*
quarkus.http.auth.permission.roles2.policy=role-policy2

Which gives you var possessed = new StringPermission("read")

Problem with this is that we add permission check like possessedPermisison.implies(requiredPermission). It felt right, but I suppose for purpose of this particular situation, requiredPermisison.implies(possessedPermission) is equivalent. Though it is not from POV of formal logic.

So ATM you do need augmentor, but this part could be changed in this PR if it is requested.

[michalvavrik]

It would be yet another breaking change though.

[michalvavrik]

And if you wonder, if user doesn't specify any permission checker but @PermissionsAllowed is applied, the QuarkusSecurityIdentity responds with false by default

quarkus/extensions/security/runtime/src/main/java/io/quarkus/security/runtime/QuarkusSecurityIdentity.java

Line 84 in 1603d2c

return Uni.createFrom().item(false);

so you don't have the permission unless you posses at least one that complies it. I think we shouldn't change it in this PR. I'll continue updating the docs with the augmentor.

[FroMage]

Sure, I guess it is a small amount of code to support beanName.paramName expressions, but indeed, it is worth highlighting how nicely it can be auto-mapped, if it proves possible to implement

I didn't realize it at first, but how it works ATM is explained here https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/PermissionsAllowed.java#L68C10-L68C11 TL;DR; autodetected are parameters based on data type. We can also add autodetection based on construct <-> secured method parameter name matches, I can implement that, but it can't be done by default otherwise it will be a breaking change. I'll have deeper look in next 2 days and see what I can do to make it easy.

Oh, I did not realise you did the matching based on type by default. Given a lot of form or path parameters are typed String or Long this will lead to duplicates fairly frequently. Isn't it more intuitive and useful to do the matching based on the parameter names (constructor param names matching endpoint method parameter names)?

Then indeed, we could also match member names of bean params when there's no ambiguity, or resort to explicit parameter names with dot prefixes (such as this PR supports) when users want to resolve any ambiguity.

[michalvavrik]

Oh, I did not realise you did the matching based on type by default. Given a lot of form or path parameters are typed String or Long this will lead to duplicates fairly frequently. Isn't it more intuitive and useful to do the matching based on the parameter names (constructor param names matching endpoint method parameter names)?

Then indeed, we could also match member names of bean params when there's no ambiguity, or resort to explicit parameter names with dot prefixes (such as this PR supports) when users want to resolve any ambiguity.

Yes, it would be better. I don't remember why I decided to use param types by default. Only positive thing about it is that it doesn't depend on concrete parameter names in permission/secured method. I think it should be changed to param name matching by default as for what I have seen, it avoid typing any PermissionsAllowed#params almost always. Original behavior (matching based on type) should be possible to activate by constant on PermissionsAllowed#autodetectUsingTypes that could be assigned to PermissionsAllowed#params.

It will be breaking change but I think @sberyozkin also suggested it in this PR, so I'll propose it in Quarkus Security API project.

[sberyozkin]

My proposal is just do the name based matching as proposed by Steph @FroMage without worrying about supporting a type only match or thinking we may be breaking something, it can't be properly supported without the name match.
If we do not explicitly suggest in the docs that users can have same parameters having different names in JAX-RS methods and custom permission constructors, then we should just fix as proposed by Steph and merge

[FroMage]

Look good, except minor language suggestions, and the unresolved question of how implies works which I find very confusing.

[FroMage]

I can understand implies as it's documented, which is about figuring out if, given an already granted permission, another one is implied by it. For example, given that the current user has the admin permission, is the read permission implied? Probably, yes.

But this is not what we're describing here with MyPermission. I see no relation.

SecurityIdentity.checkPermission(Permission) I also understand: does the current user have the right to do X? Fine. That makes sense.

But again, this does not look like what MyPermission.implies is doing. Is this something related to the current PR, or the general security model in Quarkus?

[sberyozkin]

Hi @FroMage, re the earlier comment

I can understand implies as it's documented, which is about figuring out if, given an already granted permission, another one is implied by it. For example, given that the current user has the admin permission, is the read permission implied? Probably, yes.

admin is a role, not a permission. We'd just use @RolesAllowed(admin) here.

The permission.implies(anotherPermission) approach was discussed in depth with Stuart, probably also on quarkus-dev, and we should've got for a more specific feedback from you back then, but really it is about finer grained permission control, "read:all".implies("read:book") or something like that or, in many cases, simply, about a matching check.

In any case, I propose to have this PR merged as all it does it supports @BeanParam, and continue discussing and seeing what can be optimized/improved next in your issue you opened earlier, are OK with that ?

[sberyozkin]

LGTM, @michalvavrik agreed to look at the @FroMage's idea to have a QuarkusPermission simplification in a follow up PR

[sberyozkin]

Hi @FroMage, I've copied your idea text to your issue, it does look like we have a concrete plan now, proposing to merge this PR now for Michal to start working on simplifying and also evolving it further, possibly as suggested in your issue

[michalvavrik]

Updated docs example, hope it's better. Thanks for the patience.

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 393565a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 393565a.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

[FroMage]

OK fine, agree to merge this as is, but we really have to fix the Permission confusion before we release anything.

[michalvavrik]

@sberyozkin personally I'd add one sentence to the migration guide linking updated autodetect strategy https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/PermissionsAllowed.java#L34 but decision is yours. Validation exception is IMO explanative in case anyone is affected.