p2-inc / keycloak-orgs

Single realm, multi-tenancy for SaaS apps
https://phasetwo.io
Other
417 stars 72 forks source link

Update Active Organization Attribute for v24 #204

Closed MGLL closed 8 months ago

MGLL commented 8 months ago

Fixes for https://github.com/p2-inc/keycloak-orgs/issues/195

MGLL commented 8 months ago

Hello @xgp here are the fixes to Active Organization Attribute related to version 24 upgrade. Open for review.

MGLL commented 8 months ago

@xgp updated following your feedback.

btw, I have fixed the testOrganizationSwitch() test, it's working when I run only this one (and all test when running one by one manually). But when I run mvn clean package, multiple tests are failing, example:

[ERROR]   OrganizationResourceTest.testAddGetDeleteIdps:1287->AbstractOrganizationTest.createDefaultOrg:114->AbstractOrganizationTest.createOrganization:188->AbstractOrganizationTest.createOrganization:197 
Expected: is <201>
     but: was <409>
[ERROR]   OrganizationResourceTest.testGetMe:153->AbstractOrganizationTest.createDefaultOrg:114->AbstractOrganizationTest.createOrganization:188->AbstractOrganizationTest.createOrganization:197 
Expected: is <201>
     but: was <409>
[ERROR]   OrganizationResourceTest.testOrganizationSwitch:1763->AbstractOrganizationTest.createPublicClient:379 
Expected: is <201>
     but: was <409>

I currently have no idea what could be the cause.

xgp commented 8 months ago

Thanks for the report. We're still working on a ton of "bugs" related to the 24 upgrade. I'll merge this in and we'll keep working on it.

xgp commented 8 months ago

@MGLL this is still failing in xgp/24. I've got the organizationMembershipSwitchTest method disabled. Would you mind pulling the most recent, enabling that test, and checking if you can understand why it's failing?

MGLL commented 8 months ago

@xgp this test is failing as we try to assign the active organization by writing in the user's attributes with a standard user. Which is not possible as now it's an attribute restricted to admins with UserProfile (return 400 BAD REQUEST now).

@rtufisi as you implemented it, should I adjust the code to use an admin user to write this attribute? This test was for testing user events from what I understand, but now an user can't edit this attribute anymore.

xgp commented 8 months ago

@MGLL Thanks for the explanation. Can you adjust the test?

MGLL commented 8 months ago

@xgp after trying a bit, I'm not sure this test is supposed to work. It might be linked to the TODO in util.ActiveOrganization from @rtufisi.

From what I see, there is actually no event generated, and the test use "UPDATE_PROFILE" event which is supposed to happen when the user edit his account. But as the user can't modify anymore, nothing is happening. Also tried to test with a random attribute (not read-only), and still no event is emitted.

As I didn't do this test, I don't know what to expect or to fix. 😞

rtufisi commented 8 months ago

Hei guys. I've been following this conversation. Please let me know if I can be of any use

The test logic should be the following "As a user , member of a set of orgaizations i should be able to switch my 'active-organization' ". This should be possible using the switch /switch-organization endpoint, which generates a user-event. The event 'detail' are checked.

rtufisi commented 8 months ago

I think I see your point @MGLL . If the user can no longer replace its attribute, the fix wold be the following. Instead of using: //createOrReplaceUserAttribute(kc, "user", ACTIVE_ORGANIZATION, organization.getId());

Assign the "ACTIVE_ORGANIZATION" at user creation.

@Test void organizationMembershipSwitchTest() throws IOException { // create a user CredentialRepresentation pass = new CredentialRepresentation(); pass.setType(CredentialRepresentation.PASSWORD); pass.setValue("password"); pass.setTemporary(false); UserRepresentation userRepresentation = new UserRepresentation(); userRepresentation.setEnabled(true); userRepresentation.setUsername("user"); userRepresentation.setAttributes(Map.of(ACTIVE_ORGANIZATION, List.of(organization.getId()))); userRepresentation.setCredentials(ImmutableList.of(pass));

var user =  createUser(keycloak, REALM, userRepresentation);`

// var user = createUserWithCredentials(keycloak, REALM, "user", "password"); // Assign manage-account role grantClientRoles("account", user.getId(), "manage-account", "view-profile");

// add membership
putRequest("foo", organization.getId(), "members", user.getId());

// create second organization
var organization2 =
    createOrganization(
        new OrganizationRepresentation()
            .name("example-org-2")
            .domains(List.of("example2.com")));

// add second organization membership
putRequest("foo", organization2.getId(), "members", user.getId());

// Client SETUP
// Create basic front end client to get a proper user access token
createPublicClient("test-ui");
var kc = getKeycloak(REALM, "test-ui", "user", "password");

// assign active organization
//createOrReplaceUserAttribute(kc, "user", ACTIVE_ORGANIZATION, organization.getId());

// switch to organization2
var switchToOrganization = new SwitchOrganization().id(organization2.getId());
putRequest(kc, switchToOrganization, "users", "switch-organization");

// results
var userEvents =
    getEvents(keycloak, "master").stream()
        .filter(eventRepresentation -> eventRepresentation.getType().equals("UPDATE_PROFILE"))
        .filter(
            eventRepresentation ->
                eventRepresentation.getDetails().containsKey("new_active_organization_id"))
        .filter(
            eventRepresentation ->
                eventRepresentation.getDetails().containsKey("previous_active_organization_id"))
        .toList();

assertThat(userEvents, hasSize(1));

// cleanup
deleteClient("test-ui");
deleteOrganization(keycloak, organization2.getId());
deleteUser(keycloak, REALM, user.getId());

}`

Otherwise we would need to use a "admin" user to use the method //createOrReplaceUserAttribute(kc, "user", ACTIVE_ORGANIZATION, organization.getId());

MGLL commented 8 months ago

Yes, also doesn't works if you use an admin, as it will update his account.

So adding the attribute on creation is better.

I will have a look at the end of day. I'm currently at KubeCon & CloudNativeCon in Paris