p2-inc / keycloak-orgs

Single realm, multi-tenancy for SaaS apps
Other
361 stars 65 forks source link

User attribute test in `organizationMembershipSwitchTest` fails in KC24 because of change to account representation #195

Closed xgp closed 3 months ago

xgp commented 4 months ago

  protected void createOrReplaceUserAttribute(
      Keycloak keycloak, String username, String attributeKey, String attributeValue)
      throws JsonProcessingException {
    Response response = getUserAccount(keycloak);

    ObjectMapper mapper = new ObjectMapper();
    JsonNode rootNode = mapper.readTree(response.body().asString());
    JsonNode attributeNode = rootNode.get("attributes");

    if (attributeNode.has(attributeKey)) {

attributeNode is null because account?userProfileMetadata=false now only returns an id and username

Error:

[ERROR] Tests run: 2, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 1.152 s <<< FAILURE! -- in io.phasetwo.service.events.OrganizationMembershipUserEventsTest
[ERROR] io.phasetwo.service.events.OrganizationMembershipUserEventsTest.organizationMembershipSwitchTest -- Time elapsed: 0.742 s <<< ERROR!
java.lang.NullPointerException: Cannot invoke "com.fasterxml.jackson.databind.JsonNode.has(String)" because "attributeNode" is null
    at io.phasetwo.service.AbstractOrganizationTest.createOrReplaceUserAttribute(AbstractOrganizationTest.java:483)
    at io.phasetwo.service.events.OrganizationMembershipUserEventsTest.organizationMembershipSwitchTest(OrganizationMembershipUserEventsTest.java:78)

I'm assuming this is because of the completion and final changes in the declarative user profile.

@MGLL let me know if you can take a look. I've started the port to 24 in https://github.com/p2-inc/keycloak-orgs/tree/xgp/24

MGLL commented 4 months ago

Hi @xgp is it linked to version 24 bump or is the bug happening on version 23 already ?

I will take a look, I wanted to test the impact of UserProfile (KC 24.x) anyway. I'm quite busy on wednesday, so I can maybe take a look on thursday or during the W-E.

I think this issue is indeed linked to UserProfile as "unmanaged attribute" might be disabled by default (so no "attribute" property is returned by the endpoint now (noticed that on the main branch of KC from my memory)). Not sure it impacts the feature, but I will check that anyway.

xgp commented 4 months ago

Just linked to the 24 upgrade. Thanks for looking when you have time.

MGLL commented 3 months ago

Hey @xgp after some testing and exploration, I confirm that the feature is not broken only the test.

Reason: On a clean deployment, user profile is enabled by default, which means "unmanaged attributes" are not returned anymore if not enabled. And all attributes which are not defined in the user profile are considered unmanaged.

Which then cause "attributeNode" is null as the property attributes appear in the response body if the user can see at least 1 attribute defined in the User profile.

I will provide you an improvement tomorrow (or asap) as a PR to the branch xgp/24 (I guess it's this one for the KC v24 migration).

What I will do, is that I will create a "built-in" org.ro.active attribute in User profile on start-up (if not present).

I propose to do that in the postInit of UserResourceProviderFactory if it's fine for you - else indicate where I should put that. I will make it only editable and visible to admins. I will then update the tests to ensure that this attribute is not visible by a standard user but visible to an admin.

xgp commented 3 months ago

I propose to do that in the postInit of UserResourceProviderFactory if it's fine for you - else indicate where I should put that. I will make it only editable and visible to admins.

@MGLL That is a good place for it.

Thanks for taking this on. We are working through a bunch of things related to making the declarative user profile is enabled by default.