quarkusio / quarkus

Quarkus: Supersonic Subatomic Java.
https://quarkus.io
Apache License 2.0
13.57k stars 2.63k forks source link

Support @PermissionAllowed for @BeanParam parameter #43231

Open ChMThiel opened 5 days ago

ChMThiel commented 5 days ago

Description

A JAX-RS endpoint can be secured with a custom Permission with @PermissionAllowed annotation. Which parameters of the JAX-RS-method are passed to the Permission-constructor can be defined with the params-property of the PermissionAllowed annotation.

In following example the path-param 'id' UUID-param is passed to the Permission-class constructor-param 'aOrganizationUnitId'.

@GET
@Path("{id}/2params")
@PermissionsAllowed(value = "read", permission = OrganizationUnitIdPermission.class, params = "id") 
 public OrganizationUnit find2(@PathParam("id") UUID aOrganizationUnitId, @QueryParam("second") UUID aSecondUUIDParam) {
       ...
 }

public class OrganizationUnitIdPermission extends Permission {

    private final UUID organizationUnitId;

    public OrganizationUnitIdPermission(String aName, String[] aActions, UUID aOrganizationUnitId) {
        super(aName);
        organizationUnitId = aOrganizationUnitId;
    }
...

This works currently not with @BeanParam parameters. The attempt to secure a BeanParam-JAX-RS method like here:

@GET
@Path("{id}/beanparam")
@PermissionsAllowed(value = "read", permission = OrganizationUnitIdPermission.class, params = "id")
public OrganizationUnit findBeanParam(@BeanParam PermissionParam aBeanParam) {
        return new OrganizationUnit().setName("TESTDUMMY");
}

public class PermissionParam {
    @PathParam("id") UUID organizationUnitId;
    @QueryParam("second") UUID secondUUIDParam;
}

fails with

Failed to build quarkus application: io.quarkus.builder.BuildException: Build failure: Build failed due to errors
    [error]: Build step io.quarkus.security.deployment.SecurityProcessor#gatherSecurityChecks threw an exception: java.lang.RuntimeException: No 'findBeanParam' formal parameter name matches 'io.gec.smom.sample.boundary.OrganizationUnitIdPermission' constructor parameter name 'aOrganizationUnitId' specified via '@PermissionsAllowed#params'
    at io.quarkus.security.deployment.PermissionSecurityChecks$PermissionSecurityChecksBuilder$PermissionCacheKey.userDefinedConstructorParamIndexes(PermissionSecurityChecks.java:652)
    at io.quarkus.security.deployment.PermissionSecurityChecks$PermissionSecurityChecksBuilder$PermissionCacheKey.<init>(PermissionSecurityChecks.java:621)
    at io.quarkus.security.deployment.PermissionSecurityChecks$PermissionSecurityChecksBuilder.createPermission(PermissionSecurityChecks.java:408)
    at io.quarkus.security.deployment.PermissionSecurityChecks$PermissionSecurityChecksBuilder.createPermissionPredicates(PermissionSecurityChecks.java:149)
    at io.quarkus.security.deployment.SecurityProcessor.gatherSecurityAnnotations(SecurityProcessor.java:733)
    at io.quarkus.security.deployment.SecurityProcessor.gatherSecurityChecks(SecurityProcessor.java:580)
    at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:733)
    at io.quarkus.deployment.ExtensionLoader$3.execute(ExtensionLoader.java:854)
    at io.quarkus.builder.BuildContext.run(BuildContext.java:256)
    at org.jboss.threads.ContextHandler$1.runWith(ContextHandler.java:18)
    at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
    at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
    at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
    at java.base/java.lang.Thread.run(Thread.java:1570)
    at org.jboss.threads.JBossThread.run(JBossThread.java:483)

There is afaik no way to bind the beanParam-property of the JAX-RS-method to the constructor-param of the Permission.

Implementation ideas

No response

quarkus-bot[bot] commented 5 days ago

/cc @pedroigor (bearer-token)

geoand commented 4 days ago

cc @FroMage

michalvavrik commented 4 days ago

I think we should do that. Will put this on my list. One thing I am not sure is id you used in params = "id". In your example, it refers to @PathParam("id"), but so far, params always refer to the formal parameter name. I think it could be confusing. Maybe we could do either aBeanParam.organizationUnitId or simply organizationUnitId. The latter could be ambiguous. I'd prefer aBeanParam.organizationUnitId. Thoughts?

ChMThiel commented 4 days ago

IMHO the parameter-naming conventions should be the same, regardless if it is a directly defined method-parameter or capsulated in a BeanParam. In my tests, i used the same String used as @PathParam-name in the @PermissionAllowed-params:

@GET
@Path("{id}/2params")
@PermissionsAllowed(value = "read", permission = OrganizationUnitIdPermission.class, params = "id") 
public OrganizationUnit find2(@PathParam("id") UUID aOrganizationUnitId, @QueryParam("second") UUID aSecondUUIDParam) {
       ...
 }

I just checked: using aOrganizationUnit as params also works. I don't know if that is correct by any definition, but it's convinient :wink:

michalvavrik commented 4 days ago

In my tests, i used the same String used as @PathParam-name in the @PermissionAllowed-params:

yeah, honestly, I really hoped your example was a typo. I wrote decent Javadoc to it that describes expected behavior https://github.com/quarkusio/quarkus-security/blob/main/src/main/java/io/quarkus/security/PermissionsAllowed.java#L156

I think I forgot to add validation exception when id is not found and there is at least one UUID. The aOrganizationUnitId is chosen because it has the first parameter type assignable to UUID and inside your OrganizationUnitIdPermission you have matching name in constructor: UUID aOrganizationUnitId

TL;DR; it's documented and works for you as expected, but I think we need to inform user if he specifically types id and it's not found.

I'll add it when I get to implementing this issue (which won't be immediately). It's more of user experience improvement than bug.

michalvavrik commented 4 days ago

I'd expect you can just drop params = id and it will work anyway on the same principle, but aOrganizationUnitId would be right value if you don't want to rely on order of params.

ChMThiel commented 4 days ago

TL;DR; it's documented and works for you as expected, but I think we need to inform user if he specifically types id and it's not found.

👍

FroMage commented 4 days ago

Seeing this, I'm curious about how you populate your user's permissions. Out of curiosity, @ChMThiel how do you add the permissions to you user identity for all the aOrganizationUnitId? Are these coming from Keycloak or a DB or what exactly?

FroMage commented 4 days ago

I'd prefer aBeanParam.organizationUnitId. Thoughts?

@michalvavrik agreed.

ChMThiel commented 4 days ago

Seeing this, I'm curious about how you populate your user's permissions. Out of curiosity, @ChMThiel how do you add the permissions to you user identity for all the aOrganizationUnitId? Are these coming from Keycloak or a DB or what exactly?

We are currently workin on a POC for restricting User's access to certain OrganizationUnits. Which user may acces what will be stored in a DB.

FroMage commented 4 days ago

OK, so you have those stored in your DB, so they're in your model. Thanks.