anarsultanov / keycloak-multi-tenancy

Keycloak extension for creating multi-tenant IAM for B2B SaaS applications.
Apache License 2.0
103 stars 11 forks source link

Initial setup for identity provider tenant support #7

Closed oleaasbo closed 8 months ago

oleaasbo commented 8 months ago

This allows users to select identity provider tenants instead of internally created tenants.

Features

Compatibility

These changes should be backwards compatible with the intended use of this extension. For these new features to become active Keycloak admins need to add additional mappings for identity providers.

Configure

Each identity provider need mappers to activate the option for users to login with their identity provider tenant. In your realms admin console, go to Authentication > Required actions and enable all the following actions (note that they must be placed exactly in this order):

Note: The following action has not been tested with this code change. Creating internal tenants was done through the keycloak-multi-tenancy API using a service account.

In your realms admin console, go to Identity providers > Select a provider > Mappers > Add mapper and add the following mappers to activate external tenants. This example uses Entra ID (Azure AD) as identity and tenant provider. Register the App inside Entra ID as a multitenant web app (Any Microsoft Entra ID tenant - Multitenant)

Mapper name: Microsoft organization tenant immutable identifier
Mapper type: Attribute Importer
Sync mode override: Force
Claim: tid
User Attribute Name: idp_tid
Mapper name: Microsoft organization tenant roles
Mapper type: Attribute Importer
Sync mode override: Force
Claim: wids
User Attribute Name: idp_roles
Mapper name: Microsoft identity provider alias
Mapper type: Hardcoded Attribute
Sync mode override: Force
User Attribute: idp
User Attribute Value: microsoft

This is how the active tenant mapper will look like when users choose to login with their external tenant. Note: active_tenant.provider will be "keycloak" when users login with internal tenants

"active_tenant": {
    "tenant_id": "e2b004fd-04c3-4b7a-acd0-4d997eb5cefa",
    "tenant_name": "",
    "provider": "microsoft",
    "roles": [
      "b79fbf4d-3ef9-4689-8143-76b194e85509",
      "62e90394-69f5-4237-9190-012177145e10"
    ]
  }

Development

This feature need to be tested and could potentially be improved. All input is welcome. The current state of the code proposal is "usable".

Future Improvements

anarsultanov commented 8 months ago

Hi @oleaasbo, I appreciate your contribution!

I agree that IDP support is crucial for the B2B SaaS applications targeted by this extension. However, I have some concerns about the current implementation. My main concern is that the IDP appears to replace the "tenant". While I would prefer to establish a connection between IDPs and tenants, enabling the use of multiple IDPs by a single tenant and granting access to the same tenant via multiple IDPs.

I'm curious to hear your thoughts on this approach and whether it aligns with your use case.

oleaasbo commented 8 months ago

Hi @anarsultanov, thank you for your feedback!

Regarding the concern, I do agree. Typically you relay on the active_tenant.tenant_id to be protected internally by Keycloak because you probably have resources inside your application that have strong a relation to this value. The current implementation do allow IDPs to override this claim which is a huge security concern! However, this is why I have introduced the additional claim active_tenant.provider. This claim will always be keycloak if you do not have IDPs or do not activate external tenant support by setting up the mappers described in post #1. In other words, active_tenant.tenant_id have not been overridden by any IDP when the value is keycloak. Now, if you do not care for external tenant support you can simply ignore the active_tenant.provider claim and never setup the mappers required for activating this implementation.

In terms of enabling access to the same tenant via multiple IDPs i think this is already possible today. I might misunderstand. However, my keycloak use case do not allow for signups and the login theme no longer have username and password fields. I present a list of IDPs which users must choose one to authenticate. When I send Invites to join a tenant the user has to choose an IDP to view the invite. But, the user must choose an IDP with the same email registered as the email used to receive the invite (If invites using action tokens was introduced one could choose to authenticate with an IDP email different to the one used to receive the invite. Mentioned here ). After authentication the user will become a member of the tenant using an IDP.

I view tenants as a user grouping mechanism with added functionalities like roles, groups, and invitations. While the keycloak-multi-tenancy extension is apt for small businesses, larger enterprises with intricate user setups in platforms like Azure AD pose a challenge. It's impractical to manually onboard thousands of users. This inspired my proposal to delegate user and tenant management to IDPs for easier onboarding.

