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

Move Keycloak Authorization Enforcer Tenant config to runtime and improve usability with aggregated policy enforcer paths #39512

Merged

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Mar 17, 2024

Changes:

This comment has been minimized.

Copy link

github-actions bot commented Mar 17, 2024

🙈 The PR is closed and the preview is expired.

This comment has been minimized.

@michalvavrik
Copy link
Member Author

ad integration-tests/oidc-client failure - failed over pulling docker image, not related

@michalvavrik michalvavrik requested a review from sberyozkin March 17, 2024 17:15
@michalvavrik michalvavrik force-pushed the feature/kc-authz-move-to-runtime branch from 275299f to 1d9cb03 Compare March 18, 2024 14:02
@michalvavrik
Copy link
Member Author

As said failures not related, but I've rebased on current main to (hopefully) get green CI. No other changes.

This comment has been minimized.

This comment has been minimized.

@michalvavrik
Copy link
Member Author

cc @pedroigor

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 would like @pedroigor to have a look

@sberyozkin
Copy link
Member

Hi @pedroigor, Michal's clarification re the body handler makes sense IMHO, so should be a safe update, with the tests passing

@sberyozkin
Copy link
Member

@michalvavrik Quick question, did you replace all occurrences of .path with .paths ? If yes, then I think we should keep at least one test keeping testing this deprecated property

@michalvavrik
Copy link
Member Author

@michalvavrik Quick question, did you replace all occurrences of .path with .paths ? If yes, then I think we should keep at least one test keeping testing this deprecated property

I made changes, rerun tests without adjustments to be sure it works and then I replaced all the occurrences. So I know it works at this moment. You are right, I'll revert at least one of them now.

@michalvavrik michalvavrik force-pushed the feature/kc-authz-move-to-runtime branch from 1d9cb03 to ff4f116 Compare March 20, 2024 11:45
@michalvavrik
Copy link
Member Author

Done.

Copy link

quarkus-bot bot commented Mar 20, 2024

Status for workflow Quarkus Documentation CI

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

✅ 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

quarkus-bot bot commented Mar 20, 2024

Status for workflow Quarkus CI

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

✅ 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 39e55c6 into quarkusio:main Mar 20, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.10 - main milestone Mar 20, 2024
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Mar 20, 2024
@cescoffier
Copy link
Member

This PR is not valid - it has a risk to register multiple body handlers.

@cescoffier
Copy link
Member

@sberyozkin @michalvavrik can you please amend it or revert it?

@michalvavrik
Copy link
Member Author

@sberyozkin @michalvavrik can you please amend it or revert it?

yes, but I'm just finishing PR that depends on this, hence I'd like to understand first. but sure, let's do it.

This PR is not valid - it has a risk to register multiple body handlers.

can you give some info how that will happen, please? I don't understand how situation has changed

@michalvavrik
Copy link
Member Author

btw I'd like instead of reverting to provide alternative solution because changes here are important for dynamic config resolver

@michalvavrik
Copy link
Member Author

@cescoffier I re-read changes and I am still missing how situation has changed in regards of multiple-times registering body handler. Could you please give me a hint so that I can fix it? Thank you

@cescoffier
Copy link
Member

You iterate over your supplier - if multiple return yes, you will register the body handler twice

@cescoffier
Copy link
Member

Ah there is break statement I didn't catch.

@michalvavrik
Copy link
Member Author

Yep. Thanks for checking the PR.

@cescoffier
Copy link
Member

When you change that class make sure you ask me to review. Typically, having an array is not homogeneous.

@michalvavrik
Copy link
Member Author

When you change that class make sure you ask me to review. Typically, having an array is not homogeneous.

Understood, will do that in the future.

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.

Quarkus keycloak authorization usability improvements.
3 participants