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

Zhifan Created Pause User Permission #1196

Open
wants to merge 4 commits into
base: development
Choose a base branch
from

Conversation

mashpotato9
Copy link

Description

Screenshot 2025-01-08 at 12 03 28 AM

Related PRS (if any):

This backend PR is related to the #3023 frontend PR.
To test this backend PR you need to checkout the #3023 frontend PR.

Main changes explained:

  • added pause/resume permission for Admin and Owner
  • provided ability to access user management page and the pause button to users assigned with this permission

How to test:

  1. check into current branch
  2. do npm install and ... to run this PR locally
  3. go to frontend repo for further steps

Screenshots or videos of changes:

Note:

Include the information the reviewers need to know.

Comment on lines +172 to +173
if (!(await checkPermission(req, 'getUserProfiles') ||
await checkPermission(req, 'pauseResumeUser'))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!(await checkPermission(req, 'getUserProfiles') ||
await checkPermission(req, 'pauseResumeUser'))) {
if (!(await checkPermission(req, 'getUserProfiles'))) {

You could have someone without 'getUserProfiles' where you only want them to be able to pause people on their team or people otherwise accessible without needing access to everyone.

Copy link
Author

Choose a reason for hiding this comment

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

Team leaders should not be able to pause people on their team. Confirmed with Jae that people with permission to pause anyone should have access to the user management page, which fetches all users.

Comment on lines 1233 to 1235
!((await hasPermission(req.body.requestor, 'changeUserStatus')
|| await hasPermission(req.body.requestor, 'pauseResumeUser'))
&& canEditProtectedAccount)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would allow 'pauseResumeUser' to also do what 'changeUserStatus' can do (setting final day for users?).
I don't fully understand why these need to be separate permissions, but they should be different. I asked how this should be handled in the bugs doc.

Copy link
Author

Choose a reason for hiding this comment

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

The pauseResumeUser function does not allow users to set the final day because the “Set Final Day” button only appears when the condition const canEdit = canEditUserProfile || isUserSelf;is met. Additionally, users cannot set the final day unless they have the changeUserStatus permission.

The changeUserStatus function manages various user status transitions, including deactivating a user, which can occur with or without a time constraint if one is provided. It handles pausing a user, setting the final day, and directly deactivating a user. All these actions request the same endpoint to toggle the user’s status between active and inactive. Access to these different functionalities is differentiated by the frontend. In the original implementation, deactivating a user with a time constraint required multiple permissions.

For potential security concerns, the pause user functionality has been fully isolated from this function.

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.

2 participants