p2-inc / keycloak-orgs

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

Implement active organization. #150

Closed MGLL closed 5 months ago

MGLL commented 5 months ago

Hello there, if you are still interested into "active organization" feature, here is a contribution.

Summary

Details

(more info into active-organization.md)

Endpoints

I have created 2 new endpoints:

This endpoint assigns the organization's id to the user's attributes under the org.ro.active key. Before assigning, it checks that the target organization exists, verify that the user is a member of it.

The endpoint returns a 200 "OK" response with an updated access_token and refresh_token similar to the authentication flow.

This endpoint returns the active organization information (after standard verification: that the organization exist, and the user is a member of it)

Mappers

I have added fine grained ActiveOrganization Mappers, so everyone can adjust following its taste.

Next steps?

Planned evolutions

Next iteration (later when I have more time):

End

If you have any feedbacks or request for modifications, let me know, I will adjust it.

Best regards

MGLL commented 5 months ago

@xgp You are welcome!

No problem, I will fix everything tomorrow after work or Friday at worst.

For 3. I wasn't aware it was possible, if I knew it, I would have did it directly 😄. Thanks for pointing it out and actually after a quick test, I think I could do something.

However, it seems there is a bug in Keycloak for MULTIVALUED_STRING_TYPE and MULTIVALUED_LIST_TYPE, where creating the mapper send the info in an Array, but Keycloak endpoint protocol-mappers/models doesn't expect it.

Which isn't great as you can't do a nice multivalued drop-down...

Config:

image

Request Body:

image

Error log:

image

If I use "STRING" type instead as a workaround and rely on tooltip (and documentation) to help users:

image image

It then works

image

I could then split the received String and apply some logic. But it doesn't look nice for the end user.

I will raise this issue in Keycloak tomorrow, but looking at the source code, I'm not sure there will be a fix, as config in ProtocolMapperRepresentation expect a Map<String, String>.

image

However, if I progress with a configurable mapper, please note that I will need to get access to / use ProtocolMapperModel mappingModel to get the inputed values (so I might need to override the setClaim in ActiveOrganizationMapper or change getOrganizationClaim).

Side note: if you have other picky comment, don't hesitate, I'm always looking to improve.

MGLL commented 5 months ago

@xgp hello, this PR is ready for a new review.

MGLL commented 5 months ago

Hi @xgp

I see it's still in pending, I think I have solved all requested change. If I missed something, let me know.

Best regards,

xgp commented 5 months ago

@MGLL No problems. The issue is just time. We'll get to it soon.

MGLL commented 5 months ago

@MGLL No problems. The issue is just time. We'll get to it soon.

Thanks for info, no Problem, let me know if there is anything 👍

xgp commented 5 months ago

@MGLL I was able to review again. Thanks for making the changes.

One more thing that I missed in the first review is what happens when the Organization is deleted or the User is removed from the Organization. We should remove the attribute in those cases. I see the check that makes sure the User is a member of an Organization before allowing the claim mapping, but I think it would be good style to also explicitly remove the attribute.

Other than that, I think it's almost ready to go.

MGLL commented 5 months ago

One more thing that I missed in the first review is what happens when the Organization is deleted or the User is removed from the Organization.

