diff --git a/CHANGELOG.md b/CHANGELOG.md index ccbfeedb135..801efa6a1c1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ The types of changes are: * Improved standard layout for large width screens and polished misc. pages [#2869](https://github.com/ethyca/fides/pull/2869) * Deprecated adding scopes to users directly; you can only add roles. [#2848](https://github.com/ethyca/fides/pull/2848/files) +### 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 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 f041e25f3aa..df4d5cae919 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) @@ -43,16 +45,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 associated roles.""" @@ -63,6 +80,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()) @@ -77,10 +95,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 a user's role(s). The UI assigns one role at a time, but multiple @@ -91,6 +110,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={"roles": permissions.roles}) return user.permissions.update( # type: ignore[attr-defined] 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 7210de4f664..fbc20133130 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -838,6 +838,88 @@ 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, "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, "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, "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 aafd317ac4a..5f353da7540 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, @@ -29,6 +30,7 @@ OWNER, ROLES_TO_SCOPES_MAPPING, VIEWER, + VIEWER_AND_APPROVER, ) from tests.conftest import generate_auth_header_for_user, generate_role_header_for_user @@ -168,6 +170,69 @@ def test_create_user_permissions_add_roles( assert client.scopes == [], "User create flow doesn't override client scopes" 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") @@ -260,7 +325,9 @@ def test_edit_user_scopes_are_ignored( self, db, api_client, generate_auth_header ) -> None: """Scopes in request are ignored as you can no longer edit user scopes""" - 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"}, @@ -306,7 +373,9 @@ def test_edit_user_scopes_are_ignored( 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"}, @@ -347,6 +416,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")