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

Allow permission checks via @PermissionsAllowed security annotation #31345

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Feb 22, 2023

closes: #10988

Introduces new @PermissionsAllowed annotation. I'd suggest to read security-authorize-web-endpoints-reference.adoc this PR modifies for details on this PR (as it's a lot of text).

@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-security-annotation branch from 3a525d5 to dd747b4 Compare February 23, 2023 17:51
@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-security-annotation branch from d8287e8 to 9421b80 Compare March 8, 2023 11:33
@github-actions
Copy link

github-actions bot commented Mar 8, 2023

🙈 The PR is closed and the preview is expired.

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-security-annotation branch 2 times, most recently from 4be7f6a to dd19dd7 Compare March 10, 2023 18:33
@quarkus-bot

This comment has been minimized.

@sberyozkin
Copy link
Member

sberyozkin commented Mar 13, 2023

@michalvavrik With the latest doc update it is much clearer, thanks. And there is no need to do any further enhancements, please see why below.
I have to admit,

CAUTION: Passing method parameters to a custom `Permission` constructor is not possible on RESTEasy Reactive endpoints, because there, security checks are done before serialization.

has confused me in a big way, as your example shows how it actually works now in RestEasy reactive flow - the only thing that is required to make it work is to have @PermissionAllowed shifted from the JAX-RS resource method to the bean definition and delegate from this method to that bean - to me it does work for Resteasy Reactive endpoints in a broad sense, not talking about controllers, model, etc, since the delegation to the CDI bean is an implementation detail of how this JAX-RS method is implemented.

I'd like to propose to change

`CAUTION: Passing method parameters to a custom `Permission` constructor is not possible on RESTEasy Reactive endpoints, because there, security checks are done before serialization.`

to

CAUTION: If you would like to pass method parameters to a custom `Permission` constructor from RESTEasy Reactive endpoints, make sure you have `@PermissionAllowed` annotation set not on the JAX-RS resource method itself but on the injected CDI bean to which this method will delegate to. Setting `@PermissionAllowed` on the JAX-RS resource method will not work because RestEasy Reactive performs the security checks before the deserilaization. For example:

Something like that would make it a bit clearer IMHO, thanks

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-security-annotation branch from dd19dd7 to 9dd8cf1 Compare March 13, 2023 21:07
@michalvavrik
Copy link
Member Author

Done, thank you.

@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-security-annotation branch from 9dd8cf1 to 3de45c1 Compare March 14, 2023 08:04
@quarkus-bot
Copy link

quarkus-bot bot commented Mar 14, 2023

✔️ 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.

@michalvavrik
Copy link
Member Author

Hey @stuartwdouglas , it seems like we concluded the docs discussion, please find a time to review this PR, thank you.

@sberyozkin
Copy link
Member

@michalvavrik Thanks, just wanted to approve and found I approved earlier, but now it is really approved :-)

@stuartwdouglas, please approve, would be awesome to get it into the next 3 alpha release

@sberyozkin sberyozkin merged commit 84340df into quarkusio:main Mar 15, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Mar 15, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Mar 15, 2023
@sberyozkin
Copy link
Member

Hey @michalvavrik Thanks for this work, can you please look at having the mapping done at the HTTP security policy between roles and permissions ? It probably makes sense to prioritize on this task just to make this feature you created a bit more accessible, for the users to avoid having to write custom augmentors to register. I'll look next at the mapping between OIDC token scopes and permissions

@michalvavrik
Copy link
Member Author

michalvavrik commented Mar 15, 2023

Hey @michalvavrik Thanks for this work, can you please look at having the mapping done at the HTTP security policy between roles and permissions ? It probably makes sense to prioritize on this task just to make this feature you created a bit more accessible, for the users to avoid having to write custom augmentors to register. I'll look next at the mapping between OIDC token scopes and permissions

Sure @sberyozkin , I'll work on that over weekend. Before that, I'll open some discussion in #12219 as I think this is opinionated subject and would like to here a feedback for my proposals I'll prepare.

BTW. I also have docs enhancement for Permissions and Keycloak authorizer in progress.

@michalvavrik michalvavrik deleted the feature/permissions-allowed-security-annotation branch March 15, 2023 10:35
@sberyozkin
Copy link
Member

Thanks @michalvavrik , but I do hope you won't lose your weekend on it, please have a good rest

@nikosk686
Copy link

What is the target release for this feature please?

@michalvavrik
Copy link
Member Author

@nikosk686 Quarkus 3, there are still follow-ups in progress.

@nikosk686
Copy link

@michalvavrik I see, thanks! I was hopping that this will be part of the next 2.x release.
I guess I ll have to work this around with some custom interceptor until we move on to v3.
Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

offer full fledged, simple to use Permissions
4 participants