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

Keycloak 26.0.0 #1162

Merged
merged 37 commits into from
Nov 15, 2024
Merged

Keycloak 26.0.0 #1162

merged 37 commits into from
Nov 15, 2024

Conversation

ma1uta
Copy link
Contributor

@ma1uta ma1uta commented Oct 13, 2024

What this PR does / why we need it:

Add support for Keycloak 26.0.0

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #1160

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR

Copy link
Member

@francis-pouatcha francis-pouatcha left a comment

Choose a reason for hiding this comment

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

Can you please update the change log.

@thomasdarimont
Copy link
Contributor

I just tested this branch and it worked for me.

However, I didn't test any KC26 specific configuration.

@thomasdarimont
Copy link
Contributor

I also think we can drop the support for KC 18.x with the upgrade to KC26.

@SmithJosh
Copy link

Tested and this works for us as well. There was one bug we noticed though caused by this change in Keycloak 26 https://www.keycloak.org/docs/latest/release_notes/index.html#identity-providers-no-longer-available-from-the-realm-representation. The realm representation no longer contains identity provider mappers and so keycloak-config-cli tries to recreate them even if they already exist

@thomasdarimont
Copy link
Contributor

thomasdarimont commented Oct 25, 2024

Tested and this works for us as well. There was one bug we noticed though caused by this change in Keycloak 26 keycloak.org/docs/latest/release_notes/index.html#identity-providers-no-longer-available-from-the-realm-representation. The realm representation no longer contains identity provider mappers and so keycloak-config-cli tries to recreate them even if they already exist

I also stumbled upon this, according to https://www.keycloak.org/docs/26.0.2/upgrading/#identity-providers-no-longer-available-from-the-realm-representation we now need to query the endpoint /realms/{realm}/identity-provider/instances to handle this.

@SmithJosh I just gave this another try and it worked for me.

I did the following:

  • Created a fresh KC 26.0.2 env
  • I applied a realm config without IdPs
  • I added two IdPs to the realm config and applied the config -> IdPs were created correctly in the realm
  • Changed the display name of the IdP in realm config to trigger an update -> IdPs were updated correctly in the realm
  • I commented out one IdP in the realm config, applied the config -> IdP was removed
  • Manually added the IdP in the UI again
  • I removed the comment from the IdP in realm config, applied the config -> IdP was updated again

Which IdP update scenario did not work for you?
I found a few places where org.keycloak.representations.idm.RealmRepresentation#getIdentityProviders() is used, e.g. de.adorsys.keycloak.config.factory.UsedAuthenticationFlowWorkaroundFactory.UsedAuthenticationFlowWorkaround#disableFirstBrokerLoginFlowsIfNeeded(..).
I think those usages (except RealmImport) have to be adapted to the new API, e.g. change:

List<IdentityProviderRepresentation> identityProviders = existingRealm.getIdentityProviders();
...

To:

List<IdentityProviderRepresentation> identityProviders = identityProviderRepository.getAll(existingRealm.getRealm());

@thomasdarimont
Copy link
Contributor

I updated my branch https://github.com/thomasdarimont/keycloak-config-cli/tree/update/keycloak-26.0.x with the fixed IdentityProviders lookup.

@thomasdarimont
Copy link
Contributor

I spent some time on this today but need to do other stuff now. I needed to adjust several other things as well (IdentityProviderMappers handling), see the latest commits in my branch.

Almost there:
image

@daviddavidgit
Copy link
Contributor

daviddavidgit commented Oct 30, 2024

I had a look at the branch of @thomasdarimont. Several tests are failing because Keycloak added constraints in Keycloak 26 when deleting an authentication flow. One of the constraints is that when a client overrides the default browser flow with a "custom flow", one can not just delete the "custom flow" later. If we import a config file which defines the "custom flow", the strategy so far is to first delete the "custom flow" and readd it. But this leads to error in Keycloak. I think we have to find a solution for that.