With this new implementation I can ask big companies to provide their Microsoft directory tenantId. From inside my application i can register a new company and attach their Microsoft directory tenantId and set the tenant provider to microsoft (As described in my first post). When my API receives requests I can now safely reference this external tenant and add relations to resources by combining tenantId and provider. idp_provider:tenantId. eg: microsoft:e2b004fd-04c3-4b7a-acd0-4d997eb5cefa instead of only relying on the tenantId. This means that if you have an internal keycloak tenant with the tenantId: e2b004fd-04c3-4b7a-acd0-4d997eb5cefa and an external tenant with the same tenantId. My application will not identify them as the same tenant because the active_tenant.provider claim is different. keycloak:e2b004fd-04c3-4b7a-acd0-4d997eb5cefa is not the same as microsoft:e2b004fd-04c3-4b7a-acd0-4d997eb5cefa

anarsultanov commented 8 months ago

As you noted, we rely on the active_tenant.tenant_id, and I'd really like to keep it that way, since the purpose of this extension is to take the heavy lifting of handling multitenancy, and I really want to avoid having to require users to implement different logic in the application to parse different combinations of claims. Additionally, I still believe that IDPs should complement the tenant concept represented by this extension, not replace it.

To address these concerns, I would propose the following implementation:

Then user login flow would look like this:

In both cases, if only one suitable tenant is found, no selection is needed, and authentication proceeds automatically.

With this proposed implementation, we maintain the concept of a tenant and encapsulate the entire authentication process within Keycloak. And this approach allows applications to trust active_tenant.tenant_id without the need for additional claims, enhancing both security and usability.

oleaasbo commented 8 months ago

Thank you for your detailed feedback and proposed solution about maintaining the integrity of the active_tenant.tenant_id and encapsulating the entire authentication process within Keycloak.

The implementation I suggested is meant to be complementary. It doesn't aim to replace or override any existing functionality. The primary difference for the end-user would be the additional options in the select-tenant.ftl to choose their IDP tenant. I believe this addition can further enhance the user experience without compromising the core objectives of the extension.

I'd be happy to discuss this further and work collaboratively towards a solution that meets all our requirements. Before we delve deeper into the discussion, would you be open to trying out the new implementation firsthand? Experiencing it might provide a more comprehensive understanding of its workflow and benefits. I believe this could help in our collaborative efforts to refine and improve the system. I appreciate your consideration and look forward to your feedback after testing.

anarsultanov commented 8 months ago

I understand that this approach doesn't replace any existing functionality, but it feels more like a workaround than an addition. In my opinion, it is well suited for your specific requirements, but seems less suitable when considered in the broader context of this extension.

As I've previously pointed out, my main concern is the lack of local representation and associated membership for external tenants. The problems this introduces are already visible in this PR, you had to add additional condition in the claims mapper, besides, as you noticed in the initial comment, the creation of a tenant has not been tested and when configuring it, it will constantly prompt for tenant creation for the same reason of lack of local membership, otherwise there an appropriate condition must be added as well.

Given that this extension is based on the concept of a tenant and its model, any time anyone adds attributes to this model or create functionality based on it, they will have to remember that there are external tenants that require workarounds to be implemented. Ideally I'd prefer to avoid this.

Besides, the way it handles tenant selection raises some concerns. For example, if I'm a member of two organizations, and one of them uses IDP for authentication, it feels a little strange that I can log into another organization using their IDP.

In previous comment I described a possible implementation that will avoid all these problems, but I understand that its implementation will require more time, so I can add it to my backlog and implement it myself when I have free time.

In your case, you could implement samilar workaround without modifying the extension: if your IDP mapper adds active-tenant-id session note, it should help bypass the tenant selection, while ActiveTenantMapper simply will not add any claims if membership is foractive-tenant-id was not found, so you can add a custom mapper after it, which will check for the presence of active tenant claim, and if it is missing, then add it based on the active-tenant-id and identity_provider notes (the latter is added by default when logging using IDP).

oleaasbo commented 8 months ago

Thank you for the detailed feedback.

If the issue can be addressed without modifying the extension and solely relying on mappers, it would indeed be ideal. However, the workaround you proposed seems to bypass the tenant selection, which isn't our intention. We aim to let users be members of multiple Keycloak tenants and still grant them the choice to select their IDP tenant within select-tenant.ftl.

I'm eager to see your solution when you get around to it. Thanks for your continued dedication and excellent work!

anarsultanov commented 8 months ago

Yes, the approach I proposed only allows logging into the external tenant via the IDP. But in my experience, many organizations prefer this because it gives them control over security policies such as password length, MFA, etc.

But this use case will most likely be supported by the final implementation in the extension, and I'll try to get back to it as soon as possible.

Thanks for your valuable feedback; it really helps in developing the extension!