adorsys / keycloak-config-cli

Import YAML/JSON-formatted configuration files into Keycloak - Configuration as Code for Keycloak.
Apache License 2.0
703 stars 129 forks source link

Keycloak 25.0.1 #1052

Closed jonasvoelcker closed 6 days ago

jonasvoelcker commented 2 weeks ago

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 #1051

Special notes for your reviewer:

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

sonarcloud[bot] commented 2 weeks ago

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

thomasdarimont commented 2 weeks ago

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 vs. org.keycloak.services.resources.admin.AuthenticationManagementResource#registerRequiredAction Keycloak 25.0.0

An easy fix could be to just create mock implementations for the custom requiredactions and add them to the Keycloak test container.

thomasdarimont commented 2 weeks ago

I gave this a quick spin and added the required dummy extensions to the test execution. See: https://github.com/thomasdarimont/keycloak-config-cli/commit/31a894f35e972af2cf292ebccf90ea1e3b35831b With this I "only" get 40 failed tests out of 355.

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.

jonasvoelcker commented 2 weeks ago

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 Jonas

daviddavidgit commented 2 weeks ago

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?

jonasvoelcker commented 2 weeks ago

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?

thomasdarimont commented 2 weeks ago

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?

bohmber commented 1 week ago

I analyzed one of the other failing tests ImportClientsIT#shouldUpdateRealmAddAuthorization and it's related to Do not export ids when exporting authorization settings

jonasvoelcker commented 1 week ago

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 Jonas

jonasvoelcker commented 1 week ago

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?

bohmber commented 1 week ago

Issue created in keycloak https://github.com/keycloak/keycloak/issues/30519

jonasvoelcker commented 1 week ago

@bohmber Thank you, I've added some detail as comment.

bohmber commented 1 week ago

@jonasvoelcker Why exportSettings is called instead of getSettings of AuthorizationResource in ClientRepository?

jonasvoelcker commented 1 week ago

Hi @st3v0rr @francis-pouatcha, could we please review this one?

thomasdarimont commented 1 week ago

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.

thomasdarimont commented 1 week ago

Just tested this locally, works great with Keycloak 25.0.1, great work!

services:

  keycloak:
    image: quay.io/keycloak/keycloak:25.0.1
    environment:

      # Keycloak Admin User
      KEYCLOAK_ADMIN: admin
      KEYCLOAK_ADMIN_PASSWORD: admin

      # Feature config, see: https://www.keycloak.org/server/features
      KC_FEATURES: preview

      # Disable specific features
      #      KC_FEATURES_DISABLED: "device-flow"

      # Logging, see: https://www.keycloak.org/server/logging
      KC_LOG_LEVEL: INFO

      # External frontend hostname, see: https://www.keycloak.org/server/hostname
      KC_HOSTNAME: localhost
      KC_HTTP_PORT: "8080"
      KC_HTTP_ENABLED: "true"
      KC_HTTP_RELATIVE_PATH: "/auth"

      KC_METRICS_ENABLED: "true"
      KC_HEALTH_ENABLED: "true"

      # Log Keycloak success events to the console
      KC_SPI_EVENTS_LISTENER_JBOSS_LOGGING_SUCCESS_LEVEL: "info"
      KC_SPI_EVENTS_LISTENER_JBOSS_LOGGING_ERROR_LEVEL: "warn"

      # Additional JVM options
      JAVA_OPTS_APPEND: "--show-version"

      # Enable Remote Debugging
      DEBUG: "true"
      DEBUG_PORT: "*:18787"

    ports:
      - 8080:8080   # HTTP
      - 8443:8443   # HTTPS
      - 18787:18787 # Java Remote Debug

    command:
      - "--verbose"
      - "start-dev"

  keycloak-config:
    #image: quay.io/adorsys/keycloak-config-cli:6.0.2-22.0.4
    image: docker.io/library/keycloak-config-cli:5.12.1-25.0.1-SNAPSHOT
    environment:
      KEYCLOAK_AVAILABILITYCHECK_ENABLED: "true"
      KEYCLOAK_AVAILABILITYCHECK_TIMEOUT: "180s"
      # see: https://github.com/adorsys/keycloak-config-cli/blob/v5.0.0/CHANGELOG.md
      IMPORT_FILES_LOCATIONS: "/config/*" # IMPORT_PATH: "/config"
      IMPORT_CACHE_ENABLED: "true" # IMPORT_FORCE: "false"
      IMPORT_VAR_SUBSTITUTION_ENABLED: "true" # IMPORT_VARSUBSTITUTION: "true"
      IMPORT_VALIDATE: "true"

      # See https://github.com/adorsys/keycloak-config-cli#log-level
      #LOGGING_LEVEL_KEYCLOAK_CONFIG_CLI: "DEBUG"
      # Note: the above does not work but _KCC does
      LOGGING_LEVEL_KCC: "DEBUG"

      # Veeeeery verbose HTTP log!
      #LOGGING_LEVEL_HTTP: "DEBUG"

      #LOGGING_LEVEL_ROOT: "DEBUG"
      LOGGING_LEVEL_ROOT: "INFO"

      KEYCLOAK_URL: "http://keycloak:8080/auth"
      KEYCLOAK_USER: "admin"
      KEYCLOAK_PASSWORD: "admin"
    volumes:
      - ./realms:/config:z

Example realm config file:

realms/test.yml:

realm: test
enabled: true
displayName: "Test"
thomasdarimont commented 1 week ago

One thing that is still missing is to configure the unmanagedAttributePolicy.

image

@jonasvoelcker I created https://github.com/jonasvoelcker/keycloak-config-cli/pull/1 to add support for this.

thomasdarimont commented 1 week ago

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. Alternatively we could just keep the existing

private Map<String, List<Map<String, Object>>> userProfile;

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.

jonasvoelcker commented 1 week ago

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 ;)

bohmber commented 1 week ago

Hi @jonasvoelcker, looks ok so far, thanks

thomasdarimont commented 1 week ago

@jonasvoelcker that works for me too! Thank you very much :)