Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support @PermissionAllowed for @BeanParam parameter #43231

Closed
ChMThiel opened this issue Sep 12, 2024 · 15 comments · Fixed by #43353
Closed

Support @PermissionAllowed for @BeanParam parameter #43231

ChMThiel opened this issue Sep 12, 2024 · 15 comments · Fixed by #43353
Assignees
Labels
area/security kind/enhancement New feature or request
Milestone

Comments

@ChMThiel
Copy link

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

Copy link

quarkus-bot bot commented Sep 12, 2024

/cc @pedroigor (bearer-token)

@geoand
Copy link
Contributor

geoand commented Sep 13, 2024

cc @FroMage

@michalvavrik
Copy link
Member

michalvavrik commented Sep 13, 2024

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?

@michalvavrik michalvavrik self-assigned this Sep 13, 2024
@ChMThiel
Copy link
Author

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 😉

@michalvavrik
Copy link
Member

michalvavrik commented Sep 13, 2024

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
Copy link
Member

michalvavrik commented Sep 13, 2024

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
Copy link
Author

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
Copy link
Member

FroMage commented Sep 13, 2024

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
Copy link
Member

FroMage commented Sep 13, 2024

I'd prefer aBeanParam.organizationUnitId. Thoughts?

@michalvavrik agreed.

@ChMThiel
Copy link
Author

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
Copy link
Member

FroMage commented Sep 13, 2024

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

@sberyozkin
Copy link
Member

@ChMThiel The JAX-RS parameter name is the bean parameter name, I'm not sure we have to start unwrapping it

IMHO the parameter-naming conventions should be the same, regardless if it is a directly defined method-parameter or capsulated in a BeanParam.

Why ? The custom permission class is not a JAX-RS thing. What is difficult or not dev friendly about accessing a query parameter as bean.getMyQueryParam() ?
Users choose to use BeanParam to have multiple parameters nicely wrapped in a single container. IMHO we should not try to unwrap it all for custom permissions

@ChMThiel
Copy link
Author

Without unwrapping we need a separate Permission-Class for each BeanBaram-Class containing the parameter (in my example 'aOrganinzationUnitId').
We have hundrets of APIs that take such a parameter but almost every of the API combines it's parameters in it's own BeanParam-class.
If unwrapping will not be implemented the only workaround would by to create an interface that all of our BeanParam-classes implement and to use this interface in the Permission-class (but i did not try that yet. not sure if working with an interface works here)

@sberyozkin
Copy link
Member

I see, I got it now, thanks @ChMThiel, @michalvavrik

@michalvavrik
Copy link
Member

Without unwrapping we need a separate Permission-Class for each BeanBaram-Class containing the parameter (in my example 'aOrganinzationUnitId'). We have hundrets of APIs that take such a parameter but almost every of the API combines it's parameters in it's own BeanParam-class. If unwrapping will not be implemented the only workaround would by to create an interface that all of our BeanParam-classes implement and to use this interface in the Permission-class (but i did not try that yet. not sure if working with an interface works here)

It's almost as if I wrote it :-D +1

@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment