p2-inc / keycloak-orgs

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

[Discussion] - Decouple roles from organizations - potentially breaking change #48

Open xgp opened 1 year ago

xgp commented 1 year ago

Re-posting from (https://github.com/p2-inc/phasetwo-java/pull/3#issuecomment-1464922063):

Currently, an OrganizationRole is associated with a single Organization. This means that if you have an OrganizationRole that is meaningful in your application that can be given to users in each Organization, you have to create an OrganizationRole for each Organization. This creates lots of duplication, and requires the developer to manage these roles in order to make sure that each Organization has the same OrganizationRoles. Despite our initial implementation following this model, we aren't aware of a single customer using it in a way that requires the OrganizationRole to be associated with an Organization.

We're considering de-coupling OrganizationRoles from Organizations. The API for CRUD OrganizationRoles would change to

/:realm/orgs/roles/:name

but the API for granting OrganizationRole for Users would not change, as the OrganizationRole would be associated with a User for a single Organization

/:realm/orgs/:orgId/roles/:name/users/:userId

The upside is that this would have little effect on how people are using OrganizationRoles today (very few customers use the APIs for CRUD OrganizationRoles).

In addition to the change in implementation, there would need to be a data migration that did the following:

  1. Make a temp table organization_role_tmp, with the same schema as organization_role populated by select * into organization_role_tmp from organization_role
  2. Drop constraint in user_organization_role_mapping
  3. Drop organization_id from organization_role
  4. Delete data from organization_role
  5. Add unique OrganizationRoles back into the old table with `insert into organization_role (id, name, description) select gen_random_uuid(), distinct name, description from organization_role
  6. Update the role_id in user_organization_role_mapping (tbd query)
  7. Create the constraint in user_organization_role_mapping
  8. Drop organization_role_tmp

Please post comments with reactions or issues!

paulwer commented 1 year ago

any updates on how this will evolve?

xgp commented 1 year ago

@paulwer No decisions yet, but leaning toward this proposal. Haven't heard any feedback/support for keeping it the way it is.

Frank-D commented 1 year ago

Wouldn't the proposed solution makes the "OrganizationRole" and the standard keycloak "Realm Role" more or less on the same level and confusing, living side by side?

I'm actually totally in favour of moving the "OrganizationRole" out of the "Organization", but at that point, why not leverage the ootb Realm Roles offered by keycloak, making this extension one notch closer to keycloak ootb? E.g., just a mapping at the org. level of user and existing realm roles, no need to 'create' new org. role records.


Context (..of why I am super in favour of such enhancement):

When creating a new organization, selecting it and going to the Roles tab, I see a list of predefined org. roles, which does not include any of my created realm roles. I guess that's fine, if this org. roles list is only there to have specific/unique organization roles (..even though, I have no needs for this).

However, when assigning a user ("member") to a given organization and then trying to assign roles to that user from the "Members" listing, then again, only the same list of limited 'organization roles' is made available, but it does not allow to assign any of my own created realm roles. Such org. role assignment feature could have offered multiple role sources, e.g. org. level roles + realm level roles + client level roles (even though, I believe those client roles may not be as useful, not in my context at least).

If I end up with 50 roles (treated as 'fine-grained permissions' in my case) in my application for various screens/resources/actions, I would need to create them all inside 'each' organization that I create, rather than creating them only once, at the realm level. This would create a lot of duplicated roles, and making it really hard to maintain, if I end up with let's say 1000 orgs (1000 org * 50 roles = 50K role records), and then over time if I need to rename or delete or add an application role, than I would need to update all 1000 organizations, that wouldn't scale well at all.

xgp commented 1 year ago

@Frank-D Thanks for the comments.

Regarding the "why not leverage the ootb Realm Roles offered by keycloak", it's 2 reasons. One is that Keycloak doesn't really provide great extension points for such a deep integration, that it would be difficult to build our features without forking. The second is to give users an option to use keycloak Realm and Client roles for different purposes, without overlap.

Your second point/problem of proliferation of "organization roles" would be handled by this proposal. That's exactly why were suggesting this decoupling.

Frank-D commented 1 year ago

I understand the concerns, and I can see the ability to use org roles independently of the realm and client roles interesting for some scenarios, but in the ideal world, we could offer to users a choice to map/associate a role to an organization from either [ realm roles ] or [ org roles ].

Since you've been already able to solve how to map a new custom entity/model (e.g. [ organization ] ) to an existing keycloak ootb entity/model (e.g. identity provider / IDP, I believe via this custom model referenced by the previous link I provided), I thought that, during this upcoming org roles refactoring, adding the possibility to create a new org role either from blank OR by mapping to an existing keycloak ootb entity such as a "realm role" could have been done in a similar way without too much more effort, while opening this roles refactoring door.

In the end, I must admit that I haven't had the time to investigate that much in this extension codebase and understand how it works, to propose a more clear solution to this, I've been quite busy at work recently, but because I see a very interesting potential with this org roles refactoring, I just wish we could make it as flexible as possible with the existing realm roles, without adding too much effort to this refactor. And by adding such new mapping/possibility in parallel to decoupling org roles from orgs, I don't see this as a breaking change since it would simply be adding a net new possibility for org roles creation, e.g. either you create a separate org role (the "org role" name becomes important to make it super clear this is a very separate entity from any other existing ootb kc roles..), or you create an org role from an existing kc realm role.

Your call in the end of course, and I wish I could participate, I just can't right now, but maybe I'll be able to find some time later to participate if needed.

Btw @xgp, did you have any kind of approximate ETA on when you were planning to start working on this refactor? Are we talking about couple days, weeks, months at this point? I'm just curious to know, no pressure at all.

thanks!

Frank-D commented 1 year ago

And I know you mentioned in my other ticket (Q&A # 2) that adding a mapping to keycloak "groups" represents a similar challenge, but simply to regroup my suggestions in the same ticket (I realize I did a bad job on this, should have talked about the group mapping in here in the first place..), if there's any possibility whatsoever we could find a way to support mapping org role to an existing realm role (e.g. [ org ] --> [ org role ] --> [ kc realm role ], then I suppose there could be a way to also support mapping kc realm groups to orgs, that would makes this extension so much more powerful imo (e.g. [ org ] --> [ org group ] --> [ kc realm group ] --> [ kc realm roles ]).

It would allow to deal with RBAC authorization in a much more flexible way, e.g. by allowing to support "roles to fine-grained permissions" concept, perhaps not the most advanced way like the "uma authorization service" way that keycloak can support, but at the very least, one could consider a "kc group" to be an "application role" (e.g. "org-admin"), and then you only need to map such "app role" to one or more member(s) of your organizations to make them admins of that given org; then in the ootb keycloak admin screens, you simply map all the corresponding roles (e.g. "fine-grained permissions") to that group, without needed to assign them all 'individually' to your org members, and over time, you can add more or remove any "kc role" to such "kc group", without needed to update any of your orgs members role/group assignations, making this super flexible.

Again, I would be happy to deep dive into the code once I can find some time, to see if this could be at all possible, via this extension, obviously avoiding to fork kc, definitely not the goal here. If that could be done via your extension, that would be amazing imo.

cheers

xgp commented 1 year ago

@Frank-D Thanks for the thoughtful proposals and idea. I do really appreciate it when people give concrete suggestions, and attempt to understand the advantages and disadvantages of each approach.

I also appreciate your willingness to dive into the code and make your proposals real with knowledge of the code and prototypes. We've had more recommendations for "really great" features than I can count, with zero willingness to participate in the implementation. Like every startup, we're resource constrained, and the only time we can really be responsive to those kind of requests are when they come from a paying customer.

I agree with your assessment that adding the relationships is possible (e.g. like we've hacked associating an IdP with an org), and a similar thing would be possible with Realm/Client Roles. Where the complexity arises (and we can't figure out how to do it without forking Keycloak) is all of the functionality inside Keycloak that uses those User<->Role relationships (e.g. internal authz, mapping roles to tokens, etc.). We'd have to be able to intercept where any of those relationships are loaded and inject our custom logic for how they are loaded on a per-organization basis. This may be possible by overriding the UserProvider (and the whole iceberg underneath that), but it violates our goal of keeping this as simple as possible for the largest set of use cases. We'd certainly look at a PR/prototype, but I can't say we'd be willing to go that direction until we've seen that.

For us, the roadmap for making this change (decoupling roles from organizations) is a Q3 effort. We are in the middle of launching our self-service, paid product in June, and then we will refocus on features like this after that time.

Frank-D commented 1 year ago

Thanks for your response @xgp.

I took the time to read most of the keycloak spi documentation and some other 'extending keycloak' best practices blog post on the topic, to evaluate how I could participate into current extension and push a/some PR(s)).

But at this point, now that I know most - if not all - that I need supported in keycloak in terms of multi-tenants/multi-teams/roles/groups would be doable through a keycloak extension, I feel like I may need to deviate a little bit too much from what you guys need or are doing over here (obviously, I understand, even though this is an oss project, you have your own business context that will justify accepting or not to work on any suggested feature requests, plus timeline is always a factor for everyone).

I also tried to evaluate as much as I could how I could make my use-cases to work with your current offering, and also considering that such roles decoupling initiative will only get in by Q3-Q4+, but that would prevent me from building my application architecture the way I need right from the start, accumulating an important tech debt.

Here are the options that I'm evaluating now:

1) Use your current offering as-is, without the org roles mapping. Map your organizations to my 'teams' inside my app., forcing me to support the entire notion of tenants (and tenant to teams mapping) strictly outside of keycloak, but inside my app instead, since your orgs would be second-level teams for me, not tenants. Same goes with the roles and permissions mapping (..to tenants, orgs/teams and their users), handled strictly outside keycloak, need to build this inside my app, hence not re-usable / super custom. At least, this way, I get the "org/team <--> idp" bit. But many disadvantages as well, lots of in-app non-reusable development required. At that point, perhaps better build all this as part of my own keycloak extension, so it becomes re-usable, that's option 2 right below.

2) Build my own keycloak extension (...while waiting for keycloak to officially support it at its core...might still take a while thought, but it seems to be in their plans, at the very least). See my high-level extension specs here, in that keycloak multi-tenants + multi-teams initiative. If you have time reading, you'll observe that what I'm after probably deviates too much from your current direction, and that's fine that's life and business requirements, but good news for me is, it seems to align a little better with keycloak vision about a similar multi-tenants multi-teams support that might come eventually.

3) Use an entirely different authn/authz product, like the interesting (but never tested personally) Zitadel's multi-tenants/multi-projects authn/authz open source offering.

TBD. Nevertheless, thanks for those few exchanges, it makes me think twice, before moving in any direction.

Reavolt commented 1 year ago

At the moment, I just ran into the problem that I need to have a set of roles that will be used in organizations. And each time creating new roles for each organization looks like a hard task. I would like to be able to distribute my own roles to all organizations.

xgp commented 1 year ago

@Reavolt We're leaning in the direction of this proposal, and will probably implement sometime this summer.

D4M13N-D3V commented 11 months ago

@xgp Quick bump, was a decision ever made on this?

xgp commented 11 months ago

@DamienTehDemon Nobody has yet expressed a strong preference, and we had other priorities command most of our time this summer. If you have some feedback, please share. Specifically, do you already use unique roles per organization? If so, this proposal would break your use of this extension.

D4M13N-D3V commented 11 months ago

@DamienTehDemon Nobody has yet expressed a strong preference, and we had other priorities command most of our time this summer. If you have some feedback, please share. Specifically, do you already use unique roles per organization? If so, this proposal would break your use of this extension.

I do not, I have a set of application roles that are going to be the same across all organizations. I can not really think of a situation in my use case where a role would be unique to a organization unless they were grouping together existing roles. If there was a way to flag a role to be assignable by organizations that would be amazing. It seems that I can not use these in the KeyCloak authorization policies either which is kind of having me rethinking how I have everything setup.

This is my first full week of really getting to dive into all of the configuration bits of KeyCloak. We are using it as a authentication provider for a application following a microservice architecture to give you a little bit of context. It is a multi-tenant'd SAS application.

Right now I am trying to figure out if this extension will help me achieve what I am aiming to. I want to be able to create tenants programmatically and allow them to manage their users and identity providers. I also want them to be able to assign them selfs to roles that grant them access to a scope on a resource.

Maybe I am over complicating my configuration and do not need to set everything up with policies and protecting by resource like the way I have?

What would the best current approach be since this feature is not completed?

xgp commented 11 months ago

Thanks for the feedback on "unique roles per organization". It's similar to what we've heard from others.

What would the best current approach be since this feature is not completed?

As I suggested before, we're leaning that direction, and there is a prototype that suggests the transition for existing users will be mostly painless. It will work by merging your roles into a single table, so if you just have common roles between Organizations, they will all get rolled up into a single, top level role. The only "gotcha" will be if you rely on the UUID of the role.

Maybe I am over complicating my configuration and do not need to set everything up with policies and protecting by resource like the way I have?

This is a different, longer subject, but we generally steer our customers away from using Keycloak's Authorization Services. We've found them complicated to implement and maintain, and they create tight coupling where it's unnecessary. Unless you have an application that requires it, we recommend a combination of Realm, Client and Organization roles, either mapped into the token or userinfo endpoint. From there, it is up to the application to do authorization, either internally, or using a different 3rd party tool that is "better" at that.

D4M13N-D3V commented 11 months ago

Thanks for the feedback on "unique roles per organization". It's similar to what we've heard from others.

What would the best current approach be since this feature is not completed?

As I suggested before, we're leaning that direction, and there is a prototype that suggests the transition for existing users will be mostly painless. It will work by merging your roles into a single table, so if you just have common roles between Organizations, they will all get rolled up into a single, top level role. The only "gotcha" will be if you rely on the UUID of the role.

Maybe I am over complicating my configuration and do not need to set everything up with policies and protecting by resource like the way I have?

This is a different, longer subject, but we generally steer our customers away from using Keycloak's Authorization Services. We've found them complicated to implement and maintain, and they create tight coupling where it's unnecessary. Unless you have an application that requires it, we recommend a combination of Realm, Client and Organization roles, either mapped into the token or userinfo endpoint. From there, it is up to the application to do authorization, either internally, or using a different 3rd party tool that is "better" at that.

Gotcha, so the current approach would be creating the roles for each organization when i programmatically create the org, and then a update in the feature will require me to merge roles into a single table at some point.

Thankyou for your input on the configuration, I will keep this in mind going forward

a8t3r commented 9 months ago

I agree, current organization and applications roles (eg view-organization, view-photos, etc) should be moved into separated space. There are no much choices: org level, realm roles, client roles or combination between them. Currently some global roles (view-organizations, manage-organizations) attached to realm master client - it's okey. But what if create additional client organization-client and attach org and app roles into it? These client roles should be used as a registry of available roles for organization management - registry is immutable from api and can be modified only through manual config. Mapping roles from registry to organization members should be implemented by name, not by uuid. Customer can create additional clients with subset of original roles - in that case unknown names should be ignored during token evaluation.

After that, semantic of current organization roles should be changed to groups/teams/composite roles. Something like: organization A has groups Photographers and Managers. Photographers can view-photos, Managers can manage-photos, manage-organization. Each organization can manage own groups.

michaeljauk commented 2 months ago

Is there any update on this? This is also something that we would like to have

xgp commented 2 months ago

@michaeljauk We haven't had enough interest or engagement with this from customers to switch the functionality. We're leaving it open for discussion.

michaeljauk commented 2 months ago

@xgp This would be something that would be of great advantage to us. If I'd be able to make some time, I would even be willing to work on this myself

xgp commented 2 months ago

@michaeljauk Please feel free to share any work you do here.

Some guidance on a PR here, since it is a core piece of functionality that a lot of people are using:

  1. There needs to be a migration that both updates existing data correctly, as well as preserves existing configuration in case of errors. The migration should also output useful information on what is happening so that users can easily debug if something goes wrong.
  2. API endpoints can be added, but existing ones should continue to work. We will make a decision after an upgrade when any deprecated endpoints will be removed.
  3. There need to be several tests that indicate all of the existing functionality that pertain to roles work as before.
  4. Documentation needs to be added and updated across this repo and the docs repo, specifically the openapi spec file.

There will be a lot of scrutiny on this, as it has the potential to create a lot of issues.

michaeljauk commented 2 months ago

@xgp

There will be a lot of scrutiny on this, as it has the potential to create a lot of issues.

Sure, I expected nothing less 😉

I don't want to give false hopes though, while this feature would be very helpful, it's still low to medium priority to us. If I can make time within my team, I'll make sure it gets used towards this though 👍🏻