Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WC2-434 Add support for multi-account users #1561

Merged
merged 15 commits into from
Oct 24, 2024

Conversation

bramj
Copy link
Contributor

@bramj bramj commented Aug 19, 2024

This PR adds support for multi-account users without breaking the existing user setup.
Note: The main issue is the auth_user_user_permissions table, because the permissions are linked to the Django auth_user (which we shouldn't touch) instead of the iaso_profile, we cannot do proper "user has many accounts" relations.

Proposed solution:

  1. A user has access to account A. They are a regular Django auth_user with the configuration as we know it:
classDiagram
	direction LR
    auth_user "1" --> "1" iaso_profile
    iaso_profile "*" --> "1" iaso_account
Loading
  1. User gets an invite for account B. Now the user becomes a multi-account user. The situation changes to: a main "authentication" user that's only used for authentication, 2 "account users" that are used to access account A and B respectively. Switching from account A to B means switching from account_user A to account_user B.
classDiagram
	direction LR
    auth_user "1" --> "1" iaso_profile
    iaso_profile "*" --> "1" iaso_account
    auth_user "1" --> "1" iaso_tenant_user : account_user
    auth_user "1" --> "*" iaso_tenant_user : main_user
Loading

This way, a "multi-account user" is actually a "main" auth_user that acts as a composition of auth_users, one for each account.

Caveats

  • The column username needs to be unique on the auth_user, this means we need a different username per account. We don't really want this, but we can postfix the main_user's username with the account.name. Currently, I'm not displaying the user's username, first_name, last_name and email, these infos are actually coming from the user.tenant_user.main_user so the user has a more coherent experience. This leads to the question of what about editing these new users? I think we should not allow edition of these 4 fields when a user is a multi-account user.

Unresolved

  • Password reset via email?
  • What about superusers?
  • What about users without iaso_profiles

TODO

  • Improve the invitation flow
  • Add new flow to WFP's Oauth flow
  • Add unit tests for the TenantUser
  • User editing: currently broken because of the username
  • Properly handle all cases, user is invited to a third account etc.
  • Ensure there's no security risks
  • Documentation

Related JIRA tickets : WC2-434, WC2-435

Doc

  • TODO

Print screen / video of current status

Peek.2024-08-19.15-11.mp4

@kemar
Copy link
Member

kemar commented Aug 28, 2024

I understand we need some users to be in different accounts with different permissions, e.g user 1:

  • in group A with permission x
  • in group B with permission y

Even if the implementation you're suggesting introduces complexity, I can't think of a better one…

I had a quick look on some other solutions but none seems to fit our use case.

Some thoughts:

  • we'll have some UI headaches (as you mention in your PR description)
  • it'll skew the statistics of the number of users
  • we should be careful not to put the same user several time in the same account

In my opinion, the simplest solution would be to not implement this feature and let users create several accounts when needed. But I guess that's not possible ;)

@bramj
Copy link
Contributor Author

bramj commented Aug 28, 2024

In my opinion, the simplest solution would be to not implement this feature and let users create several accounts when needed. But I guess that's not possible ;)

In an ideal world, that would be a great idea :D

@bramj bramj force-pushed the WC2-434-multi-account-special-user branch from 8c347ca to de9353e Compare September 16, 2024 14:38
@madewulf
Copy link
Member

madewulf commented Sep 17, 2024

Not implementing the feature is what we've been doing for years and my argument was that only people internal to bluesquare needed that feature, so we could delay, but now, with the CIAM requirement of WFP there is a need for having access to multiple accounts with a single email address, so, we need to provide a solution although I agree that this brings on some complexity.

@beygorghor beygorghor marked this pull request as ready for review September 20, 2024 12:16
@beygorghor beygorghor requested a review from madewulf September 20, 2024 12:21
@beygorghor beygorghor added the postrelease Should be merged just after the release label Sep 20, 2024
@bramj bramj force-pushed the WC2-434-multi-account-special-user branch 3 times, most recently from e0c667f to fdf32f5 Compare October 15, 2024 12:15
@bramj bramj self-assigned this Oct 22, 2024
@bramj bramj force-pushed the WC2-434-multi-account-special-user branch from e54a322 to 4c5785a Compare October 23, 2024 12:23
@bramj bramj force-pushed the WC2-434-multi-account-special-user branch from 4c5785a to cde6dba Compare October 24, 2024 11:47
@bramj bramj merged commit c8142a9 into main Oct 24, 2024
3 checks passed
@bramj bramj deleted the WC2-434-multi-account-special-user branch October 24, 2024 12:16
@quang-le quang-le removed the postrelease Should be merged just after the release label Nov 4, 2024
@beygorghor beygorghor added the user tested Has already been tested on staging label Nov 5, 2024
@quang-le quang-le added Released and removed user tested Has already been tested on staging labels Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants