Roles #2336

imnasnainaec · 2023-06-29T20:07:37Z

Resolves #1983

This change is

codecov-commenter · 2023-06-30T13:30:53Z

Codecov Report

Patch coverage: 57.04% and project coverage change: +12.87 🎉

Comparison is base (97e5a30) 61.08% compared to head (f25c0e3) 73.95%.

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #2336       +/-   ##
===========================================
+ Coverage   61.08%   73.95%   +12.87%     
===========================================
  Files         237       45      -192     
  Lines        8294     4396     -3898     
  Branches      522        0      -522     
===========================================
- Hits         5066     3251     -1815     
+ Misses       2848     1145     -1703     
+ Partials      380        0      -380

Flag	Coverage Δ
backend	`73.95% <57.04%> (-0.40%)`	⬇️
frontend	`?`

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

Impacted Files	Coverage Δ
Backend/Controllers/InviteController.cs	`0.00% <0.00%> (ø)`
Backend/Controllers/StatisticsController.cs	`0.00% <0.00%> (ø)`
Backend/Services/InviteService.cs	`0.00% <0.00%> (ø)`
Backend/Services/PermissionService.cs	`0.00% <0.00%> (ø)`
Backend/Controllers/LiftController.cs	`54.18% <40.00%> (ø)`
Backend/Controllers/UserRoleController.cs	`88.97% <63.63%> (-7.33%)`	⬇️
Backend/Controllers/ProjectController.cs	`57.69% <75.00%> (-2.67%)`	⬇️
Backend/Models/UserRole.cs	`81.81% <77.04%> (-18.19%)`	⬇️
Backend/Controllers/UserEditController.cs	`89.25% <100.00%> (ø)`
Backend/Controllers/WordController.cs	`98.55% <100.00%> (ø)`
... and 1 more

... and 192 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

imnasnainaec · 2023-07-05T22:44:22Z

Corresponding necessary database update here: https://docs.google.com/document/d/1L9WjBZqBNVfxk5KiIQo5Fzjsetv4JuwGU0wuYKDRtWo

imnasnainaec

@jmgrady "Another option" implemented.

Reviewable status: 0 of 47 files reviewed, all discussions resolved (waiting on @imnasnainaec)

jmgrady

Reviewed 5 of 31 files at r1, 10 of 30 files at r2, 1 of 5 files at r4.
Reviewable status: 14 of 47 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)

README.md line 863 at r5 (raw file):

   `Editor` role, or the `--admin` option to instead add the user with `Administrator` role.

#### Backup _TheCombine_

I realize that I am the one who originally put the duplicate the instructions for the add_user_to_proj.py Python script here but I think we should just say:

Run:

