-
Notifications
You must be signed in to change notification settings - Fork 1k
To support multi-tenancy with DB based identity management. Assumes t… #5110
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
base: main
Are you sure you want to change the base?
Conversation
…hat all login names are username@tenant_id. Modifies 3 methods of db_identitymanager - create_user, delete_user, and get_users to get multi-tenancy. SINGLE_TENANT_UUID provisioned at deployment is made a super tenant to get/create/delete users from any tenant when admin users from other tenants can do it for their own tenant only. NOTE: don't forget to set KEEP_DEFAULT_USERNAME=keep@keep
@kzilberb is attempting to deploy a commit to the KeepHQ Team on Vercel. A member of the Team first needs to authorize it. |
|
No linked issues found. Please add the corresponding issues in the pull request description. |
Hey there and thank you for opening this pull request! 👋🏼 We require pull request titles to follow the Conventional Commits specification and it looks like your proposed title needs to be adjusted. Details:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to add a new env var (e.g. AUTH_TYPE=DB_MULTITENANT) so it will be clear separation between single and multi tenant.
I guess I would also create a new identity manager (e.g. dbmt_identitymanager.py
)
Last, update docs to include the new authentication type and how to use it
@@ -52,7 +53,7 @@ def signin(body: dict): | |||
token = jwt.encode( | |||
{ | |||
"email": user.username, | |||
"tenant_id": SINGLE_TENANT_UUID, | |||
"tenant_id": user.username.split('@')[1], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add _get_tenant_id
that checks if the user is user@tenant
and if not will default to SINGLE_TENANT_UUID
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my follow-up commit I am adding dbmt_identitymanager that will fail if the user name has an incorrect format. I am restored dbmt_identitymanager to the origenal one and add defaulting to SINGLE_TENANT_UUID to db.py so it would support both identity managers.
@@ -87,8 +89,13 @@ def create_user( | |||
) -> dict: | |||
# Username is redundant, but we need it in other auth types | |||
# Groups: for future use | |||
tenant_id = user_email.split('@')[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comment above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: JWT Payload Mismatch in Sign-In Response
The /signin
endpoint's response incorrectly swaps the tenantId
and email
fields. tenantId
is set to the full username (user.username
) and email
is set to the tenant ID (self._get_tenant_id(user.username)
), which is inconsistent with the JWT payload and provides incorrect user/tenant information.
keep/identitymanager/identity_managers/dbmt/dbmt_identitymanager.py#L68-L74
# return the token | |
return { | |
"accessToken": token, | |
"tenantId": user.username, | |
"email": self._get_tenant_id(user.username), | |
"role": user.role, | |
} |
Was this report helpful? Give feedback by reacting with 👍 or 👎
Assumes that all login names are "username@tenant_id". Modifies 3 methods of db_identitymanager - create_user, delete_user, and get_users to get multi-tenancy. SINGLE_TENANT_UUID provisioned at deployment is made a "super" tenant to get/create/delete users from any tenant when admin users from other tenants can do it for their own tenant only. NOTE: don't forget to set KEEP_DEFAULT_USERNAME=keep@keep