DEP-262: add SUPER_ADMIN role and associated functionality to db; remove staff_user tenant_id column - #2843
Conversation
…ove staff_user tenant_id column
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2843 +/- ##
==========================================
+ Coverage 74.96% 75.16% +0.19%
==========================================
Files 517 517
Lines 19932 20055 +123
Branches 1612 1723 +111
==========================================
+ Hits 14942 15074 +132
+ Misses 4982 4972 -10
- Partials 8 9 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
jareth-whitney
left a comment
jareth-whitney
left a comment
There was a problem hiding this comment.
This is great, I think it will help with some of the login issues I've been experiencing. No dealbreakers here, you're good to merge.
I believe, from reading your PR, that we will always have a tenant_id for a user based on data from another table or app state, but please verify that's the case. Also, can you imagine a scenario where a bad actor could spoof their Keycloak role, triggering super_admin privilege? I'm guessing not, but it's worth thinking about for a moment.
| ) | ||
|
|
||
|
|
||
| def downgrade(): |
There was a problem hiding this comment.
So remove all user_group_memberships where the role ID is equal to super_admin. Yes that makes sense.
| def upgrade(): | ||
| _drop_matching_foreign_keys('staff_users', 'tenant_id', 'tenant') | ||
| with op.batch_alter_table('staff_users', schema=None) as batch_op: | ||
| batch_op.drop_column('tenant_id') |
There was a problem hiding this comment.
Are we removing tenant ID from the staff_users table because it's a redundant link? I.e. you can extract the users from the tenant, therefor the tenant doesn't need to be nested in the user? Otherwise it seems like it could be useful to have available.
| factory_user_group_membership_model(str(user.external_id), group_id=CompositeRoleId.TEAM_MEMBER.value) | ||
| claims = copy.deepcopy(TestJwtClaims.team_member_role.value) | ||
| claims['sub'] = str(user.external_id) | ||
|
|
There was a problem hiding this comment.
Thank you for updating the unit tests thoroughly.
|
|
||
| - **Feature** Sync SUPER_ADMIN tenant membership and protect super admins in user management [🎟️ DEP-262](https://citz-gdx.atlassian.net/browse/DEP-262) | ||
| - Added a migration to create the `SUPER_ADMIN` group, the `super_admin` role, and their mapping so super admins can be represented in tenant memberships. | ||
| - Updated the API to add or remove `SUPER_ADMIN` group membership based on the user's Keycloak roles during login/request processing, including unit tests for assignment and removal. |
There was a problem hiding this comment.
I think this should be safe if we're qualifying the information that we're receiving from Keycloak. I just want to make sure that there's no way a bad actor could spoof these credentials in order to gain super_admin access. If it's just textbook OAuth/SAML then please ignore.
|
3 participants
Issue #: 🎟️ DEP-262
Description of changes:
User Guide update ticket (if applicable):
Feature Sync SUPER_ADMIN tenant membership and protect super admins in user management
SUPER_ADMINgroup, thesuper_adminrole, and their mapping so super admins can be represented in tenant memberships.SUPER_ADMINgroup membership based on the user's Keycloak roles during login/request processing, including unit tests for assignment and removal.Common component changes: