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

Add annotation to allow using custom CDI bean methods as permission checkers #43846

Conversation

michalvavrik
Copy link
Member

This PR is ready for review, however CI cannot run until quarkusio/quarkus-security#56 is merged and Quarkus Security API is released and bumped in the Quarkus project.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-user-exp-improvements branch from 903b9f0 to 793a99f Compare October 13, 2024 20:55

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@sberyozkin
Copy link
Member

Thanks @michalvavrik, sorry for the delay, will start looking tomorrow, need to sign off now

@michalvavrik
Copy link
Member Author

Thanks @michalvavrik, sorry for the delay, will start looking tomorrow, need to sign off now

no worry, if don't have API anyway (I run all the related tests locally, so I don't expect CI issues), take your time, this won't be easy to review even though this PR consist mostly of tests

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-user-exp-improvements branch from 793a99f to 484114f Compare October 14, 2024 23:05

This comment has been minimized.

This comment has been minimized.

@sberyozkin sberyozkin self-requested a review October 15, 2024 18:05
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be a massive feature, thanks @michalvavrik

@michalvavrik
Copy link
Member Author

michalvavrik commented Oct 29, 2024

I've addressed @FroMage and @cescoffier comments (thanks!) and added required validations and their tests. Specifically I believe extensions/resteasy-reactive/rest/deployment/src/test/java/io/quarkus/resteasy/reactive/server/test/security/PermissionCheckerOnEndpointValidationFailureTest.java addresses @gsmet concern that @PermissionChecker and @PermissionsAllowed could be confused with each other. I also pinged Guillaume and asked for a Quarkus Security release when the time is right for him.

This comment has been minimized.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-user-exp-improvements branch from f500592 to 6aa71ec Compare October 29, 2024 17:27
@michalvavrik
Copy link
Member Author

Although I made changes when there was electricity outage, now that I rerun all the test, there was ugly hack on ExecutionModelAnnotationsProcessor that I had to workaround so that this validation would work. It works now, I am not happy about this though.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-user-exp-improvements branch from 6aa71ec to 5c78754 Compare October 29, 2024 17:30

This comment has been minimized.

This comment has been minimized.

@michalvavrik
Copy link
Member Author

No sorry, ExecutionModelAnnotationsAllowedBuildItem is not reliable check whether method is an entrypoint or not as it has false positives for checkers declared on resources. I'll just drop that validation.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-user-exp-improvements branch from 5c78754 to 776d47c Compare October 29, 2024 17:46

This comment has been minimized.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-user-exp-improvements branch from 776d47c to 3d5a7cf Compare October 30, 2024 09:33

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/permissions-allowed-user-exp-improvements branch from 3d5a7cf to ee1c852 Compare October 30, 2024 10:20
Copy link

quarkus-bot bot commented Oct 30, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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.

Copy link

github-actions bot commented Oct 30, 2024

🙈 The PR is closed and the preview is expired.

Copy link

quarkus-bot bot commented Oct 30, 2024

Status for workflow Quarkus CI

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

✅ 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.

@michalvavrik
Copy link
Member Author

CI is green!

@sberyozkin
Copy link
Member

Thanks @michalvavrik for completing this important enhancement.

@gsmet Are you still somewhat concerned with how it will all stand against the code evolution or have some other concerns ? I think we are ready to merge otherwise as @cescoffier has also approved.

Note we hope some internal code made to support this PR may go, with more simplifications planned

@michalvavrik
Copy link
Member Author

Thanks @michalvavrik for completing this important enhancement.

@gsmet Are you still somewhat concerned with how it will all stand against the code evolution or have some other concerns ? I think we are ready to merge otherwise as @cescoffier has also approved.

Note we hope some internal code made to support this PR may go, with more simplifications planned

I think @gsmet won't be around for next 4 days starting tomorrow. It has been 2 weeks since we discussed it here quarkusio/quarkus-security#56.

While I am personally fine with waiting for whatever time, I don't believe it is necessary. It takes but a minute to give heads up that you need more time to review. I will try to address any concerns raised whether it is now or when PR is merged. Thanks

@michalvavrik
Copy link
Member Author

That said, let's wait, I didn't mean to press you :-)

@sberyozkin
Copy link
Member

Sure, let me merge, @gsmet, please ping us any time to clarify the details, a brief summary of the PR:

  • Any @PermissionAllowed restriction on a JAX-RS endpoint can be approved/denied with a boolean method which accepts one or more request parameters and/or the already verified security identity and is annotated with a matching @PermissionChecker.
  • @michalvavrik added a lot of tests. 2 specific tests which may help address your earlier concerns are about failing when 1) @PermissionChecker is attached by mistake to the JAX-RS method and 2) @PermissionChecker does not have a matching @PermissionAllowed (correctly placed on the JAX-RS method)

@sberyozkin sberyozkin merged commit 4968e01 into quarkusio:main Nov 4, 2024
35 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 4, 2024
@michalvavrik michalvavrik deleted the feature/permissions-allowed-user-exp-improvements branch November 4, 2024 12:21
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.

Idea for custom permission annotations that can work on user model and endpoint parameters
4 participants