@ma1uta
Copy link
Contributor Author

ma1uta commented Oct 30, 2024

Thanks @thomasdarimont for help. Also it seems after keycloak/keycloak@c4005d2 client-common-sync from https://github.com/keycloak/keycloak-client/ need to be update. They added the new field in RealmRepresentation but not synced the keycloak-client repo.

@ma1uta
Copy link
Contributor Author

ma1uta commented Oct 30, 2024

Also keycloak 26.0.4 released.

@leoandrea7
Copy link

Keycloak 26.0.5 was released

pom.xml Outdated
@@ -59,7 +59,9 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>

<keycloak.version>${keycloak.version}</keycloak.version>
<keycloak.version>${keycloak.version}}</keycloak.version>
Copy link
Contributor

@bohmber bohmber Nov 5, 2024

Choose a reason for hiding this comment

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

${keycloak.version}} ? There are two } and I don't understand it. keycloak.version points to keycloak.version that looks weird

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a mistake

…entity providers added condition how to obtain them.
@jwklijnsma
Copy link

jwklijnsma commented Nov 5, 2024

issue is fix

bohmber added a commit to bohmber/keycloak-config-cli that referenced this pull request Nov 5, 2024
@ma1uta
Copy link
Contributor Author

ma1uta commented Nov 5, 2024

Failed tests with HTTP 500 Internal Server Error because keycloak 26 throw exception Cannot remove authentication flow, it is currently in use and linked with keycloak/keycloak#31875. Still investigating.

And failed test with NotFound HTTP 404 Not Found because in keycloak 26 existing clients.authenticationSettings is null.

@bohmber
Copy link
Contributor

bohmber commented Nov 5, 2024

Why keycloak should allow to delete and recreate a flow that is currently in use in a client?

@ma1uta
Copy link
Contributor Author

ma1uta commented Nov 5, 2024

There are two ways:

  • don't allow delete flow which is used
  • allow delete flow and replace with a standard one

But in previous versions of keycloak these tests succeed, need to find out what was changed in keycloak.

@bohmber
Copy link
Contributor

bohmber commented Nov 5, 2024

[org.keycloak.services.error.KeycloakErrorHandler] (executor-thread-2) Uncaught server error: org.keycloak.models.ModelException: Cannot remove authentication flow, it is currently in use
at org.keycloak.models.jpa.RealmAdapter.removeAuthenticationFlow(RealmAdapter.java:1460)
at org.keycloak.models.cache.infinispan.RealmAdapter.removeAuthenticationFlow(RealmAdapter.java:1349)
at org.keycloak.models.utils.KeycloakModelUtils.deepDeleteAuthenticationFlow(KeycloakModelUtils.java:967)
at org.keycloak.services.resources.admin.AuthenticationManagementResource.deleteFlow(AuthenticationManagementResource.java:370)
at org.keycloak.services.resources.admin.AuthenticationManagementResource$quarkusrestinvoker$deleteFlow_c7a3b2245d7e636a2fcab9cafd6ed82fb5d1f875.invoke(Unknown Source)
at org.jboss.resteasy.reactive.server.handlers.InvocationHandler.handle(InvocationHandler.java:29)
at io.quarkus.resteasy.reactive.server.runtime.QuarkusResteasyReactiveRequestContext.invokeHandler(QuarkusResteasyReactiveRequestContext.java:141)
at org.jboss.resteasy.reactive.common.core.AbstractResteasyReactiveContext.run(AbstractResteasyReactiveContext.java:147)
at io.quarkus.vertx.core.runtime.VertxCoreRecorder$14.runWith(VertxCoreRecorder.java:635)
at org.jboss.threads.EnhancedQueueExecutor$Task.doRunWith(EnhancedQueueExecutor.java:2516)
at org.jboss.threads.EnhancedQueueExecutor$Task.run(EnhancedQueueExecutor.java:2495)
at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1521)
at org.jboss.threads.DelegatingRunnable.run(DelegatingRunnable.java:11)
at org.jboss.threads.ThreadLocalResettingRunnable.run(ThreadLocalResettingRunnable.java:11)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Thread.java:1583)

