Role assignment UI #2739

allisonking · 2023-03-02T22:56:53Z

Closes https://github.com/ethyca/fidesplus/issues/641

~~This branches off of multiple branches, so the diff will be unseemly until those branches are merged~~

Branches are merged, though commit history has many of the commits from #2682 because of the squashed merge + I'm not good enough at rebasing 🙃

Code Changes

See the ticket

Steps to Confirm

list any manual steps for reviewers to confirm the changes

Pre-Merge Checklist

All CI Pipelines Succeeded
Documentation:
- documentation complete, PR opened in fidesdocs
- documentation issue created in fidesdocs
Issue Requirements are Met
Relevant Follow-Up Issues Created
Update CHANGELOG.md

Description Of Changes

Write some things here about the changes and any potential caveats

- Add system id list to client - Add methods to add and remove users as system managers. - When creating tokens, persist a list of system ids to the token.

…points that have limited system manager permissions. - Multiple variations required depending on whether it is the fides key or a request body as dependencies. - Separate out "system update" and "system delete" from the crud routes to give an additional system manager oauth check. - Update these endpoints to have new "system update" and "system delete" scopes. - Bump downrev of migration. - Rename _has_correct_scopes to has_scope_subset since I will share this method between files - Move some common logic in verify_oauth_util into a new method: extract_token_and_load_client that can be shared with multiple authentication methods. - Update client on login with scopes/roles/systems if client already exists, just in case - Consolidate system fixtures into one and add required fields

…gular scope doesn't grant the user permissions to 20+ endpoints. - This allows us to do things like give system managers permission to just a few endpoints, not all ctl-related resources. - Rename scopes that would conflict with existing ops scopes - Add new read scopes to the viewer role - Remove updating client on login (added last commit).

…efault an empty list to match scopes and roles. - Add new system-manager-related annotations to db dataset yaml. - Add tests with correct scopes and incorrect scopes to crud endpoints.

…s and client.scopes columns if the user has cli-objects:-* scopes. Assert "resource deleted" in crud and system delete responses.

- Fix test for TestGetSystemFromRequestBody which was not getting run.

…stem manager is deleted. - Clients may still have systems on them, but the user or system doesn't exist so this doesn't seem problematic. - Refresh client on login so it has the most up-to-date values, especially if they've been adjusted directly in the db.

…aces all the systems the user manages with those in the request body. - PUT /user/{user_id}/system-manager - GET /user/{user_id}/system-manager - GET /user/{user_id}/system-manager/{system_key} - DELETE /user/{user_id}/system-manager/{system_key} Adds new system manager scopes. Adds the read scopes to the viewer role.

… to maintain the status quo.

…m_manager_endpoints

…v is set to prod so we can populate all the starter resources.

…m_manager_endpoints

pattisdr · 2023-03-14T15:20:54Z

There are these role descriptions hanging out on the right with Owner text duplicated in each of them -

pattisdr · 2023-03-14T15:58:23Z

Looks like we're still using the old mapping in the user summary page. The new role name is "Approver" but in this summary it's being called "Privacy Request Manager", same for "Viewer and Privacy Request Manager."

Product requirements changed during this project and they renamed the roles.

pattisdr · 2023-03-14T16:06:13Z

I'm assuming this is pre-existing behavior, but the backend only allows the first name and last name of a user to be edited here after it's created, not the username. Should we disable the username from being edited here? It gives the appearance you can edit but you can't. This could be reticketed.

eastandwestwind · 2023-03-14T16:19:43Z

@pattisdr I went ahead and updated the username form field to be disabled after the user is created

pattisdr · 2023-03-14T16:21:29Z

Bravo! You're addressing changes faster than I'm finding them.

pattisdr · 2023-03-14T16:23:43Z

I do think this should be reticketed: Saving changes on ctl datasets when you don't have permissions shows a green popup that you "Success: Successfully modified collection" although the API request returned a 403 and no data changed.

eastandwestwind · 2023-03-14T16:28:45Z

@pattisdr ticketed here- #2827

… e2e testing

pattisdr · 2023-03-14T19:58:55Z

This is looking really good @allisonking and @eastandwestwind.

I've reticketed separate backend work here https://github.com/ethyca/fidesplus/issues/683 for next sprint

This is to avoid too much prop drilling, especially while delete can be called in two different flows

allisonking · 2023-03-14T20:35:53Z

Great work, @eastandwestwind ! I went in and QA'd and fixed up a few things:

It said "Update Password" and "Reset password" which bothered me so I changed it to "Update password" to keep our casing consistent 😎 Also added a success toast on Update password since that wasn't there before (totally out of the scope of this work, but it was easy)
Refactored the delete modal logic a bit so that we don't have to pass the user object all the way down. I recently made a change (maybe it was in this PR...) where we store the activeUser (i.e. the user being edited) in redux, so we can take advantage of that to avoid some prop drilling. Also, before, after you deleted a user, you'd still be left on that user's page (which shouldn't exist anymore) so I added a redirect
There was a sneaky bug where if you went to a user who had a role, i.e. "owner", and then went to another user who had no roles, that user would show up as being an "owner" in the role selection page. This was probably my fault from earlier, but should be fixed now

eastandwestwind · 2023-03-15T13:28:14Z

Just added a slight functionality change for better experience.

Before, when deleting a user on their profile page, we stayed on that page, which is misleading because you can no longer edit the user if it doesn't exist.

I'm updating such that we reroute to user management table upon user delete

eastandwestwind · 2023-03-15T13:46:33Z

Side note: I've noticed some flakiness with the Can edit a system -> Can go through the edit flow cypress test in systems.cy.ts. Sometimes cypress is able to find cy.getByTestId("continue-btn").click();, and sometimes this element doesn't exist, which causes the test to fail.

We'll soon be able to remove this line of code anyway, as it's a known bug, documented here- #2788

For now, though, re-running the test suite seems to work.

eastandwestwind · 2023-03-15T15:05:38Z

Reverting to Allison's last commit, since she already covered the reroute to user management page upon user delete!

pattisdr and others added 30 commits February 27, 2023 13:02

Add a system manager many to many table.

36d2f97

- Add system id list to client - Add methods to add and remove users as system managers. - When creating tokens, persist a list of system ids to the token.

Update client.systems default to be non-nullable - instead make the d…

f069929

…efault an empty list to match scopes and roles. - Add new system-manager-related annotations to db dataset yaml. - Add tests with correct scopes and incorrect scopes to crud endpoints.

Fix systemmanager annotations.

c470674

Use enum instead of string list for scopes

fc6c5ab

Update autogenerated TS types

cda6e90

Add user scope restrictions to nav-config

1d098dd

Additional tests for scope based navigation

03dd7b8

Give home tiles the nav-config treatment

621a699

Add tests for tile-config

bd35748

Handle datamap route

6297321

Create selector for grabbing this user's scopes

823555d

Configure roles for cypress tests

82c43ac

Add <Restrict /> example

209b971

Tack on data migration to add new scopes to fidesuserpermission.scope…

77b18c0

…s and client.scopes columns if the user has cli-objects:-* scopes. Assert "resource deleted" in crud and system delete responses.

Add login test for system manager.

036a46f

- Fix test for TestGetSystemFromRequestBody which was not getting run.

Update logs.

6070568

Implement protected route by scope

1599cd0

Refactor system tests to avoid coroutine errors.

1b0ecfb

Bypass authentication for system update and system delete in dev mode…

86402fb

… to maintain the status quo.

Merge branch 'fides_2609_add_system_managers_3' into fides_2607_syste…

2496150

…m_manager_endpoints

Login when running nox -s fides_env(...) in case CONFIG.security.en…

3e23946

…v is set to prod so we can populate all the starter resources.

Merge branch 'fides_2609_add_system_managers_3' into fides_2607_syste…

02a0c0d

…m_manager_endpoints

Update changelog

a62f37e

Merge branch 'main' into aking/fides-plus-599/access-controls-ui

c8b0ce1

Increase test coverage.

a0de19e

Fix protected route flash

87f920e

pattisdr mentioned this pull request Mar 13, 2023

Remove Scopes and Add Viewer Role #2821

Merged

11 tasks

eastandwestwind added 3 commits March 14, 2023 15:02

refactor delete user out of pw management component

ad6d8fc

pull latest main

e25be51

lint fixes

341b1cd

eastandwestwind added 2 commits March 14, 2023 16:45

fix cypress test

ad2387c

fix duplicate text in role description drawer

2ef6e8f

fix permissions labels

68a899c

eastandwestwind mentioned this pull request Mar 14, 2023

Show failure instead of success toast upon 403 for updating ctl datasets #2827

Open

eastandwestwind and others added 4 commits March 14, 2023 17:47

adds custom test id to confirmation modal component for more targeted…

a550d84

… e2e testing

use system name in confirmation modal id

86bfb28

use force click on cypress test

c7ff538

Fix casing of Update password and add success toast

b061b9d

allisonking added 2 commits March 14, 2023 16:06

Refactor delete modal to use activeUser from redux

7ce7775

This is to avoid too much prop drilling, especially while delete can be called in two different flows

Fix bug where other users' roles carried over between users

6b0c94d

eastandwestwind approved these changes Mar 15, 2023

View reviewed changes

eastandwestwind force-pushed the aking/fidesplus-641/role-assignment-ui branch from 1ff5847 to 6b0c94d Compare March 15, 2023 15:04

eastandwestwind merged commit d0d6b3c into main Mar 15, 2023

eastandwestwind deleted the aking/fidesplus-641/role-assignment-ui branch March 15, 2023 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Role assignment UI #2739

Role assignment UI #2739

allisonking commented Mar 2, 2023 •

edited by eastandwestwind

Loading

pattisdr commented Mar 14, 2023

pattisdr commented Mar 14, 2023

pattisdr commented Mar 14, 2023

eastandwestwind commented Mar 14, 2023

pattisdr commented Mar 14, 2023

pattisdr commented Mar 14, 2023

eastandwestwind commented Mar 14, 2023

pattisdr commented Mar 14, 2023 •

edited

Loading

allisonking commented Mar 14, 2023

eastandwestwind commented Mar 15, 2023 •

edited

Loading

eastandwestwind commented Mar 15, 2023

eastandwestwind commented Mar 15, 2023

Role assignment UI #2739

Role assignment UI #2739

Conversation

allisonking commented Mar 2, 2023 • edited by eastandwestwind Loading

Code Changes

Steps to Confirm

Pre-Merge Checklist

Description Of Changes

pattisdr commented Mar 14, 2023

pattisdr commented Mar 14, 2023

pattisdr commented Mar 14, 2023

eastandwestwind commented Mar 14, 2023

pattisdr commented Mar 14, 2023

pattisdr commented Mar 14, 2023

eastandwestwind commented Mar 14, 2023

pattisdr commented Mar 14, 2023 • edited Loading

allisonking commented Mar 14, 2023

eastandwestwind commented Mar 15, 2023 • edited Loading

eastandwestwind commented Mar 15, 2023

eastandwestwind commented Mar 15, 2023

allisonking commented Mar 2, 2023 •

edited by eastandwestwind

Loading

pattisdr commented Mar 14, 2023 •

edited

Loading

eastandwestwind commented Mar 15, 2023 •

edited

Loading