kubectl exec -it deployment/maintenance -- add_user_to_proj.py --help`

for additional options.

This decreases the likelihood of documentation errors and decreases maintenance work for the script.

Code quote:

1. The `--project` and `--user` options may be shortened to `--p` and `--u` respectively.
2. The user is added to the project with `Harvester` role. Add the `--editor` option to instead add the user with
   `Editor` role, or the `--admin` option to instead add the user with `Administrator` role.

Backend/Controllers/ProjectController.cs line 182 at r5 (raw file):

        [HttpDelete("{projectId}", Name = "DeleteProject")]
        [ProducesResponseType(StatusCodes.Status200OK)]
        public async Task<IActionResult> DeleteProject(string projectId)

This is beyond the scope of this PR but DeleteProject is a misnomer and will cause problems when we add the ability to actually delete projects as has been requested.

Code quote:

       public async Task<IActionResult> DeleteProject(string projectId)

jmgrady

Reviewed 2 of 30 files at r2, 1 of 5 files at r4, 3 of 3 files at r6, all commit messages.
Reviewable status: 20 of 47 files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)

Backend.Tests/Controllers/UserRoleControllerTests.cs line 104 at r7 (raw file):

            var foundPermissions = ((ObjectResult)action).Value as List<Permission>;
            Assert.AreEqual(ProjectRole.RolePermissions(userRole.Role!), foundPermissions);

This seems to assume that the permissions are always stored in the same order. That could be a valid assumption for now but since we treat the permissions more like a set than an ordered list, it might be best to sort both lists first.

Code quote:

            Assert.AreEqual(ProjectRole.RolePermissions(userRole.Role!), foundPermissions);

imnasnainaec

Reviewable status: 19 of 47 files reviewed, 1 unresolved discussion (waiting on @jmgrady)

Backend/Controllers/ProjectController.cs line 182 at r5 (raw file):

Previously, jmgrady (Jim Grady) wrote…

This is beyond the scope of this PR but DeleteProject is a misnomer and will cause problems when we add the ability to actually delete projects as has been requested.

Not a misnomer here. This function actually deletes a project, but we don't use it. Our project archiving function is defined in the frontend and just uses the project update function from this controller.

jmgrady

Reviewed 4 of 31 files at r1, 3 of 30 files at r2, 2 of 5 files at r4, 1 of 1 files at r7.
Reviewable status: 28 of 47 files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)

maintenance/scripts/combine_app.py line 28 at r7 (raw file):

    # Integer value 7 is currently unused.
    # Integer value 8 is currently unused.
    Owner = 9

This is one of 3 places where this Enum is defined - largely because it is used by code written in 3 different languages. However, the enum values are defined as integers in the Python maintenance script and the Backend but defined as strings in the Frontend.

Since each user can only have a single role in a project, I would like to suggest that the Roles enum be changed to have string values for the Backend, the Database, and these maintenance scripts. This would make the results of database queries more readable, minimized Enum definition errors and would obviate the need for "gaps" in the roles sequence (and the risk that any one gap that we choose today is not big enough as roles grow).

Presumably, the value of using an integer is so that one can use '>' of '<' to determine if one role has more permission than another. However, the only place that I have found where this is used is in the maintenance script, add_user_to_proj.py. The comments say that this is to make sure than a users role is never decreased through this script. This is not a necessary requirement of the script; in fact, it may be more useful to be able to directly set the role of a new user. Note that this script can only be executed by users who have access to running a shell on the Kubernetes cluster; it is never called by The Combine application.

Code quote:

class Role(Enum):
    """Define enumerated type for Combine user roles."""

    # None = 0
    # Integer value 1 is currently unused.
    Harvester = 2
    # Integer value 3 is currently unused.
    Editor = 4
    # Integer value 5 is currently unused.
    Administrator = 6
    # Integer value 7 is currently unused.
    # Integer value 8 is currently unused.
    Owner = 9

jmgrady

Reviewed 3 of 31 files at r1, 8 of 30 files at r2, 2 of 3 files at r3, 3 of 4 files at r5, 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)

src/components/ProjectSettings/ProjectUsers/ActiveProjectUsers.tsx line 36 at r10 (raw file):

  );

  const [projectId] = useState(getProjectId());

If there is no function to set the projectId I am not sure that useState is appropriate here. Why not just call getProjectId() each time it is used? (or once at the beginning of the function).

That said, I see in the file src/components/ProjectSettings/tests/index.test.tsx that the test relies on a change of projectId to be seen and to trigger a rerender. Is this possible in the normal operation of the application? I did not think that it was. It suggests that the current project id is coming from outside the component - shouldn't this be a redux state and useAppSelector be used instead of useState?

Code quote:

  const [projectId] = useState(getProjectId());

src/components/ProjectSettings/ProjectUsers/ActiveProjectUsers.tsx line 80 at r10 (raw file):

  const currentIsProjOwner = userRoles[currentUser.id] === Role.Owner;
  const currentIsProjAdminOrOwner =

The checks for if a user is an Administrator or Owner suggest that there is a missing permission type that should be checked rather than checking for roles. (Another place this is seen is for viewing Statistics only in that case, the check is for Site Administrator.)

Suggestion:

...AdminOrOwner

imnasnainaec

Reviewable status: 46 of 47 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

src/components/ProjectSettings/ProjectUsers/ActiveProjectUsers.tsx line 80 at r10 (raw file):

Previously, jmgrady (Jim Grady) wrote…

The checks for if a user is an Administrator or Owner suggest that there is a missing permission type that should be checked rather than checking for roles. (Another place this is seen is for viewing Statistics only in that case, the check is for Site Administrator.)

My mental model of this only has the frontend fetching permissions for the current user--when a user is managing other users, since they'll be operating in terms of roles and not permissions, it felt more natural to design the corresponding frontend operations as strictly role-based.

jmgrady

Reviewed 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @imnasnainaec)

src/components/ProjectSettings/ProjectUsers/ActiveProjectUsers.tsx line 80 at r10 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

My mental model of this only has the frontend fetching permissions for the current user--when a user is managing other users, since they'll be operating in terms of roles and not permissions, it felt more natural to design the corresponding frontend operations as strictly role-based.

I still think that this is indicative of an unidentified permission or permissions. The permission in question is does the user have permission to ModifyUserRoles (which may be different than DeleteEditSettingsAndUsers) The other possibility is to check if the user has the DeleteEditSettingsAndUsers permission. The backend checks that you just added will make sure an admin doesn't demote an owner, or other types of coups, correct?

Also, how does a user who is a site admin fit into this?

imnasnainaec

Reviewable status: 44 of 47 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

maintenance/scripts/combine_app.py line 28 at r7 (raw file):

Previously, jmgrady (Jim Grady) wrote…

This is one of 3 places where this Enum is defined - largely because it is used by code written in 3 different languages. However, the enum values are defined as integers in the Python maintenance script and the Backend but defined as strings in the Frontend.

Since each user can only have a single role in a project, I would like to suggest that the Roles enum be changed to have string values for the Backend, the Database, and these maintenance scripts. This would make the results of database queries more readable, minimized Enum definition errors and would obviate the need for "gaps" in the roles sequence (and the risk that any one gap that we choose today is not big enough as roles grow).

Presumably, the value of using an integer is so that one can use '>' of '<' to determine if one role has more permission than another. However, the only place that I have found where this is used is in the maintenance script, add_user_to_proj.py. The comments say that this is to make sure than a users role is never decreased through this script. This is not a necessary requirement of the script; in fact, it may be more useful to be able to directly set the role of a new user. Note that this script can only be executed by users who have access to running a shell on the Kubernetes cluster; it is never called by The Combine application.

Done

src/components/ProjectSettings/ProjectUsers/ActiveProjectUsers.tsx line 80 at r10 (raw file):

Previously, jmgrady (Jim Grady) wrote…

I still think that this is indicative of an unidentified permission or permissions. The permission in question is does the user have permission to ModifyUserRoles (which may be different than DeleteEditSettingsAndUsers) The other possibility is to check if the user has the DeleteEditSettingsAndUsers permission. The backend checks that you just added will make sure an admin doesn't demote an owner, or other types of coups, correct?

Also, how does a user who is a site admin fit into this?

#708 is for site admins (to be implemented in site settings).

I'll look again at the permissions used here.

imnasnainaec

Reviewable status: 43 of 47 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

src/components/ProjectSettings/ProjectUsers/ActiveProjectUsers.tsx line 80 at r10 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

#708 is for site admins (to be implemented in site settings).

I'll look again at the permissions used here.

Yes, I see what you mean about permissions. Since the component in question is only visible with permission DeleteEditSettingsAndUsers I just simplified the check here. If we add other roles with user-management ability, it may need a more permissiony redesign.

imnasnainaec

Reviewable status: 42 of 47 files reviewed, 2 unresolved discussions (waiting on @jmgrady)

src/components/ProjectSettings/ProjectUsers/ActiveProjectUsers.tsx line 36 at r10 (raw file):

Previously, jmgrady (Jim Grady) wrote…

If there is no function to set the projectId I am not sure that useState is appropriate here. Why not just call getProjectId() each time it is used? (or once at the beginning of the function).

That said, I see in the file src/components/ProjectSettings/tests/index.test.tsx that the test relies on a change of projectId to be seen and to trigger a rerender. Is this possible in the normal operation of the application? I did not think that it was. It suggests that the current project id is coming from outside the component - shouldn't this be a redux state and useAppSelector be used instead of useState?

It is possible with the ProjectSwitch component.

I updated this to get the project id from its parent, which fits what the other project settings components are doing.

jmgrady

Reviewed 3 of 3 files at r12, 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @imnasnainaec)

maintenance/scripts/add_user_to_proj.py line 50 at r14 (raw file):

    )
    return parser.parse_args()

The argument parser setup can be improved. Currently someone could specify --admin and --editor. If you do the following the user can pick one role and it stays in sync with changes to the Role enum:

remove the add_argument calls for --editor and --admin;
Add the following call:

parser.add_argument("--role", choices=[role.value for role in Role], default=Role.Harvester.value, help=...)

Remove logic for setting req_role - can set directly to args.role.
Probably want to add a check for args.role == Role.Owner.value and throw an error. The other option would be to create a function that returns all the enum values except Owner starting with the example above.

One other nice thing (that I know about now) is that you can specify short, single-dash options as an equivalent for the double-dash ones simply but adding it as an additional argument, for example,

parser.add\_argument(
   "--project", "-p", required\=True, help\="Project(s) to be removed from TheCombine."
)

would allow you to set the project using --project or -p.

imnasnainaec

Reviewable status: 46 of 47 files reviewed, 1 unresolved discussion (waiting on @jmgrady)

maintenance/scripts/add_user_to_proj.py line 50 at r14 (raw file):

Previously, jmgrady (Jim Grady) wrote…

The argument parser setup can be improved. Currently someone could specify --admin and --editor. If you do the following the user can pick one role and it stays in sync with changes to the Role enum:

remove the add_argument calls for --editor and --admin;

Add the following call:
parser.add_argument("--role", choices=[role.value for role in Role], default=Role.Harvester.value, help=...)
Remove logic for setting req_role - can set directly to args.role.

Probably want to add a check for args.role == Role.Owner.value and throw an error. The other option would be to create a function that returns all the enum values except Owner starting with the example above.

One other nice thing (that I know about now) is that you can specify short, single-dash options as an equivalent for the double-dash ones simply but adding it as an additional argument, for example,
parser.add\_argument(
   "--project", "-p", required\=True, help\="Project(s) to be removed from TheCombine."
)
would allow you to set the project using --project or -p.

Updated.

From my understanding, the first letter of an argument is automatically available as a single-dash arg (if it's the only arg that begins with that letter).

jmgrady

Reviewed 1 of 1 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @imnasnainaec)

maintenance/scripts/add_user_to_proj.py line 50 at r14 (raw file):

Previously, imnasnainaec (D. Ror.) wrote…

Updated.

From my understanding, the first letter of an argument is automatically available as a single-dash arg (if it's the only arg that begins with that letter).

No, --p would work but not -p. Any abbreviation of the option will work if it is not ambiguous - but requires both - characters.

imnasnainaec added 4 commits June 29, 2023 11:23

Define roles

b194264

Fix Backend

83e1762

Distinguish ProjectRole and Role

2bed823

Match frontend to backend

fbc98a2

imnasnainaec self-assigned this Jun 29, 2023

imnasnainaec added 2 commits June 29, 2023 16:11

Merge branch 'master' into roles

9af3bd8

Merge branch 'master' into roles

59b7893

imnasnainaec added 15 commits June 30, 2023 12:15

Merge branch 'master' into roles

e5578a4

Merge branch 'master' into roles

b483399

Obey CA2201

2b9d7cb

Change Manager -> Editor

f647a21

Merge branch 'master' into roles

96aead6

Finish frontend role/permission refactor

7891c03

Fix tests

bdffb1e

Add Editor option in project user management

4bd94fe

Fix bugs

97ff6d3

Update maintenance scripts

1d26fef

Finish backend role implementation

630c6dd

Clean up

c45d6fc

Clean up

0ce3523

Tidy

97a9594

Merge branch 'master' into roles

41200a6

imnasnainaec marked this pull request as ready for review July 5, 2023 16:45

This was referenced Jun 14, 2023

[ProjectSettings] See user roles in ActiveUsers list #705

Closed

[ProjectSettings] Update visibility for non-owner users #707

Closed

imnasnainaec added 3 commits July 6, 2023 09:59

Add ProjectSettings component permissions tests

06d71c9

Tidy

fd2c90b

[Backend.Tests/Models] Expand tests

a1d8907

imnasnainaec commented Jul 7, 2023

View reviewed changes

imnasnainaec added 2 commits July 7, 2023 10:27

Tidy

5655eae

Match role names in strings

cd4ff28

jmgrady reviewed Jul 7, 2023

View reviewed changes

Make list-equality check order-indifferent

dc3ecfb

imnasnainaec commented Jul 10, 2023

View reviewed changes

imnasnainaec added 2 commits July 10, 2023 11:11

Despecificate the README

2c83cbc

Merge branch 'master' into roles

dfa3a4d

jmgrady requested changes Jul 10, 2023

View reviewed changes

Safeguard against (e.g.) an admin changing an owner's role

3520522

imnasnainaec commented Jul 10, 2023

View reviewed changes

jmgrady reviewed Jul 10, 2023

View reviewed changes

Store Role enum as strings in mongo

11605ce

imnasnainaec commented Jul 10, 2023

View reviewed changes

Simplify canManageUser

3d909de

imnasnainaec commented Jul 10, 2023

View reviewed changes

Feed ActiveProjectUsers project id from parent, not local storage

96279fd

imnasnainaec commented Jul 10, 2023

View reviewed changes

jmgrady requested changes Jul 10, 2023

View reviewed changes

Update add_user_to_proj args

fd08c3b

imnasnainaec commented Jul 11, 2023

View reviewed changes

imnasnainaec added 2 commits July 11, 2023 09:15

tox -e fmt

f9104be

Merge branch 'master' into roles

f25c0e3

jmgrady approved these changes Jul 11, 2023

View reviewed changes

imnasnainaec mentioned this pull request Jul 11, 2023

Update the live database after #2336 is released #2369

Closed

imnasnainaec merged commit 37b3d1c into master Jul 11, 2023

imnasnainaec deleted the roles branch July 11, 2023 18:25

imnasnainaec mentioned this pull request Sep 25, 2023

[UserGuide] Reorder project settings; Add user role descriptions #2636

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roles #2336

Roles #2336

imnasnainaec commented Jun 29, 2023 •

edited by jasonleenaylor

Loading

codecov-commenter commented Jun 30, 2023 •

edited

Loading

imnasnainaec commented Jul 5, 2023

imnasnainaec left a comment

jmgrady left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

imnasnainaec left a comment

imnasnainaec left a comment

jmgrady left a comment

imnasnainaec left a comment

jmgrady left a comment

Roles #2336

Roles #2336

Conversation

imnasnainaec commented Jun 29, 2023 • edited by jasonleenaylor Loading

codecov-commenter commented Jun 30, 2023 • edited Loading

Codecov Report

imnasnainaec commented Jul 5, 2023

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec left a comment

Choose a reason for hiding this comment

jmgrady left a comment

Choose a reason for hiding this comment

imnasnainaec commented Jun 29, 2023 •

edited by jasonleenaylor

Loading

codecov-commenter commented Jun 30, 2023 •

edited

Loading