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

WP-409: Set members to superuser via TAS #931

Merged
merged 15 commits into from
Feb 13, 2024

Conversation

asimregmi
Copy link
Contributor

@asimregmi asimregmi commented Jan 25, 2024

Overview

Staff and Superuser can be set in through the _PORTAL_ELEVATED_ROLES object in settings_default.py or settings_custom.py for per-portal configuration. Either the TAS group or TACC id can be used.

_PORTAL_ELEVATED_ROLES = {
  "staff": {
    "groups": [],
    "usernames": []
  },
  "superuser": {
    "groups": [],
    "usernames": []
  }
}

Related

Changes

  • added a new dict object _PORTAL_ELEVATED_ROLES object in settings_default.py and imported it to settings.py
  • updated 'get' method to loop through the PORTAL_ELEVATED_ROLES to check if user's username or TAS groups is in the dict object. Skips if role is already applied to the user.
  • updated unit tests to reflect new dict object in settings.py
  • new settings default:
_PORTAL_ELEVATED_ROLES = {
  "is_staff": {
    "groups": ["TACC-ACI"],
    "usernames": []
  },
  "is_superuser": {
    "groups": ["TACC-ACI"],
    "usernames": []
  }
}

Testing

  1. Remove staff and superuser status, and add your username to the _PORTAL_ELEVATED_ROLES dict
  2. Open Network tab and check response of api/users/auth/ endpoint and verify the response
    "isStaff": true,
    "isSuperuser": true
    
  3. Repeat the above 2 steps for TAS group by adding TACC-ACI or other TAS groups you may be on.

UI

No visible UI change except for the Onboarding Admin view for superusers

image

Notes

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (03ce6fa) 64.76% compared to head (3dcd996) 64.72%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #931      +/-   ##
==========================================
- Coverage   64.76%   64.72%   -0.04%     
==========================================
  Files         431      431              
  Lines       12439    12457      +18     
  Branches     2598     2604       +6     
==========================================
+ Hits         8056     8063       +7     
- Misses       4157     4165       +8     
- Partials      226      229       +3     
Flag Coverage Δ
javascript 69.83% <ø> (ø)
unittests 59.53% <38.88%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
server/portal/apps/users/utils.py 57.25% <100.00%> (+0.66%) ⬆️
server/portal/apps/users/views.py 58.18% <ø> (ø)
server/portal/settings/unit_test_settings.py 99.05% <100.00%> (+<0.01%) ⬆️
server/portal/settings/settings.py 0.00% <0.00%> (ø)
server/portal/settings/settings_default.py 0.00% <0.00%> (ø)
server/portal/apps/auth/views.py 71.26% <30.76%> (-7.12%) ⬇️

@asimregmi asimregmi changed the title Wp 409/set members to superuser via tas WP-409: Set members to superuser via TAS Jan 25, 2024
@asimregmi asimregmi marked this pull request as ready for review January 25, 2024 22:14
server/portal/apps/users/unit_test.py Outdated Show resolved Hide resolved
server/portal/apps/users/views.py Outdated Show resolved Hide resolved
server/portal/settings/settings_default.py Outdated Show resolved Hide resolved
server/portal/settings/settings.py Outdated Show resolved Hide resolved
server/portal/apps/users/views.py Outdated Show resolved Hide resolved
server/portal/settings/settings_default.py Outdated Show resolved Hide resolved
server/portal/apps/users/views.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@chandra-tacc chandra-tacc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, LGTM. Just optional comments for readability.

server/portal/apps/users/utils.py Outdated Show resolved Hide resolved
server/portal/apps/users/views.py Outdated Show resolved Hide resolved
server/portal/apps/users/views.py Outdated Show resolved Hide resolved
@asimregmi asimregmi force-pushed the WP-409/set-members-to-superuser-via-TAS branch from 563a40c to 5c85ab7 Compare February 12, 2024 16:10
@asimregmi asimregmi requested a review from rstijerina February 12, 2024 16:31
Copy link
Member

@rstijerina rstijerina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just added one more suggestion wrt logging

@asimregmi
Copy link
Contributor Author

Should be good now. Thanks!! ✅

@chandra-tacc
Copy link
Collaborator

Tested with both groups and users. Log-in sets the user, below is a log of the testing via model commits:

-- with user included in list ---
In [1]: from django.contrib.auth import get_user_model

In [2]: my_user = get_user_model().objects.get(username='cyemparala')

In [3]: my_user.is_staff = False

In [4]: my_user.is_superuser = False

In [5]: my_user.save()

In [6]: my_user = get_user_model().objects.get(username='cyemparala')

In [7]: print(my_user.is_staff)
True

In [8]: print(my_user.is_superuser)
True

-- with group --

In [13]: my_user.is_staff = False

In [14]: my_user.is_superuser = False

In [15]: my_user.save()

In [16]: my_user = get_user_model().objects.get(username='cyemparala')

In [17]: print(my_user.is_superuser)
False

In [18]: my_user = get_user_model().objects.get(username='cyemparala')

In [19]: print(my_user.is_superuser)
True

In [20]:

@chandra-tacc chandra-tacc merged commit 4c59ddf into main Feb 13, 2024
4 of 6 checks passed
@chandra-tacc chandra-tacc deleted the WP-409/set-members-to-superuser-via-TAS branch February 13, 2024 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants