search

Captain-P-Goldfish / scim-for-keycloak

a third party module that extends keycloak by SCIM functionality
BSD 3-Clause "New" or "Revised" License
182 stars 46 forks source link

Update to keycloak 19 #72

Closed arthur25000 closed 1 year ago

arthur25000 commented 1 year ago

Hi @Captain-P-Goldfish

I have updated the sources for Keycloak 19.0.3.

Although the source code of kc-18-b2 is compatible, I replaced in UserHandler.java the deprecated use of org.keycloak.models.KeycloakSession#userCredentialManager by org.keycloak.models.UserModel#credentialManager as recommanded by the Keycloak documentation.

The work was mostly to fix mocks / providers in tests.

Also, I had to revert the liquibase changelog back to the version using the URL for the dbchangelog xsd. I'm fully aware that it breaks #44 and #50 but I disagree with the fix proposed.

I explain myself : the liquibase library already have the optimization to check for the xsd present in its jar (see liquibase.parser.core.xml.LiquibaseEntityResolver#resolveEntity(java.lang.String, java.lang.String, java.lang.String, java.lang.String) ) so no need to add a copy of it. The reason why it makes a call to download the xsd is because the xsd resources from the jar are not found. Thus the problem is that the liquibase jar file is NOT in the classspath of the application.

I had the same exact problem using the wildfly deployment of my ear in Keycloak and the fix was to add the dependency to the liquibase module in my ear jboss deployment. Once done, no more HTTP call.

Something like that did the trick for me in my jboss-deployment-structure.xml : 

<?xml version="1.0" encoding="UTF-8"?>
<jboss-deployment-structure xmlns="urn:jboss:deployment-structure:1.2">
  <ear-subdeployments-isolated>false</ear-subdeployments-isolated>
  <deployment>
    <dependencies>
      <module name="org.liquibase" services="import" export="true"/>
...
   </dependencies>
  </deployment>
</jboss-deployment-structure>

What is confusing though is why it could find the liquibase classes without explicitly declaring the dependency but not the ressources. But for sure, the fix makes sense since we do use the liquibase module.

I let you see if you want to leave it like that or put back the previous fix which triggered an error about using external dtd on my machine when compiling and thus would make all the tests failed.

For information, I have not ran the front-end tests by lack of time but seeing there is no modification on this side, it should not break anything.

Captain-P-Goldfish commented 1 year ago

nice. Thanks for the explanation I wasn't actually motivated enough to investigate this liquibase problem in depth. But I'm glad you did. :-) Seems everything okay to me, so merge :-)