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 authentication mechanism selection for a REST endpoint with annotation #36504

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Oct 16, 2023

closes: #34664

Right now, the only way to combine authentication mechanism is per path using HTTP permission - https://quarkus.io/guides/security-authentication-mechanisms#path-specific-authentication-mechanisms. This PR allows to do same per REST endpoint using annotations. This, combining with RBAC annotations should complement annotation-based approach with same capabilities as configuration-based approach has. And greater, for one path depending on consumed content type etc. can match different endpoints.

@michalvavrik michalvavrik force-pushed the feature/annotation-based-auth-mechanism-matching branch 2 times, most recently from 646d716 to 44a9bff Compare October 16, 2023 15:09
@sberyozkin
Copy link
Member

Super effort @michalvavrik, thanks for keeping fixing high impact issues 👍 .

Might be worth keeping in a Draft state for a bit of time

@github-actions
Copy link

github-actions bot commented Oct 16, 2023

🙈 The PR is closed and the preview is expired.

@michalvavrik
Copy link
Member Author

Super effort @michalvavrik, thanks for keeping fixing high impact issues 👍 .

Thank you Sergey.

Might be worth keeping in a Draft state for a bit of time

  • Let's see how CI runs, of course I run every test I added, but I can imagine with this volume of changes I could forget stuff.
  • I had issues to build documentation locally, so I can imagine I'll push changes when I can see it build and you find a time to review this => it will take couple of rounds anyway, so people will have time to comment.

Anyway, when we go through ^^^ and folk have time to suggest changes (cc @FroMage :-)), I don't see a reason to keep this in draft. You can see there is lot of file changes, though vast majority are tests. For example I won't touch issue on HTTP permissions runtime migration before this is merged in order to avoid merge conflicts.

@quarkus-bot

This comment has been minimized.

Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

That's a nice feature. Not sure I'll use it, but it seems useful.

@michalvavrik michalvavrik force-pushed the feature/annotation-based-auth-mechanism-matching branch 2 times, most recently from 078308b to 6b586ff Compare October 21, 2023 15:24
@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/annotation-based-auth-mechanism-matching branch from 6b586ff to c1f7ffa Compare October 22, 2023 21:54
@quarkus-bot

This comment has been minimized.

@michalvavrik
Copy link
Member Author

As far as failures go, you can see that only thing I changed was one line in documentation and previously we had different failures, so it's flaky.

@michalvavrik michalvavrik force-pushed the feature/annotation-based-auth-mechanism-matching branch from c1f7ffa to 2226886 Compare October 23, 2023 07:32
@quarkus-bot

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/annotation-based-auth-mechanism-matching branch 5 times, most recently from 1f127d2 to 789e5cd Compare April 2, 2024 20:35

This comment has been minimized.

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/annotation-based-auth-mechanism-matching branch from 789e5cd to 24c41ae Compare April 4, 2024 14:21

This comment has been minimized.

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.

LGTM, but please update the documentation

This comment has been minimized.

@michalvavrik michalvavrik force-pushed the feature/annotation-based-auth-mechanism-matching branch from 24c41ae to 1373ed0 Compare April 4, 2024 19:18
Copy link

quarkus-bot bot commented Apr 4, 2024

Status for workflow Quarkus Documentation CI

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

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

⚠️ There are other workflow runs running, you probably need to wait for their status before merging.

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.

Great work, thanks @michalvavrik

Copy link

quarkus-bot bot commented Apr 4, 2024

Status for workflow Quarkus CI

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

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

@sberyozkin sberyozkin merged commit 4b8a27a into quarkusio:main Apr 5, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Apr 5, 2024
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Apr 5, 2024
@michalvavrik michalvavrik deleted the feature/annotation-based-auth-mechanism-matching branch April 5, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

Provide a way to specify HttpAuthenticationMechanism per JAX-RS resource
3 participants