-
Notifications
You must be signed in to change notification settings - Fork 142
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
Keycloak 25.0.1 #1052
Keycloak 25.0.1 #1052
Conversation
b182b2c
to
8bbef3a
Compare
Quality Gate passedIssues Measures |
@@ -86,7 +87,7 @@ public void createExecutionFlow( | |||
logger.trace("Create non-top-level-flow in realm '{}' and top-level-flow '{}'", realmName, topLevelFlowAlias); | |||
|
|||
AuthenticationManagementResource flowsResource = authenticationFlowRepository.getFlowResources(realmName); | |||
flowsResource.addExecutionFlow(topLevelFlowAlias, executionFlowData); | |||
flowsResource.addExecutionFlow(topLevelFlowAlias, new HashMap<>(executionFlowData)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the order of the executionFlowData relevant here? If it is, then a LinkedHashMap that preserves the order would be more suitable here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @thomasdarimont, as the source type is also HashMap, I suggest that the order is not important ;)
It seems that some tests related to requiredActions are now failing because Keycloak 25 now requires that a required action with the given providerId actually exist: org.keycloak.services.resources.admin.AuthenticationManagementResource#registerRequiredAction Keycloak 24.0.5 An easy fix could be to just create mock implementations for the custom requiredactions and add them to the Keycloak test container. |
I gave this a quick spin and added the required dummy extensions to the test execution. There seem to be a few incompatible changes in the admin-client libraries and more strict validation on authorization resources... but that's something for another day. |
Hi @thomasdarimont, thanks a lot for that insight, next time I need to catch the BadRequestException and display the error message. Do you think, it may be possible to include the extensions in a kind that doesn't require extension of the CI-scripts? Unfortunately I am short on time right now, I guess I won't finish the task :( Best Regards |
Thanks @jonasvoelcker and @thomasdarimont for your contributions. It would be nice to finish the Keycloak 25 Upgrade as quick as possible. The current state works on a Keycloak 25 instance on my machine. However, a lot of tests are still failing. @thomasdarimont your fix somehow does not work on my machine. Maybe you can give some insights on what to do next? |
Hi @daviddavidgit, I am pretty sure that I won't be able to finish the task in the next few days. Also I am sure that it needs some decision about where the tool aims at. Do we want to test with custom Required Actions or not? If we change the tests to standard RAs the extension idea of @thomasdarimont would not be needed. @francis-pouatcha @st3v0rr What do you think about this? |
I think it's fine to change the tests since "unknown" required actions are now considered invalid by Keycloak 25. How about either excluding the individual tests for 25+ or use an existing required action that's not enabled by default? |
I analyzed one of the other failing tests |
Hi @bohmber, I've invested some time on that issue now. I don't see any chance we got to keep that functionality working as the ResourcesResource only has the id-method to get the ResourceResource. If we don't have the id at hand we are kind of lost. Do you mind to open an issue in the Keycloak-Repo so we might get some alternative or a revert? Best Regards |
Hi @thomasdarimont, do you maybe know a solution for the missing id to get the ResourceResource from ResourcesResource? Or could you maybe speed up things if it is an actual error inside keycloak? |
Issue created in keycloak keycloak/keycloak#30519 |
@bohmber Thank you, I've added some detail as comment. |
@jonasvoelcker Why exportSettings is called instead of getSettings of AuthorizationResource in ClientRepository? |
Hi @st3v0rr @francis-pouatcha, could we please review this one? |
0469ae0
to
2102980
Compare
Signed-off-by: Jonas Voelcker <barmer@jonas-voelcker.de>
Signed-off-by: Jonas Voelcker <barmer@jonas-voelcker.de>
…e identity providers are replaced by stars Signed-off-by: Jonas Voelcker <barmer@jonas-voelcker.de>
…ow search for the name. Signed-off-by: Jonas Voelcker <barmer@jonas-voelcker.de>
Signed-off-by: Jonas Voelcker <barmer@jonas-voelcker.de>
Signed-off-by: Jonas Voelcker <barmer@jonas-voelcker.de>
1fda99c
to
391f091
Compare
Thanks for picking this up again! Just noticed that the version number is still 5.x in the branch, but main is already on 6.x. |
Just tested this locally, works great with Keycloak 25.0.1, great work!
Example realm config file: realms/test.yml:
|
One thing that is still missing is to configure the @jonasvoelcker I created jonasvoelcker#1 to add support for this. |
Signed-off-by: Thomas Darimont <thomas.darimont@googlemail.com>
I'm at a conference today, so next week might be better to speak via teams. If the UPConfig is missing, we could just copy the class into the lib and use a custom rest call to set the config.
And add special handling for a map entry like "unamangedAttribitePolicy":[ {"value":"ENABLED"}] -> flatten that value when rendering the json. This would allow us to support the ne property without having to introduce the type. The dedicated userProfile config option was introduced to make it easier for user to config user profile without using the clumsy component syntax. |
Signed-off-by: Jonas Voelcker <barmer@jonas-voelcker.de>
Hi @bohmber, I've adapted the proposal from @thomasdarimont, if you see any problem in it, I might also roll it back. But I guess we should find a permanent solution, which fits all needs then ;) |
Hi @jonasvoelcker, looks ok so far, thanks |
@jonasvoelcker that works for me too! Thank you very much :) |
@@ -22,7 +22,7 @@ docker run -d --rm \ | |||
"quay.io/keycloak/keycloak:${KEYCLOAK_VERSION}" \ | |||
start-dev | |||
|
|||
while ! docker exec keycloak-export bash -c '/opt/keycloak/bin/kcadm.sh config credentials --server http://$HOSTNAME:8080/auth --realm master --user $KEYCLOAK_USER --password $KEYCLOAK_PASSWORD'; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not need an user and password anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mme-flendly,
the problem is, that we don't have a user in the env-vars and the Keycloak-API does not take empty values any more. This is only a helper script to generate the realm exports for the test suite as well as changing some files towards the new version so it's not part of the deliverable.
ClientResource clientResource = getResourceById(realmName, id); | ||
clientResource.authorization().resources().resource(resourceId).remove(); | ||
String resourceId = getResourceId(clientResource, resourceName); | ||
if (resourceId != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No warning, error or log when the resourceId was not found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion idempotency requires not existing entries not to throw errors when they get deleted. I am not sure if this needs a debug log or not but it doesn't need a warn log.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everytime there is an error with keycloak-config-cli we need to switch to debug log. I think there should be more warn logs in case of something unexpected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bohmber,
you are free to push changes and open pull requests as well. 😉
clientResource.authorization().policies().policy(policyId).remove(); | ||
String policyId = getPolicyId(clientResource, policyName); | ||
if (policyId != null) { | ||
clientResource.authorization().policies().policy(policyId).remove(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, no warn, error or log
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanation above 😉
assertThat(myForms2.isUserSetupAllowed(), is(false)); | ||
assertThat(myForms2.isAutheticatorFlow(), is(true)); | ||
|
||
if (VersionUtil.ge(KEYCLOAK_VERSION, "25")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there some changes regarding the order/priority in the flows in version 25?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the flows get a priority behind the highest execution, which is 25. This seems more reasonable to me than just adding some kind of random numbers before Keycloak 25.
assertThat(myForms.isUserSetupAllowed(), is(false)); | ||
assertThat(myForms.isAutheticatorFlow(), is(true)); | ||
|
||
if (VersionUtil.ge(KEYCLOAK_VERSION, "25")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there some changes regarding the order/priority in the flows in version 25?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Explanation above 😉
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #1050 #1051Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR