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

[Backend] Restrict Owner Role Creation [#2855] #2888

Merged
merged 6 commits into from
Mar 22, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ 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

Expand Down
27 changes: 24 additions & 3 deletions src/fides/api/ops/api/v1/endpoints/user_permission_endpoints.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from typing import Optional
from typing import List, Optional

from fastapi import Depends, HTTPException, Security
from fastapi.security import SecurityScopes
Expand All @@ -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,
Expand All @@ -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)

Expand All @@ -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."""
Expand All @@ -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())
Expand All @@ -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).
Expand All @@ -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}
Expand Down
4 changes: 4 additions & 0 deletions src/fides/api/ops/api/v1/scope_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down Expand Up @@ -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}"

Expand Down Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions src/fides/lib/oauth/roles.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
STORAGE_READ,
SYSTEM_MANAGER_READ,
SYSTEM_READ,
USER_PERMISSION_ASSIGN_OWNERS,
USER_READ,
WEBHOOK_READ,
)
Expand Down Expand Up @@ -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] = {
Expand Down
84 changes: 84 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
Loading