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

Adds System Manager (Data Steward) Endpoints [#2607] #2726

Merged

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Mar 1, 2023

❗ Depends on #2609
Closes #2607

Code Changes

Update systems for which user is system manager.

Replaces all systems with systems in the request body. An empty list would remove a user as manager of all systems.

  • PUT /user/{user_id}/system-manager
["demo_analytics_system", "demo_marketing_system"]

Get all systems for which user is system manager.

  • GET /user/{user_id}/system-manager

Get details about a single system a user manages

  • GET /user/{user_id}/system-manager/{system_key}

Remove user as system manager from a specific system.

  • DELETE /user/{user_id}/system-manager/{system_key}

Adds new system manager scopes.

Adds the read scopes to the viewer role.

Steps to Confirm

  • You can use these endpoints to add a user as system manager and then experiment in the UI. See instructions here: Add Object-Based Permissions Foundation #2609. Note that product has said system managers will typically also have a "viewer" role and that the system manager scopes only give them specific permissions to a specific system.

Pre-Merge Checklist

Description Of Changes

  • Add new endpoints to manage system managers. PUT system managers replaces all the systems the user manages with those in the request body.

…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.
@cypress
Copy link

cypress bot commented Mar 1, 2023

Passing run #610 ↗︎

0 3 0 0 Flakiness 0

Details:

Merge 9c7d060 into 0b99415...
Project: fides Commit: 508b8fc62e ℹ️
Status: Passed Duration: 00:44 💡
Started: Mar 3, 2023 10:27 PM Ended: Mar 3, 2023 10:28 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@pattisdr pattisdr marked this pull request as ready for review March 1, 2023 03:08
@pattisdr pattisdr added the do not merge Please don't merge yet, bad things will happen if you do label Mar 1, 2023
@pattisdr pattisdr changed the title Adds System Manager Endpoints [#2607] Adds System Manager (Data Steward) Endpoints [#2607] Mar 1, 2023
@codecov
Copy link

codecov bot commented Mar 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.04 🎉

Comparison is base (0b99415) 86.69% compared to head (9c7d060) 86.73%.

Additional details and impacted files
@@                         Coverage Diff                          @@
##           fides_2609_add_system_managers_3    #2726      +/-   ##
====================================================================
+ Coverage                             86.69%   86.73%   +0.04%     
====================================================================
  Files                                   293      293              
  Lines                                 16402    16454      +52     
  Branches                               2078     2087       +9     
====================================================================
+ Hits                                  14220    14272      +52     
  Misses                                 1795     1795              
  Partials                                387      387              
Impacted Files Coverage Δ
src/fides/lib/oauth/roles.py 100.00% <ø> (ø)
...c/fides/api/ops/api/v1/endpoints/user_endpoints.py 94.78% <100.00%> (+3.47%) ⬆️
src/fides/api/ops/api/v1/scope_registry.py 100.00% <100.00%> (ø)
src/fides/api/ops/api/v1/urn_registry.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Copy link
Contributor

@allisonking allisonking left a comment

Choose a reason for hiding this comment

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

🤩 🤩 🤩

Comment on lines 303 to 305
SYSTEM_MANAGER_READ: "Read systems users can manager",
SYSTEM_MANAGER_DELETE: "Delete systems user can manager",
SYSTEM_MANAGER_UPDATE: "Update systems user can manager",
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
SYSTEM_MANAGER_READ: "Read systems users can manager",
SYSTEM_MANAGER_DELETE: "Delete systems user can manager",
SYSTEM_MANAGER_UPDATE: "Update systems user can manager",
SYSTEM_MANAGER_READ: "Read systems users can manage",
SYSTEM_MANAGER_DELETE: "Delete systems user can manage",
SYSTEM_MANAGER_UPDATE: "Update systems user can manage",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah thank you!

@pattisdr pattisdr removed the do not merge Please don't merge yet, bad things will happen if you do label Mar 6, 2023
@pattisdr pattisdr merged commit a8b1f75 into fides_2609_add_system_managers_3 Mar 6, 2023
@pattisdr pattisdr deleted the fides_2607_system_manager_endpoints branch March 6, 2023 14:44
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