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.

Sign up for GitHub

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
Open

Zhifan Created Pause User Permission #1196

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
50851b6
Zhifan Created Pause User Permission
mashpotato9 Jan 8, 2025
0ae4ca9
Ioslated pause feature from changeUserStatus function
mashpotato9 Jan 14, 2025
79e338c
Added new pause function to route
mashpotato9 Jan 14, 2025
52779c0
added new function to router
mashpotato9 Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/controllers/userProfileController.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,8 @@ const userProfileController = function (UserProfile, Project) {
};

const getUserProfiles = async function (req, res) {
if (!(await checkPermission(req, 'getUserProfiles'))) {
if (!(await checkPermission(req, 'getUserProfiles') ||
await checkPermission(req, 'pauseResumeUser'))) {
Comment on lines +172 to +173
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.

forbidden(res, 'You are not authorized to view all users');
return;
}
Expand Down Expand Up @@ -1229,7 +1230,9 @@ const userProfileController = function (UserProfile, Project) {
);

if (
!((await hasPermission(req.body.requestor, 'changeUserStatus')) && canEditProtectedAccount)
!((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.

Copy link
Contributor

@nathanah nathanah Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Fantastic, thank you.

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.

I only raised the concern because people can use tools like Postman to circumvent the front-end limitations and manually construct the API requests. Your solution is the perfect fix for this.

) {
if (PROTECTED_EMAIL_ACCOUNT.includes(req.body.requestor.email)) {
logger.logInfo(
Expand Down
2 changes: 2 additions & 0 deletions src/utilities/createInitialPermissions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const permissionsRoles = [
'putUserProfileImportantInfo',
'changeUserStatus',
'changeUserRehireableStatus',
'pauseResumeUser',
'updatePassword',
'deleteUserProfile',
'toggleInvisibility',
Expand Down Expand Up @@ -184,6 +185,7 @@ const permissionsRoles = [
'manageTimeOffRequests',
'changeUserRehireableStatus',
'changeUserStatus',
'pauseResumeUser',
'seeBadges',
'assignBadges',
'createBadges',
Expand Down
Loading