From 81eff45614bbd99949fd0b27337b9e75c4e71575 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Tue, 21 Mar 2023 12:19:22 -0500 Subject: [PATCH 1/3] Restrict so only owners can assign other owners by adding a new USER_PERMISSION_ASSIGN_OWNERS scope to just "owners". This effectively prevents contributors from making other users "owners". --- .../v1/endpoints/user_permission_endpoints.py | 27 +++- src/fides/api/ops/api/v1/scope_registry.py | 4 + src/fides/lib/oauth/roles.py | 2 + tests/conftest.py | 84 +++++++++++ .../test_user_permission_endpoints.py | 140 +++++++++++++++++- 5 files changed, 251 insertions(+), 6 deletions(-) diff --git a/src/fides/api/ops/api/v1/endpoints/user_permission_endpoints.py b/src/fides/api/ops/api/v1/endpoints/user_permission_endpoints.py index 60de2b1cb61..ba6a768f8f5 100644 --- a/src/fides/api/ops/api/v1/endpoints/user_permission_endpoints.py +++ b/src/fides/api/ops/api/v1/endpoints/user_permission_endpoints.py @@ -1,4 +1,4 @@ -from typing import Optional +from typing import List, Optional from fastapi import Depends, HTTPException, Security from fastapi.security import SecurityScopes @@ -9,6 +9,7 @@ from fides.api.ops.api import deps from fides.api.ops.api.v1 import urn_registry as urls from fides.api.ops.api.v1.scope_registry import ( + USER_PERMISSION_ASSIGN_OWNERS, USER_PERMISSION_CREATE, USER_PERMISSION_READ, USER_PERMISSION_UPDATE, @@ -28,6 +29,7 @@ from fides.core.config import CONFIG from fides.lib.models.fides_user import FidesUser from fides.lib.models.fides_user_permissions import FidesUserPermissions +from fides.lib.oauth.roles import OWNER, RoleRegistryEnum router = APIRouter(tags=["User Permissions"], prefix=V1_URL_PREFIX) @@ -42,16 +44,31 @@ def validate_user_id(db: Session, user_id: str) -> FidesUser: return user +async def owner_role_permission_check( + db: Session, roles: List[RoleRegistryEnum], authorization: str +) -> None: + """Extra permissions check to assert that the token possesses the USER_PERMISSION_ASSIGN_OWNERS scope + if attempting to make another user an owner. + """ + if OWNER in roles: + await verify_oauth_client( + security_scopes=SecurityScopes([USER_PERMISSION_ASSIGN_OWNERS]), + authorization=authorization, + db=db, + ) + + @router.post( urls.USER_PERMISSIONS, dependencies=[Security(verify_oauth_client, scopes=[USER_PERMISSION_CREATE])], status_code=HTTP_201_CREATED, response_model=UserPermissionsResponse, ) -def create_user_permissions( +async def create_user_permissions( *, db: Session = Depends(deps.get_db), user_id: str, + authorization: str = Security(oauth2_scheme), permissions: UserPermissionsCreate, ) -> FidesUserPermissions: """Create user permissions with big picture roles and/or scopes.""" @@ -62,6 +79,7 @@ def create_user_permissions( detail="This user already has permissions set.", ) + await owner_role_permission_check(db, permissions.roles, authorization) if user.client: # Just in case - this shouldn't happen in practice. user.client.update(db=db, data=permissions.dict()) @@ -76,10 +94,11 @@ def create_user_permissions( dependencies=[Security(verify_oauth_client, scopes=[USER_PERMISSION_UPDATE])], response_model=UserPermissionsResponse, ) -def update_user_permissions( +async def update_user_permissions( *, db: Session = Depends(deps.get_db), user_id: str, + authorization: str = Security(oauth2_scheme), permissions: UserPermissionsEdit, ) -> FidesUserPermissions: """Update either a user's role(s) and/or scope(s). @@ -90,6 +109,8 @@ def update_user_permissions( user = validate_user_id(db, user_id) logger.info("Updated FidesUserPermission record") + await owner_role_permission_check(db, permissions.roles, authorization) + if user.client: user.client.update( db=db, data={"scopes": permissions.scopes, "roles": permissions.roles} diff --git a/src/fides/api/ops/api/v1/scope_registry.py b/src/fides/api/ops/api/v1/scope_registry.py index 6cfcac0e079..fc1ec61d8dc 100644 --- a/src/fides/api/ops/api/v1/scope_registry.py +++ b/src/fides/api/ops/api/v1/scope_registry.py @@ -62,6 +62,8 @@ VIEW_DATA = "view_data" WEBHOOK = "webhook" +ASSIGN_OWNERS = "assign_owners" + CLIENT_CREATE = f"{CLIENT}:{CREATE}" CLIENT_DELETE = f"{CLIENT}:{DELETE}" CLIENT_READ = f"{CLIENT}:{READ}" @@ -203,6 +205,7 @@ USER_PERMISSION_CREATE = f"{USER_PERMISSION}:{CREATE}" USER_PERMISSION_UPDATE = f"{USER_PERMISSION}:{UPDATE}" USER_PERMISSION_READ = f"{USER_PERMISSION}:{READ}" +USER_PERMISSION_ASSIGN_OWNERS = f"{USER_PERMISSION}:{ASSIGN_OWNERS}" VALIDATE_EXEC = f"{VALIDATE}:{EXEC}" @@ -315,6 +318,7 @@ USER_PASSWORD_RESET: "Reset another user's password", USER_PERMISSION_CREATE: "Create user permissions", USER_PERMISSION_UPDATE: "Update user permissions", + USER_PERMISSION_ASSIGN_OWNERS: "Assign the owner role to a user", USER_PERMISSION_READ: "View user permissions", VALIDATE_EXEC: "", WEBHOOK_CREATE_OR_UPDATE: "Create or update web hooks", diff --git a/src/fides/lib/oauth/roles.py b/src/fides/lib/oauth/roles.py index d43543917c3..bba27849cd0 100644 --- a/src/fides/lib/oauth/roles.py +++ b/src/fides/lib/oauth/roles.py @@ -40,6 +40,7 @@ STORAGE_READ, SYSTEM_MANAGER_READ, SYSTEM_READ, + USER_PERMISSION_ASSIGN_OWNERS, USER_READ, WEBHOOK_READ, ) @@ -116,6 +117,7 @@ class RoleRegistryEnum(Enum): MESSAGING_DELETE, PRIVACY_REQUEST_NOTIFICATIONS_CREATE_OR_UPDATE, CONFIG_UPDATE, + USER_PERMISSION_ASSIGN_OWNERS, ] ROLES_TO_SCOPES_MAPPING: Dict[str, List] = { diff --git a/tests/conftest.py b/tests/conftest.py index 6b78ce9be4b..5fa148edbc9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -843,6 +843,90 @@ def viewer_user(db): user.delete(db) +@pytest.fixture +def contributor_user(db): + user = FidesUser.create( + db=db, + data={ + "username": "test_fides_contributor_user", + "password": "TESTdcnG@wzJeu0&%3Qe2fGo7", + }, + ) + client = ClientDetail( + hashed_secret="thisisatest", + salt="thisisstillatest", + scopes=[], + roles=[CONTRIBUTOR], + user_id=user.id, + ) + + FidesUserPermissions.create( + db=db, data={"user_id": user.id, "scopes": [], "roles": [CONTRIBUTOR]} + ) + + db.add(client) + db.commit() + db.refresh(client) + yield user + user.delete(db) + + +@pytest.fixture +def viewer_and_approver_user(db): + user = FidesUser.create( + db=db, + data={ + "username": "test_fides_viewer_and_approver_user", + "password": "TESTdcnG@wzJeu0&%3Qe2fGo7", + }, + ) + client = ClientDetail( + hashed_secret="thisisatest", + salt="thisisstillatest", + scopes=[], + roles=[VIEWER_AND_APPROVER], + user_id=user.id, + ) + + FidesUserPermissions.create( + db=db, data={"user_id": user.id, "scopes": [], "roles": [VIEWER_AND_APPROVER]} + ) + + db.add(client) + db.commit() + db.refresh(client) + yield user + user.delete(db) + + +@pytest.fixture +def approver_user(db): + user = FidesUser.create( + db=db, + data={ + "username": "test_fides_approver_user", + "password": "TESTdcnG@wzJeu0&%3Qe2fGo7", + }, + ) + client = ClientDetail( + hashed_secret="thisisatest", + salt="thisisstillatest", + scopes=[], + roles=[APPROVER], + user_id=user.id, + ) + + FidesUserPermissions.create( + db=db, data={"user_id": user.id, "scopes": [], "roles": [APPROVER]} + ) + + db.add(client) + db.commit() + db.refresh(client) + yield user + user.delete(db) + + @pytest.fixture(scope="function") def system(db: Session) -> System: diff --git a/tests/ops/api/v1/endpoints/test_user_permission_endpoints.py b/tests/ops/api/v1/endpoints/test_user_permission_endpoints.py index 6b1f6736fc2..2180eafa9cf 100644 --- a/tests/ops/api/v1/endpoints/test_user_permission_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_user_permission_endpoints.py @@ -14,6 +14,7 @@ PRIVACY_REQUEST_READ, SAAS_CONFIG_READ, SCOPE_REGISTRY, + USER_PERMISSION_ASSIGN_OWNERS, USER_PERMISSION_CREATE, USER_PERMISSION_READ, USER_PERMISSION_UPDATE, @@ -23,7 +24,14 @@ from fides.lib.models.client import ClientDetail from fides.lib.models.fides_user import FidesUser from fides.lib.models.fides_user_permissions import FidesUserPermissions -from fides.lib.oauth.roles import OWNER, ROLES_TO_SCOPES_MAPPING, VIEWER +from fides.lib.oauth.roles import ( + APPROVER, + CONTRIBUTOR, + OWNER, + ROLES_TO_SCOPES_MAPPING, + VIEWER, + VIEWER_AND_APPROVER, +) from tests.conftest import generate_auth_header_for_user, generate_role_header_for_user @@ -187,7 +195,9 @@ def test_create_user_permissions_add_roles( def test_create_roles_on_permission_object_and_client( self, db, api_client, generate_auth_header ) -> None: - auth_header = generate_auth_header([USER_PERMISSION_CREATE]) + auth_header = generate_auth_header( + [USER_PERMISSION_CREATE, USER_PERMISSION_ASSIGN_OWNERS] + ) user = FidesUser.create( db=db, data={"username": "user_1", "password": "test_password"}, @@ -216,6 +226,69 @@ def test_create_roles_on_permission_object_and_client( assert client.roles == [OWNER] user.delete(db) + @pytest.mark.parametrize( + "acting_user,added_role,expected_response", + [ + ("owner_user", APPROVER, HTTP_201_CREATED), + ("owner_user", VIEWER_AND_APPROVER, HTTP_201_CREATED), + ("owner_user", VIEWER, HTTP_201_CREATED), + ("owner_user", CONTRIBUTOR, HTTP_201_CREATED), + ("owner_user", OWNER, HTTP_201_CREATED), + ("contributor_user", APPROVER, HTTP_201_CREATED), + ("contributor_user", VIEWER_AND_APPROVER, HTTP_201_CREATED), + ("contributor_user", VIEWER, HTTP_201_CREATED), + ("contributor_user", CONTRIBUTOR, HTTP_201_CREATED), + ("contributor_user", OWNER, HTTP_403_FORBIDDEN), + ("viewer_user", APPROVER, HTTP_403_FORBIDDEN), + ("viewer_user", VIEWER_AND_APPROVER, HTTP_403_FORBIDDEN), + ("viewer_user", VIEWER, HTTP_403_FORBIDDEN), + ("viewer_user", CONTRIBUTOR, HTTP_403_FORBIDDEN), + ("viewer_user", OWNER, HTTP_403_FORBIDDEN), + ("viewer_and_approver_user", APPROVER, HTTP_403_FORBIDDEN), + ( + "viewer_and_approver_user", + VIEWER_AND_APPROVER, + HTTP_403_FORBIDDEN, + ), + ("viewer_and_approver_user", VIEWER, HTTP_403_FORBIDDEN), + ("viewer_and_approver_user", CONTRIBUTOR, HTTP_403_FORBIDDEN), + ("viewer_and_approver_user", OWNER, HTTP_403_FORBIDDEN), + ("approver_user", APPROVER, HTTP_403_FORBIDDEN), + ("approver_user", VIEWER_AND_APPROVER, HTTP_403_FORBIDDEN), + ("approver_user", VIEWER, HTTP_403_FORBIDDEN), + ("approver_user", CONTRIBUTOR, HTTP_403_FORBIDDEN), + ("approver_user", OWNER, HTTP_403_FORBIDDEN), + ], + ) + def test_create_user_roles_permission_matrix( + self, + db, + acting_user, + added_role, + expected_response, + request, + api_client, + ): + """Test asserting what roles can be granted to other users given your current role + when assigning initial user permissions""" + acting_user = request.getfixturevalue(acting_user) + updated_user = FidesUser.create( + db=db, + data={"username": "new_user", "password": "test_password"}, + ) + + auth_header = generate_role_header_for_user( + acting_user, roles=acting_user.permissions.roles + ) + body = {"user_id": updated_user.id, "roles": [added_role]} + response = api_client.post( + f"{V1_URL_PREFIX}/user/{updated_user.id}/permission", + headers=auth_header, + json=body, + ) + assert response.status_code == expected_response + updated_user.delete(db) + class TestEditUserPermissions: @pytest.fixture(scope="function") @@ -390,7 +463,9 @@ def test_edit_user_scopes(self, db, api_client, generate_auth_header) -> None: user.delete(db) def test_edit_user_roles(self, db, api_client, generate_auth_header) -> None: - auth_header = generate_auth_header([USER_PERMISSION_UPDATE]) + auth_header = generate_auth_header( + [USER_PERMISSION_UPDATE, USER_PERMISSION_ASSIGN_OWNERS] + ) user = FidesUser.create( db=db, data={"username": "user_1", "password": "test_password"}, @@ -435,6 +510,65 @@ def test_edit_user_roles(self, db, api_client, generate_auth_header) -> None: user.delete(db) + @pytest.mark.parametrize( + "acting_user,updating_role,updated_user,expected_response", + [ + ("owner_user", APPROVER, "user", HTTP_200_OK), + ("owner_user", VIEWER_AND_APPROVER, "user", HTTP_200_OK), + ("owner_user", VIEWER, "user", HTTP_200_OK), + ("owner_user", CONTRIBUTOR, "user", HTTP_200_OK), + ("owner_user", OWNER, "user", HTTP_200_OK), + ("contributor_user", APPROVER, "user", HTTP_200_OK), + ("contributor_user", VIEWER_AND_APPROVER, "user", HTTP_200_OK), + ("contributor_user", VIEWER, "user", HTTP_200_OK), + ("contributor_user", CONTRIBUTOR, "user", HTTP_200_OK), + ("contributor_user", OWNER, "user", HTTP_403_FORBIDDEN), + ("viewer_user", APPROVER, "user", HTTP_403_FORBIDDEN), + ("viewer_user", VIEWER_AND_APPROVER, "user", HTTP_403_FORBIDDEN), + ("viewer_user", VIEWER, "user", HTTP_403_FORBIDDEN), + ("viewer_user", CONTRIBUTOR, "user", HTTP_403_FORBIDDEN), + ("viewer_user", OWNER, "user", HTTP_403_FORBIDDEN), + ("viewer_and_approver_user", APPROVER, "user", HTTP_403_FORBIDDEN), + ( + "viewer_and_approver_user", + VIEWER_AND_APPROVER, + "user", + HTTP_403_FORBIDDEN, + ), + ("viewer_and_approver_user", VIEWER, "user", HTTP_403_FORBIDDEN), + ("viewer_and_approver_user", CONTRIBUTOR, "user", HTTP_403_FORBIDDEN), + ("viewer_and_approver_user", OWNER, "user", HTTP_403_FORBIDDEN), + ("approver_user", APPROVER, "user", HTTP_403_FORBIDDEN), + ("approver_user", VIEWER_AND_APPROVER, "user", HTTP_403_FORBIDDEN), + ("approver_user", VIEWER, "user", HTTP_403_FORBIDDEN), + ("approver_user", CONTRIBUTOR, "user", HTTP_403_FORBIDDEN), + ("approver_user", OWNER, "user", HTTP_403_FORBIDDEN), + ], + ) + def test_edit_user_roles_permission_matrix( + self, + acting_user, + updating_role, + updated_user, + expected_response, + request, + api_client, + ): + """Test asserting what roles can be granted to other users given your current role""" + acting_user = request.getfixturevalue(acting_user) + updated_user = request.getfixturevalue(updated_user) + + auth_header = generate_role_header_for_user( + acting_user, roles=acting_user.permissions.roles + ) + body = {"id": updated_user.permissions.id, "roles": [updating_role]} + response = api_client.put( + f"{V1_URL_PREFIX}/user/{updated_user.id}/permission", + headers=auth_header, + json=body, + ) + assert response.status_code == expected_response + class TestGetUserPermissions: @pytest.fixture(scope="function") From 6401c9303fe702b328c48cea9619d2a87af859e6 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Tue, 21 Mar 2023 12:38:05 -0500 Subject: [PATCH 2/3] Update changelog. --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 64018e8d390..7021367b12f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,10 @@ The types of changes are: ## [Unreleased](https://github.com/ethyca/fides/compare/2.9.0...main) + ### Fixed * Fixed issue where the scopes list passed into FidesUserPermission could get mutated with the total_scopes call [#2883](https://github.com/ethyca/fides/pull/2883) +* Restricted Contributors from being able to create Owners [#2888](https://github.com/ethyca/fides/pull/2888) ## [2.9.0](https://github.com/ethyca/fides/compare/2.8.3...2.9.0) From b7c64214dd6f31e47b3b8f7d33b75fe6a6c79e89 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Wed, 22 Mar 2023 10:31:39 -0500 Subject: [PATCH 3/3] Update CHANGELOG. --- CHANGELOG.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 57838140cf7..713f44831fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,10 @@ The types of changes are: ### Changed * Improved standard layout for large width screens and polished misc. pages [#2869](https://github.com/ethyca/fides/pull/2869) +### Fixed +* Restricted Contributors from being able to create Owners [#2888](https://github.com/ethyca/fides/pull/2888) + + ## [2.9.1](https://github.com/ethyca/fides/compare/2.9.0...2.9.1) ### Added @@ -34,7 +38,6 @@ The types of changes are: ### Fixed * Fixed issue where the scopes list passed into FidesUserPermission could get mutated with the total_scopes call [#2883](https://github.com/ethyca/fides/pull/2883) -* Restricted Contributors from being able to create Owners [#2888](https://github.com/ethyca/fides/pull/2888) ### Removed * Removed the `privacyDeclarationDeprecatedFields` flag [#711](https://github.com/ethyca/fidesplus/issues/711)