Ah damn, good catch. I will fix this (I will add a check in the logic where a user is removed from an organization to remove it from attribute if it's the active one).

phamann commented 5 months ago

Really happy to see this addition @MGLL @xgp!

I'm also happy you went with the user attributes route instead of the session notes as originally proposed in one of the linked issues. As session notes do not persist across tab's in Keycloak.

Coincidentally, we've implemented something similar using keycloak-orgs in our private build of Keycloak; however, it has a bit of additional business logic unique to our needs and the legacy domain model we were transitioning from.

One thing I thought I'd suggest/note: in our implementation, instead of using a custom endpoint like PUT /:realm/users/switch-organization for user org switching, we opted for a more spec-compliant way using a customer authenticator which acted upon the presence of the OIDC query parameter promp=select_account.

From the spec:

select_account

The Authorization Server SHOULD prompt the End-User to select a user account. This enables an End-User who has multiple accounts at the Authorization Server to select amongst the multiple accounts that they might have current sessions for. If it cannot obtain an account selection choice made by the End-User, it MUST return an error, typically account_selection_required.

This way, we could use Keycloak's existing auth and token endpoints and off-the-shelf OIDC clients without any modification for account switching. We also added an account_hint=<org_id> param, allowing users to switch directly to an account without being challenged to choose from a list, similar to Keycloaks kc_idp_hint behaviour.

That being said, such an authenticator could co-exist with this implementation and endpoint. But maybe something for you to consider in the future.

MGLL commented 5 months ago

@phamann Thanks for the feedback.

From what I understood, it would require creating an Authenticator like

ActiveOrganizationAuthenticator implements Authenticator {...}

Which is displayed in the authentication flow when the prompt=select_account param is given to the keycloak /token endpoints. And here, you select an organization for the "session" similar to that: image

But if the user has no organization, it should fall into an error, according to

If it cannot obtain an account selection choice made by the End-User, it MUST return an error, typically account_selection_required.

That something I could work on in a next PR, as it requires more time for me to do that, and this implementation already kind of solve a pain point (note that I took example on another solution which provide this kind of endpoints). Basically, I'm currently missing some knowledge to know how to do that which is: How can I catch in the authentication request that the prompt=select_account was given. If you can already give me some hints, it could save me a bit of time.

Then I guess I have to use something like

ActiveOrganizationAuthenticatorCondition implements ConditionalAuthenticator {...}

To decide if I should display ActiveOrganizationAuthenticator or not.

I also like a lot the account_hint=<org_id> param which avoid asking the user to select an organization at each log-in (which add effort on the user side). But, would it mean that we have to "override" / "customize" the Keycloak /token endpoints? (so, we can add this params).

Also, this point

If it cannot obtain an account selection choice made by the End-User, it MUST return an error, typically account_selection_required.

Could be a problem for our use case (and others), as for example, organization can be optional in a SaaS Platform. For example, some users won't be part of an organization and some will be part of 1 and some will be part of multiple. So, if I do this change in another PR, I will try to add an externally configurable condition to decide if the organization selection should be mandatory (and then "return an error, typically account_selection_required") or optional (allow the user to proceed to the platform).


EDIT 1: _I think that's something I'm able to get from AuthenticationFlowContext, but not sure.

EDIT 2: _Regarding account_hint=<org_id> found out there is additionalReqParams in AuthorizationEndpointRequest;_

Or private MultivaluedMap<String, String> formParams; from the TokenEndpoint which I guess is here I should look and understand if those params are provided in the AuthenticationFlowContext.

EDIT 3: Spent a bit of time digging into Keycloak codebase. I think I could do it in another PR. Basically, I saw that prompt param could be retrieved from (AuthenticationSessionModel) authSession.getClientNote(OIDCLoginProtocol.PROMPT_PARAM); and AuthenticationSessionModel can be retrieved from AuthenticationFlowContext.getAuthenticationSession(). Given that, I think I could add an Authenticator which is displayed if OIDCLoginProtocol.PROMPT_VALUE_SELECT_ACCOUNT is given.

But here, I assume that OIDCLoginProtocol.PROMPT_PARAM is set into the notes in AuthenticationSessionModel.

However, I'm still looking how to get the additional params like account_hint=<org_id> from Authenticator. I will check that tomorrow or during the W-E.

EDIT 4: Actually, everything can be retrieved with

MultivaluedMap<String, String> formData = context.getHttpRequest().getDecodedFormParameters();

where context is AuthenticationFlowContext.

:smile:

I think all is good now, I think I have found all the info necessary to proceed. But that's not something I will add in this PR, as it requires more changes and more time. I prefer to split this task in another PR.

Sorry for all that, I tend to like to dig for a solution (until I find a way to get to it) or how to do it and be very focused.

MGLL commented 5 months ago

@xgp I have added some changes to remove the active org attribute when the membership is removed (and if it was the active organization). However, regarding the

Organization is deleted

Should I add the changes too? Because from what I see, it would require to

Which I fear might be a bit extensive depending of the number of members. If this is fine for you, I will add it. Else, ActiveOrganization.isValid() should deal with it as it verifies the membership and if the organization exists.

Let me know

xgp commented 5 months ago

Which I fear might be a bit extensive depending of the number of members.

Agreed. Is there a good place to put in a lazy evaluation of that? I.e. if we come across an "active org" user attribute for a nonexistent org, can we remove it then?

Otherwise, I'm ready to merge this. Thanks again for all of the great work on this @MGLL (and patience in waiting for me to review).

MGLL commented 5 months ago

Hmm we could check "during login" ? for example, through the ActiveOrganization utility when the ActiveOrganizationMapper calls the isValid method.

Because, this method check here if the user is a member of the organization and return false if it's not the case. I could add an additional check there to verify if the organization still exists (and remove the user's attribute if not) before returning the false state.
The downside is that it makes an additional database access, but actually it would makes sense. Because, if you land in this condition, the organization might not exists anymore but you still have the active org attribute for this one.

What do you think ?

xgp commented 5 months ago

What do you think ?

That sounds like a good place to do the check. Thanks.

MGLL commented 5 months ago

@xgp Did the change.

Added

      // verify that the organization still exists
      organization = organizationProvider.getOrganizationById(realm, activeOrganizationId);
      if (organization == null) {
        log.warnf("organization doesn't exists anymore.");
        user.removeAttribute(ACTIVE_ORGANIZATION);
      }

in ActiveOrganization utility (in isValid)

Updated test with

    // test that active org attribute is removed after login when organization is deleted
    // switch to org3
    response = putRequest(kc, switchToOrganization3, "users", "switch-organization");
    assertThat(response.getStatusCode(), is(Status.OK.getStatusCode()));

    // verify attribute
    validateActiveOrganizationFromUserAttributes(getUserAccount(kc), true, org3Id);

    // delete organization
    deleteOrganization(org3Id);

    // re-authenticate
    kc = getKeycloak(REALM, "test-ui", "user", "password");

    // verify attribute is removed
    validateActiveOrganizationFromUserAttributes(getUserAccount(kc), false, null);

to cover the case