Detect permission constructor parameters based on formal method names match with a method secured by the @PermissionsAllowed annotation

[michalvavrik]

Proposes to detect parameters of a custom java.security.Permission based on matching formal parameter names between Permission constructor and a method secured with the @PermissionsAllowed annotation. For ambiguous scenarios, user can specify params attribute explicitly like @PermissionsAllowed(params = "beanParam.nested"). This allows to determine concrete secured method parameter that needs to be passed to a Permission specified via permisson attribute, like @PermissionsAllowed(permission = "CustomPermission").

This is motivated by discussion in quarkusio/quarkus#43353.

[FroMage]

This seems to be missing the implementation change. I'd suggest full names for the constants, so PARAMETER instead of `PARAM.

[michalvavrik]

This seems to be missing the implementation change.

Autodetection based on parameter names is implemented and it is already done, what is different at the moment is that: it's only activated when you specify @PermissionsAllowed(params = "something"). It is in the Quarkus, but extra validation to improve user experience had to be done.

I'd suggest full names for the constants, so PARAMETER instead of `PARAM.

Will change that, thank you.

[michalvavrik]

This seems to be missing the implementation change.

But yeah, it can only be merged/released/bumped to Quarkus when quarkusio/quarkus#43353 is finalized and approved, not before. Sorry I didn't mention it in the PR description, but I thought Sergey knew that.

[sberyozkin]

I have a feeling we should not be really introducing these strategies but should simply enhance Quarkus directly to cover ambiguities. In many cases, just having BeanParam alone can handle all the ambiguities with multiple String types for example.

Otherwise, in those advanced cases where passing a BeanParam bean is not enough, expecting a precise name match is a totally normal expectation if a user, having 5 String JAX-RS parameters, only wants the 1st and last String parameter at the custom Permission level. Matching by types alone just does not work.

[sberyozkin]

Michal, are you concerned that we have JAX-RS: String a but a custom permission having String theAValue ? Does any of our examples suggest such an option ? If not, we should just fix it to use names without having to deal with all the breaking changes flow, where in fact, we are just making it work properly in all the variations

[sberyozkin]

@michalvavrik We can discuss it in a wider forum, and I guess I can add a breaking change note to the Migration guide, we just don't need an API change to support types only IMHO, this option's robustness is very limited

[michalvavrik]

Michal, are you concerned that we have JAX-RS: String a but a custom permission having String theAValue ?

Yes, that is exactly my concern. I agree with above-mentioned arguments (your and Stephane comments).

Does any of our examples suggest such an option ?

I have checked https://quarkus.io/guides/security-authorize-web-endpoints-reference#permission-annotation and only problematic part I can see is one sentence The formal parameter update is identified as the first Library parameter and gets passed to the LibraryPermission class. .

If not, we should just fix it to use names without having to deal with all the breaking changes flow, where in fact, we are just making it work properly in all the variations

What we can do is to do matching based on parameter names. If there is not a one match based on name, we can fallback to matching based on the parameter types. I think we should drop behavior where it's identified based on data type if multiple strategies like proposed in this PR are not desirable. However this is why I think it is breaking change:

@PermissionsAllowed(permission=Custom.class)
public String getSomething(Library update, Library create) {

}

# and permission constructor

Custom(Library create) {}

that previously would be update and now is create.

[michalvavrik]

However, that create update example is hopefully edge case. I believe if I add there validation exception that explains permission constructor and secured method parameters are not matching, it should be alright and users will know how to fix it easily. Let's drop autodetection based on parameter types completely. I'll integrate it.

[michalvavrik]

@sberyozkin @FroMage updated API in this PR, now I'll update quarkusio/quarkus#43353 accordingly; comment if you want, or wait till the implementation is done

[sberyozkin]

Thanks @michalvavrik I think it looks good, let me just have another look early next week :-)

@cescoffier, FYI, here, Michal is only updating the JavaDocs advising how to ensure the JAX-RS parameter names match the custom permission constructor parameters, if you'd like, please comment

[sberyozkin]

@michalvavrik, I've just looked again, and it is really all very well explained, and indeed, like you said, that create update example is hopefully edge case. - definitely very edge case :-)

[sberyozkin]

Let's keep the PR itself open for a while till you get time to implement it in Quarkus, and in case some more comments will follow here, thanks for your help, @michalvavrik

[cescoffier]

Could you rephrase the PR to highlight that it's a javadoc enhancement.

[michalvavrik]

Could you rephrase the PR to highlight that it's a javadoc enhancement.

Rewrote the PR title and description. If additional adjustments are needed, please let me know.