@bohmber
Copy link
Contributor

bohmber commented Nov 6, 2024

The specific code is here in AuthenticationFlowsImportService#recreateTopLevelFlow
To update a flow the flow will be deleted and recreated. Before the deletion all references to this flow will be disabled/unasigned and afterwards enabled/asign again. This workaround is not done on clients. I think the check on client flows was not working < Keycloak 26. And I don't know why there is a difference in updating build-in flows and other flows. Build-in flows will be updated without this workaround.

   /**
     * Deletes the top-level flow and all its executions and recreates them.
     */
    private void recreateTopLevelFlow(
            RealmImport realmImport,
            AuthenticationFlowRepresentation topLevelFlowToImport,
            AuthenticationFlowRepresentation existingAuthenticationFlow
    ) {
        AuthenticationFlowRepresentation patchedAuthenticationFlow = CloneUtil.patch(
                existingAuthenticationFlow, topLevelFlowToImport, "id"
        );

        if (existingAuthenticationFlow.isBuiltIn()) {
            throw new InvalidImportException(String.format(
                    "Unable to recreate flow '%s' in realm '%s': Deletion or creation of built-in flows is not possible",
                    patchedAuthenticationFlow.getAlias(), realmImport.getRealm()
            ));
        }

        UsedAuthenticationFlowWorkaroundFactory.UsedAuthenticationFlowWorkaround workaround = workaroundFactory.buildFor(realmImport);
        workaround.disableTopLevelFlowIfNeeded(topLevelFlowToImport.getAlias());

        authenticatorConfigImportService.deleteAuthenticationConfigs(realmImport, patchedAuthenticationFlow);
        authenticationFlowRepository.delete(realmImport.getRealm(), patchedAuthenticationFlow.getId());
        authenticationFlowRepository.createTopLevel(realmImport.getRealm(), patchedAuthenticationFlow);

        AuthenticationFlowRepresentation createdTopLevelFlow = authenticationFlowRepository.getByAlias(
                realmImport.getRealm(), topLevelFlowToImport.getAlias()
        );
        executionFlowsImportService.createExecutionsAndExecutionFlows(realmImport, topLevelFlowToImport, createdTopLevelFlow);

        workaround.resetFlowIfNeeded();
    }

@ma1uta
Copy link
Contributor Author

ma1uta commented Nov 13, 2024

@bohmber I found the original issue: keycloak/keycloak#30707. So, I replace all overrides with temporary flow before deletion and restore after creation.

@bohmber
Copy link
Contributor

bohmber commented Nov 13, 2024

@ma1uta The last failing test is something similar 8f2f5dc in the file 44_update_realm_remove_authz_policy_realm-management.json the realm-management client has authorizationServicesEnabled with an empty resources section in authorizationSettings. The authorizationSettings will be deleted during update of the client. Later the code tries to update the deleted authorizationSettings. That is the jakarta.ws.rs.NotFoundException: HTTP 404 Not Found.

@ma1uta
Copy link
Contributor Author

ma1uta commented Nov 14, 2024

Keycloak create AuthorizationSettings in two cases:

@bohmber
Copy link
Contributor

bohmber commented Nov 14, 2024

@ma1uta good point. That was exactly the code I was searching for the behavior

Copy link
Contributor

@bohmber bohmber left a comment

Choose a reason for hiding this comment

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

Looks good for me. Tested it with Keycloak 26 and our configuration yesterday

@francis-pouatcha francis-pouatcha merged commit 40133bb into adorsys:main Nov 15, 2024
14 checks passed
@service-github-lifetrust

Hey @francis-pouatcha, when can we expect a new release with the v26 compatible jar?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Keycloak 26
9 participants