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.

Sign up for GitHub

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

Jump to bottom

Move HTTP Permissions and Roles policies from build-time to runtime #36874

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Move HTTP Permissions and Roles policies from build-time to runtime #36874

merged 1 commit into from
Nov 10, 2023

Conversation

michalvavrik
Copy link
Member

fixes: #12594

@quarkus-bot quarkus-bot bot added area/docstyle issues related for manual docstyle review area/documentation area/vertx labels Nov 4, 2023
@github-actions GitHub Actions
Copy link

github-actions bot commented Nov 4, 2023 •
edited
Loading

🎊 PR Preview fded7c0 has been successfully built and deployed to https://quarkus-pr-main-36874-preview.surge.sh/version/main/guides/

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@michalvavrik michalvavrik force-pushed the feature/move-http-perms-to-runtime branch from dda1ea1 to c840758 Compare November 5, 2023 15:27
@michalvavrik michalvavrik force-pushed the feature/move-http-perms-to-runtime branch from c840758 to e32e52e Compare November 5, 2023 18:08
sberyozkin
sberyozkin reviewed Nov 5, 2023
View reviewed changes
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.

I've looked through and it looks perfect, but I'll need a few more iterations to make sure I follow it. Thanks Michal, nicely researched and done

@sberyozkin
Copy link
Member

@stuartwdouglas This is a very important PR done by Michal, have a look if you get a chance, LGTM but I haven't looked very deeply yet

@sberyozkin
Copy link
Member

@michalvavrik So what do you think about making the named policy concept realized in a follow up PR ? I recommend but not insist, IMHO it would be better - it is an independent enhancement of its own. I'll let you decide

@michalvavrik
Copy link
Member Author

@michalvavrik So what do you think about making the named policy concept realized in a follow up PR ? I recommend but not insist, IMHO it would be better - it is an independent enhancement of its own. I'll let you decide

Problem is that we don't know whether someone else is not using io.quarkus.vertx.http.deployment.HttpSecurityPolicyBuildItem. This PR transform the build item to the named policy and I'd like to initialize HTTP permissions inside AbstractPathMatchingHttpSecurityPolicy constructor because of what I am preparing for #14047. So if you want to avoid named policy, I can transform the build item for example to a package-private bean class that users won't be able to create them via CDI...

I'll let you decide

If you let me decide, then I think because of the io.quarkus.vertx.http.deployment.HttpSecurityPolicyBuildItem it is easiest to keep it in this PR. But if you or Stuart find this PR hard to review, I'll drop it. Let me know if you change your mind, it's not a big deal.

@quarkus-bot quarkus-bot

This comment has been minimized.

Sign in to view
@sberyozkin
Copy link
Member

@michalvavrik Thanks, I'd find it easier to isolate the changes related to the configuration scope only if the named policy enhancement was done in another PR, it is hard to be certain otherwise if a given change is related to the move to the runtime scope or the named policy update

@michalvavrik michalvavrik force-pushed the feature/move-http-perms-to-runtime branch from e32e52e to 19ce35f Compare November 6, 2023 18:40
@michalvavrik michalvavrik changed the title Move HTTP Security Policies and Permissions from build-time to runtime Move HTTP Permissions and Roles policies from build-time to runtime Nov 6, 2023
@quarkus-bot quarkus-bot
Copy link

quarkus-bot bot commented Nov 6, 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.

@sberyozkin
Copy link
Member

I don't see anything concerning now, fine PR, important enhancement, lets have it settled a bit in 3.6.0 CR1

@sberyozkin sberyozkin merged commit 183a165 into quarkusio:main Nov 10, 2023
50 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Nov 10, 2023
@michalvavrik michalvavrik deleted the feature/move-http-perms-to-runtime branch November 10, 2023 12:22
This was referenced Nov 10, 2023
Closed
Closed
Merged
sebveit added a commit to lucimber/quarkus-mfa that referenced this pull request Mar 31, 2024
@sebveit
Adapt usage of moved HTTP permissions
222ead7 
HTTP Permissions and Roles policies have been moved from build-time to runtime.
See PR quarkusio/quarkus#36874

Signed-off-by: Sebastian Veit <[email protected]>
sebveit added a commit to lucimber/quarkus-mfa that referenced this pull request Apr 1, 2024
@sebveit
Adapt usage of moved HTTP permissions
94f0ff9 
HTTP Permissions and Roles policies have been moved from build-time to runtime.
See PR quarkusio/quarkus#36874

Signed-off-by: Sebastian Veit <[email protected]>
@sebveit sebveit mentioned this pull request Apr 1, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sberyozkin sberyozkin sberyozkin approved these changes

@stuartwdouglas stuartwdouglas Awaiting requested review from stuartwdouglas

Assignees
No one assigned
Labels
area/docstyle issues related for manual docstyle review area/documentation area/vertx kind/bugfix release/noteworthy-feature
Projects
Quarkus Documentation
Status: Done
Milestone
3.6.0.CR1
Development

Successfully merging this pull request may close these issues.

Quarkus Security issue in the Native Executable
2 participants
@michalvavrik @sberyozkin