From 3ca6f4c503a7159b974e817d9981092062a19cd6 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sat, 26 Jul 2025 23:03:25 -0700 Subject: [PATCH 01/19] feat(workspaces): add rwx perms to blueprints and ranges --- api/src/app/crud/crud_permissions.py | 243 ++++++++++++++++++ api/src/app/crud/crud_ranges.py | 145 +++++++++-- api/src/app/models/__init__.py | 6 + api/src/app/models/permission_models.py | 107 ++++++++ api/src/app/models/range_models.py | 15 ++ api/src/app/schemas/range_schemas.py | 72 +++++- api/tests/common/api/v1/config.py | 2 + .../common/api/v1/test_blueprint_ranges.py | 2 + api/tests/unit/api/v1/config.py | 5 + 9 files changed, 577 insertions(+), 20 deletions(-) create mode 100644 api/src/app/crud/crud_permissions.py create mode 100644 api/src/app/models/permission_models.py diff --git a/api/src/app/crud/crud_permissions.py b/api/src/app/crud/crud_permissions.py new file mode 100644 index 00000000..e9629dc1 --- /dev/null +++ b/api/src/app/crud/crud_permissions.py @@ -0,0 +1,243 @@ +import logging +from typing import Literal + +from sqlalchemy import select +from sqlalchemy.ext.asyncio import AsyncSession +from sqlalchemy.exc import SQLAlchemyError + +from ..models.permission_models import ( + BlueprintRangePermissionModel, + DeployedRangePermissionModel, +) + +logger = logging.getLogger(__name__) + + +async def grant_blueprint_permission( + db: AsyncSession, + blueprint_range_id: int, + user_id: int, + permission_type: Literal["read", "write"], +) -> BlueprintRangePermissionModel: + """Grant permission to a blueprint range. + + Args: + ---- + db: Database session + blueprint_range_id: ID of blueprint range to grant permission for + user_id: ID of user to grant permission to + permission_type: Type of permission to grant + + Returns: + ------- + BlueprintRangePermissionModel: The created permission + + Raises: + ------ + SQLAlchemyError: If database operation fails + """ + permission = BlueprintRangePermissionModel( + blueprint_range_id=blueprint_range_id, + user_id=user_id, + permission_type=permission_type, + ) + + db.add(permission) + + try: + await db.flush() + await db.refresh(permission) + logger.debug( + "Granted %s permission on blueprint range %s to user %s", + permission_type, + blueprint_range_id, + user_id, + ) + except SQLAlchemyError as e: + logger.exception( + "Failed to grant %s permission on blueprint range %s to user %s: %s", + permission_type, + blueprint_range_id, + user_id, + e, + ) + raise + + return permission + + +async def grant_deployed_permission( + db: AsyncSession, + deployed_range_id: int, + user_id: int, + permission_type: Literal["read", "write", "execute"], +) -> DeployedRangePermissionModel: + """Grant permission to a deployed range. + + Args: + ---- + db: Database session + deployed_range_id: ID of deployed range to grant permission for + user_id: ID of user to grant permission to + permission_type: Type of permission to grant + + Returns: + ------- + DeployedRangePermissionModel: The created permission + + Raises: + ------ + SQLAlchemyError: If database operation fails + """ + permission = DeployedRangePermissionModel( + deployed_range_id=deployed_range_id, + user_id=user_id, + permission_type=permission_type, + ) + + db.add(permission) + + try: + await db.flush() + await db.refresh(permission) + logger.debug( + "Granted %s permission on deployed range %s to user %s", + permission_type, + deployed_range_id, + user_id, + ) + except SQLAlchemyError as e: + logger.exception( + "Failed to grant %s permission on deployed range %s to user %s: %s", + permission_type, + deployed_range_id, + user_id, + e, + ) + raise + + return permission + + +async def revoke_blueprint_permission( + db: AsyncSession, + blueprint_range_id: int, + user_id: int, + permission_type: Literal["read", "write"], +) -> bool: + """Revoke permission from a blueprint range. + + Args: + ---- + db: Database session + blueprint_range_id: ID of blueprint range to revoke permission from + user_id: ID of user to revoke permission from + permission_type: Type of permission to revoke + + Returns: + ------- + bool: True if permission was revoked, False if not found + + Raises: + ------ + SQLAlchemyError: If database operation fails + """ + stmt = select(BlueprintRangePermissionModel).where( + BlueprintRangePermissionModel.blueprint_range_id == blueprint_range_id, + BlueprintRangePermissionModel.user_id == user_id, + BlueprintRangePermissionModel.permission_type == permission_type, + ) + result = await db.execute(stmt) + permission = result.scalar_one_or_none() + + if not permission: + logger.warning( + "Permission %s on blueprint range %s for user %s not found", + permission_type, + blueprint_range_id, + user_id, + ) + return False + + try: + await db.delete(permission) + await db.flush() + logger.debug( + "Revoked %s permission on blueprint range %s from user %s", + permission_type, + blueprint_range_id, + user_id, + ) + except SQLAlchemyError as e: + logger.exception( + "Failed to revoke %s permission on blueprint range %s from user %s: %s", + permission_type, + blueprint_range_id, + user_id, + e, + ) + raise + + return True + + +async def revoke_deployed_permission( + db: AsyncSession, + deployed_range_id: int, + user_id: int, + permission_type: Literal["read", "write", "execute"], +) -> bool: + """Revoke permission from a deployed range. + + Args: + ---- + db: Database session + deployed_range_id: ID of deployed range to revoke permission from + user_id: ID of user to revoke permission from + permission_type: Type of permission to revoke + + Returns: + ------- + bool: True if permission was revoked, False if not found + + Raises: + ------ + SQLAlchemyError: If database operation fails + """ + stmt = select(DeployedRangePermissionModel).where( + DeployedRangePermissionModel.deployed_range_id == deployed_range_id, + DeployedRangePermissionModel.user_id == user_id, + DeployedRangePermissionModel.permission_type == permission_type, + ) + result = await db.execute(stmt) + permission = result.scalar_one_or_none() + + if not permission: + logger.warning( + "Permission %s on deployed range %s for user %s not found", + permission_type, + deployed_range_id, + user_id, + ) + return False + + try: + await db.delete(permission) + await db.flush() + logger.debug( + "Revoked %s permission on deployed range %s from user %s", + permission_type, + deployed_range_id, + user_id, + ) + except SQLAlchemyError as e: + logger.exception( + "Failed to revoke %s permission on deployed range %s from user %s: %s", + permission_type, + deployed_range_id, + user_id, + e, + ) + raise + + return True diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index 7adb1638..7ea63e2f 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -5,6 +5,10 @@ from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.orm import selectinload +from ..models.permission_models import ( + BlueprintRangePermissionModel, + DeployedRangePermissionModel, +) from ..models.range_models import BlueprintRangeModel, DeployedRangeModel from ..models.subnet_models import BlueprintSubnetModel, DeployedSubnetModel from ..models.vpc_models import BlueprintVPCModel, DeployedVPCModel @@ -17,11 +21,60 @@ DeployedRangeKeySchema, DeployedRangeSchema, ) +from .crud_permissions import grant_blueprint_permission, grant_deployed_permission from .crud_vpcs import build_blueprint_vpc_models, build_deployed_vpc_models logger = logging.getLogger(__name__) +def get_permissions(model): + """Safely get permissions from a model, handling mocks.""" + permissions = getattr(model, 'permissions', []) + if hasattr(permissions, '_mock_name'): + return [] + return permissions or [] + + +def can_read_blueprint(range_model, user_id: int) -> bool: + """Check if user can read a blueprint range.""" + if range_model.owner_id == user_id: + return True + return any(p.user_id == user_id and p.permission_type in ('read', 'write') + for p in get_permissions(range_model)) + + +def can_write_blueprint(range_model, user_id: int) -> bool: + """Check if user can write a blueprint range.""" + if range_model.owner_id == user_id: + return True + return any(p.user_id == user_id and p.permission_type == 'write' + for p in get_permissions(range_model)) + + +def can_read_deployed(range_model, user_id: int) -> bool: + """Check if user can read a deployed range.""" + if range_model.owner_id == user_id: + return True + return any(p.user_id == user_id and p.permission_type in ('read', 'write', 'execute') + for p in get_permissions(range_model)) + + +def can_write_deployed(range_model, user_id: int) -> bool: + """Check if user can write a deployed range.""" + if range_model.owner_id == user_id: + return True + return any(p.user_id == user_id and p.permission_type == 'write' + for p in get_permissions(range_model)) + + +def can_execute_deployed(range_model, user_id: int) -> bool: + """Check if user can execute a deployed range.""" + if range_model.owner_id == user_id: + return True + return any(p.user_id == user_id and p.permission_type == 'execute' + for p in get_permissions(range_model)) + + # ==================== Blueprints ===================== @@ -53,7 +106,16 @@ async def get_blueprint_range_headers( ).select_from(BlueprintRangeModel) if not is_admin: - stmt = stmt.filter(BlueprintRangeModel.owner_id == user_id) + stmt = stmt.outerjoin( + BlueprintRangePermissionModel, + BlueprintRangeModel.id == BlueprintRangePermissionModel.blueprint_range_id + ).filter( + (BlueprintRangeModel.owner_id == user_id) | + ( + (BlueprintRangePermissionModel.user_id == user_id) & + (BlueprintRangePermissionModel.permission_type.in_(["read", "write"])) + ) + ).distinct() result = await db.execute(stmt) @@ -91,6 +153,7 @@ async def get_blueprint_range( selectinload(BlueprintRangeModel.vpcs) .selectinload(BlueprintVPCModel.subnets) .selectinload(BlueprintSubnetModel.hosts), + selectinload(BlueprintRangeModel.permissions), ] range_model = await db.get(BlueprintRangeModel, range_id, options=options) @@ -100,7 +163,7 @@ async def get_blueprint_range( ) return None - if is_admin or range_model.owner_id == user_id: + if is_admin or can_read_blueprint(range_model, user_id): logger.debug("Fetched range blueprint: %s for user %s.", range_id, user_id) return BlueprintRangeSchema.model_validate(range_model) @@ -131,7 +194,8 @@ def build_blueprint_range_models( range_models: list[BlueprintRangeModel] = [] for range_schema in ranges: range_model = BlueprintRangeModel( - **range_schema.model_dump(exclude={"vpcs"}), owner_id=user_id + **range_schema.model_dump(exclude={"vpcs", "readers", "writers"}), + owner_id=user_id ) range_model.vpcs = build_blueprint_vpc_models(range_schema.vpcs, user_id) range_models.append(range_model) @@ -188,6 +252,15 @@ async def create_blueprint_range( range_model.id, user_id, ) + + for reader_id in blueprint.readers: + if reader_id != user_id: + await grant_blueprint_permission(db, range_model.id, reader_id, "read") + + for writer_id in blueprint.writers: + if writer_id != user_id: + await grant_blueprint_permission(db, range_model.id, writer_id, "write") + except SQLAlchemyError as e: logger.exception( "Database error while flushing range blueprint to database session for user: %s. Exception: %s.", @@ -226,7 +299,11 @@ async def delete_blueprint_range( Optional[BlueprintRangeHeaderSchema]: Range header data if it exists in database and was successfully deleted. """ - range_model = await db.get(BlueprintRangeModel, range_id) + range_model = await db.get( + BlueprintRangeModel, + range_id, + options=[selectinload(BlueprintRangeModel.permissions)] + ) if not range_model: logger.warning( "Range blueprint: %s not found for deletion as user: %s. Does user have permissions?", @@ -242,7 +319,7 @@ async def delete_blueprint_range( ) return None - if not is_admin and range_model.owner_id != user_id: + if not is_admin and not can_write_blueprint(range_model, user_id): logger.warning( "User: %s is not authorized to delete range blueprint: %s.", user_id, @@ -310,7 +387,16 @@ async def get_deployed_range_headers( ).select_from(DeployedRangeModel) if not is_admin: - stmt = stmt.filter(DeployedRangeModel.owner_id == user_id) + stmt = stmt.outerjoin( + DeployedRangePermissionModel, + DeployedRangeModel.id == DeployedRangePermissionModel.deployed_range_id + ).filter( + (DeployedRangeModel.owner_id == user_id) | + ( + (DeployedRangePermissionModel.user_id == user_id) & + (DeployedRangePermissionModel.permission_type.in_(["read", "write", "execute"])) + ) + ).distinct() result = await db.execute(stmt) @@ -347,7 +433,8 @@ async def get_deployed_range( options = [ selectinload(DeployedRangeModel.vpcs) .selectinload(DeployedVPCModel.subnets) - .selectinload(DeployedSubnetModel.hosts) + .selectinload(DeployedSubnetModel.hosts), + selectinload(DeployedRangeModel.permissions), ] range_model = await db.get(DeployedRangeModel, range_id, options=options) @@ -357,7 +444,7 @@ async def get_deployed_range( ) return None - if is_admin or range_model.owner_id == user_id: + if is_admin or can_read_deployed(range_model, user_id): logger.debug("Fetched deployed range: %s for user %s.", range_id, user_id) return DeployedRangeSchema.model_validate(range_model) @@ -389,16 +476,13 @@ async def get_deployed_range_key( Optional[DeployedRangeKeySchema]: Deployed range key data if the range exists and the user is authorized, otherwise None. """ - stmt = select(DeployedRangeModel.range_private_key).where( - DeployedRangeModel.id == range_id + range_model = await db.get( + DeployedRangeModel, + range_id, + options=[selectinload(DeployedRangeModel.permissions)] ) - if not is_admin: - stmt = stmt.where(DeployedRangeModel.owner_id == user_id) - - range_private_key = await db.scalar(stmt) - - if range_private_key is None: + if not range_model or (not is_admin and not can_execute_deployed(range_model, user_id)): logger.warning( "Failed to fetch deployed range key for range: %s for user %s. Range not found or unauthorized.", range_id, @@ -406,6 +490,11 @@ async def get_deployed_range_key( ) return None + stmt = select(DeployedRangeModel.range_private_key).where( + DeployedRangeModel.id == range_id + ) + range_private_key = await db.scalar(stmt) + logger.debug( "Fetched deployed range key for range: %s for user %s.", range_id, user_id ) @@ -433,7 +522,8 @@ def build_deployed_range_models( range_models: list[DeployedRangeModel] = [] for range_schema in ranges: range_model = DeployedRangeModel( - **range_schema.model_dump(exclude={"vpcs"}), owner_id=user_id + **range_schema.model_dump(exclude={"vpcs", "readers", "writers", "executors"}), + owner_id=user_id ) range_model.vpcs = build_deployed_vpc_models(range_schema.vpcs, user_id) range_models.append(range_model) @@ -490,6 +580,19 @@ async def create_deployed_range( range_model.id, user_id, ) + + for reader_id in range_schema.readers: + if reader_id != user_id: + await grant_deployed_permission(db, range_model.id, reader_id, "read") + + for writer_id in range_schema.writers: + if writer_id != user_id: + await grant_deployed_permission(db, range_model.id, writer_id, "write") + + for executor_id in range_schema.executors: + if executor_id != user_id: + await grant_deployed_permission(db, range_model.id, executor_id, "execute") + except SQLAlchemyError as e: logger.exception( "Database error while flushing deployed range to database session for user: %s. Exception: %s.", @@ -528,7 +631,11 @@ async def delete_deployed_range( Optional[DeployedRangeHeaderSchema]: Deployed range header data if it exists in database and was successfully deleted. """ - range_model = await db.get(DeployedRangeModel, range_id) + range_model = await db.get( + DeployedRangeModel, + range_id, + options=[selectinload(DeployedRangeModel.permissions)] + ) if not range_model: logger.warning( "Deployed range: %s not found for deletion as user: %s. Does user have permissions?", @@ -537,7 +644,7 @@ async def delete_deployed_range( ) return None - if not is_admin and range_model.owner_id != user_id: + if not is_admin and not can_write_deployed(range_model, user_id): logger.warning( "User: %s is not authorized to delete deployed range: %s.", user_id, diff --git a/api/src/app/models/__init__.py b/api/src/app/models/__init__.py index 29103aa2..b640a7de 100644 --- a/api/src/app/models/__init__.py +++ b/api/src/app/models/__init__.py @@ -2,6 +2,10 @@ from .host_models import BlueprintHostModel, DeployedHostModel from .mixin_models import OpenLabsUserMixin, OwnableObjectMixin +from .permission_models import ( + BlueprintRangePermissionModel, + DeployedRangePermissionModel, +) from .range_models import BlueprintRangeModel, DeployedRangeModel from .secret_model import SecretModel from .subnet_models import BlueprintSubnetModel, DeployedSubnetModel @@ -11,10 +15,12 @@ __all__ = [ "BlueprintHostModel", "BlueprintRangeModel", + "BlueprintRangePermissionModel", "BlueprintSubnetModel", "BlueprintVPCModel", "DeployedHostModel", "DeployedRangeModel", + "DeployedRangePermissionModel", "DeployedSubnetModel", "DeployedVPCModel", "OpenLabsUserMixin", diff --git a/api/src/app/models/permission_models.py b/api/src/app/models/permission_models.py new file mode 100644 index 00000000..0364d1f6 --- /dev/null +++ b/api/src/app/models/permission_models.py @@ -0,0 +1,107 @@ +from sqlalchemy import BigInteger, CheckConstraint, ForeignKey, String, UniqueConstraint +from sqlalchemy.orm import Mapped, MappedAsDataclass, mapped_column, relationship + +from ..core.db.database import Base +from .mixin_models import OwnableObjectMixin + + +class BlueprintRangePermissionModel(Base, MappedAsDataclass): + """Model for blueprint range permission grants to users.""" + + __tablename__ = "blueprint_range_permissions" + + id: Mapped[int] = mapped_column( + BigInteger, + primary_key=True, + init=False, + comment="Primary key (BIGSERIAL)", + ) + + # The blueprint range being shared + blueprint_range_id: Mapped[int] = mapped_column( + BigInteger, + ForeignKey("blueprint_ranges.id", ondelete="CASCADE"), + nullable=False, + ) + + # The user being granted permission + user_id: Mapped[int] = mapped_column( + BigInteger, + ForeignKey("users.id", ondelete="CASCADE"), + nullable=False, + ) + + # Type of permission granted (read, write) + permission_type: Mapped[str] = mapped_column( + String(10), + nullable=False, + ) + + # Relationships + blueprint_range = relationship("BlueprintRangeModel", back_populates="permissions") + user = relationship("UserModel") + + # Constraints + __table_args__ = ( + UniqueConstraint( + "blueprint_range_id", + "user_id", + "permission_type", + name="uq_blueprint_range_permissions" + ), + CheckConstraint( + "permission_type IN ('read', 'write')", + name="ck_blueprint_range_permission_type" + ), + ) + + +class DeployedRangePermissionModel(Base, MappedAsDataclass): + """Model for deployed range permission grants to users.""" + + __tablename__ = "deployed_range_permissions" + + id: Mapped[int] = mapped_column( + BigInteger, + primary_key=True, + init=False, + comment="Primary key (BIGSERIAL)", + ) + + # The deployed range being shared + deployed_range_id: Mapped[int] = mapped_column( + BigInteger, + ForeignKey("deployed_ranges.id", ondelete="CASCADE"), + nullable=False, + ) + + # The user being granted permission + user_id: Mapped[int] = mapped_column( + BigInteger, + ForeignKey("users.id", ondelete="CASCADE"), + nullable=False, + ) + + # Type of permission granted (read, write, execute) + permission_type: Mapped[str] = mapped_column( + String(10), + nullable=False, + ) + + # Relationships + deployed_range = relationship("DeployedRangeModel", back_populates="permissions") + user = relationship("UserModel") + + # Constraints + __table_args__ = ( + UniqueConstraint( + "deployed_range_id", + "user_id", + "permission_type", + name="uq_deployed_range_permissions" + ), + CheckConstraint( + "permission_type IN ('read', 'write', 'execute')", + name="ck_deployed_range_permission_type" + ), + ) diff --git a/api/src/app/models/range_models.py b/api/src/app/models/range_models.py index dd44127e..e1f731b1 100644 --- a/api/src/app/models/range_models.py +++ b/api/src/app/models/range_models.py @@ -41,6 +41,13 @@ class BlueprintRangeModel(Base, OwnableObjectMixin, RangeMixin): passive_deletes=True, ) + permissions = relationship( + "BlueprintRangePermissionModel", + back_populates="blueprint_range", + cascade="all, delete-orphan", + passive_deletes=True, + ) + def is_standalone(self) -> bool: """Return whether blueprint range model is standalone. @@ -82,3 +89,11 @@ class DeployedRangeModel(Base, OwnableObjectMixin, RangeMixin): cascade="all, delete-orphan", passive_deletes=True, ) + + permissions = relationship( + "DeployedRangePermissionModel", + back_populates="deployed_range", + cascade="all, delete-orphan", + passive_deletes=True, + ) + diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index 33a31d8d..f2cb7a0e 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -2,7 +2,7 @@ from ipaddress import IPv4Address from typing import Any -from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, field_validator +from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, computed_field, field_validator from ..enums.providers import OpenLabsProvider from ..enums.range_states import RangeState @@ -53,6 +53,12 @@ class BlueprintRangeCreateSchema(BlueprintRangeBaseSchema): vpcs: list[BlueprintVPCCreateSchema] = Field( ..., description="All blueprint VPCs in range." ) + readers: list[int] = Field( + default=[], description="List of user IDs with read access to this blueprint." + ) + writers: list[int] = Field( + default=[], description="List of user IDs with write access to this blueprint." + ) @field_validator("vpcs") @classmethod @@ -91,6 +97,28 @@ class BlueprintRangeSchema(BlueprintRangeBaseSchema): vpcs: list[BlueprintVPCSchema] = Field( ..., description="All blueprint VPCs in range." ) + + @computed_field + @property + def readers(self) -> list[int]: + """Get list of user IDs with read access.""" + if not hasattr(self, 'permissions'): + return [] + return [ + perm.user_id for perm in self.permissions + if perm.permission_type == 'read' + ] + + @computed_field + @property + def writers(self) -> list[int]: + """Get list of user IDs with write access.""" + if not hasattr(self, 'permissions'): + return [] + return [ + perm.user_id for perm in self.permissions + if perm.permission_type == 'write' + ] model_config = ConfigDict(from_attributes=True) @@ -157,6 +185,15 @@ class DeployedRangeCreateSchema(DeployedRangeBaseSchema): vpcs: list[DeployedVPCCreateSchema] = Field( ..., description="Deployed VPCs in the range." ) + readers: list[int] = Field( + default=[], description="List of user IDs with read access to this deployed range." + ) + writers: list[int] = Field( + default=[], description="List of user IDs with write access to this deployed range." + ) + executors: list[int] = Field( + default=[], description="List of user IDs with execute access to this deployed range." + ) @field_validator("vpcs") @classmethod @@ -195,6 +232,39 @@ class DeployedRangeSchema(DeployedRangeBaseSchema): vpcs: list[DeployedVPCSchema] = Field( ..., description="All deployed VPCs in the range." ) + + @computed_field + @property + def readers(self) -> list[int]: + """Get list of user IDs with read access.""" + if not hasattr(self, 'permissions'): + return [] + return [ + perm.user_id for perm in self.permissions + if perm.permission_type == 'read' + ] + + @computed_field + @property + def writers(self) -> list[int]: + """Get list of user IDs with write access.""" + if not hasattr(self, 'permissions'): + return [] + return [ + perm.user_id for perm in self.permissions + if perm.permission_type == 'write' + ] + + @computed_field + @property + def executors(self) -> list[int]: + """Get list of user IDs with execute access.""" + if not hasattr(self, 'permissions'): + return [] + return [ + perm.user_id for perm in self.permissions + if perm.permission_type == 'execute' + ] model_config = ConfigDict(from_attributes=True) diff --git a/api/tests/common/api/v1/config.py b/api/tests/common/api/v1/config.py index ca5af7d9..150fc18f 100644 --- a/api/tests/common/api/v1/config.py +++ b/api/tests/common/api/v1/config.py @@ -71,6 +71,8 @@ "name": "example-range-1", "vnc": False, "vpn": False, + "readers": [], + "writers": [], } # Valid range payload for deployment diff --git a/api/tests/common/api/v1/test_blueprint_ranges.py b/api/tests/common/api/v1/test_blueprint_ranges.py index 5c5df9e6..fe462a5b 100644 --- a/api/tests/common/api/v1/test_blueprint_ranges.py +++ b/api/tests/common/api/v1/test_blueprint_ranges.py @@ -593,6 +593,8 @@ async def test_blueprint_range_get_non_empty_list( # Mimic the header response with the valid JSON valid_blueprint_copy = copy.deepcopy(valid_blueprint_range_create_payload) del valid_blueprint_copy["vpcs"] + del valid_blueprint_copy["readers"] + del valid_blueprint_copy["writers"] remove_key_recursively( recieved_range, "id" ) # Our creation payload doesn't have IDs diff --git a/api/tests/unit/api/v1/config.py b/api/tests/unit/api/v1/config.py index d8126aed..9d3cf109 100644 --- a/api/tests/unit/api/v1/config.py +++ b/api/tests/unit/api/v1/config.py @@ -44,6 +44,8 @@ "name": "example-range-1", "vnc": False, "vpn": False, + "readers": [], + "writers": [], } # Valid range payload for deployment @@ -270,6 +272,9 @@ "name": "openlabs-deployed-test-v2", "vnc": True, "vpn": False, + "readers": [], + "writers": [], + "executors": [], } valid_deployed_vpc_data: dict[str, Any] = copy.deepcopy( From cfd38d802a9152cee293cc68e1ddc83d28c5e549 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sat, 26 Jul 2025 23:45:39 -0700 Subject: [PATCH 02/19] fix(tests): fix unit test for range crud admin check to check perms instead --- api/tests/unit/crud/test_crud_ranges.py | 45 +++++++++++++++---------- 1 file changed, 28 insertions(+), 17 deletions(-) diff --git a/api/tests/unit/crud/test_crud_ranges.py b/api/tests/unit/crud/test_crud_ranges.py index 1366a4b0..7221ff65 100644 --- a/api/tests/unit/crud/test_crud_ranges.py +++ b/api/tests/unit/crud/test_crud_ranges.py @@ -499,35 +499,46 @@ async def test_get_non_existent_deployed_range() -> None: @pytest.mark.parametrize( - "is_admin, expect_owner_filter", + "is_admin, user_owns_range", [ - (False, True), - (True, False), + (False, True), # Regular user, owns range - should get key + (False, False), # Regular user, doesn't own range - should not get key + (True, False), # Admin user, doesn't own range - should get key ], ) -async def test_get_deployed_range_key_filters( +async def test_get_deployed_range_key_permissions( is_admin: bool, - expect_owner_filter: bool, + user_owns_range: bool, ) -> None: - """Test the deployed range key crud function filters results appropriately.""" + """Test the deployed range key crud function respects permissions.""" dummy_db = DummyDB() - + dummy_range = DummyDeployedRange() + + range_id = 1 + user_id = 1 + + # Set ownership based on test parameters + if user_owns_range: + dummy_range.owner_id = user_id + else: + dummy_range.owner_id = user_id + 1 + # Configure return of mock result + dummy_db.get.return_value = dummy_range dummy_db.scalar.return_value = "fake_private_key" - range_id = random.randint(1, 100) # noqa: S311 - user_id = 1 - await get_deployed_range_key( + result = await get_deployed_range_key( dummy_db, range_id=range_id, user_id=user_id, is_admin=is_admin ) - # Build filter clauses - range_id_clause = str(DeployedRangeModel.id == range_id) - ownership_clause = str(DeployedRangeModel.owner_id == user_id) - where_clause = str(dummy_db.scalar.call_args[0][0].whereclause) - - assert range_id_clause in where_clause # Always only pull key of specified range - assert (ownership_clause in where_clause) == expect_owner_filter + # Check if we should expect a result + should_have_access = is_admin or user_owns_range + + if should_have_access: + assert result is not None + assert result.range_private_key == "fake_private_key" + else: + assert result is None async def test_get_non_existent_deployed_range_key() -> None: From 8b8887a2df0150bde00498716541763e6afac862 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sun, 27 Jul 2025 00:34:39 -0700 Subject: [PATCH 03/19] fix(types): fix mypy issues in rwx --- api/src/app/crud/crud_ranges.py | 23 ++++++++++++----------- api/src/app/schemas/range_schemas.py | 5 ----- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index 7ea63e2f..537b1c81 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -1,4 +1,5 @@ import logging +from typing import Union from sqlalchemy import select from sqlalchemy.exc import SQLAlchemyError @@ -27,7 +28,7 @@ logger = logging.getLogger(__name__) -def get_permissions(model): +def get_permissions(model: object) -> list[BlueprintRangePermissionModel | DeployedRangePermissionModel]: """Safely get permissions from a model, handling mocks.""" permissions = getattr(model, 'permissions', []) if hasattr(permissions, '_mock_name'): @@ -35,41 +36,41 @@ def get_permissions(model): return permissions or [] -def can_read_blueprint(range_model, user_id: int) -> bool: +def can_read_blueprint(range_model: Union[BlueprintRangeModel, object], user_id: int) -> bool: """Check if user can read a blueprint range.""" - if range_model.owner_id == user_id: + if getattr(range_model, 'owner_id', None) == user_id: return True return any(p.user_id == user_id and p.permission_type in ('read', 'write') for p in get_permissions(range_model)) -def can_write_blueprint(range_model, user_id: int) -> bool: +def can_write_blueprint(range_model: Union[BlueprintRangeModel, object], user_id: int) -> bool: """Check if user can write a blueprint range.""" - if range_model.owner_id == user_id: + if getattr(range_model, 'owner_id', None) == user_id: return True return any(p.user_id == user_id and p.permission_type == 'write' for p in get_permissions(range_model)) -def can_read_deployed(range_model, user_id: int) -> bool: +def can_read_deployed(range_model: Union[DeployedRangeModel, object], user_id: int) -> bool: """Check if user can read a deployed range.""" - if range_model.owner_id == user_id: + if getattr(range_model, 'owner_id', None) == user_id: return True return any(p.user_id == user_id and p.permission_type in ('read', 'write', 'execute') for p in get_permissions(range_model)) -def can_write_deployed(range_model, user_id: int) -> bool: +def can_write_deployed(range_model: Union[DeployedRangeModel, object], user_id: int) -> bool: """Check if user can write a deployed range.""" - if range_model.owner_id == user_id: + if getattr(range_model, 'owner_id', None) == user_id: return True return any(p.user_id == user_id and p.permission_type == 'write' for p in get_permissions(range_model)) -def can_execute_deployed(range_model, user_id: int) -> bool: +def can_execute_deployed(range_model: Union[DeployedRangeModel, object], user_id: int) -> bool: """Check if user can execute a deployed range.""" - if range_model.owner_id == user_id: + if getattr(range_model, 'owner_id', None) == user_id: return True return any(p.user_id == user_id and p.permission_type == 'execute' for p in get_permissions(range_model)) diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index f2cb7a0e..1f34866f 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -99,7 +99,6 @@ class BlueprintRangeSchema(BlueprintRangeBaseSchema): ) @computed_field - @property def readers(self) -> list[int]: """Get list of user IDs with read access.""" if not hasattr(self, 'permissions'): @@ -110,7 +109,6 @@ def readers(self) -> list[int]: ] @computed_field - @property def writers(self) -> list[int]: """Get list of user IDs with write access.""" if not hasattr(self, 'permissions'): @@ -234,7 +232,6 @@ class DeployedRangeSchema(DeployedRangeBaseSchema): ) @computed_field - @property def readers(self) -> list[int]: """Get list of user IDs with read access.""" if not hasattr(self, 'permissions'): @@ -245,7 +242,6 @@ def readers(self) -> list[int]: ] @computed_field - @property def writers(self) -> list[int]: """Get list of user IDs with write access.""" if not hasattr(self, 'permissions'): @@ -256,7 +252,6 @@ def writers(self) -> list[int]: ] @computed_field - @property def executors(self) -> list[int]: """Get list of user IDs with execute access.""" if not hasattr(self, 'permissions'): From 350593bd16375d32ab55e6cdb09e9e25b86a2b9a Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sun, 27 Jul 2025 00:47:16 -0700 Subject: [PATCH 04/19] test(rwx): add tests for blueprint and range permissions --- api/tests/unit/crud/test_crud_permissions.py | 337 +++++++++++++++++++ 1 file changed, 337 insertions(+) create mode 100644 api/tests/unit/crud/test_crud_permissions.py diff --git a/api/tests/unit/crud/test_crud_permissions.py b/api/tests/unit/crud/test_crud_permissions.py new file mode 100644 index 00000000..d160fed9 --- /dev/null +++ b/api/tests/unit/crud/test_crud_permissions.py @@ -0,0 +1,337 @@ +import logging +import pytest +from unittest.mock import Mock +from sqlalchemy.exc import SQLAlchemyError + +from src.app.crud.crud_permissions import ( + grant_blueprint_permission, + grant_deployed_permission, + revoke_blueprint_permission, + revoke_deployed_permission, +) +from src.app.models.permission_models import ( + BlueprintRangePermissionModel, + DeployedRangePermissionModel, +) + +from .crud_mocks import DummyDB + + +# ==================== Grant Permissions ===================== + + +async def test_grant_blueprint_read_permission() -> None: + """Test granting read permission to a blueprint range.""" + dummy_db = DummyDB() + + blueprint_range_id = 1 + user_id = 2 + + result = await grant_blueprint_permission( + dummy_db, blueprint_range_id, user_id, "read" + ) + + # Check that permission was added to db + dummy_db.add.assert_called_once() + added_permission = dummy_db.add.call_args[0][0] + assert isinstance(added_permission, BlueprintRangePermissionModel) + assert added_permission.blueprint_range_id == blueprint_range_id + assert added_permission.user_id == user_id + assert added_permission.permission_type == "read" + + # Check that db operations were called + dummy_db.flush.assert_called_once() + dummy_db.refresh.assert_called_once_with(added_permission) + + # Check return value + assert result == added_permission + + +async def test_grant_blueprint_write_permission() -> None: + """Test granting write permission to a blueprint range.""" + dummy_db = DummyDB() + + blueprint_range_id = 1 + user_id = 3 + + result = await grant_blueprint_permission( + dummy_db, blueprint_range_id, user_id, "write" + ) + + # Check that permission was created correctly + added_permission = dummy_db.add.call_args[0][0] + assert added_permission.blueprint_range_id == blueprint_range_id + assert added_permission.user_id == user_id + assert added_permission.permission_type == "write" + + assert result == added_permission + + +async def test_grant_deployed_read_permission() -> None: + """Test granting read permission to a deployed range.""" + dummy_db = DummyDB() + + deployed_range_id = 1 + user_id = 2 + + result = await grant_deployed_permission( + dummy_db, deployed_range_id, user_id, "read" + ) + + # Check that permission was added to db + dummy_db.add.assert_called_once() + added_permission = dummy_db.add.call_args[0][0] + assert isinstance(added_permission, DeployedRangePermissionModel) + assert added_permission.deployed_range_id == deployed_range_id + assert added_permission.user_id == user_id + assert added_permission.permission_type == "read" + + # Check that db operations were called + dummy_db.flush.assert_called_once() + dummy_db.refresh.assert_called_once_with(added_permission) + + # Check return value + assert result == added_permission + + +async def test_grant_deployed_write_permission() -> None: + """Test granting write permission to a deployed range.""" + dummy_db = DummyDB() + + deployed_range_id = 1 + user_id = 3 + + result = await grant_deployed_permission( + dummy_db, deployed_range_id, user_id, "write" + ) + + # Check that permission was created correctly + added_permission = dummy_db.add.call_args[0][0] + assert added_permission.deployed_range_id == deployed_range_id + assert added_permission.user_id == user_id + assert added_permission.permission_type == "write" + + assert result == added_permission + + +async def test_grant_deployed_execute_permission() -> None: + """Test granting execute permission to a deployed range.""" + dummy_db = DummyDB() + + deployed_range_id = 1 + user_id = 4 + + result = await grant_deployed_permission( + dummy_db, deployed_range_id, user_id, "execute" + ) + + # Check that permission was created correctly + added_permission = dummy_db.add.call_args[0][0] + assert added_permission.deployed_range_id == deployed_range_id + assert added_permission.user_id == user_id + assert added_permission.permission_type == "execute" + + assert result == added_permission + + +async def test_grant_blueprint_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that grant_blueprint_permission handles database errors.""" + dummy_db = DummyDB() + + # Force a db exception + test_except_msg = "Fake DB error!" + dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + with pytest.raises(SQLAlchemyError, match=test_except_msg): + await grant_blueprint_permission(dummy_db, 1, 2, "read") + + # Check that we properly logger.exception() db errors + assert any( + record.levelno == logging.ERROR + and record.exc_info is not None + and test_except_msg in record.message + for record in caplog.records + ) + + +async def test_grant_deployed_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that grant_deployed_permission handles database errors.""" + dummy_db = DummyDB() + + # Force a db exception + test_except_msg = "Fake DB error!" + dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + with pytest.raises(SQLAlchemyError, match=test_except_msg): + await grant_deployed_permission(dummy_db, 1, 2, "read") + + # Check that we properly logger.exception() db errors + assert any( + record.levelno == logging.ERROR + and record.exc_info is not None + and test_except_msg in record.message + for record in caplog.records + ) + + +# ==================== Revoke Permissions ===================== + + +async def test_revoke_blueprint_permission_success() -> None: + """Test successfully revoking a blueprint permission.""" + dummy_db = DummyDB() + + # Create a mock permission to be found and deleted + mock_permission = BlueprintRangePermissionModel( + blueprint_range_id=1, + user_id=2, + permission_type="read", + ) + + # Mock the execute result chain + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_permission + dummy_db.execute.return_value = mock_result + + result = await revoke_blueprint_permission(dummy_db, 1, 2, "read") + + # Check that the permission was deleted + dummy_db.delete.assert_called_once_with(mock_permission) + dummy_db.flush.assert_called_once() + + # Check return value + assert result is True + + +async def test_revoke_blueprint_permission_not_found() -> None: + """Test revoking a blueprint permission that doesn't exist.""" + dummy_db = DummyDB() + + # Mock the execute result chain to return None (permission not found) + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = None + dummy_db.execute.return_value = mock_result + + result = await revoke_blueprint_permission(dummy_db, 1, 2, "read") + + # Check that delete was not called + dummy_db.delete.assert_not_called() + dummy_db.flush.assert_not_called() + + # Check return value + assert result is False + + +async def test_revoke_deployed_permission_success() -> None: + """Test successfully revoking a deployed permission.""" + dummy_db = DummyDB() + + # Create a mock permission to be found and deleted + mock_permission = DeployedRangePermissionModel( + deployed_range_id=1, + user_id=2, + permission_type="execute", + ) + + # Mock the execute result chain + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_permission + dummy_db.execute.return_value = mock_result + + result = await revoke_deployed_permission(dummy_db, 1, 2, "execute") + + # Check that the permission was deleted + dummy_db.delete.assert_called_once_with(mock_permission) + dummy_db.flush.assert_called_once() + + # Check return value + assert result is True + + +async def test_revoke_deployed_permission_not_found() -> None: + """Test revoking a deployed permission that doesn't exist.""" + dummy_db = DummyDB() + + # Mock the execute result chain to return None (permission not found) + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = None + dummy_db.execute.return_value = mock_result + + result = await revoke_deployed_permission(dummy_db, 1, 2, "write") + + # Check that delete was not called + dummy_db.delete.assert_not_called() + dummy_db.flush.assert_not_called() + + # Check return value + assert result is False + + +async def test_revoke_blueprint_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that revoke_blueprint_permission handles database errors.""" + dummy_db = DummyDB() + + # Create a mock permission + mock_permission = BlueprintRangePermissionModel( + blueprint_range_id=1, + user_id=2, + permission_type="read", + ) + # Mock the execute result chain + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_permission + dummy_db.execute.return_value = mock_result + + # Force a db exception + test_except_msg = "Fake DB error!" + dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + with pytest.raises(SQLAlchemyError, match=test_except_msg): + await revoke_blueprint_permission(dummy_db, 1, 2, "read") + + # Check that we properly logger.exception() db errors + assert any( + record.levelno == logging.ERROR + and record.exc_info is not None + and test_except_msg in record.message + for record in caplog.records + ) + + +async def test_revoke_deployed_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that revoke_deployed_permission handles database errors.""" + dummy_db = DummyDB() + + # Create a mock permission + mock_permission = DeployedRangePermissionModel( + deployed_range_id=1, + user_id=2, + permission_type="execute", + ) + # Mock the execute result chain + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_permission + dummy_db.execute.return_value = mock_result + + # Force a db exception + test_except_msg = "Fake DB error!" + dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + with pytest.raises(SQLAlchemyError, match=test_except_msg): + await revoke_deployed_permission(dummy_db, 1, 2, "execute") + + # Check that we properly logger.exception() db errors + assert any( + record.levelno == logging.ERROR + and record.exc_info is not None + and test_except_msg in record.message + for record in caplog.records + ) \ No newline at end of file From 718acf9f6b4c8b0357e12dd8d5fdcb2468546aa2 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sun, 27 Jul 2025 00:48:20 -0700 Subject: [PATCH 05/19] style(rwx): fix ruff and black errors for range and blueprint perms --- api/src/app/crud/crud_permissions.py | 56 +++--- api/src/app/crud/crud_ranges.py | 171 ++++++++++++------- api/src/app/models/permission_models.py | 17 +- api/src/app/models/range_models.py | 1 - api/src/app/schemas/range_schemas.py | 57 ++++--- api/tests/unit/crud/test_crud_permissions.py | 114 ++++++------- api/tests/unit/crud/test_crud_ranges.py | 12 +- 7 files changed, 241 insertions(+), 187 deletions(-) diff --git a/api/src/app/crud/crud_permissions.py b/api/src/app/crud/crud_permissions.py index e9629dc1..4232191e 100644 --- a/api/src/app/crud/crud_permissions.py +++ b/api/src/app/crud/crud_permissions.py @@ -2,8 +2,8 @@ from typing import Literal from sqlalchemy import select -from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.ext.asyncio import AsyncSession from ..models.permission_models import ( BlueprintRangePermissionModel, @@ -16,34 +16,35 @@ async def grant_blueprint_permission( db: AsyncSession, blueprint_range_id: int, - user_id: int, + user_id: int, permission_type: Literal["read", "write"], ) -> BlueprintRangePermissionModel: """Grant permission to a blueprint range. - + Args: ---- db: Database session blueprint_range_id: ID of blueprint range to grant permission for user_id: ID of user to grant permission to permission_type: Type of permission to grant - + Returns: ------- BlueprintRangePermissionModel: The created permission - + Raises: ------ SQLAlchemyError: If database operation fails + """ permission = BlueprintRangePermissionModel( blueprint_range_id=blueprint_range_id, user_id=user_id, permission_type=permission_type, ) - + db.add(permission) - + try: await db.flush() await db.refresh(permission) @@ -62,7 +63,7 @@ async def grant_blueprint_permission( e, ) raise - + return permission @@ -73,30 +74,31 @@ async def grant_deployed_permission( permission_type: Literal["read", "write", "execute"], ) -> DeployedRangePermissionModel: """Grant permission to a deployed range. - + Args: ---- db: Database session deployed_range_id: ID of deployed range to grant permission for user_id: ID of user to grant permission to permission_type: Type of permission to grant - + Returns: ------- DeployedRangePermissionModel: The created permission - + Raises: ------ SQLAlchemyError: If database operation fails + """ permission = DeployedRangePermissionModel( deployed_range_id=deployed_range_id, user_id=user_id, permission_type=permission_type, ) - + db.add(permission) - + try: await db.flush() await db.refresh(permission) @@ -115,7 +117,7 @@ async def grant_deployed_permission( e, ) raise - + return permission @@ -126,21 +128,22 @@ async def revoke_blueprint_permission( permission_type: Literal["read", "write"], ) -> bool: """Revoke permission from a blueprint range. - + Args: ---- db: Database session blueprint_range_id: ID of blueprint range to revoke permission from user_id: ID of user to revoke permission from permission_type: Type of permission to revoke - + Returns: ------- bool: True if permission was revoked, False if not found - + Raises: ------ SQLAlchemyError: If database operation fails + """ stmt = select(BlueprintRangePermissionModel).where( BlueprintRangePermissionModel.blueprint_range_id == blueprint_range_id, @@ -149,7 +152,7 @@ async def revoke_blueprint_permission( ) result = await db.execute(stmt) permission = result.scalar_one_or_none() - + if not permission: logger.warning( "Permission %s on blueprint range %s for user %s not found", @@ -158,7 +161,7 @@ async def revoke_blueprint_permission( user_id, ) return False - + try: await db.delete(permission) await db.flush() @@ -177,7 +180,7 @@ async def revoke_blueprint_permission( e, ) raise - + return True @@ -188,21 +191,22 @@ async def revoke_deployed_permission( permission_type: Literal["read", "write", "execute"], ) -> bool: """Revoke permission from a deployed range. - + Args: ---- db: Database session deployed_range_id: ID of deployed range to revoke permission from user_id: ID of user to revoke permission from permission_type: Type of permission to revoke - + Returns: ------- bool: True if permission was revoked, False if not found - + Raises: ------ SQLAlchemyError: If database operation fails + """ stmt = select(DeployedRangePermissionModel).where( DeployedRangePermissionModel.deployed_range_id == deployed_range_id, @@ -211,7 +215,7 @@ async def revoke_deployed_permission( ) result = await db.execute(stmt) permission = result.scalar_one_or_none() - + if not permission: logger.warning( "Permission %s on deployed range %s for user %s not found", @@ -220,7 +224,7 @@ async def revoke_deployed_permission( user_id, ) return False - + try: await db.delete(permission) await db.flush() @@ -239,5 +243,5 @@ async def revoke_deployed_permission( e, ) raise - + return True diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index 537b1c81..95fb2e24 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -28,52 +28,74 @@ logger = logging.getLogger(__name__) -def get_permissions(model: object) -> list[BlueprintRangePermissionModel | DeployedRangePermissionModel]: +def get_permissions( + model: object, +) -> list[BlueprintRangePermissionModel | DeployedRangePermissionModel]: """Safely get permissions from a model, handling mocks.""" - permissions = getattr(model, 'permissions', []) - if hasattr(permissions, '_mock_name'): + permissions = getattr(model, "permissions", []) + if hasattr(permissions, "_mock_name"): return [] return permissions or [] -def can_read_blueprint(range_model: Union[BlueprintRangeModel, object], user_id: int) -> bool: +def can_read_blueprint( + range_model: Union[BlueprintRangeModel, object], user_id: int +) -> bool: """Check if user can read a blueprint range.""" - if getattr(range_model, 'owner_id', None) == user_id: + if getattr(range_model, "owner_id", None) == user_id: return True - return any(p.user_id == user_id and p.permission_type in ('read', 'write') - for p in get_permissions(range_model)) + return any( + p.user_id == user_id and p.permission_type in ("read", "write") + for p in get_permissions(range_model) + ) -def can_write_blueprint(range_model: Union[BlueprintRangeModel, object], user_id: int) -> bool: +def can_write_blueprint( + range_model: Union[BlueprintRangeModel, object], user_id: int +) -> bool: """Check if user can write a blueprint range.""" - if getattr(range_model, 'owner_id', None) == user_id: + if getattr(range_model, "owner_id", None) == user_id: return True - return any(p.user_id == user_id and p.permission_type == 'write' - for p in get_permissions(range_model)) + return any( + p.user_id == user_id and p.permission_type == "write" + for p in get_permissions(range_model) + ) -def can_read_deployed(range_model: Union[DeployedRangeModel, object], user_id: int) -> bool: +def can_read_deployed( + range_model: Union[DeployedRangeModel, object], user_id: int +) -> bool: """Check if user can read a deployed range.""" - if getattr(range_model, 'owner_id', None) == user_id: + if getattr(range_model, "owner_id", None) == user_id: return True - return any(p.user_id == user_id and p.permission_type in ('read', 'write', 'execute') - for p in get_permissions(range_model)) + return any( + p.user_id == user_id and p.permission_type in ("read", "write", "execute") + for p in get_permissions(range_model) + ) -def can_write_deployed(range_model: Union[DeployedRangeModel, object], user_id: int) -> bool: +def can_write_deployed( + range_model: Union[DeployedRangeModel, object], user_id: int +) -> bool: """Check if user can write a deployed range.""" - if getattr(range_model, 'owner_id', None) == user_id: + if getattr(range_model, "owner_id", None) == user_id: return True - return any(p.user_id == user_id and p.permission_type == 'write' - for p in get_permissions(range_model)) + return any( + p.user_id == user_id and p.permission_type == "write" + for p in get_permissions(range_model) + ) -def can_execute_deployed(range_model: Union[DeployedRangeModel, object], user_id: int) -> bool: +def can_execute_deployed( + range_model: Union[DeployedRangeModel, object], user_id: int +) -> bool: """Check if user can execute a deployed range.""" - if getattr(range_model, 'owner_id', None) == user_id: + if getattr(range_model, "owner_id", None) == user_id: return True - return any(p.user_id == user_id and p.permission_type == 'execute' - for p in get_permissions(range_model)) + return any( + p.user_id == user_id and p.permission_type == "execute" + for p in get_permissions(range_model) + ) # ==================== Blueprints ===================== @@ -107,16 +129,25 @@ async def get_blueprint_range_headers( ).select_from(BlueprintRangeModel) if not is_admin: - stmt = stmt.outerjoin( - BlueprintRangePermissionModel, - BlueprintRangeModel.id == BlueprintRangePermissionModel.blueprint_range_id - ).filter( - (BlueprintRangeModel.owner_id == user_id) | - ( - (BlueprintRangePermissionModel.user_id == user_id) & - (BlueprintRangePermissionModel.permission_type.in_(["read", "write"])) + stmt = ( + stmt.outerjoin( + BlueprintRangePermissionModel, + BlueprintRangeModel.id + == BlueprintRangePermissionModel.blueprint_range_id, + ) + .filter( + (BlueprintRangeModel.owner_id == user_id) + | ( + (BlueprintRangePermissionModel.user_id == user_id) + & ( + BlueprintRangePermissionModel.permission_type.in_( + ["read", "write"] + ) + ) + ) ) - ).distinct() + .distinct() + ) result = await db.execute(stmt) @@ -195,8 +226,8 @@ def build_blueprint_range_models( range_models: list[BlueprintRangeModel] = [] for range_schema in ranges: range_model = BlueprintRangeModel( - **range_schema.model_dump(exclude={"vpcs", "readers", "writers"}), - owner_id=user_id + **range_schema.model_dump(exclude={"vpcs", "readers", "writers"}), + owner_id=user_id, ) range_model.vpcs = build_blueprint_vpc_models(range_schema.vpcs, user_id) range_models.append(range_model) @@ -253,15 +284,15 @@ async def create_blueprint_range( range_model.id, user_id, ) - + for reader_id in blueprint.readers: if reader_id != user_id: await grant_blueprint_permission(db, range_model.id, reader_id, "read") - + for writer_id in blueprint.writers: if writer_id != user_id: await grant_blueprint_permission(db, range_model.id, writer_id, "write") - + except SQLAlchemyError as e: logger.exception( "Database error while flushing range blueprint to database session for user: %s. Exception: %s.", @@ -301,9 +332,9 @@ async def delete_blueprint_range( """ range_model = await db.get( - BlueprintRangeModel, - range_id, - options=[selectinload(BlueprintRangeModel.permissions)] + BlueprintRangeModel, + range_id, + options=[selectinload(BlueprintRangeModel.permissions)], ) if not range_model: logger.warning( @@ -388,16 +419,24 @@ async def get_deployed_range_headers( ).select_from(DeployedRangeModel) if not is_admin: - stmt = stmt.outerjoin( - DeployedRangePermissionModel, - DeployedRangeModel.id == DeployedRangePermissionModel.deployed_range_id - ).filter( - (DeployedRangeModel.owner_id == user_id) | - ( - (DeployedRangePermissionModel.user_id == user_id) & - (DeployedRangePermissionModel.permission_type.in_(["read", "write", "execute"])) + stmt = ( + stmt.outerjoin( + DeployedRangePermissionModel, + DeployedRangeModel.id == DeployedRangePermissionModel.deployed_range_id, ) - ).distinct() + .filter( + (DeployedRangeModel.owner_id == user_id) + | ( + (DeployedRangePermissionModel.user_id == user_id) + & ( + DeployedRangePermissionModel.permission_type.in_( + ["read", "write", "execute"] + ) + ) + ) + ) + .distinct() + ) result = await db.execute(stmt) @@ -478,12 +517,14 @@ async def get_deployed_range_key( """ range_model = await db.get( - DeployedRangeModel, - range_id, - options=[selectinload(DeployedRangeModel.permissions)] + DeployedRangeModel, + range_id, + options=[selectinload(DeployedRangeModel.permissions)], ) - if not range_model or (not is_admin and not can_execute_deployed(range_model, user_id)): + if not range_model or ( + not is_admin and not can_execute_deployed(range_model, user_id) + ): logger.warning( "Failed to fetch deployed range key for range: %s for user %s. Range not found or unauthorized.", range_id, @@ -523,8 +564,10 @@ def build_deployed_range_models( range_models: list[DeployedRangeModel] = [] for range_schema in ranges: range_model = DeployedRangeModel( - **range_schema.model_dump(exclude={"vpcs", "readers", "writers", "executors"}), - owner_id=user_id + **range_schema.model_dump( + exclude={"vpcs", "readers", "writers", "executors"} + ), + owner_id=user_id, ) range_model.vpcs = build_deployed_vpc_models(range_schema.vpcs, user_id) range_models.append(range_model) @@ -581,19 +624,21 @@ async def create_deployed_range( range_model.id, user_id, ) - + for reader_id in range_schema.readers: if reader_id != user_id: await grant_deployed_permission(db, range_model.id, reader_id, "read") - + for writer_id in range_schema.writers: if writer_id != user_id: await grant_deployed_permission(db, range_model.id, writer_id, "write") - + for executor_id in range_schema.executors: if executor_id != user_id: - await grant_deployed_permission(db, range_model.id, executor_id, "execute") - + await grant_deployed_permission( + db, range_model.id, executor_id, "execute" + ) + except SQLAlchemyError as e: logger.exception( "Database error while flushing deployed range to database session for user: %s. Exception: %s.", @@ -633,9 +678,9 @@ async def delete_deployed_range( """ range_model = await db.get( - DeployedRangeModel, - range_id, - options=[selectinload(DeployedRangeModel.permissions)] + DeployedRangeModel, + range_id, + options=[selectinload(DeployedRangeModel.permissions)], ) if not range_model: logger.warning( diff --git a/api/src/app/models/permission_models.py b/api/src/app/models/permission_models.py index 0364d1f6..fc1e3acc 100644 --- a/api/src/app/models/permission_models.py +++ b/api/src/app/models/permission_models.py @@ -2,7 +2,6 @@ from sqlalchemy.orm import Mapped, MappedAsDataclass, mapped_column, relationship from ..core.db.database import Base -from .mixin_models import OwnableObjectMixin class BlueprintRangePermissionModel(Base, MappedAsDataclass): @@ -44,14 +43,14 @@ class BlueprintRangePermissionModel(Base, MappedAsDataclass): # Constraints __table_args__ = ( UniqueConstraint( - "blueprint_range_id", - "user_id", + "blueprint_range_id", + "user_id", "permission_type", - name="uq_blueprint_range_permissions" + name="uq_blueprint_range_permissions", ), CheckConstraint( "permission_type IN ('read', 'write')", - name="ck_blueprint_range_permission_type" + name="ck_blueprint_range_permission_type", ), ) @@ -95,13 +94,13 @@ class DeployedRangePermissionModel(Base, MappedAsDataclass): # Constraints __table_args__ = ( UniqueConstraint( - "deployed_range_id", - "user_id", + "deployed_range_id", + "user_id", "permission_type", - name="uq_deployed_range_permissions" + name="uq_deployed_range_permissions", ), CheckConstraint( "permission_type IN ('read', 'write', 'execute')", - name="ck_deployed_range_permission_type" + name="ck_deployed_range_permission_type", ), ) diff --git a/api/src/app/models/range_models.py b/api/src/app/models/range_models.py index e1f731b1..0397395b 100644 --- a/api/src/app/models/range_models.py +++ b/api/src/app/models/range_models.py @@ -96,4 +96,3 @@ class DeployedRangeModel(Base, OwnableObjectMixin, RangeMixin): cascade="all, delete-orphan", passive_deletes=True, ) - diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index 1f34866f..4cf5908a 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -2,7 +2,14 @@ from ipaddress import IPv4Address from typing import Any -from pydantic import BaseModel, ConfigDict, Field, ValidationInfo, computed_field, field_validator +from pydantic import ( + BaseModel, + ConfigDict, + Field, + ValidationInfo, + computed_field, + field_validator, +) from ..enums.providers import OpenLabsProvider from ..enums.range_states import RangeState @@ -97,25 +104,23 @@ class BlueprintRangeSchema(BlueprintRangeBaseSchema): vpcs: list[BlueprintVPCSchema] = Field( ..., description="All blueprint VPCs in range." ) - + @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" - if not hasattr(self, 'permissions'): + if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions - if perm.permission_type == 'read' + perm.user_id for perm in self.permissions if perm.permission_type == "read" ] - + @computed_field def writers(self) -> list[int]: - """Get list of user IDs with write access.""" - if not hasattr(self, 'permissions'): + """Get list of user IDs with write access.""" + if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions - if perm.permission_type == 'write' + perm.user_id for perm in self.permissions if perm.permission_type == "write" ] model_config = ConfigDict(from_attributes=True) @@ -184,13 +189,16 @@ class DeployedRangeCreateSchema(DeployedRangeBaseSchema): ..., description="Deployed VPCs in the range." ) readers: list[int] = Field( - default=[], description="List of user IDs with read access to this deployed range." + default=[], + description="List of user IDs with read access to this deployed range.", ) writers: list[int] = Field( - default=[], description="List of user IDs with write access to this deployed range." + default=[], + description="List of user IDs with write access to this deployed range.", ) executors: list[int] = Field( - default=[], description="List of user IDs with execute access to this deployed range." + default=[], + description="List of user IDs with execute access to this deployed range.", ) @field_validator("vpcs") @@ -230,35 +238,34 @@ class DeployedRangeSchema(DeployedRangeBaseSchema): vpcs: list[DeployedVPCSchema] = Field( ..., description="All deployed VPCs in the range." ) - + @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" - if not hasattr(self, 'permissions'): + if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions - if perm.permission_type == 'read' + perm.user_id for perm in self.permissions if perm.permission_type == "read" ] - + @computed_field def writers(self) -> list[int]: """Get list of user IDs with write access.""" - if not hasattr(self, 'permissions'): + if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions - if perm.permission_type == 'write' + perm.user_id for perm in self.permissions if perm.permission_type == "write" ] - + @computed_field def executors(self) -> list[int]: """Get list of user IDs with execute access.""" - if not hasattr(self, 'permissions'): + if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions - if perm.permission_type == 'execute' + perm.user_id + for perm in self.permissions + if perm.permission_type == "execute" ] model_config = ConfigDict(from_attributes=True) diff --git a/api/tests/unit/crud/test_crud_permissions.py b/api/tests/unit/crud/test_crud_permissions.py index d160fed9..a69d2ce7 100644 --- a/api/tests/unit/crud/test_crud_permissions.py +++ b/api/tests/unit/crud/test_crud_permissions.py @@ -1,6 +1,7 @@ import logging -import pytest from unittest.mock import Mock + +import pytest from sqlalchemy.exc import SQLAlchemyError from src.app.crud.crud_permissions import ( @@ -16,21 +17,20 @@ from .crud_mocks import DummyDB - # ==================== Grant Permissions ===================== async def test_grant_blueprint_read_permission() -> None: """Test granting read permission to a blueprint range.""" dummy_db = DummyDB() - + blueprint_range_id = 1 user_id = 2 - + result = await grant_blueprint_permission( dummy_db, blueprint_range_id, user_id, "read" ) - + # Check that permission was added to db dummy_db.add.assert_called_once() added_permission = dummy_db.add.call_args[0][0] @@ -38,11 +38,11 @@ async def test_grant_blueprint_read_permission() -> None: assert added_permission.blueprint_range_id == blueprint_range_id assert added_permission.user_id == user_id assert added_permission.permission_type == "read" - + # Check that db operations were called dummy_db.flush.assert_called_once() dummy_db.refresh.assert_called_once_with(added_permission) - + # Check return value assert result == added_permission @@ -50,34 +50,34 @@ async def test_grant_blueprint_read_permission() -> None: async def test_grant_blueprint_write_permission() -> None: """Test granting write permission to a blueprint range.""" dummy_db = DummyDB() - + blueprint_range_id = 1 user_id = 3 - + result = await grant_blueprint_permission( dummy_db, blueprint_range_id, user_id, "write" ) - + # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.blueprint_range_id == blueprint_range_id assert added_permission.user_id == user_id assert added_permission.permission_type == "write" - + assert result == added_permission async def test_grant_deployed_read_permission() -> None: """Test granting read permission to a deployed range.""" dummy_db = DummyDB() - + deployed_range_id = 1 user_id = 2 - + result = await grant_deployed_permission( dummy_db, deployed_range_id, user_id, "read" ) - + # Check that permission was added to db dummy_db.add.assert_called_once() added_permission = dummy_db.add.call_args[0][0] @@ -85,11 +85,11 @@ async def test_grant_deployed_read_permission() -> None: assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id assert added_permission.permission_type == "read" - + # Check that db operations were called dummy_db.flush.assert_called_once() dummy_db.refresh.assert_called_once_with(added_permission) - + # Check return value assert result == added_permission @@ -97,40 +97,40 @@ async def test_grant_deployed_read_permission() -> None: async def test_grant_deployed_write_permission() -> None: """Test granting write permission to a deployed range.""" dummy_db = DummyDB() - + deployed_range_id = 1 user_id = 3 - + result = await grant_deployed_permission( dummy_db, deployed_range_id, user_id, "write" ) - + # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id assert added_permission.permission_type == "write" - + assert result == added_permission async def test_grant_deployed_execute_permission() -> None: """Test granting execute permission to a deployed range.""" dummy_db = DummyDB() - + deployed_range_id = 1 user_id = 4 - + result = await grant_deployed_permission( dummy_db, deployed_range_id, user_id, "execute" ) - + # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id assert added_permission.permission_type == "execute" - + assert result == added_permission @@ -139,14 +139,14 @@ async def test_grant_blueprint_permission_db_error( ) -> None: """Test that grant_blueprint_permission handles database errors.""" dummy_db = DummyDB() - + # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + with pytest.raises(SQLAlchemyError, match=test_except_msg): await grant_blueprint_permission(dummy_db, 1, 2, "read") - + # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR @@ -161,14 +161,14 @@ async def test_grant_deployed_permission_db_error( ) -> None: """Test that grant_deployed_permission handles database errors.""" dummy_db = DummyDB() - + # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + with pytest.raises(SQLAlchemyError, match=test_except_msg): await grant_deployed_permission(dummy_db, 1, 2, "read") - + # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR @@ -184,25 +184,25 @@ async def test_grant_deployed_permission_db_error( async def test_revoke_blueprint_permission_success() -> None: """Test successfully revoking a blueprint permission.""" dummy_db = DummyDB() - + # Create a mock permission to be found and deleted mock_permission = BlueprintRangePermissionModel( blueprint_range_id=1, user_id=2, permission_type="read", ) - + # Mock the execute result chain mock_result = Mock() mock_result.scalar_one_or_none.return_value = mock_permission dummy_db.execute.return_value = mock_result - + result = await revoke_blueprint_permission(dummy_db, 1, 2, "read") - + # Check that the permission was deleted dummy_db.delete.assert_called_once_with(mock_permission) dummy_db.flush.assert_called_once() - + # Check return value assert result is True @@ -210,18 +210,18 @@ async def test_revoke_blueprint_permission_success() -> None: async def test_revoke_blueprint_permission_not_found() -> None: """Test revoking a blueprint permission that doesn't exist.""" dummy_db = DummyDB() - + # Mock the execute result chain to return None (permission not found) mock_result = Mock() mock_result.scalar_one_or_none.return_value = None dummy_db.execute.return_value = mock_result - + result = await revoke_blueprint_permission(dummy_db, 1, 2, "read") - + # Check that delete was not called dummy_db.delete.assert_not_called() dummy_db.flush.assert_not_called() - + # Check return value assert result is False @@ -229,25 +229,25 @@ async def test_revoke_blueprint_permission_not_found() -> None: async def test_revoke_deployed_permission_success() -> None: """Test successfully revoking a deployed permission.""" dummy_db = DummyDB() - + # Create a mock permission to be found and deleted mock_permission = DeployedRangePermissionModel( deployed_range_id=1, user_id=2, permission_type="execute", ) - + # Mock the execute result chain mock_result = Mock() mock_result.scalar_one_or_none.return_value = mock_permission dummy_db.execute.return_value = mock_result - + result = await revoke_deployed_permission(dummy_db, 1, 2, "execute") - + # Check that the permission was deleted dummy_db.delete.assert_called_once_with(mock_permission) dummy_db.flush.assert_called_once() - + # Check return value assert result is True @@ -255,18 +255,18 @@ async def test_revoke_deployed_permission_success() -> None: async def test_revoke_deployed_permission_not_found() -> None: """Test revoking a deployed permission that doesn't exist.""" dummy_db = DummyDB() - + # Mock the execute result chain to return None (permission not found) mock_result = Mock() mock_result.scalar_one_or_none.return_value = None dummy_db.execute.return_value = mock_result - + result = await revoke_deployed_permission(dummy_db, 1, 2, "write") - + # Check that delete was not called dummy_db.delete.assert_not_called() dummy_db.flush.assert_not_called() - + # Check return value assert result is False @@ -276,7 +276,7 @@ async def test_revoke_blueprint_permission_db_error( ) -> None: """Test that revoke_blueprint_permission handles database errors.""" dummy_db = DummyDB() - + # Create a mock permission mock_permission = BlueprintRangePermissionModel( blueprint_range_id=1, @@ -287,14 +287,14 @@ async def test_revoke_blueprint_permission_db_error( mock_result = Mock() mock_result.scalar_one_or_none.return_value = mock_permission dummy_db.execute.return_value = mock_result - + # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + with pytest.raises(SQLAlchemyError, match=test_except_msg): await revoke_blueprint_permission(dummy_db, 1, 2, "read") - + # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR @@ -309,7 +309,7 @@ async def test_revoke_deployed_permission_db_error( ) -> None: """Test that revoke_deployed_permission handles database errors.""" dummy_db = DummyDB() - + # Create a mock permission mock_permission = DeployedRangePermissionModel( deployed_range_id=1, @@ -320,18 +320,18 @@ async def test_revoke_deployed_permission_db_error( mock_result = Mock() mock_result.scalar_one_or_none.return_value = mock_permission dummy_db.execute.return_value = mock_result - + # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + with pytest.raises(SQLAlchemyError, match=test_except_msg): await revoke_deployed_permission(dummy_db, 1, 2, "execute") - + # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR and record.exc_info is not None and test_except_msg in record.message for record in caplog.records - ) \ No newline at end of file + ) diff --git a/api/tests/unit/crud/test_crud_ranges.py b/api/tests/unit/crud/test_crud_ranges.py index 7221ff65..d051f70d 100644 --- a/api/tests/unit/crud/test_crud_ranges.py +++ b/api/tests/unit/crud/test_crud_ranges.py @@ -501,9 +501,9 @@ async def test_get_non_existent_deployed_range() -> None: @pytest.mark.parametrize( "is_admin, user_owns_range", [ - (False, True), # Regular user, owns range - should get key + (False, True), # Regular user, owns range - should get key (False, False), # Regular user, doesn't own range - should not get key - (True, False), # Admin user, doesn't own range - should get key + (True, False), # Admin user, doesn't own range - should get key ], ) async def test_get_deployed_range_key_permissions( @@ -513,16 +513,16 @@ async def test_get_deployed_range_key_permissions( """Test the deployed range key crud function respects permissions.""" dummy_db = DummyDB() dummy_range = DummyDeployedRange() - + range_id = 1 user_id = 1 - + # Set ownership based on test parameters if user_owns_range: dummy_range.owner_id = user_id else: dummy_range.owner_id = user_id + 1 - + # Configure return of mock result dummy_db.get.return_value = dummy_range dummy_db.scalar.return_value = "fake_private_key" @@ -533,7 +533,7 @@ async def test_get_deployed_range_key_permissions( # Check if we should expect a result should_have_access = is_admin or user_owns_range - + if should_have_access: assert result is not None assert result.range_private_key == "fake_private_key" From 43133e74cad9711573747f494b0e88f5e23c9c7c Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 20:46:24 -0700 Subject: [PATCH 06/19] fix(rwx): move id and perm type to mixin --- api/src/app/models/permission_models.py | 58 +++++++++---------------- 1 file changed, 21 insertions(+), 37 deletions(-) diff --git a/api/src/app/models/permission_models.py b/api/src/app/models/permission_models.py index fc1e3acc..93ef77a7 100644 --- a/api/src/app/models/permission_models.py +++ b/api/src/app/models/permission_models.py @@ -1,13 +1,11 @@ from sqlalchemy import BigInteger, CheckConstraint, ForeignKey, String, UniqueConstraint -from sqlalchemy.orm import Mapped, MappedAsDataclass, mapped_column, relationship +from sqlalchemy.orm import Mapped, MappedAsDataclass, mapped_column, relationship, declared_attr from ..core.db.database import Base -class BlueprintRangePermissionModel(Base, MappedAsDataclass): - """Model for blueprint range permission grants to users.""" - - __tablename__ = "blueprint_range_permissions" +class PermissionMixin(MappedAsDataclass): + """Mixin class for common permission fields.""" id: Mapped[int] = mapped_column( BigInteger, @@ -16,29 +14,36 @@ class BlueprintRangePermissionModel(Base, MappedAsDataclass): comment="Primary key (BIGSERIAL)", ) - # The blueprint range being shared - blueprint_range_id: Mapped[int] = mapped_column( - BigInteger, - ForeignKey("blueprint_ranges.id", ondelete="CASCADE"), - nullable=False, - ) - - # The user being granted permission user_id: Mapped[int] = mapped_column( BigInteger, ForeignKey("users.id", ondelete="CASCADE"), nullable=False, ) - # Type of permission granted (read, write) permission_type: Mapped[str] = mapped_column( String(10), nullable=False, ) + @declared_attr + def user(cls): + return relationship("UserModel") + + +class BlueprintRangePermissionModel(Base, PermissionMixin): + """Model for blueprint range permission grants to users.""" + + __tablename__ = "blueprint_range_permissions" + + # The blueprint range being shared + blueprint_range_id: Mapped[int] = mapped_column( + BigInteger, + ForeignKey("blueprint_ranges.id", ondelete="CASCADE"), + nullable=False, + ) + # Relationships blueprint_range = relationship("BlueprintRangeModel", back_populates="permissions") - user = relationship("UserModel") # Constraints __table_args__ = ( @@ -55,18 +60,11 @@ class BlueprintRangePermissionModel(Base, MappedAsDataclass): ) -class DeployedRangePermissionModel(Base, MappedAsDataclass): +class DeployedRangePermissionModel(Base, PermissionMixin): """Model for deployed range permission grants to users.""" __tablename__ = "deployed_range_permissions" - id: Mapped[int] = mapped_column( - BigInteger, - primary_key=True, - init=False, - comment="Primary key (BIGSERIAL)", - ) - # The deployed range being shared deployed_range_id: Mapped[int] = mapped_column( BigInteger, @@ -74,22 +72,8 @@ class DeployedRangePermissionModel(Base, MappedAsDataclass): nullable=False, ) - # The user being granted permission - user_id: Mapped[int] = mapped_column( - BigInteger, - ForeignKey("users.id", ondelete="CASCADE"), - nullable=False, - ) - - # Type of permission granted (read, write, execute) - permission_type: Mapped[str] = mapped_column( - String(10), - nullable=False, - ) - # Relationships deployed_range = relationship("DeployedRangeModel", back_populates="permissions") - user = relationship("UserModel") # Constraints __table_args__ = ( From b08cbb4018a7f860ce6d2956e9102a4906b8d7e3 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 20:48:48 -0700 Subject: [PATCH 07/19] fix(rwx): make all schemas inherit rwx --- api/src/app/schemas/range_schemas.py | 30 +++++++++++++--------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index 12a8e13c..a54d528f 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -51,8 +51,12 @@ class BlueprintRangeBaseSchema(RangeCommonSchema): description="Description of blueprint range.", examples=["This is my test range."], ) - - pass + readers: list[int] = Field( + default=[], description="List of user IDs with read access to this blueprint." + ) + writers: list[int] = Field( + default=[], description="List of user IDs with write access to this blueprint." + ) class BlueprintRangeCreateSchema(BlueprintRangeBaseSchema): @@ -61,12 +65,6 @@ class BlueprintRangeCreateSchema(BlueprintRangeBaseSchema): vpcs: list[BlueprintVPCCreateSchema] = Field( ..., description="All blueprint VPCs in range." ) - readers: list[int] = Field( - default=[], description="List of user IDs with read access to this blueprint." - ) - writers: list[int] = Field( - default=[], description="List of user IDs with write access to this blueprint." - ) @field_validator("vpcs") @classmethod @@ -181,14 +179,6 @@ class DeployedRangeBaseSchema(RangeCommonSchema): min_length=1, description="SSH private key for the range.", ) - - -class DeployedRangeCreateSchema(DeployedRangeBaseSchema): - """Schema to create deployed range object.""" - - vpcs: list[DeployedVPCCreateSchema] = Field( - ..., description="Deployed VPCs in the range." - ) readers: list[int] = Field( default=[], description="List of user IDs with read access to this deployed range.", @@ -202,6 +192,14 @@ class DeployedRangeCreateSchema(DeployedRangeBaseSchema): description="List of user IDs with execute access to this deployed range.", ) + +class DeployedRangeCreateSchema(DeployedRangeBaseSchema): + """Schema to create deployed range object.""" + + vpcs: list[DeployedVPCCreateSchema] = Field( + ..., description="Deployed VPCs in the range." + ) + @field_validator("vpcs") @classmethod def validate_unique_vpc_names( From 5ec03d13d4b5f995da1002bf1494f676a9bc6c92 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 20:51:21 -0700 Subject: [PATCH 08/19] fix(rwx): make all schemas inherit rwx computed fields --- api/src/app/schemas/range_schemas.py | 94 ++++++++++++++-------------- 1 file changed, 47 insertions(+), 47 deletions(-) diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index a54d528f..0955344e 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -58,6 +58,24 @@ class BlueprintRangeBaseSchema(RangeCommonSchema): default=[], description="List of user IDs with write access to this blueprint." ) + @computed_field + def readers(self) -> list[int]: + """Get list of user IDs with read access.""" + if not hasattr(self, "permissions"): + return [] + return [ + perm.user_id for perm in self.permissions if perm.permission_type == "read" + ] + + @computed_field + def writers(self) -> list[int]: + """Get list of user IDs with write access.""" + if not hasattr(self, "permissions"): + return [] + return [ + perm.user_id for perm in self.permissions if perm.permission_type == "write" + ] + class BlueprintRangeCreateSchema(BlueprintRangeBaseSchema): """Schema to create blueprint range objects.""" @@ -104,24 +122,6 @@ class BlueprintRangeSchema(BlueprintRangeBaseSchema): ..., description="All blueprint VPCs in range." ) - @computed_field - def readers(self) -> list[int]: - """Get list of user IDs with read access.""" - if not hasattr(self, "permissions"): - return [] - return [ - perm.user_id for perm in self.permissions if perm.permission_type == "read" - ] - - @computed_field - def writers(self) -> list[int]: - """Get list of user IDs with write access.""" - if not hasattr(self, "permissions"): - return [] - return [ - perm.user_id for perm in self.permissions if perm.permission_type == "write" - ] - model_config = ConfigDict(from_attributes=True) @@ -192,6 +192,35 @@ class DeployedRangeBaseSchema(RangeCommonSchema): description="List of user IDs with execute access to this deployed range.", ) + @computed_field + def readers(self) -> list[int]: + """Get list of user IDs with read access.""" + if not hasattr(self, "permissions"): + return [] + return [ + perm.user_id for perm in self.permissions if perm.permission_type == "read" + ] + + @computed_field + def writers(self) -> list[int]: + """Get list of user IDs with write access.""" + if not hasattr(self, "permissions"): + return [] + return [ + perm.user_id for perm in self.permissions if perm.permission_type == "write" + ] + + @computed_field + def executors(self) -> list[int]: + """Get list of user IDs with execute access.""" + if not hasattr(self, "permissions"): + return [] + return [ + perm.user_id + for perm in self.permissions + if perm.permission_type == "execute" + ] + class DeployedRangeCreateSchema(DeployedRangeBaseSchema): """Schema to create deployed range object.""" @@ -238,35 +267,6 @@ class DeployedRangeSchema(DeployedRangeBaseSchema): ..., description="All deployed VPCs in the range." ) - @computed_field - def readers(self) -> list[int]: - """Get list of user IDs with read access.""" - if not hasattr(self, "permissions"): - return [] - return [ - perm.user_id for perm in self.permissions if perm.permission_type == "read" - ] - - @computed_field - def writers(self) -> list[int]: - """Get list of user IDs with write access.""" - if not hasattr(self, "permissions"): - return [] - return [ - perm.user_id for perm in self.permissions if perm.permission_type == "write" - ] - - @computed_field - def executors(self) -> list[int]: - """Get list of user IDs with execute access.""" - if not hasattr(self, "permissions"): - return [] - return [ - perm.user_id - for perm in self.permissions - if perm.permission_type == "execute" - ] - model_config = ConfigDict(from_attributes=True) From a42af7f6e8f701f900660ce7bc9aee08593f5b84 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 21:02:57 -0700 Subject: [PATCH 09/19] fix(rwx): no longer delete rw fields now that theyre in base schema --- api/src/app/schemas/range_schemas.py | 20 ------------------- .../common/api/v1/test_blueprint_ranges.py | 2 -- 2 files changed, 22 deletions(-) diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index 0955344e..c94e9e1a 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -51,13 +51,6 @@ class BlueprintRangeBaseSchema(RangeCommonSchema): description="Description of blueprint range.", examples=["This is my test range."], ) - readers: list[int] = Field( - default=[], description="List of user IDs with read access to this blueprint." - ) - writers: list[int] = Field( - default=[], description="List of user IDs with write access to this blueprint." - ) - @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" @@ -179,19 +172,6 @@ class DeployedRangeBaseSchema(RangeCommonSchema): min_length=1, description="SSH private key for the range.", ) - readers: list[int] = Field( - default=[], - description="List of user IDs with read access to this deployed range.", - ) - writers: list[int] = Field( - default=[], - description="List of user IDs with write access to this deployed range.", - ) - executors: list[int] = Field( - default=[], - description="List of user IDs with execute access to this deployed range.", - ) - @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" diff --git a/api/tests/common/api/v1/test_blueprint_ranges.py b/api/tests/common/api/v1/test_blueprint_ranges.py index fe462a5b..5c5df9e6 100644 --- a/api/tests/common/api/v1/test_blueprint_ranges.py +++ b/api/tests/common/api/v1/test_blueprint_ranges.py @@ -593,8 +593,6 @@ async def test_blueprint_range_get_non_empty_list( # Mimic the header response with the valid JSON valid_blueprint_copy = copy.deepcopy(valid_blueprint_range_create_payload) del valid_blueprint_copy["vpcs"] - del valid_blueprint_copy["readers"] - del valid_blueprint_copy["writers"] remove_key_recursively( recieved_range, "id" ) # Our creation payload doesn't have IDs From 439ad61fc7d78aade329e664571b156736cbb97a Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 21:15:01 -0700 Subject: [PATCH 10/19] fix(rwx): remove duplicate test data --- api/tests/common/api/v1/config.py | 3 + api/tests/unit/api/v1/config.py | 367 +++++------------------------- 2 files changed, 66 insertions(+), 304 deletions(-) diff --git a/api/tests/common/api/v1/config.py b/api/tests/common/api/v1/config.py index 2b21fdc5..95d89b61 100644 --- a/api/tests/common/api/v1/config.py +++ b/api/tests/common/api/v1/config.py @@ -299,6 +299,9 @@ "name": "openlabs-deployed-test-v2", "vnc": True, "vpn": False, + "readers": [], + "writers": [], + "executors": [], } valid_deployed_vpc_data: dict[str, Any] = copy.deepcopy( diff --git a/api/tests/unit/api/v1/config.py b/api/tests/unit/api/v1/config.py index 9d3cf109..9c7743cc 100644 --- a/api/tests/unit/api/v1/config.py +++ b/api/tests/unit/api/v1/config.py @@ -1,76 +1,83 @@ import copy -import random -from datetime import datetime, timezone from typing import Any from src.app.enums.job_status import OpenLabsJobStatus -from src.app.enums.providers import OpenLabsProvider -from src.app.enums.range_states import RangeState -from src.app.enums.regions import OpenLabsRegion -# Base route -BASE_ROUTE = "/api/v1" +# Import all common test data +from tests.common.api.v1.config import * # noqa: F403, F401 +# Override BASE_ROUTE for unit tests (common uses dynamic route) +BASE_ROUTE = "/api/v1" # ============================== -# Blueprint Payloads +# Job Payloads # ============================== -# Valid payload for comparison -valid_blueprint_range_create_payload: dict[str, Any] = { - "vpcs": [ - { - "cidr": "192.168.0.0/16", - "name": "example-vpc-1", - "subnets": [ - { - "cidr": "192.168.1.0/24", - "name": "example-subnet-1", - "hosts": [ - { - "hostname": "example-host-1", - "os": "debian_11", - "spec": "tiny", - "size": 8, - "tags": ["web", "linux"], - } - ], - } - ], - } - ], - "description": "This is a test range blueprint.", - "provider": "aws", - "name": "example-range-1", - "vnc": False, - "vpn": False, - "readers": [], - "writers": [], +queued_job_payload = { + "arq_job_id": "e8ce953f4f6c4a7c884a9afe8112d31f", + "job_name": "deploy_range", + "job_try": None, + "enqueue_time": "2025-07-02T10:22:42.407000Z", + "start_time": None, + "finish_time": None, + "status": "queued", + "result": None, + "error_message": None, + "id": 1, } -# Valid range payload for deployment -valid_range_deploy_payload: dict[str, Any] = { - "name": "test-deploy-range-1", - "description": "test range to deploy", - "blueprint_id": str(random.randint(1, 100)), # noqa: S311 - "region": OpenLabsRegion.US_EAST_1.value, +in_progress_job_payload = { + "arq_job_id": "e8ce953f4f6c4a7c884a9afe8112d31f", + "job_name": "deploy_range", + "job_try": 1, + "enqueue_time": "2025-07-02T10:22:42.407000Z", + "start_time": "2025-07-02T10:22:42.814805Z", + "finish_time": None, + "status": "in_progress", + "result": None, + "error_message": None, + "id": 1, } -valid_blueprint_vpc_create_payload = copy.deepcopy( - valid_blueprint_range_create_payload["vpcs"][0] -) -valid_blueprint_subnet_create_payload = copy.deepcopy( - valid_blueprint_vpc_create_payload["subnets"][0] -) -valid_blueprint_host_create_payload = copy.deepcopy( - valid_blueprint_subnet_create_payload["hosts"][0] -) +complete_job_payload = { + "arq_job_id": "e8ce953f4f6c4a7c884a9afe8112d31f", + "job_name": "deploy_range", + "job_try": 1, + "enqueue_time": "2025-07-02T10:22:42.407000Z", + "start_time": "2025-07-02T10:22:42.814805Z", + "finish_time": "2025-07-02T10:24:49.224000Z", + "status": "complete", + "result": { + "id": 1, + "vnc": False, + "vpn": False, + "date": "2025-07-02T10:24:49.169656Z", + "name": "Test-ARQ-Range-v1", + "state": "on", + "region": "us_east_1", + "provider": "aws", + "description": "This is my test range.", + }, + "error_message": None, + "id": 1, +} +# Updates to completed job payload to +# create a failed job payload +failed_job_updates = { + "result": None, + "status": OpenLabsJobStatus.FAILED.value, + "error_message": "Mock error message.", +} +failed_job_payload = copy.deepcopy(complete_job_payload) +failed_job_payload.update(failed_job_updates) + +# Override specific payloads that need unit-test specific values valid_blueprint_range_multi_create_payload: dict[str, Any] = { "vpcs": [ { "cidr": "10.0.0.0/16", - "name": "dev-vpc", + "name": "dev-vpc", # Unit tests use simpler names without spaces "subnets": [ { "cidr": "10.0.1.0/24", @@ -113,7 +120,7 @@ "subnets": [ { "cidr": "172.16.1.0/24", - "name": "prod-subnet-dmz", + "name": "prod-subnet-dmz", # Unit tests use simpler names without spaces "hosts": [ { "hostname": "prod-gateway-01", @@ -139,252 +146,4 @@ ) valid_blueprint_subnet_multi_create_payload: dict[str, Any] = copy.deepcopy( valid_blueprint_vpc_multi_create_payload["subnets"][0] -) - -# ============================== -# Deployed Payloads -# ============================== - -valid_deployed_range_header_data: dict[str, Any] = { - "id": random.randint(1, 100), # noqa: S311 - "name": "Fake Deployed Range", - "description": "Description of fake deployed range for testing.", - "date": datetime.now(tz=timezone.utc), - "state": RangeState.ON, - "region": OpenLabsRegion.US_EAST_1, - "provider": OpenLabsProvider.AWS, -} - -valid_deployed_range_data: dict[str, Any] = { - "id": 999, - "vpcs": [ - { - "id": 10, - "name": "production-vpc-main", - "cidr": "10.100.0.0/16", - "resource_id": "vpc-abc123xyz789", - "subnets": [ - { - "id": 101, - "name": "prod-subnet-web-a", - "cidr": "10.100.1.0/24", - "resource_id": "subnet-def456uvw123", - "hosts": [ - { - "id": 1001, - "hostname": "webserver-01", - "os": "debian_11", - "spec": "small", - "size": 30, - "tags": ["web", "frontend", "nginx"], - "resource_id": "i-hostabc111", - "ip_address": "10.100.1.10", - }, - { - "id": 1002, - "hostname": "webserver-02", - "os": "debian_11", - "spec": "small", - "size": 30, - "tags": ["web", "frontend", "nginx"], - "resource_id": "i-hostabc222", - "ip_address": "10.100.1.11", - }, - ], - }, - { - "id": 102, - "name": "prod-subnet-app-a", - "cidr": "10.100.2.0/24", - "resource_id": "subnet-ghi789rst456", - "hosts": [ - { - "id": 1003, - "hostname": "appserver-01", - "os": "ubuntu_22", - "spec": "medium", - "size": 50, - "tags": ["app", "backend", "api"], - "resource_id": "i-hostdef333", - "ip_address": "10.100.2.20", - } - ], - }, - { - "id": 103, - "name": "prod-subnet-db-a", - "cidr": "10.100.3.0/24", - "resource_id": "subnet-jkl012pqr789", - "hosts": [ - { - "id": 1004, - "hostname": "dbserver-01", - "os": "windows_2022", - "spec": "large", - "size": 80, - "tags": ["database", "sql", "critical"], - "resource_id": "i-hostghi444", - "ip_address": "10.100.3.30", - } - ], - }, - ], - }, - { - "id": 20, - "name": "staging-vpc", - "cidr": "10.200.0.0/16", - "resource_id": "vpc-lmn456opq012", - "subnets": [ - { - "id": 201, - "name": "staging-subnet-main", - "cidr": "10.200.1.0/24", - "resource_id": "subnet-rst789uvw345", - "hosts": [ - { - "id": 2001, - "hostname": "staging-app-01", - "os": "kali", - "spec": "medium", - "size": 60, - "tags": ["staging", "pentest"], - "resource_id": "i-hostjkl555", - "ip_address": "10.200.1.15", - } - ], - } - ], - }, - ], - "description": "Comprehensive deployed test range with corrected schema attributes.", - "date": "2025-06-03T10:00:00Z", - "readme": "# Test Deployed Range: Version 2\n\nThis data uses the updated and correct schema definitions for all nested objects.\n\n## Environment Overview:\n- **Production VPC**: Contains web, app, and database subnets.\n- **Staging VPC**: Contains a general-purpose subnet for testing.\n", - "state_file": { - "simplified_mock_state": "Test data placeholder - not a real Terraform state." - }, - "state": "on", - "region": "us_east_1", - "jumpbox_resource_id": "i-jumpbox789012", - "jumpbox_public_ip": "203.0.113.25", - "range_private_key": "-----BEGIN OPENSSH PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDIfSjVkaDRtKNO\n... (rest of a mock SSH private key) ...\nhNUC8ZLe06edaNBX6N2jS9Wp3mk3JNGxQjagtrh9TGUrscedop4hCQABAoGALKe1\n-----END OPENSSH PRIVATE KEY-----", - "provider": "aws", - "name": "openlabs-deployed-test-v2", - "vnc": True, - "vpn": False, - "readers": [], - "writers": [], - "executors": [], -} - -valid_deployed_vpc_data: dict[str, Any] = copy.deepcopy( - valid_deployed_range_data["vpcs"][0] -) - -valid_deployed_subnet_data: dict[str, Any] = copy.deepcopy( - valid_deployed_vpc_data["subnets"][0] -) - -valid_deployed_host_data: dict[str, Any] = copy.deepcopy( - valid_deployed_subnet_data["hosts"][0] -) - -valid_range_private_key_data: dict[str, Any] = { - "range_private_key": "-----BEGIN OPENSSH PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDIfSjVkaDRtKNO\n... (rest of a mock SSH private key) ...\nhNUC8ZLe06edaNBX6N2jS9Wp3mk3JNGxQjagtrh9TGUrscedop4hCQABAoGALKe1\n-----END OPENSSH PRIVATE KEY-----", -} - -# ============================== -# User/Auth Payloads -# ============================== - -# Test user credentials -base_user_register_payload = { - "email": "test@ufsit.club", - "password": "password123", - "name": "Test User", -} - -base_user_login_payload = copy.deepcopy(base_user_register_payload) -base_user_login_payload.pop("name") - -# Test data for password update -password_update_payload = { - "current_password": "password123", - "new_password": "newpassword123", -} - -# Test data for AWS secrets -aws_secrets_payload = { - "aws_access_key": "AKIAIOSFODNN7EXAMPLE", - "aws_secret_key": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", -} - -# Test data for Azure secrets -azure_secrets_payload = { - "azure_client_id": "00000000-0000-0000-0000-000000000000", - "azure_client_secret": "example-client-secret-value", - "azure_tenant_id": "00000000-0000-0000-0000-000000000000", - "azure_subscription_id": "00000000-0000-0000-0000-000000000000", -} -# ============================== -# Job Payloads -# ============================== - -queued_job_payload = { - "arq_job_id": "e8ce953f4f6c4a7c884a9afe8112d31f", - "job_name": "deploy_range", - "job_try": None, - "enqueue_time": "2025-07-02T10:22:42.407000Z", - "start_time": None, - "finish_time": None, - "status": "queued", - "result": None, - "error_message": None, - "id": 1, -} - -in_progress_job_payload = { - "arq_job_id": "e8ce953f4f6c4a7c884a9afe8112d31f", - "job_name": "deploy_range", - "job_try": 1, - "enqueue_time": "2025-07-02T10:22:42.407000Z", - "start_time": "2025-07-02T10:22:42.814805Z", - "finish_time": None, - "status": "in_progress", - "result": None, - "error_message": None, - "id": 1, -} - -complete_job_payload = { - "arq_job_id": "e8ce953f4f6c4a7c884a9afe8112d31f", - "job_name": "deploy_range", - "job_try": 1, - "enqueue_time": "2025-07-02T10:22:42.407000Z", - "start_time": "2025-07-02T10:22:42.814805Z", - "finish_time": "2025-07-02T10:24:49.224000Z", - "status": "complete", - "result": { - "id": 1, - "vnc": False, - "vpn": False, - "date": "2025-07-02T10:24:49.169656Z", - "name": "Test-ARQ-Range-v1", - "state": "on", - "region": "us_east_1", - "provider": "aws", - "description": "This is my test range.", - }, - "error_message": None, - "id": 1, -} - -# Updates to completed job payload to -# create a failed job payload -failed_job_updates = { - "result": None, - "status": OpenLabsJobStatus.FAILED.value, - "error_message": "Mock error message.", -} -failed_job_payload = copy.deepcopy(complete_job_payload) -failed_job_payload.update(failed_job_updates) +) \ No newline at end of file From f1d57b581cc52ba1a44e577dd55387f6fab063ac Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 21:30:36 -0700 Subject: [PATCH 11/19] fix(rwx): move from literalstring to enum for perm types --- api/src/app/crud/crud_permissions.py | 10 +++++----- api/src/app/enums/permissions.py | 26 +++++++++++++++++++++++++ api/src/app/models/permission_models.py | 5 +++-- api/src/app/schemas/range_schemas.py | 11 ++++++----- 4 files changed, 40 insertions(+), 12 deletions(-) create mode 100644 api/src/app/enums/permissions.py diff --git a/api/src/app/crud/crud_permissions.py b/api/src/app/crud/crud_permissions.py index 4232191e..1bc420a0 100644 --- a/api/src/app/crud/crud_permissions.py +++ b/api/src/app/crud/crud_permissions.py @@ -1,10 +1,10 @@ import logging -from typing import Literal from sqlalchemy import select from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.ext.asyncio import AsyncSession +from ..enums.permissions import BlueprintPermissionType, DeployedRangePermissionType from ..models.permission_models import ( BlueprintRangePermissionModel, DeployedRangePermissionModel, @@ -17,7 +17,7 @@ async def grant_blueprint_permission( db: AsyncSession, blueprint_range_id: int, user_id: int, - permission_type: Literal["read", "write"], + permission_type: BlueprintPermissionType, ) -> BlueprintRangePermissionModel: """Grant permission to a blueprint range. @@ -71,7 +71,7 @@ async def grant_deployed_permission( db: AsyncSession, deployed_range_id: int, user_id: int, - permission_type: Literal["read", "write", "execute"], + permission_type: DeployedRangePermissionType, ) -> DeployedRangePermissionModel: """Grant permission to a deployed range. @@ -125,7 +125,7 @@ async def revoke_blueprint_permission( db: AsyncSession, blueprint_range_id: int, user_id: int, - permission_type: Literal["read", "write"], + permission_type: BlueprintPermissionType, ) -> bool: """Revoke permission from a blueprint range. @@ -188,7 +188,7 @@ async def revoke_deployed_permission( db: AsyncSession, deployed_range_id: int, user_id: int, - permission_type: Literal["read", "write", "execute"], + permission_type: DeployedRangePermissionType, ) -> bool: """Revoke permission from a deployed range. diff --git a/api/src/app/enums/permissions.py b/api/src/app/enums/permissions.py new file mode 100644 index 00000000..06933d9c --- /dev/null +++ b/api/src/app/enums/permissions.py @@ -0,0 +1,26 @@ +from enum import Enum + + +class BlueprintPermissionType(str, Enum): + """Permission types for blueprint ranges.""" + + READ = "read" + WRITE = "write" + + @classmethod + def values(cls) -> str: + """Return formatted tuple string of all permission values.""" + return f"({', '.join(repr(t.value) for t in cls)})" + + +class DeployedRangePermissionType(str, Enum): + """Permission types for deployed ranges.""" + + READ = "read" + WRITE = "write" + EXECUTE = "execute" + + @classmethod + def values(cls) -> str: + """Return formatted tuple string of all permission values.""" + return f"({', '.join(repr(t.value) for t in cls)})" \ No newline at end of file diff --git a/api/src/app/models/permission_models.py b/api/src/app/models/permission_models.py index 93ef77a7..98f1cdd5 100644 --- a/api/src/app/models/permission_models.py +++ b/api/src/app/models/permission_models.py @@ -2,6 +2,7 @@ from sqlalchemy.orm import Mapped, MappedAsDataclass, mapped_column, relationship, declared_attr from ..core.db.database import Base +from ..enums.permissions import BlueprintPermissionType, DeployedRangePermissionType class PermissionMixin(MappedAsDataclass): @@ -54,7 +55,7 @@ class BlueprintRangePermissionModel(Base, PermissionMixin): name="uq_blueprint_range_permissions", ), CheckConstraint( - "permission_type IN ('read', 'write')", + f"permission_type IN {BlueprintPermissionType.values()}", name="ck_blueprint_range_permission_type", ), ) @@ -84,7 +85,7 @@ class DeployedRangePermissionModel(Base, PermissionMixin): name="uq_deployed_range_permissions", ), CheckConstraint( - "permission_type IN ('read', 'write', 'execute')", + f"permission_type IN {DeployedRangePermissionType.values()}", name="ck_deployed_range_permission_type", ), ) diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index c94e9e1a..fc7b091b 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -11,6 +11,7 @@ field_validator, ) +from ..enums.permissions import BlueprintPermissionType, DeployedRangePermissionType from ..enums.providers import OpenLabsProvider from ..enums.range_states import RangeState from ..enums.regions import OpenLabsRegion @@ -57,7 +58,7 @@ def readers(self) -> list[int]: if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == "read" + perm.user_id for perm in self.permissions if perm.permission_type == BlueprintPermissionType.READ.value ] @computed_field @@ -66,7 +67,7 @@ def writers(self) -> list[int]: if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == "write" + perm.user_id for perm in self.permissions if perm.permission_type == BlueprintPermissionType.WRITE.value ] @@ -178,7 +179,7 @@ def readers(self) -> list[int]: if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == "read" + perm.user_id for perm in self.permissions if perm.permission_type == DeployedRangePermissionType.READ.value ] @computed_field @@ -187,7 +188,7 @@ def writers(self) -> list[int]: if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == "write" + perm.user_id for perm in self.permissions if perm.permission_type == DeployedRangePermissionType.WRITE.value ] @computed_field @@ -198,7 +199,7 @@ def executors(self) -> list[int]: return [ perm.user_id for perm in self.permissions - if perm.permission_type == "execute" + if perm.permission_type == DeployedRangePermissionType.EXECUTE.value ] From 7b58b3a212b7b8256a11a04ad40c5af28e2d3f8b Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 22:07:40 -0700 Subject: [PATCH 12/19] fix(rwx): better permission checks in crud --- api/src/app/crud/crud_permissions.py | 57 ++++++++++++++++++++++++++++ api/src/app/models/range_models.py | 2 + 2 files changed, 59 insertions(+) diff --git a/api/src/app/crud/crud_permissions.py b/api/src/app/crud/crud_permissions.py index 1bc420a0..7c97379b 100644 --- a/api/src/app/crud/crud_permissions.py +++ b/api/src/app/crud/crud_permissions.py @@ -9,6 +9,7 @@ BlueprintRangePermissionModel, DeployedRangePermissionModel, ) +from ..models.range_models import BlueprintRangeModel, DeployedRangeModel logger = logging.getLogger(__name__) @@ -18,6 +19,7 @@ async def grant_blueprint_permission( blueprint_range_id: int, user_id: int, permission_type: BlueprintPermissionType, + requesting_user_id: int, ) -> BlueprintRangePermissionModel: """Grant permission to a blueprint range. @@ -27,6 +29,7 @@ async def grant_blueprint_permission( blueprint_range_id: ID of blueprint range to grant permission for user_id: ID of user to grant permission to permission_type: Type of permission to grant + requesting_user_id: ID of user requesting to grant permission (must be owner) Returns: ------- @@ -35,8 +38,20 @@ async def grant_blueprint_permission( Raises: ------ SQLAlchemyError: If database operation fails + ValueError: If requesting user is not the owner """ + # Check ownership - only owners can grant permissions + stmt = select(BlueprintRangeModel).where(BlueprintRangeModel.id == blueprint_range_id) + result = await db.execute(stmt) + blueprint_range = result.scalar_one_or_none() + + if not blueprint_range: + raise ValueError(f"Blueprint range {blueprint_range_id} not found") + + if blueprint_range.owner_id != requesting_user_id: + raise ValueError(f"Only the owner can grant permissions on blueprint range {blueprint_range_id}") + permission = BlueprintRangePermissionModel( blueprint_range_id=blueprint_range_id, user_id=user_id, @@ -72,6 +87,7 @@ async def grant_deployed_permission( deployed_range_id: int, user_id: int, permission_type: DeployedRangePermissionType, + requesting_user_id: int, ) -> DeployedRangePermissionModel: """Grant permission to a deployed range. @@ -81,6 +97,7 @@ async def grant_deployed_permission( deployed_range_id: ID of deployed range to grant permission for user_id: ID of user to grant permission to permission_type: Type of permission to grant + requesting_user_id: ID of user requesting to grant permission (must be owner) Returns: ------- @@ -89,8 +106,20 @@ async def grant_deployed_permission( Raises: ------ SQLAlchemyError: If database operation fails + ValueError: If requesting user is not the owner """ + # Check ownership + stmt = select(DeployedRangeModel).where(DeployedRangeModel.id == deployed_range_id) + result = await db.execute(stmt) + deployed_range = result.scalar_one_or_none() + + if not deployed_range: + raise ValueError(f"Deployed range {deployed_range_id} not found") + + if deployed_range.owner_id != requesting_user_id: + raise ValueError(f"User {requesting_user_id} is not the owner of deployed range {deployed_range_id}") + permission = DeployedRangePermissionModel( deployed_range_id=deployed_range_id, user_id=user_id, @@ -126,6 +155,7 @@ async def revoke_blueprint_permission( blueprint_range_id: int, user_id: int, permission_type: BlueprintPermissionType, + requesting_user_id: int, ) -> bool: """Revoke permission from a blueprint range. @@ -135,6 +165,7 @@ async def revoke_blueprint_permission( blueprint_range_id: ID of blueprint range to revoke permission from user_id: ID of user to revoke permission from permission_type: Type of permission to revoke + requesting_user_id: ID of user requesting to revoke permission (must be owner) Returns: ------- @@ -143,8 +174,20 @@ async def revoke_blueprint_permission( Raises: ------ SQLAlchemyError: If database operation fails + ValueError: If requesting user is not the owner """ + # Check ownership + stmt = select(BlueprintRangeModel).where(BlueprintRangeModel.id == blueprint_range_id) + result = await db.execute(stmt) + blueprint_range = result.scalar_one_or_none() + + if not blueprint_range: + raise ValueError(f"Blueprint range {blueprint_range_id} not found") + + if blueprint_range.owner_id != requesting_user_id: + raise ValueError(f"User {requesting_user_id} is not the owner of blueprint range {blueprint_range_id}") + stmt = select(BlueprintRangePermissionModel).where( BlueprintRangePermissionModel.blueprint_range_id == blueprint_range_id, BlueprintRangePermissionModel.user_id == user_id, @@ -189,6 +232,7 @@ async def revoke_deployed_permission( deployed_range_id: int, user_id: int, permission_type: DeployedRangePermissionType, + requesting_user_id: int, ) -> bool: """Revoke permission from a deployed range. @@ -198,6 +242,7 @@ async def revoke_deployed_permission( deployed_range_id: ID of deployed range to revoke permission from user_id: ID of user to revoke permission from permission_type: Type of permission to revoke + requesting_user_id: ID of user requesting to revoke permission (must be owner) Returns: ------- @@ -206,8 +251,20 @@ async def revoke_deployed_permission( Raises: ------ SQLAlchemyError: If database operation fails + ValueError: If requesting user is not the owner """ + # Check ownership + stmt = select(DeployedRangeModel).where(DeployedRangeModel.id == deployed_range_id) + result = await db.execute(stmt) + deployed_range = result.scalar_one_or_none() + + if not deployed_range: + raise ValueError(f"Deployed range {deployed_range_id} not found") + + if deployed_range.owner_id != requesting_user_id: + raise ValueError(f"User {requesting_user_id} is not the owner of deployed range {deployed_range_id}") + stmt = select(DeployedRangePermissionModel).where( DeployedRangePermissionModel.deployed_range_id == deployed_range_id, DeployedRangePermissionModel.user_id == user_id, diff --git a/api/src/app/models/range_models.py b/api/src/app/models/range_models.py index 0397395b..7f82b174 100644 --- a/api/src/app/models/range_models.py +++ b/api/src/app/models/range_models.py @@ -62,6 +62,7 @@ def is_standalone(self) -> bool: return True + # ==================== Deployed (Instances) ===================== @@ -96,3 +97,4 @@ class DeployedRangeModel(Base, OwnableObjectMixin, RangeMixin): cascade="all, delete-orphan", passive_deletes=True, ) + From 6be65793532377864f438d304f3b6a28dadc35df Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 22:29:19 -0700 Subject: [PATCH 13/19] fix(rwx): better permission checks in crud --- api/src/app/crud/crud_ranges.py | 18 +- api/tests/unit/crud/crud_mocks.py | 10 +- api/tests/unit/crud/test_crud_permissions.py | 270 ++++++++++++++----- 3 files changed, 222 insertions(+), 76 deletions(-) diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index 95fb2e24..aa941084 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -28,14 +28,6 @@ logger = logging.getLogger(__name__) -def get_permissions( - model: object, -) -> list[BlueprintRangePermissionModel | DeployedRangePermissionModel]: - """Safely get permissions from a model, handling mocks.""" - permissions = getattr(model, "permissions", []) - if hasattr(permissions, "_mock_name"): - return [] - return permissions or [] def can_read_blueprint( @@ -46,7 +38,7 @@ def can_read_blueprint( return True return any( p.user_id == user_id and p.permission_type in ("read", "write") - for p in get_permissions(range_model) + for p in (range_model.permissions or []) ) @@ -58,7 +50,7 @@ def can_write_blueprint( return True return any( p.user_id == user_id and p.permission_type == "write" - for p in get_permissions(range_model) + for p in (range_model.permissions or []) ) @@ -70,7 +62,7 @@ def can_read_deployed( return True return any( p.user_id == user_id and p.permission_type in ("read", "write", "execute") - for p in get_permissions(range_model) + for p in (range_model.permissions or []) ) @@ -82,7 +74,7 @@ def can_write_deployed( return True return any( p.user_id == user_id and p.permission_type == "write" - for p in get_permissions(range_model) + for p in (range_model.permissions or []) ) @@ -94,7 +86,7 @@ def can_execute_deployed( return True return any( p.user_id == user_id and p.permission_type == "execute" - for p in get_permissions(range_model) + for p in (range_model.permissions or []) ) diff --git a/api/tests/unit/crud/crud_mocks.py b/api/tests/unit/crud/crud_mocks.py index de020629..18f427c4 100644 --- a/api/tests/unit/crud/crud_mocks.py +++ b/api/tests/unit/crud/crud_mocks.py @@ -39,13 +39,19 @@ def is_standalone(self) -> bool: class DummyBlueprintRange(Mock): """Dummy blueprint range model for testing.""" - pass + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.permissions = [] + self.owner_id = 1 # Default owner class DummyDeployedRange(Mock): """Dummy deployed range model for testing.""" - pass + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.permissions = [] + self.owner_id = 1 # Default owner class DummyJob(Mock): diff --git a/api/tests/unit/crud/test_crud_permissions.py b/api/tests/unit/crud/test_crud_permissions.py index a69d2ce7..c3df211f 100644 --- a/api/tests/unit/crud/test_crud_permissions.py +++ b/api/tests/unit/crud/test_crud_permissions.py @@ -20,16 +20,28 @@ # ==================== Grant Permissions ===================== -async def test_grant_blueprint_read_permission() -> None: +async def test_grant_blueprint_read_permission(caplog) -> None: """Test granting read permission to a blueprint range.""" + from src.app.models.range_models import BlueprintRangeModel + from src.app.enums.permissions import BlueprintPermissionType + dummy_db = DummyDB() + + # Mock the ownership check + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result blueprint_range_id = 1 user_id = 2 + requesting_user_id = 1 # Owner - result = await grant_blueprint_permission( - dummy_db, blueprint_range_id, user_id, "read" - ) + with caplog.at_level(logging.DEBUG): + result = await grant_blueprint_permission( + dummy_db, blueprint_range_id, user_id, BlueprintPermissionType.READ, requesting_user_id + ) # Check that permission was added to db dummy_db.add.assert_called_once() @@ -37,7 +49,7 @@ async def test_grant_blueprint_read_permission() -> None: assert isinstance(added_permission, BlueprintRangePermissionModel) assert added_permission.blueprint_range_id == blueprint_range_id assert added_permission.user_id == user_id - assert added_permission.permission_type == "read" + assert added_permission.permission_type == BlueprintPermissionType.READ # Check that db operations were called dummy_db.flush.assert_called_once() @@ -49,33 +61,53 @@ async def test_grant_blueprint_read_permission() -> None: async def test_grant_blueprint_write_permission() -> None: """Test granting write permission to a blueprint range.""" + from src.app.enums.permissions import BlueprintPermissionType + dummy_db = DummyDB() + + # Mock the ownership check + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result blueprint_range_id = 1 user_id = 3 + requesting_user_id = 1 # Owner result = await grant_blueprint_permission( - dummy_db, blueprint_range_id, user_id, "write" + dummy_db, blueprint_range_id, user_id, BlueprintPermissionType.WRITE, requesting_user_id ) # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.blueprint_range_id == blueprint_range_id assert added_permission.user_id == user_id - assert added_permission.permission_type == "write" + assert added_permission.permission_type == BlueprintPermissionType.WRITE assert result == added_permission async def test_grant_deployed_read_permission() -> None: """Test granting read permission to a deployed range.""" + from src.app.enums.permissions import DeployedRangePermissionType + dummy_db = DummyDB() + + # Mock the ownership check + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result deployed_range_id = 1 user_id = 2 + requesting_user_id = 1 # Owner result = await grant_deployed_permission( - dummy_db, deployed_range_id, user_id, "read" + dummy_db, deployed_range_id, user_id, DeployedRangePermissionType.READ, requesting_user_id ) # Check that permission was added to db @@ -84,7 +116,7 @@ async def test_grant_deployed_read_permission() -> None: assert isinstance(added_permission, DeployedRangePermissionModel) assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id - assert added_permission.permission_type == "read" + assert added_permission.permission_type == DeployedRangePermissionType.READ # Check that db operations were called dummy_db.flush.assert_called_once() @@ -96,40 +128,60 @@ async def test_grant_deployed_read_permission() -> None: async def test_grant_deployed_write_permission() -> None: """Test granting write permission to a deployed range.""" + from src.app.enums.permissions import DeployedRangePermissionType + dummy_db = DummyDB() + + # Mock the ownership check + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result deployed_range_id = 1 user_id = 3 + requesting_user_id = 1 # Owner result = await grant_deployed_permission( - dummy_db, deployed_range_id, user_id, "write" + dummy_db, deployed_range_id, user_id, DeployedRangePermissionType.WRITE, requesting_user_id ) # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id - assert added_permission.permission_type == "write" + assert added_permission.permission_type == DeployedRangePermissionType.WRITE assert result == added_permission async def test_grant_deployed_execute_permission() -> None: """Test granting execute permission to a deployed range.""" + from src.app.enums.permissions import DeployedRangePermissionType + dummy_db = DummyDB() + + # Mock the ownership check + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result deployed_range_id = 1 user_id = 4 + requesting_user_id = 1 # Owner result = await grant_deployed_permission( - dummy_db, deployed_range_id, user_id, "execute" + dummy_db, deployed_range_id, user_id, DeployedRangePermissionType.EXECUTE, requesting_user_id ) # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id - assert added_permission.permission_type == "execute" + assert added_permission.permission_type == DeployedRangePermissionType.EXECUTE assert result == added_permission @@ -138,14 +190,25 @@ async def test_grant_blueprint_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that grant_blueprint_permission handles database errors.""" + from src.app.enums.permissions import BlueprintPermissionType + dummy_db = DummyDB() + + # Mock the ownership check + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await grant_blueprint_permission(dummy_db, 1, 2, "read") + await grant_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) # Check that we properly logger.exception() db errors assert any( @@ -160,14 +223,25 @@ async def test_grant_deployed_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that grant_deployed_permission handles database errors.""" + from src.app.enums.permissions import DeployedRangePermissionType + dummy_db = DummyDB() + + # Mock the ownership check + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await grant_deployed_permission(dummy_db, 1, 2, "read") + await grant_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.READ, requesting_user_id) # Check that we properly logger.exception() db errors assert any( @@ -183,21 +257,33 @@ async def test_grant_deployed_permission_db_error( async def test_revoke_blueprint_permission_success() -> None: """Test successfully revoking a blueprint permission.""" + from src.app.enums.permissions import BlueprintPermissionType + dummy_db = DummyDB() - + + # Mock the ownership check first (first call) + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + # Create a mock permission to be found and deleted mock_permission = BlueprintRangePermissionModel( blueprint_range_id=1, user_id=2, - permission_type="read", + permission_type=BlueprintPermissionType.READ, ) - - # Mock the execute result chain - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_permission - dummy_db.execute.return_value = mock_result - - result = await revoke_blueprint_permission(dummy_db, 1, 2, "read") + + # Mock the permission lookup (second call) + mock_permission_result = Mock() + mock_permission_result.scalar_one_or_none.return_value = mock_permission + + # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] + + requesting_user_id = 1 # Owner + + result = await revoke_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) # Check that the permission was deleted dummy_db.delete.assert_called_once_with(mock_permission) @@ -209,14 +295,26 @@ async def test_revoke_blueprint_permission_success() -> None: async def test_revoke_blueprint_permission_not_found() -> None: """Test revoking a blueprint permission that doesn't exist.""" + from src.app.enums.permissions import BlueprintPermissionType + dummy_db = DummyDB() - - # Mock the execute result chain to return None (permission not found) - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = None - dummy_db.execute.return_value = mock_result - - result = await revoke_blueprint_permission(dummy_db, 1, 2, "read") + + # Mock the ownership check first (first call) + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + + # Mock the permission lookup to return None (permission not found) + mock_permission_result = Mock() + mock_permission_result.scalar_one_or_none.return_value = None + + # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] + + requesting_user_id = 1 # Owner + + result = await revoke_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) # Check that delete was not called dummy_db.delete.assert_not_called() @@ -228,21 +326,33 @@ async def test_revoke_blueprint_permission_not_found() -> None: async def test_revoke_deployed_permission_success() -> None: """Test successfully revoking a deployed permission.""" + from src.app.enums.permissions import DeployedRangePermissionType + dummy_db = DummyDB() - + + # Mock the ownership check first (first call) + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + # Create a mock permission to be found and deleted mock_permission = DeployedRangePermissionModel( deployed_range_id=1, user_id=2, - permission_type="execute", + permission_type=DeployedRangePermissionType.EXECUTE, ) - - # Mock the execute result chain - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_permission - dummy_db.execute.return_value = mock_result - - result = await revoke_deployed_permission(dummy_db, 1, 2, "execute") + + # Mock the permission lookup (second call) + mock_permission_result = Mock() + mock_permission_result.scalar_one_or_none.return_value = mock_permission + + # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] + + requesting_user_id = 1 # Owner + + result = await revoke_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id) # Check that the permission was deleted dummy_db.delete.assert_called_once_with(mock_permission) @@ -254,14 +364,26 @@ async def test_revoke_deployed_permission_success() -> None: async def test_revoke_deployed_permission_not_found() -> None: """Test revoking a deployed permission that doesn't exist.""" + from src.app.enums.permissions import DeployedRangePermissionType + dummy_db = DummyDB() - - # Mock the execute result chain to return None (permission not found) - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = None - dummy_db.execute.return_value = mock_result - - result = await revoke_deployed_permission(dummy_db, 1, 2, "write") + + # Mock the ownership check first (first call) + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + + # Mock the permission lookup to return None (permission not found) + mock_permission_result = Mock() + mock_permission_result.scalar_one_or_none.return_value = None + + # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] + + requesting_user_id = 1 # Owner + + result = await revoke_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.WRITE, requesting_user_id) # Check that delete was not called dummy_db.delete.assert_not_called() @@ -275,25 +397,38 @@ async def test_revoke_blueprint_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that revoke_blueprint_permission handles database errors.""" + from src.app.enums.permissions import BlueprintPermissionType + dummy_db = DummyDB() - + + # Mock the ownership check first (first call) + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + # Create a mock permission mock_permission = BlueprintRangePermissionModel( blueprint_range_id=1, user_id=2, - permission_type="read", + permission_type=BlueprintPermissionType.READ, ) - # Mock the execute result chain - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_permission - dummy_db.execute.return_value = mock_result + + # Mock the permission lookup (second call) + mock_permission_result = Mock() + mock_permission_result.scalar_one_or_none.return_value = mock_permission + + # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await revoke_blueprint_permission(dummy_db, 1, 2, "read") + await revoke_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) # Check that we properly logger.exception() db errors assert any( @@ -308,25 +443,38 @@ async def test_revoke_deployed_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that revoke_deployed_permission handles database errors.""" + from src.app.enums.permissions import DeployedRangePermissionType + dummy_db = DummyDB() - + + # Mock the ownership check first (first call) + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + # Create a mock permission mock_permission = DeployedRangePermissionModel( deployed_range_id=1, user_id=2, - permission_type="execute", + permission_type=DeployedRangePermissionType.EXECUTE, ) - # Mock the execute result chain - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_permission - dummy_db.execute.return_value = mock_result + + # Mock the permission lookup (second call) + mock_permission_result = Mock() + mock_permission_result.scalar_one_or_none.return_value = mock_permission + + # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await revoke_deployed_permission(dummy_db, 1, 2, "execute") + await revoke_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id) # Check that we properly logger.exception() db errors assert any( From 80f6bf8d15aed367be7b0043b8281c7814a3c9c9 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Wed, 30 Jul 2025 22:32:10 -0700 Subject: [PATCH 14/19] fix(rwx): improve type hints for range model --- api/src/app/crud/crud_ranges.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index aa941084..fc5d4c3a 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -1,5 +1,4 @@ import logging -from typing import Union from sqlalchemy import select from sqlalchemy.exc import SQLAlchemyError @@ -31,10 +30,10 @@ def can_read_blueprint( - range_model: Union[BlueprintRangeModel, object], user_id: int + range_model: BlueprintRangeModel, user_id: int ) -> bool: """Check if user can read a blueprint range.""" - if getattr(range_model, "owner_id", None) == user_id: + if range_model.owner_id == user_id: return True return any( p.user_id == user_id and p.permission_type in ("read", "write") @@ -43,10 +42,10 @@ def can_read_blueprint( def can_write_blueprint( - range_model: Union[BlueprintRangeModel, object], user_id: int + range_model: BlueprintRangeModel, user_id: int ) -> bool: """Check if user can write a blueprint range.""" - if getattr(range_model, "owner_id", None) == user_id: + if range_model.owner_id == user_id: return True return any( p.user_id == user_id and p.permission_type == "write" @@ -55,10 +54,10 @@ def can_write_blueprint( def can_read_deployed( - range_model: Union[DeployedRangeModel, object], user_id: int + range_model: DeployedRangeModel, user_id: int ) -> bool: """Check if user can read a deployed range.""" - if getattr(range_model, "owner_id", None) == user_id: + if range_model.owner_id == user_id: return True return any( p.user_id == user_id and p.permission_type in ("read", "write", "execute") @@ -67,10 +66,10 @@ def can_read_deployed( def can_write_deployed( - range_model: Union[DeployedRangeModel, object], user_id: int + range_model: DeployedRangeModel, user_id: int ) -> bool: """Check if user can write a deployed range.""" - if getattr(range_model, "owner_id", None) == user_id: + if range_model.owner_id == user_id: return True return any( p.user_id == user_id and p.permission_type == "write" @@ -79,10 +78,10 @@ def can_write_deployed( def can_execute_deployed( - range_model: Union[DeployedRangeModel, object], user_id: int + range_model: DeployedRangeModel, user_id: int ) -> bool: """Check if user can execute a deployed range.""" - if getattr(range_model, "owner_id", None) == user_id: + if range_model.owner_id == user_id: return True return any( p.user_id == user_id and p.permission_type == "execute" From 763a8ce4f90ba2c3472d911d9742cd55887ae6f7 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sat, 2 Aug 2025 13:36:27 -0700 Subject: [PATCH 15/19] fix(rwx): various formatting and doc fixes --- api/src/app/crud/crud_permissions.py | 51 +- api/src/app/crud/crud_ranges.py | 55 +- api/src/app/enums/permissions.py | 29 +- api/src/app/models/permission_models.py | 21 +- api/src/app/models/range_models.py | 2 - api/src/app/schemas/range_schemas.py | 18 +- api/tests/unit/api/v1/config.py | 4 +- api/tests/unit/crud/crud_mocks.py | 6 +- api/tests/unit/crud/test_crud_permissions.py | 221 +++---- api/tests/unit/crud/test_crud_ranges.py | 634 ++++++++++++++++++- 10 files changed, 849 insertions(+), 192 deletions(-) diff --git a/api/src/app/crud/crud_permissions.py b/api/src/app/crud/crud_permissions.py index 7c97379b..cead6e15 100644 --- a/api/src/app/crud/crud_permissions.py +++ b/api/src/app/crud/crud_permissions.py @@ -41,16 +41,19 @@ async def grant_blueprint_permission( ValueError: If requesting user is not the owner """ - # Check ownership - only owners can grant permissions - stmt = select(BlueprintRangeModel).where(BlueprintRangeModel.id == blueprint_range_id) + stmt = select(BlueprintRangeModel).where( + BlueprintRangeModel.id == blueprint_range_id + ) result = await db.execute(stmt) blueprint_range = result.scalar_one_or_none() - + if not blueprint_range: - raise ValueError(f"Blueprint range {blueprint_range_id} not found") - + msg = f"Blueprint range {blueprint_range_id} not found" + raise ValueError(msg) + if blueprint_range.owner_id != requesting_user_id: - raise ValueError(f"Only the owner can grant permissions on blueprint range {blueprint_range_id}") + msg = f"Only the owner can grant permissions on blueprint range {blueprint_range_id}" + raise ValueError(msg) permission = BlueprintRangePermissionModel( blueprint_range_id=blueprint_range_id, @@ -109,16 +112,17 @@ async def grant_deployed_permission( ValueError: If requesting user is not the owner """ - # Check ownership stmt = select(DeployedRangeModel).where(DeployedRangeModel.id == deployed_range_id) result = await db.execute(stmt) deployed_range = result.scalar_one_or_none() - + if not deployed_range: - raise ValueError(f"Deployed range {deployed_range_id} not found") - + msg = f"Deployed range {deployed_range_id} not found" + raise ValueError(msg) + if deployed_range.owner_id != requesting_user_id: - raise ValueError(f"User {requesting_user_id} is not the owner of deployed range {deployed_range_id}") + msg = f"User {requesting_user_id} is not the owner of deployed range {deployed_range_id}" + raise ValueError(msg) permission = DeployedRangePermissionModel( deployed_range_id=deployed_range_id, @@ -178,15 +182,19 @@ async def revoke_blueprint_permission( """ # Check ownership - stmt = select(BlueprintRangeModel).where(BlueprintRangeModel.id == blueprint_range_id) + stmt = select(BlueprintRangeModel).where( + BlueprintRangeModel.id == blueprint_range_id + ) result = await db.execute(stmt) blueprint_range = result.scalar_one_or_none() - + if not blueprint_range: - raise ValueError(f"Blueprint range {blueprint_range_id} not found") - + msg = f"Blueprint range {blueprint_range_id} not found" + raise ValueError(msg) + if blueprint_range.owner_id != requesting_user_id: - raise ValueError(f"User {requesting_user_id} is not the owner of blueprint range {blueprint_range_id}") + msg = f"User {requesting_user_id} is not the owner of blueprint range {blueprint_range_id}" + raise ValueError(msg) stmt = select(BlueprintRangePermissionModel).where( BlueprintRangePermissionModel.blueprint_range_id == blueprint_range_id, @@ -254,16 +262,17 @@ async def revoke_deployed_permission( ValueError: If requesting user is not the owner """ - # Check ownership stmt = select(DeployedRangeModel).where(DeployedRangeModel.id == deployed_range_id) result = await db.execute(stmt) deployed_range = result.scalar_one_or_none() - + if not deployed_range: - raise ValueError(f"Deployed range {deployed_range_id} not found") - + msg = f"Deployed range {deployed_range_id} not found" + raise ValueError(msg) + if deployed_range.owner_id != requesting_user_id: - raise ValueError(f"User {requesting_user_id} is not the owner of deployed range {deployed_range_id}") + msg = f"User {requesting_user_id} is not the owner of deployed range {deployed_range_id}" + raise ValueError(msg) stmt = select(DeployedRangePermissionModel).where( DeployedRangePermissionModel.deployed_range_id == deployed_range_id, diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index fc5d4c3a..848cdd0d 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -27,12 +27,24 @@ logger = logging.getLogger(__name__) +def can_read_blueprint(range_model: BlueprintRangeModel, user_id: int) -> bool: + """Check if user can read a blueprint range. + Users can read a blueprint range if they: + 1. Are the owner (owners have all permissions by default) + 2. Have explicit read permission + 3. Have write permission (write includes read access) -def can_read_blueprint( - range_model: BlueprintRangeModel, user_id: int -) -> bool: - """Check if user can read a blueprint range.""" + Args: + ---- + range_model: The blueprint range to check access for + user_id: ID of user requesting access + + Returns: + ------- + bool: True if user can read the range, False otherwise + + """ if range_model.owner_id == user_id: return True return any( @@ -41,10 +53,15 @@ def can_read_blueprint( ) -def can_write_blueprint( - range_model: BlueprintRangeModel, user_id: int -) -> bool: - """Check if user can write a blueprint range.""" +def can_write_blueprint(range_model: BlueprintRangeModel, user_id: int) -> bool: + """Check if user can write a blueprint range. + + Users can write a blueprint range if they: + 1. Are the owner (owners have all permissions by default) + 2. Have explicit write permission + + Note: Write permission allows modifying blueprint configuration and deploying it. + """ if range_model.owner_id == user_id: return True return any( @@ -53,9 +70,7 @@ def can_write_blueprint( ) -def can_read_deployed( - range_model: DeployedRangeModel, user_id: int -) -> bool: +def can_read_deployed(range_model: DeployedRangeModel, user_id: int) -> bool: """Check if user can read a deployed range.""" if range_model.owner_id == user_id: return True @@ -65,9 +80,7 @@ def can_read_deployed( ) -def can_write_deployed( - range_model: DeployedRangeModel, user_id: int -) -> bool: +def can_write_deployed(range_model: DeployedRangeModel, user_id: int) -> bool: """Check if user can write a deployed range.""" if range_model.owner_id == user_id: return True @@ -77,10 +90,16 @@ def can_write_deployed( ) -def can_execute_deployed( - range_model: DeployedRangeModel, user_id: int -) -> bool: - """Check if user can execute a deployed range.""" +def can_execute_deployed(range_model: DeployedRangeModel, user_id: int) -> bool: + """Check if user can execute a deployed range. + + Users can execute a deployed range if they: + 1. Are the owner (owners have all permissions by default) + 2. Have explicit execute permission + + Note: Execute permission allows accessing SSH keys and interacting with the + live infrastructure, but does not grant read or write permissions. + """ if range_model.owner_id == user_id: return True return any( diff --git a/api/src/app/enums/permissions.py b/api/src/app/enums/permissions.py index 06933d9c..b3c59802 100644 --- a/api/src/app/enums/permissions.py +++ b/api/src/app/enums/permissions.py @@ -2,7 +2,15 @@ class BlueprintPermissionType(str, Enum): - """Permission types for blueprint ranges.""" + """Permission types for blueprint ranges. + + READ: Allows user to view the blueprint range configuration and details. + Users can see the blueprint in lists and access its properties. + + WRITE: Allows user to modify the blueprint range configuration and deploy it. + Users can edit the blueprint, update its settings, and create deployed + ranges from it. Write permission includes read permission. + """ READ = "read" WRITE = "write" @@ -14,7 +22,21 @@ def values(cls) -> str: class DeployedRangePermissionType(str, Enum): - """Permission types for deployed ranges.""" + """Permission types for deployed ranges. + + READ: Allows user to view the deployed range status and configuration. + Users can see the range in lists, check its state, and view properties + but cannot interact with or modify it. + + WRITE: Allows user to modify deployed range settings and manage its lifecycle. + Users can start/stop the range, update configurations, and delete the + deployed range. Write permission includes read permission. + + EXECUTE: Allows user to interact with the live deployed range infrastructure. + Users can access SSH keys, connect to VMs, and interact with the + running environment. Execute permission includes read permission but + not write permission. + """ READ = "read" WRITE = "write" @@ -23,4 +45,5 @@ class DeployedRangePermissionType(str, Enum): @classmethod def values(cls) -> str: """Return formatted tuple string of all permission values.""" - return f"({', '.join(repr(t.value) for t in cls)})" \ No newline at end of file + return f"({', '.join(repr(t.value) for t in cls)})" + diff --git a/api/src/app/models/permission_models.py b/api/src/app/models/permission_models.py index 98f1cdd5..85d60a75 100644 --- a/api/src/app/models/permission_models.py +++ b/api/src/app/models/permission_models.py @@ -1,5 +1,11 @@ from sqlalchemy import BigInteger, CheckConstraint, ForeignKey, String, UniqueConstraint -from sqlalchemy.orm import Mapped, MappedAsDataclass, mapped_column, relationship, declared_attr +from sqlalchemy.orm import ( + Mapped, + MappedAsDataclass, + declared_attr, + mapped_column, + relationship, +) from ..core.db.database import Base from ..enums.permissions import BlueprintPermissionType, DeployedRangePermissionType @@ -27,8 +33,9 @@ class PermissionMixin(MappedAsDataclass): ) @declared_attr - def user(cls): - return relationship("UserModel") + def user(self) -> relationship: + """User relationship.""" + return relationship("UserModel", init=False) class BlueprintRangePermissionModel(Base, PermissionMixin): @@ -36,17 +43,13 @@ class BlueprintRangePermissionModel(Base, PermissionMixin): __tablename__ = "blueprint_range_permissions" - # The blueprint range being shared blueprint_range_id: Mapped[int] = mapped_column( BigInteger, ForeignKey("blueprint_ranges.id", ondelete="CASCADE"), nullable=False, ) - # Relationships blueprint_range = relationship("BlueprintRangeModel", back_populates="permissions") - - # Constraints __table_args__ = ( UniqueConstraint( "blueprint_range_id", @@ -66,17 +69,13 @@ class DeployedRangePermissionModel(Base, PermissionMixin): __tablename__ = "deployed_range_permissions" - # The deployed range being shared deployed_range_id: Mapped[int] = mapped_column( BigInteger, ForeignKey("deployed_ranges.id", ondelete="CASCADE"), nullable=False, ) - # Relationships deployed_range = relationship("DeployedRangeModel", back_populates="permissions") - - # Constraints __table_args__ = ( UniqueConstraint( "deployed_range_id", diff --git a/api/src/app/models/range_models.py b/api/src/app/models/range_models.py index 7f82b174..0397395b 100644 --- a/api/src/app/models/range_models.py +++ b/api/src/app/models/range_models.py @@ -62,7 +62,6 @@ def is_standalone(self) -> bool: return True - # ==================== Deployed (Instances) ===================== @@ -97,4 +96,3 @@ class DeployedRangeModel(Base, OwnableObjectMixin, RangeMixin): cascade="all, delete-orphan", passive_deletes=True, ) - diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index fc7b091b..75b089c5 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -52,13 +52,16 @@ class BlueprintRangeBaseSchema(RangeCommonSchema): description="Description of blueprint range.", examples=["This is my test range."], ) + @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == BlueprintPermissionType.READ.value + perm.user_id + for perm in self.permissions + if perm.permission_type == BlueprintPermissionType.READ.value ] @computed_field @@ -67,7 +70,9 @@ def writers(self) -> list[int]: if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == BlueprintPermissionType.WRITE.value + perm.user_id + for perm in self.permissions + if perm.permission_type == BlueprintPermissionType.WRITE.value ] @@ -173,13 +178,16 @@ class DeployedRangeBaseSchema(RangeCommonSchema): min_length=1, description="SSH private key for the range.", ) + @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == DeployedRangePermissionType.READ.value + perm.user_id + for perm in self.permissions + if perm.permission_type == DeployedRangePermissionType.READ.value ] @computed_field @@ -188,7 +196,9 @@ def writers(self) -> list[int]: if not hasattr(self, "permissions"): return [] return [ - perm.user_id for perm in self.permissions if perm.permission_type == DeployedRangePermissionType.WRITE.value + perm.user_id + for perm in self.permissions + if perm.permission_type == DeployedRangePermissionType.WRITE.value ] @computed_field diff --git a/api/tests/unit/api/v1/config.py b/api/tests/unit/api/v1/config.py index 9c7743cc..ad104bd3 100644 --- a/api/tests/unit/api/v1/config.py +++ b/api/tests/unit/api/v1/config.py @@ -4,7 +4,7 @@ from src.app.enums.job_status import OpenLabsJobStatus # Import all common test data -from tests.common.api.v1.config import * # noqa: F403, F401 +from tests.common.api.v1.config import * # noqa: F403 # Override BASE_ROUTE for unit tests (common uses dynamic route) BASE_ROUTE = "/api/v1" @@ -146,4 +146,4 @@ ) valid_blueprint_subnet_multi_create_payload: dict[str, Any] = copy.deepcopy( valid_blueprint_vpc_multi_create_payload["subnets"][0] -) \ No newline at end of file +) diff --git a/api/tests/unit/crud/crud_mocks.py b/api/tests/unit/crud/crud_mocks.py index 18f427c4..b35c5dd4 100644 --- a/api/tests/unit/crud/crud_mocks.py +++ b/api/tests/unit/crud/crud_mocks.py @@ -39,7 +39,8 @@ def is_standalone(self) -> bool: class DummyBlueprintRange(Mock): """Dummy blueprint range model for testing.""" - def __init__(self, *args, **kwargs): + def __init__(self, *args: object, **kwargs: object) -> None: + """Initialize dummy blueprint range.""" super().__init__(*args, **kwargs) self.permissions = [] self.owner_id = 1 # Default owner @@ -48,7 +49,8 @@ def __init__(self, *args, **kwargs): class DummyDeployedRange(Mock): """Dummy deployed range model for testing.""" - def __init__(self, *args, **kwargs): + def __init__(self, *args: object, **kwargs: object) -> None: + """Initialize dummy deployed range.""" super().__init__(*args, **kwargs) self.permissions = [] self.owner_id = 1 # Default owner diff --git a/api/tests/unit/crud/test_crud_permissions.py b/api/tests/unit/crud/test_crud_permissions.py index c3df211f..fe5300cc 100644 --- a/api/tests/unit/crud/test_crud_permissions.py +++ b/api/tests/unit/crud/test_crud_permissions.py @@ -10,6 +10,10 @@ revoke_blueprint_permission, revoke_deployed_permission, ) +from src.app.enums.permissions import ( + BlueprintPermissionType, + DeployedRangePermissionType, +) from src.app.models.permission_models import ( BlueprintRangePermissionModel, DeployedRangePermissionModel, @@ -20,14 +24,12 @@ # ==================== Grant Permissions ===================== -async def test_grant_blueprint_read_permission(caplog) -> None: +async def test_grant_blueprint_read_permission( + caplog: pytest.LogCaptureFixture, +) -> None: """Test granting read permission to a blueprint range.""" - from src.app.models.range_models import BlueprintRangeModel - from src.app.enums.permissions import BlueprintPermissionType - dummy_db = DummyDB() - - # Mock the ownership check + mock_range = Mock() mock_range.owner_id = 1 mock_result = Mock() @@ -40,10 +42,13 @@ async def test_grant_blueprint_read_permission(caplog) -> None: with caplog.at_level(logging.DEBUG): result = await grant_blueprint_permission( - dummy_db, blueprint_range_id, user_id, BlueprintPermissionType.READ, requesting_user_id + dummy_db, + blueprint_range_id, + user_id, + BlueprintPermissionType.READ, + requesting_user_id, ) - # Check that permission was added to db dummy_db.add.assert_called_once() added_permission = dummy_db.add.call_args[0][0] assert isinstance(added_permission, BlueprintRangePermissionModel) @@ -51,21 +56,16 @@ async def test_grant_blueprint_read_permission(caplog) -> None: assert added_permission.user_id == user_id assert added_permission.permission_type == BlueprintPermissionType.READ - # Check that db operations were called dummy_db.flush.assert_called_once() dummy_db.refresh.assert_called_once_with(added_permission) - # Check return value assert result == added_permission async def test_grant_blueprint_write_permission() -> None: """Test granting write permission to a blueprint range.""" - from src.app.enums.permissions import BlueprintPermissionType - dummy_db = DummyDB() - - # Mock the ownership check + mock_range = Mock() mock_range.owner_id = 1 mock_result = Mock() @@ -77,10 +77,13 @@ async def test_grant_blueprint_write_permission() -> None: requesting_user_id = 1 # Owner result = await grant_blueprint_permission( - dummy_db, blueprint_range_id, user_id, BlueprintPermissionType.WRITE, requesting_user_id + dummy_db, + blueprint_range_id, + user_id, + BlueprintPermissionType.WRITE, + requesting_user_id, ) - # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.blueprint_range_id == blueprint_range_id assert added_permission.user_id == user_id @@ -91,11 +94,8 @@ async def test_grant_blueprint_write_permission() -> None: async def test_grant_deployed_read_permission() -> None: """Test granting read permission to a deployed range.""" - from src.app.enums.permissions import DeployedRangePermissionType - dummy_db = DummyDB() - - # Mock the ownership check + mock_range = Mock() mock_range.owner_id = 1 mock_result = Mock() @@ -107,10 +107,13 @@ async def test_grant_deployed_read_permission() -> None: requesting_user_id = 1 # Owner result = await grant_deployed_permission( - dummy_db, deployed_range_id, user_id, DeployedRangePermissionType.READ, requesting_user_id + dummy_db, + deployed_range_id, + user_id, + DeployedRangePermissionType.READ, + requesting_user_id, ) - # Check that permission was added to db dummy_db.add.assert_called_once() added_permission = dummy_db.add.call_args[0][0] assert isinstance(added_permission, DeployedRangePermissionModel) @@ -118,21 +121,16 @@ async def test_grant_deployed_read_permission() -> None: assert added_permission.user_id == user_id assert added_permission.permission_type == DeployedRangePermissionType.READ - # Check that db operations were called dummy_db.flush.assert_called_once() dummy_db.refresh.assert_called_once_with(added_permission) - # Check return value assert result == added_permission async def test_grant_deployed_write_permission() -> None: """Test granting write permission to a deployed range.""" - from src.app.enums.permissions import DeployedRangePermissionType - dummy_db = DummyDB() - - # Mock the ownership check + mock_range = Mock() mock_range.owner_id = 1 mock_result = Mock() @@ -144,10 +142,13 @@ async def test_grant_deployed_write_permission() -> None: requesting_user_id = 1 # Owner result = await grant_deployed_permission( - dummy_db, deployed_range_id, user_id, DeployedRangePermissionType.WRITE, requesting_user_id + dummy_db, + deployed_range_id, + user_id, + DeployedRangePermissionType.WRITE, + requesting_user_id, ) - # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id @@ -158,11 +159,8 @@ async def test_grant_deployed_write_permission() -> None: async def test_grant_deployed_execute_permission() -> None: """Test granting execute permission to a deployed range.""" - from src.app.enums.permissions import DeployedRangePermissionType - dummy_db = DummyDB() - - # Mock the ownership check + mock_range = Mock() mock_range.owner_id = 1 mock_result = Mock() @@ -174,10 +172,13 @@ async def test_grant_deployed_execute_permission() -> None: requesting_user_id = 1 # Owner result = await grant_deployed_permission( - dummy_db, deployed_range_id, user_id, DeployedRangePermissionType.EXECUTE, requesting_user_id + dummy_db, + deployed_range_id, + user_id, + DeployedRangePermissionType.EXECUTE, + requesting_user_id, ) - # Check that permission was created correctly added_permission = dummy_db.add.call_args[0][0] assert added_permission.deployed_range_id == deployed_range_id assert added_permission.user_id == user_id @@ -190,11 +191,8 @@ async def test_grant_blueprint_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that grant_blueprint_permission handles database errors.""" - from src.app.enums.permissions import BlueprintPermissionType - dummy_db = DummyDB() - - # Mock the ownership check + mock_range = Mock() mock_range.owner_id = 1 mock_result = Mock() @@ -204,13 +202,14 @@ async def test_grant_blueprint_permission_db_error( # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await grant_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) + await grant_blueprint_permission( + dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id + ) - # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR and record.exc_info is not None @@ -223,11 +222,8 @@ async def test_grant_deployed_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that grant_deployed_permission handles database errors.""" - from src.app.enums.permissions import DeployedRangePermissionType - dummy_db = DummyDB() - - # Mock the ownership check + mock_range = Mock() mock_range.owner_id = 1 mock_result = Mock() @@ -237,13 +233,14 @@ async def test_grant_deployed_permission_db_error( # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await grant_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.READ, requesting_user_id) + await grant_deployed_permission( + dummy_db, 1, 2, DeployedRangePermissionType.READ, requesting_user_id + ) - # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR and record.exc_info is not None @@ -257,139 +254,117 @@ async def test_grant_deployed_permission_db_error( async def test_revoke_blueprint_permission_success() -> None: """Test successfully revoking a blueprint permission.""" - from src.app.enums.permissions import BlueprintPermissionType - dummy_db = DummyDB() - - # Mock the ownership check first (first call) + mock_range = Mock() mock_range.owner_id = 1 mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - - # Create a mock permission to be found and deleted + mock_permission = BlueprintRangePermissionModel( blueprint_range_id=1, user_id=2, permission_type=BlueprintPermissionType.READ, ) - - # Mock the permission lookup (second call) + mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = mock_permission - - # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - + requesting_user_id = 1 # Owner - result = await revoke_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) + result = await revoke_blueprint_permission( + dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id + ) - # Check that the permission was deleted dummy_db.delete.assert_called_once_with(mock_permission) dummy_db.flush.assert_called_once() - # Check return value assert result is True async def test_revoke_blueprint_permission_not_found() -> None: """Test revoking a blueprint permission that doesn't exist.""" - from src.app.enums.permissions import BlueprintPermissionType - dummy_db = DummyDB() - - # Mock the ownership check first (first call) + mock_range = Mock() mock_range.owner_id = 1 mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - - # Mock the permission lookup to return None (permission not found) + mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = None - - # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - + requesting_user_id = 1 # Owner - result = await revoke_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) + result = await revoke_blueprint_permission( + dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id + ) - # Check that delete was not called dummy_db.delete.assert_not_called() dummy_db.flush.assert_not_called() - # Check return value assert result is False async def test_revoke_deployed_permission_success() -> None: """Test successfully revoking a deployed permission.""" - from src.app.enums.permissions import DeployedRangePermissionType - dummy_db = DummyDB() - - # Mock the ownership check first (first call) + mock_range = Mock() mock_range.owner_id = 1 mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - - # Create a mock permission to be found and deleted + mock_permission = DeployedRangePermissionModel( deployed_range_id=1, user_id=2, permission_type=DeployedRangePermissionType.EXECUTE, ) - - # Mock the permission lookup (second call) + mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = mock_permission - - # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - + requesting_user_id = 1 # Owner - result = await revoke_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id) + result = await revoke_deployed_permission( + dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id + ) - # Check that the permission was deleted dummy_db.delete.assert_called_once_with(mock_permission) dummy_db.flush.assert_called_once() - # Check return value assert result is True async def test_revoke_deployed_permission_not_found() -> None: """Test revoking a deployed permission that doesn't exist.""" - from src.app.enums.permissions import DeployedRangePermissionType - dummy_db = DummyDB() - - # Mock the ownership check first (first call) + mock_range = Mock() mock_range.owner_id = 1 mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - - # Mock the permission lookup to return None (permission not found) + mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = None - - # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - + requesting_user_id = 1 # Owner - result = await revoke_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.WRITE, requesting_user_id) + result = await revoke_deployed_permission( + dummy_db, 1, 2, DeployedRangePermissionType.WRITE, requesting_user_id + ) - # Check that delete was not called dummy_db.delete.assert_not_called() dummy_db.flush.assert_not_called() - # Check return value assert result is False @@ -397,40 +372,35 @@ async def test_revoke_blueprint_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that revoke_blueprint_permission handles database errors.""" - from src.app.enums.permissions import BlueprintPermissionType - dummy_db = DummyDB() - - # Mock the ownership check first (first call) + mock_range = Mock() mock_range.owner_id = 1 mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - - # Create a mock permission + mock_permission = BlueprintRangePermissionModel( blueprint_range_id=1, user_id=2, permission_type=BlueprintPermissionType.READ, ) - - # Mock the permission lookup (second call) + mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = mock_permission - - # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await revoke_blueprint_permission(dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id) + await revoke_blueprint_permission( + dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id + ) - # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR and record.exc_info is not None @@ -443,40 +413,35 @@ async def test_revoke_deployed_permission_db_error( caplog: pytest.LogCaptureFixture, ) -> None: """Test that revoke_deployed_permission handles database errors.""" - from src.app.enums.permissions import DeployedRangePermissionType - dummy_db = DummyDB() - - # Mock the ownership check first (first call) + mock_range = Mock() mock_range.owner_id = 1 mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - - # Create a mock permission + mock_permission = DeployedRangePermissionModel( deployed_range_id=1, user_id=2, permission_type=DeployedRangePermissionType.EXECUTE, ) - - # Mock the permission lookup (second call) + mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = mock_permission - - # Mock the execute method to return different results for different calls + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] # Force a db exception test_except_msg = "Fake DB error!" dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - + requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await revoke_deployed_permission(dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id) + await revoke_deployed_permission( + dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id + ) - # Check that we properly logger.exception() db errors assert any( record.levelno == logging.ERROR and record.exc_info is not None diff --git a/api/tests/unit/crud/test_crud_ranges.py b/api/tests/unit/crud/test_crud_ranges.py index cba08b30..27c7e25e 100644 --- a/api/tests/unit/crud/test_crud_ranges.py +++ b/api/tests/unit/crud/test_crud_ranges.py @@ -1,6 +1,6 @@ import logging import random -from unittest.mock import MagicMock +from unittest.mock import MagicMock, Mock import pytest from pytest_mock import MockerFixture @@ -9,6 +9,11 @@ from src.app.crud.crud_ranges import ( build_blueprint_range_models, build_deployed_range_models, + can_execute_deployed, + can_read_blueprint, + can_read_deployed, + can_write_blueprint, + can_write_deployed, create_blueprint_range, create_deployed_range, delete_blueprint_range, @@ -763,3 +768,630 @@ async def test_delete_deployed_range_raises_generic_errors( and test_except_msg in record.message for record in caplog.records ) + + +# ==================== Permission Helper Function Tests ===================== + + +class MockPermission(Mock): + """Mock permission object for testing.""" + + def __init__( + self, user_id: int, permission_type: str, *args: object, **kwargs: object + ) -> None: + """Initialize mock permission.""" + super().__init__(*args, **kwargs) + self.user_id = user_id + self.permission_type = permission_type + + +class TestBlueprintPermissionHelpers: + """Test blueprint range permission helper functions.""" + + def test_can_read_blueprint_owner_access(self) -> None: + """Owner can always read blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 1 + dummy_range.owner_id = user_id + dummy_range.permissions = [] + + assert can_read_blueprint(dummy_range, user_id) + + def test_can_read_blueprint_explicit_read_permission(self) -> None: + """User with explicit read permission can read blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + assert can_read_blueprint(dummy_range, user_id) + + def test_can_read_blueprint_write_implies_read(self) -> None: + """User with write permission can also read blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + assert can_read_blueprint(dummy_range, user_id) + + def test_can_read_blueprint_no_permission(self) -> None: + """User without permissions cannot read blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] + + assert not can_read_blueprint(dummy_range, user_id) + + def test_can_read_blueprint_wrong_user_permission(self) -> None: + """User cannot read with permission granted to another user.""" + dummy_range = DummyBlueprintRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(3, "read")] # Permission for user 3 + + assert not can_read_blueprint(dummy_range, user_id) + + def test_can_write_blueprint_owner_access(self) -> None: + """Owner can always write blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 1 + dummy_range.owner_id = user_id + dummy_range.permissions = [] + + assert can_write_blueprint(dummy_range, user_id) + + def test_can_write_blueprint_explicit_write_permission(self) -> None: + """User with explicit write permission can write blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + assert can_write_blueprint(dummy_range, user_id) + + def test_can_write_blueprint_read_does_not_imply_write(self) -> None: + """User with only read permission cannot write blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + assert not can_write_blueprint(dummy_range, user_id) + + def test_can_write_blueprint_no_permission(self) -> None: + """User without permissions cannot write blueprint ranges.""" + dummy_range = DummyBlueprintRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] + + assert not can_write_blueprint(dummy_range, user_id) + + +class TestDeployedRangePermissionHelpers: + """Test deployed range permission helper functions.""" + + def test_can_read_deployed_owner_access(self) -> None: + """Owner can always read deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 1 + dummy_range.owner_id = user_id + dummy_range.permissions = [] + + assert can_read_deployed(dummy_range, user_id) + + def test_can_read_deployed_explicit_read_permission(self) -> None: + """User with explicit read permission can read deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + assert can_read_deployed(dummy_range, user_id) + + def test_can_read_deployed_write_implies_read(self) -> None: + """User with write permission can also read deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + assert can_read_deployed(dummy_range, user_id) + + def test_can_read_deployed_execute_implies_read(self) -> None: + """User with execute permission can also read deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "execute")] + + assert can_read_deployed(dummy_range, user_id) + + def test_can_read_deployed_no_permission(self) -> None: + """User without permissions cannot read deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] + + assert not can_read_deployed(dummy_range, user_id) + + def test_can_write_deployed_owner_access(self) -> None: + """Owner can always write deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 1 + dummy_range.owner_id = user_id + dummy_range.permissions = [] + + assert can_write_deployed(dummy_range, user_id) + + def test_can_write_deployed_explicit_write_permission(self) -> None: + """User with explicit write permission can write deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + assert can_write_deployed(dummy_range, user_id) + + def test_can_write_deployed_read_does_not_imply_write(self) -> None: + """User with only read permission cannot write deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + assert not can_write_deployed(dummy_range, user_id) + + def test_can_write_deployed_execute_does_not_imply_write(self) -> None: + """User with only execute permission cannot write deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "execute")] + + assert not can_write_deployed(dummy_range, user_id) + + def test_can_write_deployed_no_permission(self) -> None: + """User without permissions cannot write deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] + + assert not can_write_deployed(dummy_range, user_id) + + def test_can_execute_deployed_owner_access(self) -> None: + """Owner can always execute deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 1 + dummy_range.owner_id = user_id + dummy_range.permissions = [] + + assert can_execute_deployed(dummy_range, user_id) + + def test_can_execute_deployed_explicit_execute_permission(self) -> None: + """User with explicit execute permission can execute deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "execute")] + + assert can_execute_deployed(dummy_range, user_id) + + def test_can_execute_deployed_read_does_not_imply_execute(self) -> None: + """User with only read permission cannot execute deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + assert not can_execute_deployed(dummy_range, user_id) + + def test_can_execute_deployed_write_does_not_imply_execute(self) -> None: + """User with only write permission cannot execute deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + assert not can_execute_deployed(dummy_range, user_id) + + def test_can_execute_deployed_no_permission(self) -> None: + """User without permissions cannot execute deployed ranges.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] + + assert not can_execute_deployed(dummy_range, user_id) + + def test_permission_inheritance_multiple_permissions(self) -> None: + """User with multiple permissions should have all access rights.""" + dummy_range = DummyDeployedRange() + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [ + MockPermission(user_id, "read"), + MockPermission(user_id, "write"), + MockPermission(user_id, "execute"), + ] + + assert can_read_deployed(dummy_range, user_id) + assert can_write_deployed(dummy_range, user_id) + assert can_execute_deployed(dummy_range, user_id) + + +# ==================== CRUD Integration Permission Tests ===================== + + +class TestBlueprintRangeCRUDPermissions: + """Test blueprint range CRUD operations with permissions.""" + + async def test_get_blueprint_range_with_read_permission( + self, mocker: MockerFixture + ) -> None: + """User with read permission can access blueprint ranges.""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + BlueprintRangeSchema, "model_validate", return_value=dummy_range + ) + + result = await get_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + + async def test_get_blueprint_range_with_write_permission( + self, mocker: MockerFixture + ) -> None: + """User with write permission can access blueprint ranges (write implies read).""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + BlueprintRangeSchema, "model_validate", return_value=dummy_range + ) + + result = await get_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + + async def test_get_blueprint_range_denied_no_permission(self) -> None: + """User without permission cannot access blueprint ranges.""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + + result = await get_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + + async def test_delete_blueprint_range_with_write_permission( + self, mocker: MockerFixture + ) -> None: + """User with write permission can delete blueprint ranges.""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + BlueprintRangeHeaderSchema, "model_validate", return_value=dummy_range + ) + + result = await delete_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + dummy_db.delete.assert_called_once() + dummy_db.flush.assert_called_once() + + async def test_delete_blueprint_range_denied_read_only_permission(self) -> None: + """User with only read permission cannot delete blueprint ranges.""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] # Read only + + dummy_db.get.return_value = dummy_range + + result = await delete_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + dummy_db.delete.assert_not_called() + + async def test_delete_blueprint_range_denied_no_permission(self) -> None: + """User without permission cannot delete blueprint ranges.""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + + result = await delete_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + dummy_db.delete.assert_not_called() + + +class TestDeployedRangeCRUDPermissions: + """Test deployed range CRUD operations with permissions.""" + + async def test_get_deployed_range_with_read_permission( + self, mocker: MockerFixture + ) -> None: + """User with read permission can access deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + DeployedRangeSchema, "model_validate", return_value=dummy_range + ) + + result = await get_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + + async def test_get_deployed_range_with_write_permission( + self, mocker: MockerFixture + ) -> None: + """User with write permission can access deployed ranges (write implies read).""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + DeployedRangeSchema, "model_validate", return_value=dummy_range + ) + + result = await get_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + + async def test_get_deployed_range_with_execute_permission( + self, mocker: MockerFixture + ) -> None: + """User with execute permission can access deployed ranges (execute implies read).""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "execute")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + DeployedRangeSchema, "model_validate", return_value=dummy_range + ) + + result = await get_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + + async def test_get_deployed_range_denied_no_permission(self) -> None: + """User without permission cannot access deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + + result = await get_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + + async def test_delete_deployed_range_with_write_permission( + self, mocker: MockerFixture + ) -> None: + """User with write permission can delete deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + DeployedRangeHeaderSchema, "model_validate", return_value=dummy_range + ) + + result = await delete_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + dummy_db.delete.assert_called_once() + dummy_db.flush.assert_called_once() + + async def test_delete_deployed_range_denied_read_only_permission(self) -> None: + """User with only read permission cannot delete deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] # Read only + + dummy_db.get.return_value = dummy_range + + result = await delete_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + dummy_db.delete.assert_not_called() + + async def test_delete_deployed_range_denied_execute_only_permission(self) -> None: + """User with only execute permission cannot delete deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "execute")] # Execute only + + dummy_db.get.return_value = dummy_range + + result = await delete_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + dummy_db.delete.assert_not_called() + + async def test_delete_deployed_range_denied_no_permission(self) -> None: + """User without permission cannot delete deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + + result = await delete_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + dummy_db.delete.assert_not_called() + + async def test_get_deployed_range_key_with_execute_permission(self) -> None: + """User with execute permission can access SSH keys.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "execute")] + + dummy_db.get.return_value = dummy_range + dummy_db.scalar.return_value = "fake_private_key" + + result = await get_deployed_range_key( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + assert result.range_private_key == "fake_private_key" + + async def test_get_deployed_range_key_denied_read_only_permission(self) -> None: + """User with only read permission cannot access SSH keys.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] # Read only + + dummy_db.get.return_value = dummy_range + dummy_db.scalar.return_value = "fake_private_key" + + result = await get_deployed_range_key( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + + async def test_get_deployed_range_key_denied_write_only_permission(self) -> None: + """User with only write permission cannot access SSH keys.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] # Write only + + dummy_db.get.return_value = dummy_range + dummy_db.scalar.return_value = "fake_private_key" + + result = await get_deployed_range_key( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + + async def test_get_deployed_range_key_denied_no_permission(self) -> None: + """User without permission cannot access SSH keys.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + dummy_db.scalar.return_value = "fake_private_key" + + result = await get_deployed_range_key( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None From ff934c93e676a36d8e088f82efe921f2d7b77381 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sun, 3 Aug 2025 00:59:24 -0700 Subject: [PATCH 16/19] fix(rwx): styling and type fixes --- api/src/app/crud/crud_permissions.py | 8 +- api/src/app/crud/crud_ranges.py | 89 ++---- api/src/app/enums/permissions.py | 1 - api/src/app/models/permission_models.py | 4 +- api/tests/unit/api/v1/config.py | 269 ++++++++++++------ api/tests/unit/crud/crud_mocks.py | 8 +- api/tests/unit/crud/test_admin_permissions.py | 142 +++++++++ api/tests/unit/crud/test_crud_ranges.py | 14 +- .../unit/crud/test_permission_isolation.py | 149 ++++++++++ 9 files changed, 523 insertions(+), 161 deletions(-) create mode 100644 api/tests/unit/crud/test_admin_permissions.py create mode 100644 api/tests/unit/crud/test_permission_isolation.py diff --git a/api/src/app/crud/crud_permissions.py b/api/src/app/crud/crud_permissions.py index cead6e15..bf66621d 100644 --- a/api/src/app/crud/crud_permissions.py +++ b/api/src/app/crud/crud_permissions.py @@ -196,12 +196,12 @@ async def revoke_blueprint_permission( msg = f"User {requesting_user_id} is not the owner of blueprint range {blueprint_range_id}" raise ValueError(msg) - stmt = select(BlueprintRangePermissionModel).where( + permission_stmt = select(BlueprintRangePermissionModel).where( BlueprintRangePermissionModel.blueprint_range_id == blueprint_range_id, BlueprintRangePermissionModel.user_id == user_id, BlueprintRangePermissionModel.permission_type == permission_type, ) - result = await db.execute(stmt) + result = await db.execute(permission_stmt) permission = result.scalar_one_or_none() if not permission: @@ -274,12 +274,12 @@ async def revoke_deployed_permission( msg = f"User {requesting_user_id} is not the owner of deployed range {deployed_range_id}" raise ValueError(msg) - stmt = select(DeployedRangePermissionModel).where( + permission_stmt = select(DeployedRangePermissionModel).where( DeployedRangePermissionModel.deployed_range_id == deployed_range_id, DeployedRangePermissionModel.user_id == user_id, DeployedRangePermissionModel.permission_type == permission_type, ) - result = await db.execute(stmt) + result = await db.execute(permission_stmt) permission = result.scalar_one_or_none() if not permission: diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index 848cdd0d..73c8b1f2 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -11,6 +11,7 @@ ) from ..models.range_models import BlueprintRangeModel, DeployedRangeModel from ..models.subnet_models import BlueprintSubnetModel, DeployedSubnetModel +from ..models.user_model import UserModel from ..models.vpc_models import BlueprintVPCModel, DeployedVPCModel from ..schemas.range_schemas import ( BlueprintRangeCreateSchema, @@ -21,66 +22,56 @@ DeployedRangeKeySchema, DeployedRangeSchema, ) -from .crud_permissions import grant_blueprint_permission, grant_deployed_permission from .crud_vpcs import build_blueprint_vpc_models, build_deployed_vpc_models logger = logging.getLogger(__name__) -def can_read_blueprint(range_model: BlueprintRangeModel, user_id: int) -> bool: - """Check if user can read a blueprint range. - - Users can read a blueprint range if they: - 1. Are the owner (owners have all permissions by default) - 2. Have explicit read permission - 3. Have write permission (write includes read access) - - Args: - ---- - range_model: The blueprint range to check access for - user_id: ID of user requesting access - - Returns: - ------- - bool: True if user can read the range, False otherwise - - """ +def can_read_blueprint( + range_model: BlueprintRangeModel, user_id: int, user: UserModel | None = None +) -> bool: + """Check if user can read a blueprint range.""" if range_model.owner_id == user_id: return True + if user and hasattr(user, "is_admin") and user.is_admin: + return True return any( p.user_id == user_id and p.permission_type in ("read", "write") for p in (range_model.permissions or []) ) -def can_write_blueprint(range_model: BlueprintRangeModel, user_id: int) -> bool: - """Check if user can write a blueprint range. - - Users can write a blueprint range if they: - 1. Are the owner (owners have all permissions by default) - 2. Have explicit write permission - - Note: Write permission allows modifying blueprint configuration and deploying it. - """ +def can_write_blueprint( + range_model: BlueprintRangeModel, user_id: int, user: UserModel | None = None +) -> bool: + """Check if user can write a blueprint range.""" if range_model.owner_id == user_id: return True + if user and hasattr(user, "is_admin") and user.is_admin: + return True return any( p.user_id == user_id and p.permission_type == "write" for p in (range_model.permissions or []) ) -def can_read_deployed(range_model: DeployedRangeModel, user_id: int) -> bool: +def can_read_deployed( + range_model: DeployedRangeModel, user_id: int, user: UserModel | None = None +) -> bool: """Check if user can read a deployed range.""" if range_model.owner_id == user_id: return True + if user and hasattr(user, "is_admin") and user.is_admin: + return True return any( - p.user_id == user_id and p.permission_type in ("read", "write", "execute") + p.user_id == user_id and p.permission_type in ("read", "write") for p in (range_model.permissions or []) ) -def can_write_deployed(range_model: DeployedRangeModel, user_id: int) -> bool: +def can_write_deployed( + range_model: DeployedRangeModel, user_id: int, user: UserModel | None = None +) -> bool: """Check if user can write a deployed range.""" if range_model.owner_id == user_id: return True @@ -90,16 +81,10 @@ def can_write_deployed(range_model: DeployedRangeModel, user_id: int) -> bool: ) -def can_execute_deployed(range_model: DeployedRangeModel, user_id: int) -> bool: - """Check if user can execute a deployed range. - - Users can execute a deployed range if they: - 1. Are the owner (owners have all permissions by default) - 2. Have explicit execute permission - - Note: Execute permission allows accessing SSH keys and interacting with the - live infrastructure, but does not grant read or write permissions. - """ +def can_execute_deployed( + range_model: DeployedRangeModel, user_id: int, user: UserModel | None = None +) -> bool: + """Check if user can execute a deployed range.""" if range_model.owner_id == user_id: return True return any( @@ -295,14 +280,6 @@ async def create_blueprint_range( user_id, ) - for reader_id in blueprint.readers: - if reader_id != user_id: - await grant_blueprint_permission(db, range_model.id, reader_id, "read") - - for writer_id in blueprint.writers: - if writer_id != user_id: - await grant_blueprint_permission(db, range_model.id, writer_id, "write") - except SQLAlchemyError as e: logger.exception( "Database error while flushing range blueprint to database session for user: %s. Exception: %s.", @@ -635,20 +612,6 @@ async def create_deployed_range( user_id, ) - for reader_id in range_schema.readers: - if reader_id != user_id: - await grant_deployed_permission(db, range_model.id, reader_id, "read") - - for writer_id in range_schema.writers: - if writer_id != user_id: - await grant_deployed_permission(db, range_model.id, writer_id, "write") - - for executor_id in range_schema.executors: - if executor_id != user_id: - await grant_deployed_permission( - db, range_model.id, executor_id, "execute" - ) - except SQLAlchemyError as e: logger.exception( "Database error while flushing deployed range to database session for user: %s. Exception: %s.", diff --git a/api/src/app/enums/permissions.py b/api/src/app/enums/permissions.py index b3c59802..bb151b3a 100644 --- a/api/src/app/enums/permissions.py +++ b/api/src/app/enums/permissions.py @@ -46,4 +46,3 @@ class DeployedRangePermissionType(str, Enum): def values(cls) -> str: """Return formatted tuple string of all permission values.""" return f"({', '.join(repr(t.value) for t in cls)})" - diff --git a/api/src/app/models/permission_models.py b/api/src/app/models/permission_models.py index 85d60a75..48ac3b2b 100644 --- a/api/src/app/models/permission_models.py +++ b/api/src/app/models/permission_models.py @@ -32,8 +32,8 @@ class PermissionMixin(MappedAsDataclass): nullable=False, ) - @declared_attr - def user(self) -> relationship: + @declared_attr # type: ignore[arg-type] + def user(self) -> object: """User relationship.""" return relationship("UserModel", init=False) diff --git a/api/tests/unit/api/v1/config.py b/api/tests/unit/api/v1/config.py index ad104bd3..ec933477 100644 --- a/api/tests/unit/api/v1/config.py +++ b/api/tests/unit/api/v1/config.py @@ -1,14 +1,199 @@ import copy +import random +from datetime import datetime, timezone from typing import Any from src.app.enums.job_status import OpenLabsJobStatus +from src.app.enums.providers import OpenLabsProvider +from src.app.enums.range_states import RangeState +from src.app.enums.regions import OpenLabsRegion -# Import all common test data -from tests.common.api.v1.config import * # noqa: F403 - -# Override BASE_ROUTE for unit tests (common uses dynamic route) +# Base route BASE_ROUTE = "/api/v1" + +# ============================== +# Deployed Payloads +# ============================== + +valid_deployed_range_header_data: dict[str, Any] = { + "id": random.randint(1, 100), # noqa: S311 + "name": "Fake Deployed Range", + "description": "Description of fake deployed range for testing.", + "date": datetime.now(tz=timezone.utc), + "state": RangeState.ON, + "region": OpenLabsRegion.US_EAST_1, + "provider": OpenLabsProvider.AWS, +} + +valid_deployed_range_data: dict[str, Any] = { + "id": 999, + "vpcs": [ + { + "id": 10, + "name": "production-vpc-main", + "cidr": "10.100.0.0/16", + "resource_id": "vpc-abc123xyz789", + "subnets": [ + { + "id": 101, + "name": "prod-subnet-web-a", + "cidr": "10.100.1.0/24", + "resource_id": "subnet-def456uvw123", + "hosts": [ + { + "id": 1001, + "hostname": "webserver-01", + "os": "debian_11", + "spec": "small", + "size": 30, + "tags": ["web", "frontend", "nginx"], + "resource_id": "i-hostabc111", + "ip_address": "10.100.1.10", + }, + { + "id": 1002, + "hostname": "webserver-02", + "os": "debian_11", + "spec": "small", + "size": 30, + "tags": ["web", "frontend", "nginx"], + "resource_id": "i-hostabc222", + "ip_address": "10.100.1.11", + }, + ], + }, + { + "id": 102, + "name": "prod-subnet-app-a", + "cidr": "10.100.2.0/24", + "resource_id": "subnet-ghi789rst456", + "hosts": [ + { + "id": 1003, + "hostname": "appserver-01", + "os": "ubuntu_22", + "spec": "medium", + "size": 50, + "tags": ["app", "backend", "api"], + "resource_id": "i-hostdef333", + "ip_address": "10.100.2.20", + } + ], + }, + { + "id": 103, + "name": "prod-subnet-db-a", + "cidr": "10.100.3.0/24", + "resource_id": "subnet-jkl012pqr789", + "hosts": [ + { + "id": 1004, + "hostname": "dbserver-01", + "os": "windows_2022", + "spec": "large", + "size": 80, + "tags": ["database", "sql", "critical"], + "resource_id": "i-hostghi444", + "ip_address": "10.100.3.30", + } + ], + }, + ], + }, + { + "id": 20, + "name": "staging-vpc", + "cidr": "10.200.0.0/16", + "resource_id": "vpc-lmn456opq012", + "subnets": [ + { + "id": 201, + "name": "staging-subnet-main", + "cidr": "10.200.1.0/24", + "resource_id": "subnet-rst789uvw345", + "hosts": [ + { + "id": 2001, + "hostname": "staging-app-01", + "os": "kali", + "spec": "medium", + "size": 60, + "tags": ["staging", "pentest"], + "resource_id": "i-hostjkl555", + "ip_address": "10.200.1.15", + } + ], + } + ], + }, + ], + "description": "Comprehensive deployed test range with corrected schema attributes.", + "date": "2025-06-03T10:00:00Z", + "readme": "# Test Deployed Range: Version 2\n\nThis data uses the updated and correct schema definitions for all nested objects.\n\n## Environment Overview:\n- **Production VPC**: Contains web, app, and database subnets.\n- **Staging VPC**: Contains a general-purpose subnet for testing.\n", + "state_file": { + "simplified_mock_state": "Test data placeholder - not a real Terraform state." + }, + "state": "on", + "region": "us_east_1", + "jumpbox_resource_id": "i-jumpbox789012", + "jumpbox_public_ip": "203.0.113.25", + "range_private_key": "-----BEGIN OPENSSH PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDIfSjVkaDRtKNO\n... (rest of a mock SSH private key) ...\nhNUC8ZLe06edaNBX6N2jS9Wp3mk3JNGxQjagtrh9TGUrscedop4hCQABAoGALKe1\n-----END OPENSSH PRIVATE KEY-----", + "provider": "aws", + "name": "openlabs-deployed-test-v2", + "vnc": True, + "vpn": False, +} + +valid_deployed_vpc_data: dict[str, Any] = copy.deepcopy( + valid_deployed_range_data["vpcs"][0] +) + +valid_deployed_subnet_data: dict[str, Any] = copy.deepcopy( + valid_deployed_vpc_data["subnets"][0] +) + +valid_deployed_host_data: dict[str, Any] = copy.deepcopy( + valid_deployed_subnet_data["hosts"][0] +) + +valid_range_private_key_data: dict[str, Any] = { + "range_private_key": "-----BEGIN OPENSSH PRIVATE KEY-----\nMIIEvAIBADANBgkqhkiG9w0BAQEFAASCBKYwggSiAgEAAoIBAQDIfSjVkaDRtKNO\n... (rest of a mock SSH private key) ...\nhNUC8ZLe06edaNBX6N2jS9Wp3mk3JNGxQjagtrh9TGUrscedop4hCQABAoGALKe1\n-----END OPENSSH PRIVATE KEY-----", +} + +# ============================== +# User/Auth Payloads +# ============================== + +# Test user credentials +base_user_register_payload = { + "email": "test@ufsit.club", + "password": "password123", + "name": "Test User", +} + +base_user_login_payload = copy.deepcopy(base_user_register_payload) +base_user_login_payload.pop("name") + +# Test data for password update +password_update_payload = { + "current_password": "password123", + "new_password": "newpassword123", +} + +# Test data for AWS secrets +aws_secrets_payload = { + "aws_access_key": "AKIAIOSFODNN7EXAMPLE", + "aws_secret_key": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", +} + +# Test data for Azure secrets +azure_secrets_payload = { + "azure_client_id": "00000000-0000-0000-0000-000000000000", + "azure_client_secret": "example-client-secret-value", + "azure_tenant_id": "00000000-0000-0000-0000-000000000000", + "azure_subscription_id": "00000000-0000-0000-0000-000000000000", +} # ============================== # Job Payloads # ============================== @@ -71,79 +256,3 @@ } failed_job_payload = copy.deepcopy(complete_job_payload) failed_job_payload.update(failed_job_updates) - -# Override specific payloads that need unit-test specific values -valid_blueprint_range_multi_create_payload: dict[str, Any] = { - "vpcs": [ - { - "cidr": "10.0.0.0/16", - "name": "dev-vpc", # Unit tests use simpler names without spaces - "subnets": [ - { - "cidr": "10.0.1.0/24", - "name": "dev-subnet-web", - "hosts": [ - { - "hostname": "dev-web-01", - "os": "ubuntu_22", - "spec": "medium", - "size": 20, - "tags": ["web", "frontend", "ubuntu"], - }, - { - "hostname": "dev-db-01", - "os": "suse_15", - "spec": "large", - "size": 50, - "tags": ["database", "backend", "rocky"], - }, - ], - }, - { - "cidr": "10.0.2.0/24", - "name": "dev-subnet-app", - "hosts": [ - { - "hostname": "dev-app-01", - "os": "debian_11", - "spec": "medium", - "size": 30, - "tags": ["app", "linux"], - } - ], - }, - ], - }, - { - "cidr": "172.16.0.0/16", - "name": "prod-vpc", - "subnets": [ - { - "cidr": "172.16.1.0/24", - "name": "prod-subnet-dmz", # Unit tests use simpler names without spaces - "hosts": [ - { - "hostname": "prod-gateway-01", - "os": "kali", - "spec": "small", - "size": 32, - "tags": ["gateway", "security"], - } - ], - } - ], - }, - ], - "description": "Multi-VPC, Multi-Subnet, Multi-Host test blueprint for OpenLabs.", - "provider": "aws", - "name": "multi-env-test-range", - "vnc": True, - "vpn": True, -} - -valid_blueprint_vpc_multi_create_payload: dict[str, Any] = copy.deepcopy( - valid_blueprint_range_multi_create_payload["vpcs"][0] -) -valid_blueprint_subnet_multi_create_payload: dict[str, Any] = copy.deepcopy( - valid_blueprint_vpc_multi_create_payload["subnets"][0] -) diff --git a/api/tests/unit/crud/crud_mocks.py b/api/tests/unit/crud/crud_mocks.py index b35c5dd4..19e95e1d 100644 --- a/api/tests/unit/crud/crud_mocks.py +++ b/api/tests/unit/crud/crud_mocks.py @@ -39,21 +39,21 @@ def is_standalone(self) -> bool: class DummyBlueprintRange(Mock): """Dummy blueprint range model for testing.""" - def __init__(self, *args: object, **kwargs: object) -> None: + def __init__(self, *args: Any, **kwargs: dict[str, Any]) -> None: # noqa: ANN401 """Initialize dummy blueprint range.""" super().__init__(*args, **kwargs) self.permissions = [] - self.owner_id = 1 # Default owner + self.owner_id = 1 class DummyDeployedRange(Mock): """Dummy deployed range model for testing.""" - def __init__(self, *args: object, **kwargs: object) -> None: + def __init__(self, *args: Any, **kwargs: dict[str, Any]) -> None: # noqa: ANN401 """Initialize dummy deployed range.""" super().__init__(*args, **kwargs) self.permissions = [] - self.owner_id = 1 # Default owner + self.owner_id = 1 class DummyJob(Mock): diff --git a/api/tests/unit/crud/test_admin_permissions.py b/api/tests/unit/crud/test_admin_permissions.py new file mode 100644 index 00000000..000df153 --- /dev/null +++ b/api/tests/unit/crud/test_admin_permissions.py @@ -0,0 +1,142 @@ +from unittest.mock import Mock + +from src.app.crud.crud_ranges import ( + can_execute_deployed, + can_read_blueprint, + can_read_deployed, + can_write_blueprint, + can_write_deployed, +) +from src.app.enums.permissions import ( + DeployedRangePermissionType, +) + + +class TestAdminPermissions: + """Test admin permission behavior per end-to-end encryption model.""" + + def test_admin_has_full_blueprint_access(self) -> None: + """Test that admins can read and write any blueprint (not encrypted).""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + # Admin user (different from owner) + admin_user = Mock() + admin_user.id = 2 + admin_user.is_admin = True + + # Admin should have full access to blueprints + assert can_read_blueprint(range_model, admin_user.id, admin_user) is True + assert can_write_blueprint(range_model, admin_user.id, admin_user) is True + + def test_admin_has_read_only_deployed_access(self) -> None: + """Test that admins can only read deployed ranges (due to encryption).""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + # Admin user (different from owner) + admin_user = Mock() + admin_user.id = 2 + admin_user.is_admin = True + + # Admin should only have read access to deployed ranges + assert can_read_deployed(range_model, admin_user.id, admin_user) is True + assert can_write_deployed(range_model, admin_user.id, admin_user) is False + assert can_execute_deployed(range_model, admin_user.id, admin_user) is False + + def test_admin_permissions_override_no_explicit_grants(self) -> None: + """Test admin permissions work even without explicit permission grants.""" + blueprint_range = Mock() + blueprint_range.owner_id = 1 + blueprint_range.permissions = [] + + deployed_range = Mock() + deployed_range.owner_id = 1 + deployed_range.permissions = [] + + admin_user = Mock() + admin_user.id = 3 + admin_user.is_admin = True + + # Admin should override lack of explicit permissions + assert can_read_blueprint(blueprint_range, admin_user.id, admin_user) is True + assert can_write_blueprint(blueprint_range, admin_user.id, admin_user) is True + assert can_read_deployed(deployed_range, admin_user.id, admin_user) is True + assert can_write_deployed(deployed_range, admin_user.id, admin_user) is False + assert can_execute_deployed(deployed_range, admin_user.id, admin_user) is False + + def test_non_admin_follows_normal_permission_rules(self) -> None: + """Test that non-admin users still follow normal permission rules.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + regular_user = Mock() + regular_user.id = 2 + regular_user.is_admin = False + + # Regular user should have no access without explicit permissions + assert can_read_blueprint(range_model, regular_user.id, regular_user) is False + assert can_write_blueprint(range_model, regular_user.id, regular_user) is False + assert can_read_deployed(range_model, regular_user.id, regular_user) is False + assert can_write_deployed(range_model, regular_user.id, regular_user) is False + assert can_execute_deployed(range_model, regular_user.id, regular_user) is False + + def test_owner_still_has_full_access_regardless_of_admin_status(self) -> None: + """Test that owners always have full access regardless of admin status.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + # Owner who is not admin + owner_user = Mock() + owner_user.id = 1 + owner_user.is_admin = False + + # Owner should have full access regardless of admin status + assert can_read_blueprint(range_model, owner_user.id, owner_user) is True + assert can_write_blueprint(range_model, owner_user.id, owner_user) is True + assert can_read_deployed(range_model, owner_user.id, owner_user) is True + assert can_write_deployed(range_model, owner_user.id, owner_user) is True + assert can_execute_deployed(range_model, owner_user.id, owner_user) is True + + def test_admin_and_owner_same_user(self) -> None: + """Test behavior when user is both admin and owner.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + admin_owner = Mock() + admin_owner.id = 1 + admin_owner.is_admin = True + + # Should have full access (owner privileges take precedence) + assert can_read_blueprint(range_model, admin_owner.id, admin_owner) is True + assert can_write_blueprint(range_model, admin_owner.id, admin_owner) is True + assert can_read_deployed(range_model, admin_owner.id, admin_owner) is True + assert can_write_deployed(range_model, admin_owner.id, admin_owner) is True + assert can_execute_deployed(range_model, admin_owner.id, admin_owner) is True + + def test_encryption_boundary_prevents_admin_deployed_modifications(self) -> None: + """Test that encryption boundary prevents admin modifications to deployed ranges.""" + deployed_range = Mock() + deployed_range.owner_id = 1 + deployed_range.permissions = [ + Mock(user_id=3, permission_type=DeployedRangePermissionType.WRITE.value), + Mock(user_id=4, permission_type=DeployedRangePermissionType.EXECUTE.value), + ] + + admin_user = Mock() + admin_user.id = 2 + admin_user.is_admin = True + + # Admin can read but cannot write/execute due to encryption + assert can_read_deployed(deployed_range, admin_user.id, admin_user) is True + assert can_write_deployed(deployed_range, admin_user.id, admin_user) is False + assert can_execute_deployed(deployed_range, admin_user.id, admin_user) is False + + # Regular users with explicit permissions should still work + assert can_write_deployed(deployed_range, 3) is True + assert can_execute_deployed(deployed_range, 4) is True diff --git a/api/tests/unit/crud/test_crud_ranges.py b/api/tests/unit/crud/test_crud_ranges.py index 27c7e25e..5a48cd56 100644 --- a/api/tests/unit/crud/test_crud_ranges.py +++ b/api/tests/unit/crud/test_crud_ranges.py @@ -900,14 +900,14 @@ def test_can_read_deployed_write_implies_read(self) -> None: assert can_read_deployed(dummy_range, user_id) - def test_can_read_deployed_execute_implies_read(self) -> None: - """User with execute permission can also read deployed ranges.""" + def test_can_read_deployed_execute_isolated(self) -> None: + """User with execute permission cannot read deployed ranges (execute is isolated).""" dummy_range = DummyDeployedRange() user_id = 2 dummy_range.owner_id = 1 # Different owner dummy_range.permissions = [MockPermission(user_id, "execute")] - assert can_read_deployed(dummy_range, user_id) + assert not can_read_deployed(dummy_range, user_id) def test_can_read_deployed_no_permission(self) -> None: """User without permissions cannot read deployed ranges.""" @@ -1204,10 +1204,10 @@ async def test_get_deployed_range_with_write_permission( assert result is not None mock_model_validate.assert_called_once() - async def test_get_deployed_range_with_execute_permission( + async def test_get_deployed_range_with_execute_permission_denied( self, mocker: MockerFixture ) -> None: - """User with execute permission can access deployed ranges (execute implies read).""" + """User with execute permission cannot access deployed ranges (execute is isolated).""" dummy_db = DummyDB() dummy_range = DummyDeployedRange() @@ -1224,8 +1224,8 @@ async def test_get_deployed_range_with_execute_permission( dummy_db, range_id=1, user_id=user_id, is_admin=False ) - assert result is not None - mock_model_validate.assert_called_once() + assert result is None + mock_model_validate.assert_not_called() async def test_get_deployed_range_denied_no_permission(self) -> None: """User without permission cannot access deployed ranges.""" diff --git a/api/tests/unit/crud/test_permission_isolation.py b/api/tests/unit/crud/test_permission_isolation.py new file mode 100644 index 00000000..e2cf80a7 --- /dev/null +++ b/api/tests/unit/crud/test_permission_isolation.py @@ -0,0 +1,149 @@ +from unittest.mock import Mock + +from src.app.crud.crud_ranges import ( + can_execute_deployed, + can_read_blueprint, + can_read_deployed, + can_write_blueprint, + can_write_deployed, +) +from src.app.enums.permissions import ( + BlueprintPermissionType, + DeployedRangePermissionType, +) + + +class TestPermissionIsolation: + """Test that permissions are properly isolated and don't grant unintended access.""" + + def test_blueprint_read_permission_isolation(self) -> None: + """Test that blueprint read permission only grants read access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=2, permission_type=BlueprintPermissionType.READ.value) + ] + + assert can_read_blueprint(range_model, 2) is True + assert can_write_blueprint(range_model, 2) is False + + def test_blueprint_write_permission_includes_read(self) -> None: + """Test that blueprint write permission includes read access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=2, permission_type=BlueprintPermissionType.WRITE.value) + ] + + assert can_read_blueprint(range_model, 2) is True + assert can_write_blueprint(range_model, 2) is True + + def test_deployed_read_permission_isolation(self) -> None: + """Test that deployed read permission only grants read access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.READ.value) + ] + + assert can_read_deployed(range_model, 2) is True + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is False + + def test_deployed_write_permission_includes_read(self) -> None: + """Test that deployed write permission includes read access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.WRITE.value) + ] + + assert can_read_deployed(range_model, 2) is True + assert can_write_deployed(range_model, 2) is True + assert can_execute_deployed(range_model, 2) is False + + def test_deployed_execute_permission_isolation(self) -> None: + """Test that deployed execute permission is completely isolated.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.EXECUTE.value) + ] + + assert can_read_deployed(range_model, 2) is False + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is True + + def test_no_permission_denies_all_access(self) -> None: + """Test that users with no permissions are denied all access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + assert can_read_deployed(range_model, 2) is False + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is False + + def test_wrong_user_permission_denies_access(self) -> None: + """Test that permissions for other users don't grant access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=3, permission_type=DeployedRangePermissionType.READ.value) + ] + + assert can_read_deployed(range_model, 2) is False + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is False + + def test_owner_always_has_all_permissions(self) -> None: + """Test that owners always have all permissions regardless of explicit grants.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + assert can_read_deployed(range_model, 1) is True + assert can_write_deployed(range_model, 1) is True + assert can_execute_deployed(range_model, 1) is True + + def test_multiple_permissions_work_correctly(self) -> None: + """Test that users with multiple permissions get all granted access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.READ.value), + Mock(user_id=2, permission_type=DeployedRangePermissionType.EXECUTE.value), + ] + + assert can_read_deployed(range_model, 2) is True + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is True + + def test_permission_boundaries_are_strict(self) -> None: + """Test edge cases to ensure permission boundaries are strictly enforced.""" + range_model = Mock() + range_model.owner_id = 1 + + # User with only execute should not get read/write + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.EXECUTE.value) + ] + assert can_read_deployed(range_model, 2) is False + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is True + + # User with only read should not get write/execute + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.READ.value) + ] + assert can_read_deployed(range_model, 2) is True + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is False + + # User with only write should get read but not execute + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.WRITE.value) + ] + assert can_read_deployed(range_model, 2) is True + assert can_write_deployed(range_model, 2) is True + assert can_execute_deployed(range_model, 2) is False From d482741f683dcfebc3ba95918651748e7f342e60 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sun, 3 Aug 2025 10:08:19 -0700 Subject: [PATCH 17/19] fix(rwx): clean up redundant tests --- api/src/app/crud/crud_ranges.py | 3 +- api/src/app/schemas/range_schemas.py | 25 +- api/tests/common/api/v1/config.py | 5 - .../common/api/v1/test_blueprint_ranges.py | 15 +- .../common/api/v1/test_yaml_middleware.py | 8 +- api/tests/conftest.py | 2 + api/tests/unit/crud/crud_mocks.py | 4 +- api/tests/unit/crud/test_admin_permissions.py | 142 ---------- api/tests/unit/crud/test_crud_ranges.py | 252 +----------------- .../unit/crud/test_permission_isolation.py | 149 ----------- api/tests/unit/crud/test_permissions.py | 211 +++++++++++++++ 11 files changed, 255 insertions(+), 561 deletions(-) delete mode 100644 api/tests/unit/crud/test_admin_permissions.py delete mode 100644 api/tests/unit/crud/test_permission_isolation.py create mode 100644 api/tests/unit/crud/test_permissions.py diff --git a/api/src/app/crud/crud_ranges.py b/api/src/app/crud/crud_ranges.py index 73c8b1f2..e95efb09 100644 --- a/api/src/app/crud/crud_ranges.py +++ b/api/src/app/crud/crud_ranges.py @@ -64,7 +64,7 @@ def can_read_deployed( if user and hasattr(user, "is_admin") and user.is_admin: return True return any( - p.user_id == user_id and p.permission_type in ("read", "write") + p.user_id == user_id and p.permission_type in ("read", "write", "execute") for p in (range_model.permissions or []) ) @@ -279,7 +279,6 @@ async def create_blueprint_range( range_model.id, user_id, ) - except SQLAlchemyError as e: logger.exception( "Database error while flushing range blueprint to database session for user: %s. Exception: %s.", diff --git a/api/src/app/schemas/range_schemas.py b/api/src/app/schemas/range_schemas.py index 75b089c5..c1703ff7 100644 --- a/api/src/app/schemas/range_schemas.py +++ b/api/src/app/schemas/range_schemas.py @@ -56,22 +56,24 @@ class BlueprintRangeBaseSchema(RangeCommonSchema): @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" - if not hasattr(self, "permissions"): + permissions = getattr(self, "permissions", None) + if not permissions: return [] return [ perm.user_id - for perm in self.permissions + for perm in permissions if perm.permission_type == BlueprintPermissionType.READ.value ] @computed_field def writers(self) -> list[int]: """Get list of user IDs with write access.""" - if not hasattr(self, "permissions"): + permissions = getattr(self, "permissions", None) + if not permissions: return [] return [ perm.user_id - for perm in self.permissions + for perm in permissions if perm.permission_type == BlueprintPermissionType.WRITE.value ] @@ -182,33 +184,36 @@ class DeployedRangeBaseSchema(RangeCommonSchema): @computed_field def readers(self) -> list[int]: """Get list of user IDs with read access.""" - if not hasattr(self, "permissions"): + permissions = getattr(self, "permissions", None) + if not permissions: return [] return [ perm.user_id - for perm in self.permissions + for perm in permissions if perm.permission_type == DeployedRangePermissionType.READ.value ] @computed_field def writers(self) -> list[int]: """Get list of user IDs with write access.""" - if not hasattr(self, "permissions"): + permissions = getattr(self, "permissions", None) + if not permissions: return [] return [ perm.user_id - for perm in self.permissions + for perm in permissions if perm.permission_type == DeployedRangePermissionType.WRITE.value ] @computed_field def executors(self) -> list[int]: """Get list of user IDs with execute access.""" - if not hasattr(self, "permissions"): + permissions = getattr(self, "permissions", None) + if not permissions: return [] return [ perm.user_id - for perm in self.permissions + for perm in permissions if perm.permission_type == DeployedRangePermissionType.EXECUTE.value ] diff --git a/api/tests/common/api/v1/config.py b/api/tests/common/api/v1/config.py index 95d89b61..e6e9821f 100644 --- a/api/tests/common/api/v1/config.py +++ b/api/tests/common/api/v1/config.py @@ -71,8 +71,6 @@ "name": "example-range-1", "vnc": False, "vpn": False, - "readers": [], - "writers": [], } # Valid range payload for deployment @@ -299,9 +297,6 @@ "name": "openlabs-deployed-test-v2", "vnc": True, "vpn": False, - "readers": [], - "writers": [], - "executors": [], } valid_deployed_vpc_data: dict[str, Any] = copy.deepcopy( diff --git a/api/tests/common/api/v1/test_blueprint_ranges.py b/api/tests/common/api/v1/test_blueprint_ranges.py index 5c5df9e6..77a63fe5 100644 --- a/api/tests/common/api/v1/test_blueprint_ranges.py +++ b/api/tests/common/api/v1/test_blueprint_ranges.py @@ -245,7 +245,11 @@ async def test_blueprint_range_get_range( # Check response data recieved_blueprint = response.json() remove_key_recursively(recieved_blueprint, "id") # Our payload doesn't have IDs - assert recieved_blueprint == valid_blueprint_range_create_payload + # Add expected computed permission fields for comparison + expected_payload = copy.deepcopy(valid_blueprint_range_create_payload) + expected_payload["readers"] = [] + expected_payload["writers"] = [] + assert recieved_blueprint == expected_payload async def test_blueprint_range_get_nonexistent_range( self, @@ -529,7 +533,11 @@ async def test_blueprint_range_add_remove_user_flow( remove_key_recursively( recieved_range, "id" ) # Our creation payload doesn't have IDs - assert recieved_range == valid_blueprint_range_create_payload + # Add expected computed permission fields for comparison + expected_payload = copy.deepcopy(valid_blueprint_range_create_payload) + expected_payload["readers"] = [] + expected_payload["writers"] = [] + assert recieved_range == expected_payload # Attempt to list out all of user's blueprints response = await auth_api_client.get(f"{BASE_ROUTE}/blueprints/ranges") @@ -593,6 +601,9 @@ async def test_blueprint_range_get_non_empty_list( # Mimic the header response with the valid JSON valid_blueprint_copy = copy.deepcopy(valid_blueprint_range_create_payload) del valid_blueprint_copy["vpcs"] + # Add expected computed permission fields + valid_blueprint_copy["readers"] = [] + valid_blueprint_copy["writers"] = [] remove_key_recursively( recieved_range, "id" ) # Our creation payload doesn't have IDs diff --git a/api/tests/common/api/v1/test_yaml_middleware.py b/api/tests/common/api/v1/test_yaml_middleware.py index 6072c7d3..3801c059 100644 --- a/api/tests/common/api/v1/test_yaml_middleware.py +++ b/api/tests/common/api/v1/test_yaml_middleware.py @@ -1,3 +1,5 @@ +import copy + import pytest import yaml from fastapi import status @@ -47,7 +49,11 @@ async def test_blueprint_range_yaml_to_json( # Validate response recieved_range = response.json() remove_key_recursively(recieved_range, "id") - assert recieved_range == valid_blueprint_range_create_payload + # Add expected computed permission fields for comparison + expected_payload = copy.deepcopy(valid_blueprint_range_create_payload) + expected_payload["readers"] = [] + expected_payload["writers"] = [] + assert recieved_range == expected_payload async def test_blueprint_vpc_yaml_to_json( self, auth_api_client: AsyncClient diff --git a/api/tests/conftest.py b/api/tests/conftest.py index 4601f9c9..c2216a97 100644 --- a/api/tests/conftest.py +++ b/api/tests/conftest.py @@ -25,6 +25,8 @@ from testcontainers.compose import DockerCompose from testcontainers.postgres import PostgresContainer +# Import all models to ensure they're registered with SQLAlchemy metadata +import src.app.models # noqa: F401 from src.app.core.config import settings from src.app.core.db.database import Base, async_get_db from src.app.enums.job_status import OpenLabsJobStatus diff --git a/api/tests/unit/crud/crud_mocks.py b/api/tests/unit/crud/crud_mocks.py index 19e95e1d..ce24cac6 100644 --- a/api/tests/unit/crud/crud_mocks.py +++ b/api/tests/unit/crud/crud_mocks.py @@ -42,7 +42,7 @@ class DummyBlueprintRange(Mock): def __init__(self, *args: Any, **kwargs: dict[str, Any]) -> None: # noqa: ANN401 """Initialize dummy blueprint range.""" super().__init__(*args, **kwargs) - self.permissions = [] + self.permissions: list[Any] = [] self.owner_id = 1 @@ -52,7 +52,7 @@ class DummyDeployedRange(Mock): def __init__(self, *args: Any, **kwargs: dict[str, Any]) -> None: # noqa: ANN401 """Initialize dummy deployed range.""" super().__init__(*args, **kwargs) - self.permissions = [] + self.permissions: list[Any] = [] self.owner_id = 1 diff --git a/api/tests/unit/crud/test_admin_permissions.py b/api/tests/unit/crud/test_admin_permissions.py deleted file mode 100644 index 000df153..00000000 --- a/api/tests/unit/crud/test_admin_permissions.py +++ /dev/null @@ -1,142 +0,0 @@ -from unittest.mock import Mock - -from src.app.crud.crud_ranges import ( - can_execute_deployed, - can_read_blueprint, - can_read_deployed, - can_write_blueprint, - can_write_deployed, -) -from src.app.enums.permissions import ( - DeployedRangePermissionType, -) - - -class TestAdminPermissions: - """Test admin permission behavior per end-to-end encryption model.""" - - def test_admin_has_full_blueprint_access(self) -> None: - """Test that admins can read and write any blueprint (not encrypted).""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [] - - # Admin user (different from owner) - admin_user = Mock() - admin_user.id = 2 - admin_user.is_admin = True - - # Admin should have full access to blueprints - assert can_read_blueprint(range_model, admin_user.id, admin_user) is True - assert can_write_blueprint(range_model, admin_user.id, admin_user) is True - - def test_admin_has_read_only_deployed_access(self) -> None: - """Test that admins can only read deployed ranges (due to encryption).""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [] - - # Admin user (different from owner) - admin_user = Mock() - admin_user.id = 2 - admin_user.is_admin = True - - # Admin should only have read access to deployed ranges - assert can_read_deployed(range_model, admin_user.id, admin_user) is True - assert can_write_deployed(range_model, admin_user.id, admin_user) is False - assert can_execute_deployed(range_model, admin_user.id, admin_user) is False - - def test_admin_permissions_override_no_explicit_grants(self) -> None: - """Test admin permissions work even without explicit permission grants.""" - blueprint_range = Mock() - blueprint_range.owner_id = 1 - blueprint_range.permissions = [] - - deployed_range = Mock() - deployed_range.owner_id = 1 - deployed_range.permissions = [] - - admin_user = Mock() - admin_user.id = 3 - admin_user.is_admin = True - - # Admin should override lack of explicit permissions - assert can_read_blueprint(blueprint_range, admin_user.id, admin_user) is True - assert can_write_blueprint(blueprint_range, admin_user.id, admin_user) is True - assert can_read_deployed(deployed_range, admin_user.id, admin_user) is True - assert can_write_deployed(deployed_range, admin_user.id, admin_user) is False - assert can_execute_deployed(deployed_range, admin_user.id, admin_user) is False - - def test_non_admin_follows_normal_permission_rules(self) -> None: - """Test that non-admin users still follow normal permission rules.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [] - - regular_user = Mock() - regular_user.id = 2 - regular_user.is_admin = False - - # Regular user should have no access without explicit permissions - assert can_read_blueprint(range_model, regular_user.id, regular_user) is False - assert can_write_blueprint(range_model, regular_user.id, regular_user) is False - assert can_read_deployed(range_model, regular_user.id, regular_user) is False - assert can_write_deployed(range_model, regular_user.id, regular_user) is False - assert can_execute_deployed(range_model, regular_user.id, regular_user) is False - - def test_owner_still_has_full_access_regardless_of_admin_status(self) -> None: - """Test that owners always have full access regardless of admin status.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [] - - # Owner who is not admin - owner_user = Mock() - owner_user.id = 1 - owner_user.is_admin = False - - # Owner should have full access regardless of admin status - assert can_read_blueprint(range_model, owner_user.id, owner_user) is True - assert can_write_blueprint(range_model, owner_user.id, owner_user) is True - assert can_read_deployed(range_model, owner_user.id, owner_user) is True - assert can_write_deployed(range_model, owner_user.id, owner_user) is True - assert can_execute_deployed(range_model, owner_user.id, owner_user) is True - - def test_admin_and_owner_same_user(self) -> None: - """Test behavior when user is both admin and owner.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [] - - admin_owner = Mock() - admin_owner.id = 1 - admin_owner.is_admin = True - - # Should have full access (owner privileges take precedence) - assert can_read_blueprint(range_model, admin_owner.id, admin_owner) is True - assert can_write_blueprint(range_model, admin_owner.id, admin_owner) is True - assert can_read_deployed(range_model, admin_owner.id, admin_owner) is True - assert can_write_deployed(range_model, admin_owner.id, admin_owner) is True - assert can_execute_deployed(range_model, admin_owner.id, admin_owner) is True - - def test_encryption_boundary_prevents_admin_deployed_modifications(self) -> None: - """Test that encryption boundary prevents admin modifications to deployed ranges.""" - deployed_range = Mock() - deployed_range.owner_id = 1 - deployed_range.permissions = [ - Mock(user_id=3, permission_type=DeployedRangePermissionType.WRITE.value), - Mock(user_id=4, permission_type=DeployedRangePermissionType.EXECUTE.value), - ] - - admin_user = Mock() - admin_user.id = 2 - admin_user.is_admin = True - - # Admin can read but cannot write/execute due to encryption - assert can_read_deployed(deployed_range, admin_user.id, admin_user) is True - assert can_write_deployed(deployed_range, admin_user.id, admin_user) is False - assert can_execute_deployed(deployed_range, admin_user.id, admin_user) is False - - # Regular users with explicit permissions should still work - assert can_write_deployed(deployed_range, 3) is True - assert can_execute_deployed(deployed_range, 4) is True diff --git a/api/tests/unit/crud/test_crud_ranges.py b/api/tests/unit/crud/test_crud_ranges.py index 5a48cd56..f1d9b979 100644 --- a/api/tests/unit/crud/test_crud_ranges.py +++ b/api/tests/unit/crud/test_crud_ranges.py @@ -9,11 +9,6 @@ from src.app.crud.crud_ranges import ( build_blueprint_range_models, build_deployed_range_models, - can_execute_deployed, - can_read_blueprint, - can_read_deployed, - can_write_blueprint, - can_write_deployed, create_blueprint_range, create_deployed_range, delete_blueprint_range, @@ -785,245 +780,6 @@ def __init__( self.permission_type = permission_type -class TestBlueprintPermissionHelpers: - """Test blueprint range permission helper functions.""" - - def test_can_read_blueprint_owner_access(self) -> None: - """Owner can always read blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 1 - dummy_range.owner_id = user_id - dummy_range.permissions = [] - - assert can_read_blueprint(dummy_range, user_id) - - def test_can_read_blueprint_explicit_read_permission(self) -> None: - """User with explicit read permission can read blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] - - assert can_read_blueprint(dummy_range, user_id) - - def test_can_read_blueprint_write_implies_read(self) -> None: - """User with write permission can also read blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] - - assert can_read_blueprint(dummy_range, user_id) - - def test_can_read_blueprint_no_permission(self) -> None: - """User without permissions cannot read blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] - - assert not can_read_blueprint(dummy_range, user_id) - - def test_can_read_blueprint_wrong_user_permission(self) -> None: - """User cannot read with permission granted to another user.""" - dummy_range = DummyBlueprintRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(3, "read")] # Permission for user 3 - - assert not can_read_blueprint(dummy_range, user_id) - - def test_can_write_blueprint_owner_access(self) -> None: - """Owner can always write blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 1 - dummy_range.owner_id = user_id - dummy_range.permissions = [] - - assert can_write_blueprint(dummy_range, user_id) - - def test_can_write_blueprint_explicit_write_permission(self) -> None: - """User with explicit write permission can write blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] - - assert can_write_blueprint(dummy_range, user_id) - - def test_can_write_blueprint_read_does_not_imply_write(self) -> None: - """User with only read permission cannot write blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] - - assert not can_write_blueprint(dummy_range, user_id) - - def test_can_write_blueprint_no_permission(self) -> None: - """User without permissions cannot write blueprint ranges.""" - dummy_range = DummyBlueprintRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] - - assert not can_write_blueprint(dummy_range, user_id) - - -class TestDeployedRangePermissionHelpers: - """Test deployed range permission helper functions.""" - - def test_can_read_deployed_owner_access(self) -> None: - """Owner can always read deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 1 - dummy_range.owner_id = user_id - dummy_range.permissions = [] - - assert can_read_deployed(dummy_range, user_id) - - def test_can_read_deployed_explicit_read_permission(self) -> None: - """User with explicit read permission can read deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] - - assert can_read_deployed(dummy_range, user_id) - - def test_can_read_deployed_write_implies_read(self) -> None: - """User with write permission can also read deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] - - assert can_read_deployed(dummy_range, user_id) - - def test_can_read_deployed_execute_isolated(self) -> None: - """User with execute permission cannot read deployed ranges (execute is isolated).""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "execute")] - - assert not can_read_deployed(dummy_range, user_id) - - def test_can_read_deployed_no_permission(self) -> None: - """User without permissions cannot read deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] - - assert not can_read_deployed(dummy_range, user_id) - - def test_can_write_deployed_owner_access(self) -> None: - """Owner can always write deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 1 - dummy_range.owner_id = user_id - dummy_range.permissions = [] - - assert can_write_deployed(dummy_range, user_id) - - def test_can_write_deployed_explicit_write_permission(self) -> None: - """User with explicit write permission can write deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] - - assert can_write_deployed(dummy_range, user_id) - - def test_can_write_deployed_read_does_not_imply_write(self) -> None: - """User with only read permission cannot write deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] - - assert not can_write_deployed(dummy_range, user_id) - - def test_can_write_deployed_execute_does_not_imply_write(self) -> None: - """User with only execute permission cannot write deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "execute")] - - assert not can_write_deployed(dummy_range, user_id) - - def test_can_write_deployed_no_permission(self) -> None: - """User without permissions cannot write deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] - - assert not can_write_deployed(dummy_range, user_id) - - def test_can_execute_deployed_owner_access(self) -> None: - """Owner can always execute deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 1 - dummy_range.owner_id = user_id - dummy_range.permissions = [] - - assert can_execute_deployed(dummy_range, user_id) - - def test_can_execute_deployed_explicit_execute_permission(self) -> None: - """User with explicit execute permission can execute deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "execute")] - - assert can_execute_deployed(dummy_range, user_id) - - def test_can_execute_deployed_read_does_not_imply_execute(self) -> None: - """User with only read permission cannot execute deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] - - assert not can_execute_deployed(dummy_range, user_id) - - def test_can_execute_deployed_write_does_not_imply_execute(self) -> None: - """User with only write permission cannot execute deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] - - assert not can_execute_deployed(dummy_range, user_id) - - def test_can_execute_deployed_no_permission(self) -> None: - """User without permissions cannot execute deployed ranges.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] - - assert not can_execute_deployed(dummy_range, user_id) - - def test_permission_inheritance_multiple_permissions(self) -> None: - """User with multiple permissions should have all access rights.""" - dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [ - MockPermission(user_id, "read"), - MockPermission(user_id, "write"), - MockPermission(user_id, "execute"), - ] - - assert can_read_deployed(dummy_range, user_id) - assert can_write_deployed(dummy_range, user_id) - assert can_execute_deployed(dummy_range, user_id) - - # ==================== CRUD Integration Permission Tests ===================== @@ -1204,10 +960,10 @@ async def test_get_deployed_range_with_write_permission( assert result is not None mock_model_validate.assert_called_once() - async def test_get_deployed_range_with_execute_permission_denied( + async def test_get_deployed_range_with_execute_permission_allowed( self, mocker: MockerFixture ) -> None: - """User with execute permission cannot access deployed ranges (execute is isolated).""" + """User with execute permission can access deployed ranges (execute includes read).""" dummy_db = DummyDB() dummy_range = DummyDeployedRange() @@ -1224,8 +980,8 @@ async def test_get_deployed_range_with_execute_permission_denied( dummy_db, range_id=1, user_id=user_id, is_admin=False ) - assert result is None - mock_model_validate.assert_not_called() + assert result is not None + mock_model_validate.assert_called_once() async def test_get_deployed_range_denied_no_permission(self) -> None: """User without permission cannot access deployed ranges.""" diff --git a/api/tests/unit/crud/test_permission_isolation.py b/api/tests/unit/crud/test_permission_isolation.py deleted file mode 100644 index e2cf80a7..00000000 --- a/api/tests/unit/crud/test_permission_isolation.py +++ /dev/null @@ -1,149 +0,0 @@ -from unittest.mock import Mock - -from src.app.crud.crud_ranges import ( - can_execute_deployed, - can_read_blueprint, - can_read_deployed, - can_write_blueprint, - can_write_deployed, -) -from src.app.enums.permissions import ( - BlueprintPermissionType, - DeployedRangePermissionType, -) - - -class TestPermissionIsolation: - """Test that permissions are properly isolated and don't grant unintended access.""" - - def test_blueprint_read_permission_isolation(self) -> None: - """Test that blueprint read permission only grants read access.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [ - Mock(user_id=2, permission_type=BlueprintPermissionType.READ.value) - ] - - assert can_read_blueprint(range_model, 2) is True - assert can_write_blueprint(range_model, 2) is False - - def test_blueprint_write_permission_includes_read(self) -> None: - """Test that blueprint write permission includes read access.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [ - Mock(user_id=2, permission_type=BlueprintPermissionType.WRITE.value) - ] - - assert can_read_blueprint(range_model, 2) is True - assert can_write_blueprint(range_model, 2) is True - - def test_deployed_read_permission_isolation(self) -> None: - """Test that deployed read permission only grants read access.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [ - Mock(user_id=2, permission_type=DeployedRangePermissionType.READ.value) - ] - - assert can_read_deployed(range_model, 2) is True - assert can_write_deployed(range_model, 2) is False - assert can_execute_deployed(range_model, 2) is False - - def test_deployed_write_permission_includes_read(self) -> None: - """Test that deployed write permission includes read access.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [ - Mock(user_id=2, permission_type=DeployedRangePermissionType.WRITE.value) - ] - - assert can_read_deployed(range_model, 2) is True - assert can_write_deployed(range_model, 2) is True - assert can_execute_deployed(range_model, 2) is False - - def test_deployed_execute_permission_isolation(self) -> None: - """Test that deployed execute permission is completely isolated.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [ - Mock(user_id=2, permission_type=DeployedRangePermissionType.EXECUTE.value) - ] - - assert can_read_deployed(range_model, 2) is False - assert can_write_deployed(range_model, 2) is False - assert can_execute_deployed(range_model, 2) is True - - def test_no_permission_denies_all_access(self) -> None: - """Test that users with no permissions are denied all access.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [] - - assert can_read_deployed(range_model, 2) is False - assert can_write_deployed(range_model, 2) is False - assert can_execute_deployed(range_model, 2) is False - - def test_wrong_user_permission_denies_access(self) -> None: - """Test that permissions for other users don't grant access.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [ - Mock(user_id=3, permission_type=DeployedRangePermissionType.READ.value) - ] - - assert can_read_deployed(range_model, 2) is False - assert can_write_deployed(range_model, 2) is False - assert can_execute_deployed(range_model, 2) is False - - def test_owner_always_has_all_permissions(self) -> None: - """Test that owners always have all permissions regardless of explicit grants.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [] - - assert can_read_deployed(range_model, 1) is True - assert can_write_deployed(range_model, 1) is True - assert can_execute_deployed(range_model, 1) is True - - def test_multiple_permissions_work_correctly(self) -> None: - """Test that users with multiple permissions get all granted access.""" - range_model = Mock() - range_model.owner_id = 1 - range_model.permissions = [ - Mock(user_id=2, permission_type=DeployedRangePermissionType.READ.value), - Mock(user_id=2, permission_type=DeployedRangePermissionType.EXECUTE.value), - ] - - assert can_read_deployed(range_model, 2) is True - assert can_write_deployed(range_model, 2) is False - assert can_execute_deployed(range_model, 2) is True - - def test_permission_boundaries_are_strict(self) -> None: - """Test edge cases to ensure permission boundaries are strictly enforced.""" - range_model = Mock() - range_model.owner_id = 1 - - # User with only execute should not get read/write - range_model.permissions = [ - Mock(user_id=2, permission_type=DeployedRangePermissionType.EXECUTE.value) - ] - assert can_read_deployed(range_model, 2) is False - assert can_write_deployed(range_model, 2) is False - assert can_execute_deployed(range_model, 2) is True - - # User with only read should not get write/execute - range_model.permissions = [ - Mock(user_id=2, permission_type=DeployedRangePermissionType.READ.value) - ] - assert can_read_deployed(range_model, 2) is True - assert can_write_deployed(range_model, 2) is False - assert can_execute_deployed(range_model, 2) is False - - # User with only write should get read but not execute - range_model.permissions = [ - Mock(user_id=2, permission_type=DeployedRangePermissionType.WRITE.value) - ] - assert can_read_deployed(range_model, 2) is True - assert can_write_deployed(range_model, 2) is True - assert can_execute_deployed(range_model, 2) is False diff --git a/api/tests/unit/crud/test_permissions.py b/api/tests/unit/crud/test_permissions.py new file mode 100644 index 00000000..cdeeee70 --- /dev/null +++ b/api/tests/unit/crud/test_permissions.py @@ -0,0 +1,211 @@ +"""Test permission system behavior including admin boundaries and security isolation.""" + +from unittest.mock import Mock + +import pytest + +from src.app.crud.crud_ranges import ( + can_execute_deployed, + can_read_blueprint, + can_read_deployed, + can_write_blueprint, + can_write_deployed, +) +from src.app.enums.permissions import ( + BlueprintPermissionType, + DeployedRangePermissionType, +) + + +class TestAdminPermissions: + """Test admin permission behavior per end-to-end encryption model.""" + + def test_admin_has_full_blueprint_access(self) -> None: + """Admins can read and write any blueprint (not encrypted).""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + admin_user = Mock() + admin_user.id = 2 + admin_user.is_admin = True + + assert can_read_blueprint(range_model, admin_user.id, admin_user) is True + assert can_write_blueprint(range_model, admin_user.id, admin_user) is True + + def test_admin_has_read_only_deployed_access(self) -> None: + """Admins can only read deployed ranges (due to encryption).""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + admin_user = Mock() + admin_user.id = 2 + admin_user.is_admin = True + + assert can_read_deployed(range_model, admin_user.id, admin_user) is True + assert can_write_deployed(range_model, admin_user.id, admin_user) is False + assert can_execute_deployed(range_model, admin_user.id, admin_user) is False + + def test_encryption_boundary_prevents_admin_deployed_modifications(self) -> None: + """E2E encryption prevents admin write/execute on deployed ranges.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + admin_user = Mock() + admin_user.id = 2 + admin_user.is_admin = True + + # Admin cannot modify or execute deployed ranges due to encryption + assert can_write_deployed(range_model, admin_user.id, admin_user) is False + assert can_execute_deployed(range_model, admin_user.id, admin_user) is False + # But can still read metadata + assert can_read_deployed(range_model, admin_user.id, admin_user) is True + + def test_owner_always_has_full_access_regardless_of_admin_status(self) -> None: + """Owners always have full access regardless of admin status.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [] + + # Test owner who is also admin + admin_owner = Mock() + admin_owner.id = 1 + admin_owner.is_admin = True + + assert can_read_blueprint(range_model, admin_owner.id, admin_owner) is True + assert can_write_blueprint(range_model, admin_owner.id, admin_owner) is True + assert can_read_deployed(range_model, admin_owner.id, admin_owner) is True + assert can_write_deployed(range_model, admin_owner.id, admin_owner) is True + assert can_execute_deployed(range_model, admin_owner.id, admin_owner) is True + + +class TestPermissionIsolation: + """Test that permissions are properly isolated and don't grant unintended access.""" + + @pytest.mark.parametrize( + "permission_type,expected_read,expected_write", + [ + (BlueprintPermissionType.READ.value, True, False), + (BlueprintPermissionType.WRITE.value, True, True), + ], + ) + def test_blueprint_permission_boundaries( + self, permission_type: str, expected_read: bool, expected_write: bool + ) -> None: + """Blueprint permissions grant appropriate access levels.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [Mock(user_id=2, permission_type=permission_type)] + + assert can_read_blueprint(range_model, 2) is expected_read + assert can_write_blueprint(range_model, 2) is expected_write + + @pytest.mark.parametrize( + "permission_type,expected_read,expected_write,expected_execute", + [ + (DeployedRangePermissionType.READ.value, True, False, False), + (DeployedRangePermissionType.WRITE.value, True, True, False), + (DeployedRangePermissionType.EXECUTE.value, True, False, True), + ], + ) + def test_deployed_permission_boundaries( + self, + permission_type: str, + expected_read: bool, + expected_write: bool, + expected_execute: bool, + ) -> None: + """Deployed range permissions grant appropriate access levels.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [Mock(user_id=2, permission_type=permission_type)] + + assert can_read_deployed(range_model, 2) is expected_read + assert can_write_deployed(range_model, 2) is expected_write + assert can_execute_deployed(range_model, 2) is expected_execute + + def test_no_permission_denies_all_access(self) -> None: + """Users without permissions cannot access ranges.""" + blueprint_range = Mock() + blueprint_range.owner_id = 1 + blueprint_range.permissions = [] + + deployed_range = Mock() + deployed_range.owner_id = 1 + deployed_range.permissions = [] + + user_id = 2 # Not owner, no permissions + + # Blueprint access denied + assert can_read_blueprint(blueprint_range, user_id) is False + assert can_write_blueprint(blueprint_range, user_id) is False + + # Deployed range access denied + assert can_read_deployed(deployed_range, user_id) is False + assert can_write_deployed(deployed_range, user_id) is False + assert can_execute_deployed(deployed_range, user_id) is False + + def test_wrong_user_permission_denies_access(self) -> None: + """Permissions granted to other users don't grant access.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=3, permission_type=BlueprintPermissionType.WRITE.value) + ] + + user_id = 2 # Different user than permission grant + + assert can_read_blueprint(range_model, user_id) is False + assert can_write_blueprint(range_model, user_id) is False + + def test_multiple_permissions_work_correctly(self) -> None: + """Multiple permission grants work independently.""" + range_model = Mock() + range_model.owner_id = 1 + range_model.permissions = [ + Mock(user_id=2, permission_type=DeployedRangePermissionType.READ.value), + Mock(user_id=3, permission_type=DeployedRangePermissionType.EXECUTE.value), + Mock(user_id=4, permission_type=DeployedRangePermissionType.WRITE.value), + ] + + # User 2: read only + assert can_read_deployed(range_model, 2) is True + assert can_write_deployed(range_model, 2) is False + assert can_execute_deployed(range_model, 2) is False + + # User 3: read + execute (execute includes read) + assert can_read_deployed(range_model, 3) is True + assert can_write_deployed(range_model, 3) is False + assert can_execute_deployed(range_model, 3) is True + + # User 4: read + write + assert can_read_deployed(range_model, 4) is True + assert can_write_deployed(range_model, 4) is True + assert can_execute_deployed(range_model, 4) is False + + +class TestOwnerPermissions: + """Test that owners always have full access regardless of explicit grants.""" + + def test_owner_bypasses_explicit_permissions(self) -> None: + """Owners have access even without explicit permission grants.""" + blueprint_range = Mock() + blueprint_range.owner_id = 1 + blueprint_range.permissions = [] # No explicit permissions + + deployed_range = Mock() + deployed_range.owner_id = 1 + deployed_range.permissions = [] # No explicit permissions + + owner_id = 1 + + # Owner has full blueprint access + assert can_read_blueprint(blueprint_range, owner_id) is True + assert can_write_blueprint(blueprint_range, owner_id) is True + + # Owner has full deployed range access + assert can_read_deployed(deployed_range, owner_id) is True + assert can_write_deployed(deployed_range, owner_id) is True + assert can_execute_deployed(deployed_range, owner_id) is True From 6022ece4afdcb781732532766c7088d3505889f2 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sun, 3 Aug 2025 10:57:32 -0700 Subject: [PATCH 18/19] fix(rwx): clean up redundant tests --- api/tests/unit/crud/test_crud_permissions.py | 478 ++++++------------- api/tests/unit/crud/test_crud_ranges.py | 441 ++++------------- 2 files changed, 255 insertions(+), 664 deletions(-) diff --git a/api/tests/unit/crud/test_crud_permissions.py b/api/tests/unit/crud/test_crud_permissions.py index fe5300cc..b5c5095a 100644 --- a/api/tests/unit/crud/test_crud_permissions.py +++ b/api/tests/unit/crud/test_crud_permissions.py @@ -24,10 +24,34 @@ # ==================== Grant Permissions ===================== -async def test_grant_blueprint_read_permission( +@pytest.mark.parametrize( + "range_type,permission_type,expected_model_class,user_id", + [ + ("blueprint", BlueprintPermissionType.READ, BlueprintRangePermissionModel, 2), + ("blueprint", BlueprintPermissionType.WRITE, BlueprintRangePermissionModel, 3), + ("deployed", DeployedRangePermissionType.READ, DeployedRangePermissionModel, 2), + ( + "deployed", + DeployedRangePermissionType.WRITE, + DeployedRangePermissionModel, + 3, + ), + ( + "deployed", + DeployedRangePermissionType.EXECUTE, + DeployedRangePermissionModel, + 4, + ), + ], +) +async def test_grant_permission( + range_type: str, + permission_type: BlueprintPermissionType | DeployedRangePermissionType, + expected_model_class: type, + user_id: int, caplog: pytest.LogCaptureFixture, ) -> None: - """Test granting read permission to a blueprint range.""" + """Test granting permissions to ranges.""" dummy_db = DummyDB() mock_range = Mock() @@ -36,192 +60,48 @@ async def test_grant_blueprint_read_permission( mock_result.scalar_one_or_none.return_value = mock_range dummy_db.execute.return_value = mock_result - blueprint_range_id = 1 - user_id = 2 + range_id = 1 requesting_user_id = 1 # Owner with caplog.at_level(logging.DEBUG): - result = await grant_blueprint_permission( - dummy_db, - blueprint_range_id, - user_id, - BlueprintPermissionType.READ, - requesting_user_id, - ) + if range_type == "blueprint": + result = await grant_blueprint_permission( + dummy_db, range_id, user_id, permission_type, requesting_user_id + ) + else: + result = await grant_deployed_permission( + dummy_db, range_id, user_id, permission_type, requesting_user_id + ) dummy_db.add.assert_called_once() added_permission = dummy_db.add.call_args[0][0] - assert isinstance(added_permission, BlueprintRangePermissionModel) - assert added_permission.blueprint_range_id == blueprint_range_id + assert isinstance(added_permission, expected_model_class) assert added_permission.user_id == user_id - assert added_permission.permission_type == BlueprintPermissionType.READ - - dummy_db.flush.assert_called_once() - dummy_db.refresh.assert_called_once_with(added_permission) - - assert result == added_permission - - -async def test_grant_blueprint_write_permission() -> None: - """Test granting write permission to a blueprint range.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_range - dummy_db.execute.return_value = mock_result - - blueprint_range_id = 1 - user_id = 3 - requesting_user_id = 1 # Owner - - result = await grant_blueprint_permission( - dummy_db, - blueprint_range_id, - user_id, - BlueprintPermissionType.WRITE, - requesting_user_id, - ) - - added_permission = dummy_db.add.call_args[0][0] - assert added_permission.blueprint_range_id == blueprint_range_id - assert added_permission.user_id == user_id - assert added_permission.permission_type == BlueprintPermissionType.WRITE - - assert result == added_permission - + assert added_permission.permission_type == permission_type -async def test_grant_deployed_read_permission() -> None: - """Test granting read permission to a deployed range.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_range - dummy_db.execute.return_value = mock_result - - deployed_range_id = 1 - user_id = 2 - requesting_user_id = 1 # Owner - - result = await grant_deployed_permission( - dummy_db, - deployed_range_id, - user_id, - DeployedRangePermissionType.READ, - requesting_user_id, - ) - - dummy_db.add.assert_called_once() - added_permission = dummy_db.add.call_args[0][0] - assert isinstance(added_permission, DeployedRangePermissionModel) - assert added_permission.deployed_range_id == deployed_range_id - assert added_permission.user_id == user_id - assert added_permission.permission_type == DeployedRangePermissionType.READ + if range_type == "blueprint": + assert added_permission.blueprint_range_id == range_id + else: + assert added_permission.deployed_range_id == range_id dummy_db.flush.assert_called_once() dummy_db.refresh.assert_called_once_with(added_permission) - assert result == added_permission -async def test_grant_deployed_write_permission() -> None: - """Test granting write permission to a deployed range.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_range - dummy_db.execute.return_value = mock_result - - deployed_range_id = 1 - user_id = 3 - requesting_user_id = 1 # Owner - - result = await grant_deployed_permission( - dummy_db, - deployed_range_id, - user_id, - DeployedRangePermissionType.WRITE, - requesting_user_id, - ) - - added_permission = dummy_db.add.call_args[0][0] - assert added_permission.deployed_range_id == deployed_range_id - assert added_permission.user_id == user_id - assert added_permission.permission_type == DeployedRangePermissionType.WRITE - - assert result == added_permission - - -async def test_grant_deployed_execute_permission() -> None: - """Test granting execute permission to a deployed range.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_range - dummy_db.execute.return_value = mock_result - - deployed_range_id = 1 - user_id = 4 - requesting_user_id = 1 # Owner - - result = await grant_deployed_permission( - dummy_db, - deployed_range_id, - user_id, - DeployedRangePermissionType.EXECUTE, - requesting_user_id, - ) - - added_permission = dummy_db.add.call_args[0][0] - assert added_permission.deployed_range_id == deployed_range_id - assert added_permission.user_id == user_id - assert added_permission.permission_type == DeployedRangePermissionType.EXECUTE - - assert result == added_permission - - -async def test_grant_blueprint_permission_db_error( - caplog: pytest.LogCaptureFixture, -) -> None: - """Test that grant_blueprint_permission handles database errors.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_result = Mock() - mock_result.scalar_one_or_none.return_value = mock_range - dummy_db.execute.return_value = mock_result - - # Force a db exception - test_except_msg = "Fake DB error!" - dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - - requesting_user_id = 1 # Owner - - with pytest.raises(SQLAlchemyError, match=test_except_msg): - await grant_blueprint_permission( - dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id - ) - - assert any( - record.levelno == logging.ERROR - and record.exc_info is not None - and test_except_msg in record.message - for record in caplog.records - ) - - -async def test_grant_deployed_permission_db_error( +@pytest.mark.parametrize( + "range_type,permission_type", + [ + ("blueprint", BlueprintPermissionType.READ), + ("deployed", DeployedRangePermissionType.READ), + ], +) +async def test_grant_permission_db_error( + range_type: str, + permission_type: BlueprintPermissionType | DeployedRangePermissionType, caplog: pytest.LogCaptureFixture, ) -> None: - """Test that grant_deployed_permission handles database errors.""" + """Test that grant permission functions handle database errors.""" dummy_db = DummyDB() mock_range = Mock() @@ -237,9 +117,14 @@ async def test_grant_deployed_permission_db_error( requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await grant_deployed_permission( - dummy_db, 1, 2, DeployedRangePermissionType.READ, requesting_user_id - ) + if range_type == "blueprint": + await grant_blueprint_permission( + dummy_db, 1, 2, permission_type, requesting_user_id + ) + else: + await grant_deployed_permission( + dummy_db, 1, 2, permission_type, requesting_user_id + ) assert any( record.levelno == logging.ERROR @@ -252,126 +137,42 @@ async def test_grant_deployed_permission_db_error( # ==================== Revoke Permissions ===================== -async def test_revoke_blueprint_permission_success() -> None: - """Test successfully revoking a blueprint permission.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_ownership_result = Mock() - mock_ownership_result.scalar_one_or_none.return_value = mock_range - - mock_permission = BlueprintRangePermissionModel( - blueprint_range_id=1, - user_id=2, - permission_type=BlueprintPermissionType.READ, - ) - - mock_permission_result = Mock() - mock_permission_result.scalar_one_or_none.return_value = mock_permission - - dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - - requesting_user_id = 1 # Owner - - result = await revoke_blueprint_permission( - dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id - ) - - dummy_db.delete.assert_called_once_with(mock_permission) - dummy_db.flush.assert_called_once() - - assert result is True - - -async def test_revoke_blueprint_permission_not_found() -> None: - """Test revoking a blueprint permission that doesn't exist.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_ownership_result = Mock() - mock_ownership_result.scalar_one_or_none.return_value = mock_range - - mock_permission_result = Mock() - mock_permission_result.scalar_one_or_none.return_value = None - - dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - - requesting_user_id = 1 # Owner - - result = await revoke_blueprint_permission( - dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id - ) - - dummy_db.delete.assert_not_called() - dummy_db.flush.assert_not_called() - - assert result is False - - -async def test_revoke_deployed_permission_success() -> None: - """Test successfully revoking a deployed permission.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_ownership_result = Mock() - mock_ownership_result.scalar_one_or_none.return_value = mock_range - - mock_permission = DeployedRangePermissionModel( - deployed_range_id=1, - user_id=2, - permission_type=DeployedRangePermissionType.EXECUTE, - ) - - mock_permission_result = Mock() - mock_permission_result.scalar_one_or_none.return_value = mock_permission - - dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - - requesting_user_id = 1 # Owner - - result = await revoke_deployed_permission( - dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id - ) - - dummy_db.delete.assert_called_once_with(mock_permission) - dummy_db.flush.assert_called_once() - - assert result is True - - -async def test_revoke_deployed_permission_not_found() -> None: - """Test revoking a deployed permission that doesn't exist.""" - dummy_db = DummyDB() - - mock_range = Mock() - mock_range.owner_id = 1 - mock_ownership_result = Mock() - mock_ownership_result.scalar_one_or_none.return_value = mock_range - - mock_permission_result = Mock() - mock_permission_result.scalar_one_or_none.return_value = None - - dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - - requesting_user_id = 1 # Owner - - result = await revoke_deployed_permission( - dummy_db, 1, 2, DeployedRangePermissionType.WRITE, requesting_user_id - ) - - dummy_db.delete.assert_not_called() - dummy_db.flush.assert_not_called() - - assert result is False - - -async def test_revoke_blueprint_permission_db_error( - caplog: pytest.LogCaptureFixture, +@pytest.mark.parametrize( + "range_type,permission_type,expected_model_class,permission_exists", + [ + ( + "blueprint", + BlueprintPermissionType.READ, + BlueprintRangePermissionModel, + True, + ), + ( + "blueprint", + BlueprintPermissionType.READ, + BlueprintRangePermissionModel, + False, + ), + ( + "deployed", + DeployedRangePermissionType.EXECUTE, + DeployedRangePermissionModel, + True, + ), + ( + "deployed", + DeployedRangePermissionType.WRITE, + DeployedRangePermissionModel, + False, + ), + ], +) +async def test_revoke_permission( + range_type: str, + permission_type: BlueprintPermissionType | DeployedRangePermissionType, + expected_model_class: type, + permission_exists: bool, ) -> None: - """Test that revoke_blueprint_permission handles database errors.""" + """Test revoking permissions from ranges.""" dummy_db = DummyDB() mock_range = Mock() @@ -379,40 +180,57 @@ async def test_revoke_blueprint_permission_db_error( mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - mock_permission = BlueprintRangePermissionModel( - blueprint_range_id=1, - user_id=2, - permission_type=BlueprintPermissionType.READ, - ) - mock_permission_result = Mock() - mock_permission_result.scalar_one_or_none.return_value = mock_permission + if permission_exists: + if range_type == "blueprint": + mock_permission = BlueprintRangePermissionModel( + blueprint_range_id=1, user_id=2, permission_type=permission_type + ) + else: + mock_permission = DeployedRangePermissionModel( + deployed_range_id=1, user_id=2, permission_type=permission_type + ) + mock_permission_result.scalar_one_or_none.return_value = mock_permission + else: + mock_permission_result.scalar_one_or_none.return_value = None dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] - # Force a db exception - test_except_msg = "Fake DB error!" - dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) - requesting_user_id = 1 # Owner - with pytest.raises(SQLAlchemyError, match=test_except_msg): - await revoke_blueprint_permission( - dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id + if range_type == "blueprint": + result = await revoke_blueprint_permission( + dummy_db, 1, 2, permission_type, requesting_user_id + ) + else: + result = await revoke_deployed_permission( + dummy_db, 1, 2, permission_type, requesting_user_id ) - assert any( - record.levelno == logging.ERROR - and record.exc_info is not None - and test_except_msg in record.message - for record in caplog.records - ) - - -async def test_revoke_deployed_permission_db_error( + if permission_exists: + dummy_db.delete.assert_called_once_with(mock_permission) + dummy_db.flush.assert_called_once() + assert result is True + else: + dummy_db.delete.assert_not_called() + dummy_db.flush.assert_not_called() + assert result is False + + +@pytest.mark.parametrize( + "range_type,permission_type,expected_model_class", + [ + ("blueprint", BlueprintPermissionType.READ, BlueprintRangePermissionModel), + ("deployed", DeployedRangePermissionType.EXECUTE, DeployedRangePermissionModel), + ], +) +async def test_revoke_permission_db_error( + range_type: str, + permission_type: BlueprintPermissionType | DeployedRangePermissionType, + expected_model_class: type, caplog: pytest.LogCaptureFixture, ) -> None: - """Test that revoke_deployed_permission handles database errors.""" + """Test that revoke permission functions handle database errors.""" dummy_db = DummyDB() mock_range = Mock() @@ -420,11 +238,14 @@ async def test_revoke_deployed_permission_db_error( mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - mock_permission = DeployedRangePermissionModel( - deployed_range_id=1, - user_id=2, - permission_type=DeployedRangePermissionType.EXECUTE, - ) + if range_type == "blueprint": + mock_permission = BlueprintRangePermissionModel( + blueprint_range_id=1, user_id=2, permission_type=permission_type + ) + else: + mock_permission = DeployedRangePermissionModel( + deployed_range_id=1, user_id=2, permission_type=permission_type + ) mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = mock_permission @@ -438,9 +259,14 @@ async def test_revoke_deployed_permission_db_error( requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - await revoke_deployed_permission( - dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id - ) + if range_type == "blueprint": + await revoke_blueprint_permission( + dummy_db, 1, 2, permission_type, requesting_user_id + ) + else: + await revoke_deployed_permission( + dummy_db, 1, 2, permission_type, requesting_user_id + ) assert any( record.levelno == logging.ERROR diff --git a/api/tests/unit/crud/test_crud_ranges.py b/api/tests/unit/crud/test_crud_ranges.py index f1d9b979..7ff47387 100644 --- a/api/tests/unit/crud/test_crud_ranges.py +++ b/api/tests/unit/crud/test_crud_ranges.py @@ -765,7 +765,7 @@ async def test_delete_deployed_range_raises_generic_errors( ) -# ==================== Permission Helper Function Tests ===================== +# ==================== Permission Integration Tests ===================== class MockPermission(Mock): @@ -780,374 +780,139 @@ def __init__( self.permission_type = permission_type -# ==================== CRUD Integration Permission Tests ===================== - - -class TestBlueprintRangeCRUDPermissions: - """Test blueprint range CRUD operations with permissions.""" - - async def test_get_blueprint_range_with_read_permission( - self, mocker: MockerFixture - ) -> None: - """User with read permission can access blueprint ranges.""" - dummy_db = DummyDB() - dummy_range = DummyBlueprintRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] - - dummy_db.get.return_value = dummy_range - mock_model_validate = mocker.patch.object( - BlueprintRangeSchema, "model_validate", return_value=dummy_range - ) - - result = await get_blueprint_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is not None - mock_model_validate.assert_called_once() +@pytest.mark.parametrize( + "range_type,has_permission,permission_type,should_access", + [ + ("blueprint", True, "read", True), + ("blueprint", False, None, False), + ("deployed", True, "read", True), + ("deployed", False, None, False), + ], +) +async def test_get_range_with_permissions( + range_type: str, + has_permission: bool, + permission_type: str | None, + should_access: bool, + mocker: MockerFixture, +) -> None: + """Test that get operations respect permission system.""" + dummy_db = DummyDB() - async def test_get_blueprint_range_with_write_permission( - self, mocker: MockerFixture - ) -> None: - """User with write permission can access blueprint ranges (write implies read).""" - dummy_db = DummyDB() + if range_type == "blueprint": dummy_range = DummyBlueprintRange() + get_func = get_blueprint_range + schema_class = BlueprintRangeSchema + else: + dummy_range = DummyDeployedRange() + get_func = get_deployed_range + schema_class = DeployedRangeSchema - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = ( + [MockPermission(user_id, permission_type)] if has_permission else [] + ) - dummy_db.get.return_value = dummy_range - mock_model_validate = mocker.patch.object( - BlueprintRangeSchema, "model_validate", return_value=dummy_range - ) + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + schema_class, "model_validate", return_value=dummy_range + ) - result = await get_blueprint_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) + result = await get_func(dummy_db, range_id=1, user_id=user_id, is_admin=False) + if should_access: assert result is not None mock_model_validate.assert_called_once() - - async def test_get_blueprint_range_denied_no_permission(self) -> None: - """User without permission cannot access blueprint ranges.""" - dummy_db = DummyDB() - dummy_range = DummyBlueprintRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] # No permissions - - dummy_db.get.return_value = dummy_range - - result = await get_blueprint_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - + else: assert result is None + mock_model_validate.assert_not_called() - async def test_delete_blueprint_range_with_write_permission( - self, mocker: MockerFixture - ) -> None: - """User with write permission can delete blueprint ranges.""" - dummy_db = DummyDB() - dummy_range = DummyBlueprintRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] - - dummy_db.get.return_value = dummy_range - mock_model_validate = mocker.patch.object( - BlueprintRangeHeaderSchema, "model_validate", return_value=dummy_range - ) - - result = await delete_blueprint_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is not None - mock_model_validate.assert_called_once() - dummy_db.delete.assert_called_once() - dummy_db.flush.assert_called_once() - - async def test_delete_blueprint_range_denied_read_only_permission(self) -> None: - """User with only read permission cannot delete blueprint ranges.""" - dummy_db = DummyDB() - dummy_range = DummyBlueprintRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] # Read only - - dummy_db.get.return_value = dummy_range - result = await delete_blueprint_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is None - dummy_db.delete.assert_not_called() +@pytest.mark.parametrize( + "range_type,has_write_permission,should_delete", + [ + ("blueprint", True, True), + ("blueprint", False, False), + ("deployed", True, True), + ("deployed", False, False), + ], +) +async def test_delete_range_with_permissions( + range_type: str, + has_write_permission: bool, + should_delete: bool, + mocker: MockerFixture, +) -> None: + """Test that delete operations respect write permissions.""" + dummy_db = DummyDB() - async def test_delete_blueprint_range_denied_no_permission(self) -> None: - """User without permission cannot delete blueprint ranges.""" - dummy_db = DummyDB() + if range_type == "blueprint": dummy_range = DummyBlueprintRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] # No permissions - - dummy_db.get.return_value = dummy_range - - result = await delete_blueprint_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is None - dummy_db.delete.assert_not_called() - - -class TestDeployedRangeCRUDPermissions: - """Test deployed range CRUD operations with permissions.""" - - async def test_get_deployed_range_with_read_permission( - self, mocker: MockerFixture - ) -> None: - """User with read permission can access deployed ranges.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] - - dummy_db.get.return_value = dummy_range - mock_model_validate = mocker.patch.object( - DeployedRangeSchema, "model_validate", return_value=dummy_range - ) - - result = await get_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is not None - mock_model_validate.assert_called_once() - - async def test_get_deployed_range_with_write_permission( - self, mocker: MockerFixture - ) -> None: - """User with write permission can access deployed ranges (write implies read).""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] - - dummy_db.get.return_value = dummy_range - mock_model_validate = mocker.patch.object( - DeployedRangeSchema, "model_validate", return_value=dummy_range - ) - - result = await get_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is not None - mock_model_validate.assert_called_once() - - async def test_get_deployed_range_with_execute_permission_allowed( - self, mocker: MockerFixture - ) -> None: - """User with execute permission can access deployed ranges (execute includes read).""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "execute")] - - dummy_db.get.return_value = dummy_range - mock_model_validate = mocker.patch.object( - DeployedRangeSchema, "model_validate", return_value=dummy_range - ) - - result = await get_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is not None - mock_model_validate.assert_called_once() - - async def test_get_deployed_range_denied_no_permission(self) -> None: - """User without permission cannot access deployed ranges.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] # No permissions - - dummy_db.get.return_value = dummy_range - - result = await get_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is None - - async def test_delete_deployed_range_with_write_permission( - self, mocker: MockerFixture - ) -> None: - """User with write permission can delete deployed ranges.""" - dummy_db = DummyDB() + delete_func = delete_blueprint_range + header_schema = BlueprintRangeHeaderSchema + else: dummy_range = DummyDeployedRange() + delete_func = delete_deployed_range + header_schema = DeployedRangeHeaderSchema - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = ( + [MockPermission(user_id, "write")] if has_write_permission else [] + ) - dummy_db.get.return_value = dummy_range - mock_model_validate = mocker.patch.object( - DeployedRangeHeaderSchema, "model_validate", return_value=dummy_range - ) + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + header_schema, "model_validate", return_value=dummy_range + ) - result = await delete_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) + result = await delete_func(dummy_db, range_id=1, user_id=user_id, is_admin=False) + if should_delete: assert result is not None + dummy_db.delete.assert_called_once_with(dummy_range) mock_model_validate.assert_called_once() - dummy_db.delete.assert_called_once() - dummy_db.flush.assert_called_once() - - async def test_delete_deployed_range_denied_read_only_permission(self) -> None: - """User with only read permission cannot delete deployed ranges.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] # Read only - - dummy_db.get.return_value = dummy_range - - result = await delete_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is None - dummy_db.delete.assert_not_called() - - async def test_delete_deployed_range_denied_execute_only_permission(self) -> None: - """User with only execute permission cannot delete deployed ranges.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "execute")] # Execute only - - dummy_db.get.return_value = dummy_range - - result = await delete_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is None - dummy_db.delete.assert_not_called() - - async def test_delete_deployed_range_denied_no_permission(self) -> None: - """User without permission cannot delete deployed ranges.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] # No permissions - - dummy_db.get.return_value = dummy_range - - result = await delete_deployed_range( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - + else: assert result is None dummy_db.delete.assert_not_called() + mock_model_validate.assert_not_called() - async def test_get_deployed_range_key_with_execute_permission(self) -> None: - """User with execute permission can access SSH keys.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "execute")] - - dummy_db.get.return_value = dummy_range - dummy_db.scalar.return_value = "fake_private_key" - - result = await get_deployed_range_key( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - - assert result is not None - assert result.range_private_key == "fake_private_key" - - async def test_get_deployed_range_key_denied_read_only_permission(self) -> None: - """User with only read permission cannot access SSH keys.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() - - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "read")] # Read only - - dummy_db.get.return_value = dummy_range - dummy_db.scalar.return_value = "fake_private_key" - result = await get_deployed_range_key( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) +async def test_get_deployed_range_key_with_execute_permission() -> None: + """Test that users with execute permission can access deployed range keys.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() - assert result is None + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "execute")] - async def test_get_deployed_range_key_denied_write_only_permission(self) -> None: - """User with only write permission cannot access SSH keys.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() + dummy_db.get.return_value = dummy_range + dummy_db.scalar.return_value = "fake_private_key" - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [MockPermission(user_id, "write")] # Write only + result = await get_deployed_range_key( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) - dummy_db.get.return_value = dummy_range - dummy_db.scalar.return_value = "fake_private_key" + assert result is not None + assert result.range_private_key == "fake_private_key" - result = await get_deployed_range_key( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) - assert result is None - - async def test_get_deployed_range_key_denied_no_permission(self) -> None: - """User without permission cannot access SSH keys.""" - dummy_db = DummyDB() - dummy_range = DummyDeployedRange() +async def test_get_deployed_range_key_denied_no_permission() -> None: + """Test that users without execute permission cannot access deployed range keys.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() - user_id = 2 - dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = [] # No permissions + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions - dummy_db.get.return_value = dummy_range - dummy_db.scalar.return_value = "fake_private_key" + dummy_db.get.return_value = dummy_range - result = await get_deployed_range_key( - dummy_db, range_id=1, user_id=user_id, is_admin=False - ) + result = await get_deployed_range_key( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) - assert result is None + assert result is None + dummy_db.scalar.assert_not_called() # Should not even try to get the key From 06ea9027df2ff801fbfee29ff63e171c8eaf12c0 Mon Sep 17 00:00:00 2001 From: Adam Hassan Date: Sun, 3 Aug 2025 11:39:05 -0700 Subject: [PATCH 19/19] fix(rwx): type safety issues --- api/tests/unit/crud/test_crud_permissions.py | 316 ++++++++++++------- api/tests/unit/crud/test_crud_ranges.py | 227 +++++++++---- 2 files changed, 354 insertions(+), 189 deletions(-) diff --git a/api/tests/unit/crud/test_crud_permissions.py b/api/tests/unit/crud/test_crud_permissions.py index b5c5095a..61f608f0 100644 --- a/api/tests/unit/crud/test_crud_permissions.py +++ b/api/tests/unit/crud/test_crud_permissions.py @@ -21,37 +21,22 @@ from .crud_mocks import DummyDB -# ==================== Grant Permissions ===================== +# ==================== Grant Blueprint Permissions ===================== @pytest.mark.parametrize( - "range_type,permission_type,expected_model_class,user_id", + "permission_type,user_id", [ - ("blueprint", BlueprintPermissionType.READ, BlueprintRangePermissionModel, 2), - ("blueprint", BlueprintPermissionType.WRITE, BlueprintRangePermissionModel, 3), - ("deployed", DeployedRangePermissionType.READ, DeployedRangePermissionModel, 2), - ( - "deployed", - DeployedRangePermissionType.WRITE, - DeployedRangePermissionModel, - 3, - ), - ( - "deployed", - DeployedRangePermissionType.EXECUTE, - DeployedRangePermissionModel, - 4, - ), + (BlueprintPermissionType.READ, 2), + (BlueprintPermissionType.WRITE, 3), ], ) -async def test_grant_permission( - range_type: str, - permission_type: BlueprintPermissionType | DeployedRangePermissionType, - expected_model_class: type, +async def test_grant_blueprint_permission( + permission_type: BlueprintPermissionType, user_id: int, caplog: pytest.LogCaptureFixture, ) -> None: - """Test granting permissions to ranges.""" + """Test granting permissions to blueprint ranges.""" dummy_db = DummyDB() mock_range = Mock() @@ -64,44 +49,102 @@ async def test_grant_permission( requesting_user_id = 1 # Owner with caplog.at_level(logging.DEBUG): - if range_type == "blueprint": - result = await grant_blueprint_permission( - dummy_db, range_id, user_id, permission_type, requesting_user_id - ) - else: - result = await grant_deployed_permission( - dummy_db, range_id, user_id, permission_type, requesting_user_id - ) + result = await grant_blueprint_permission( + dummy_db, range_id, user_id, permission_type, requesting_user_id + ) dummy_db.add.assert_called_once() added_permission = dummy_db.add.call_args[0][0] - assert isinstance(added_permission, expected_model_class) + assert isinstance(added_permission, BlueprintRangePermissionModel) assert added_permission.user_id == user_id assert added_permission.permission_type == permission_type - - if range_type == "blueprint": - assert added_permission.blueprint_range_id == range_id - else: - assert added_permission.deployed_range_id == range_id + assert added_permission.blueprint_range_id == range_id dummy_db.flush.assert_called_once() dummy_db.refresh.assert_called_once_with(added_permission) assert result == added_permission +async def test_grant_blueprint_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that grant_blueprint_permission handles database errors.""" + dummy_db = DummyDB() + + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result + + # Force a db exception + test_except_msg = "Fake DB error!" + dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + requesting_user_id = 1 # Owner + + with pytest.raises(SQLAlchemyError, match=test_except_msg): + await grant_blueprint_permission( + dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id + ) + + assert any( + record.levelno == logging.ERROR + and record.exc_info is not None + and test_except_msg in record.message + for record in caplog.records + ) + + +# ==================== Grant Deployed Permissions ===================== + + @pytest.mark.parametrize( - "range_type,permission_type", + "permission_type,user_id", [ - ("blueprint", BlueprintPermissionType.READ), - ("deployed", DeployedRangePermissionType.READ), + (DeployedRangePermissionType.READ, 2), + (DeployedRangePermissionType.WRITE, 3), + (DeployedRangePermissionType.EXECUTE, 4), ], ) -async def test_grant_permission_db_error( - range_type: str, - permission_type: BlueprintPermissionType | DeployedRangePermissionType, +async def test_grant_deployed_permission( + permission_type: DeployedRangePermissionType, + user_id: int, caplog: pytest.LogCaptureFixture, ) -> None: - """Test that grant permission functions handle database errors.""" + """Test granting permissions to deployed ranges.""" + dummy_db = DummyDB() + + mock_range = Mock() + mock_range.owner_id = 1 + mock_result = Mock() + mock_result.scalar_one_or_none.return_value = mock_range + dummy_db.execute.return_value = mock_result + + range_id = 1 + requesting_user_id = 1 # Owner + + with caplog.at_level(logging.DEBUG): + result = await grant_deployed_permission( + dummy_db, range_id, user_id, permission_type, requesting_user_id + ) + + dummy_db.add.assert_called_once() + added_permission = dummy_db.add.call_args[0][0] + assert isinstance(added_permission, DeployedRangePermissionModel) + assert added_permission.user_id == user_id + assert added_permission.permission_type == permission_type + assert added_permission.deployed_range_id == range_id + + dummy_db.flush.assert_called_once() + dummy_db.refresh.assert_called_once_with(added_permission) + assert result == added_permission + + +async def test_grant_deployed_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that grant_deployed_permission handles database errors.""" dummy_db = DummyDB() mock_range = Mock() @@ -117,14 +160,9 @@ async def test_grant_permission_db_error( requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - if range_type == "blueprint": - await grant_blueprint_permission( - dummy_db, 1, 2, permission_type, requesting_user_id - ) - else: - await grant_deployed_permission( - dummy_db, 1, 2, permission_type, requesting_user_id - ) + await grant_deployed_permission( + dummy_db, 1, 2, DeployedRangePermissionType.READ, requesting_user_id + ) assert any( record.levelno == logging.ERROR @@ -134,45 +172,22 @@ async def test_grant_permission_db_error( ) -# ==================== Revoke Permissions ===================== +# ==================== Revoke Blueprint Permissions ===================== @pytest.mark.parametrize( - "range_type,permission_type,expected_model_class,permission_exists", + "permission_type,permission_exists", [ - ( - "blueprint", - BlueprintPermissionType.READ, - BlueprintRangePermissionModel, - True, - ), - ( - "blueprint", - BlueprintPermissionType.READ, - BlueprintRangePermissionModel, - False, - ), - ( - "deployed", - DeployedRangePermissionType.EXECUTE, - DeployedRangePermissionModel, - True, - ), - ( - "deployed", - DeployedRangePermissionType.WRITE, - DeployedRangePermissionModel, - False, - ), + (BlueprintPermissionType.READ, True), + (BlueprintPermissionType.READ, False), + (BlueprintPermissionType.WRITE, True), ], ) -async def test_revoke_permission( - range_type: str, - permission_type: BlueprintPermissionType | DeployedRangePermissionType, - expected_model_class: type, +async def test_revoke_blueprint_permission( + permission_type: BlueprintPermissionType, permission_exists: bool, ) -> None: - """Test revoking permissions from ranges.""" + """Test revoking permissions from blueprint ranges.""" dummy_db = DummyDB() mock_range = Mock() @@ -182,14 +197,9 @@ async def test_revoke_permission( mock_permission_result = Mock() if permission_exists: - if range_type == "blueprint": - mock_permission = BlueprintRangePermissionModel( - blueprint_range_id=1, user_id=2, permission_type=permission_type - ) - else: - mock_permission = DeployedRangePermissionModel( - deployed_range_id=1, user_id=2, permission_type=permission_type - ) + mock_permission = BlueprintRangePermissionModel( + blueprint_range_id=1, user_id=2, permission_type=permission_type + ) mock_permission_result.scalar_one_or_none.return_value = mock_permission else: mock_permission_result.scalar_one_or_none.return_value = None @@ -198,14 +208,9 @@ async def test_revoke_permission( requesting_user_id = 1 # Owner - if range_type == "blueprint": - result = await revoke_blueprint_permission( - dummy_db, 1, 2, permission_type, requesting_user_id - ) - else: - result = await revoke_deployed_permission( - dummy_db, 1, 2, permission_type, requesting_user_id - ) + result = await revoke_blueprint_permission( + dummy_db, 1, 2, permission_type, requesting_user_id + ) if permission_exists: dummy_db.delete.assert_called_once_with(mock_permission) @@ -217,20 +222,61 @@ async def test_revoke_permission( assert result is False +async def test_revoke_blueprint_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that revoke_blueprint_permission handles database errors.""" + dummy_db = DummyDB() + + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + + mock_permission = BlueprintRangePermissionModel( + blueprint_range_id=1, user_id=2, permission_type=BlueprintPermissionType.READ + ) + + mock_permission_result = Mock() + mock_permission_result.scalar_one_or_none.return_value = mock_permission + + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] + + # Force a db exception + test_except_msg = "Fake DB error!" + dummy_db.flush.side_effect = SQLAlchemyError(test_except_msg) + + requesting_user_id = 1 # Owner + + with pytest.raises(SQLAlchemyError, match=test_except_msg): + await revoke_blueprint_permission( + dummy_db, 1, 2, BlueprintPermissionType.READ, requesting_user_id + ) + + assert any( + record.levelno == logging.ERROR + and record.exc_info is not None + and test_except_msg in record.message + for record in caplog.records + ) + + +# ==================== Revoke Deployed Permissions ===================== + + @pytest.mark.parametrize( - "range_type,permission_type,expected_model_class", + "permission_type,permission_exists", [ - ("blueprint", BlueprintPermissionType.READ, BlueprintRangePermissionModel), - ("deployed", DeployedRangePermissionType.EXECUTE, DeployedRangePermissionModel), + (DeployedRangePermissionType.EXECUTE, True), + (DeployedRangePermissionType.WRITE, False), + (DeployedRangePermissionType.READ, True), ], ) -async def test_revoke_permission_db_error( - range_type: str, - permission_type: BlueprintPermissionType | DeployedRangePermissionType, - expected_model_class: type, - caplog: pytest.LogCaptureFixture, +async def test_revoke_deployed_permission( + permission_type: DeployedRangePermissionType, + permission_exists: bool, ) -> None: - """Test that revoke permission functions handle database errors.""" + """Test revoking permissions from deployed ranges.""" dummy_db = DummyDB() mock_range = Mock() @@ -238,14 +284,49 @@ async def test_revoke_permission_db_error( mock_ownership_result = Mock() mock_ownership_result.scalar_one_or_none.return_value = mock_range - if range_type == "blueprint": - mock_permission = BlueprintRangePermissionModel( - blueprint_range_id=1, user_id=2, permission_type=permission_type - ) - else: + mock_permission_result = Mock() + if permission_exists: mock_permission = DeployedRangePermissionModel( deployed_range_id=1, user_id=2, permission_type=permission_type ) + mock_permission_result.scalar_one_or_none.return_value = mock_permission + else: + mock_permission_result.scalar_one_or_none.return_value = None + + dummy_db.execute.side_effect = [mock_ownership_result, mock_permission_result] + + requesting_user_id = 1 # Owner + + result = await revoke_deployed_permission( + dummy_db, 1, 2, permission_type, requesting_user_id + ) + + if permission_exists: + dummy_db.delete.assert_called_once_with(mock_permission) + dummy_db.flush.assert_called_once() + assert result is True + else: + dummy_db.delete.assert_not_called() + dummy_db.flush.assert_not_called() + assert result is False + + +async def test_revoke_deployed_permission_db_error( + caplog: pytest.LogCaptureFixture, +) -> None: + """Test that revoke_deployed_permission handles database errors.""" + dummy_db = DummyDB() + + mock_range = Mock() + mock_range.owner_id = 1 + mock_ownership_result = Mock() + mock_ownership_result.scalar_one_or_none.return_value = mock_range + + mock_permission = DeployedRangePermissionModel( + deployed_range_id=1, + user_id=2, + permission_type=DeployedRangePermissionType.EXECUTE, + ) mock_permission_result = Mock() mock_permission_result.scalar_one_or_none.return_value = mock_permission @@ -259,14 +340,9 @@ async def test_revoke_permission_db_error( requesting_user_id = 1 # Owner with pytest.raises(SQLAlchemyError, match=test_except_msg): - if range_type == "blueprint": - await revoke_blueprint_permission( - dummy_db, 1, 2, permission_type, requesting_user_id - ) - else: - await revoke_deployed_permission( - dummy_db, 1, 2, permission_type, requesting_user_id - ) + await revoke_deployed_permission( + dummy_db, 1, 2, DeployedRangePermissionType.EXECUTE, requesting_user_id + ) assert any( record.levelno == logging.ERROR diff --git a/api/tests/unit/crud/test_crud_ranges.py b/api/tests/unit/crud/test_crud_ranges.py index 7ff47387..e1f685eb 100644 --- a/api/tests/unit/crud/test_crud_ranges.py +++ b/api/tests/unit/crud/test_crud_ranges.py @@ -780,103 +780,192 @@ def __init__( self.permission_type = permission_type -@pytest.mark.parametrize( - "range_type,has_permission,permission_type,should_access", - [ - ("blueprint", True, "read", True), - ("blueprint", False, None, False), - ("deployed", True, "read", True), - ("deployed", False, None, False), - ], -) -async def test_get_range_with_permissions( - range_type: str, - has_permission: bool, - permission_type: str | None, - should_access: bool, - mocker: MockerFixture, -) -> None: - """Test that get operations respect permission system.""" +async def test_get_blueprint_range_with_read_permission(mocker: MockerFixture) -> None: + """Test that users with read permission can access blueprint ranges.""" dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() - if range_type == "blueprint": - dummy_range = DummyBlueprintRange() - get_func = get_blueprint_range - schema_class = BlueprintRangeSchema - else: - dummy_range = DummyDeployedRange() - get_func = get_deployed_range - schema_class = DeployedRangeSchema + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + BlueprintRangeSchema, "model_validate", return_value=dummy_range + ) + + result = await get_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + mock_model_validate.assert_called_once() + + +async def test_get_blueprint_range_denied_no_permission(mocker: MockerFixture) -> None: + """Test that users without permission cannot access blueprint ranges.""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() user_id = 2 dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = ( - [MockPermission(user_id, permission_type)] if has_permission else [] + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + BlueprintRangeSchema, "model_validate", return_value=dummy_range ) + result = await get_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + mock_model_validate.assert_not_called() + + +async def test_get_deployed_range_with_read_permission(mocker: MockerFixture) -> None: + """Test that users with read permission can access deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "read")] + dummy_db.get.return_value = dummy_range mock_model_validate = mocker.patch.object( - schema_class, "model_validate", return_value=dummy_range + DeployedRangeSchema, "model_validate", return_value=dummy_range ) - result = await get_func(dummy_db, range_id=1, user_id=user_id, is_admin=False) + result = await get_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) - if should_access: - assert result is not None - mock_model_validate.assert_called_once() - else: - assert result is None - mock_model_validate.assert_not_called() + assert result is not None + mock_model_validate.assert_called_once() -@pytest.mark.parametrize( - "range_type,has_write_permission,should_delete", - [ - ("blueprint", True, True), - ("blueprint", False, False), - ("deployed", True, True), - ("deployed", False, False), - ], -) -async def test_delete_range_with_permissions( - range_type: str, - has_write_permission: bool, - should_delete: bool, +async def test_get_deployed_range_denied_no_permission(mocker: MockerFixture) -> None: + """Test that users without permission cannot access deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + DeployedRangeSchema, "model_validate", return_value=dummy_range + ) + + result = await get_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + mock_model_validate.assert_not_called() + + +async def test_delete_blueprint_range_with_write_permission( mocker: MockerFixture, ) -> None: - """Test that delete operations respect write permissions.""" + """Test that users with write permission can delete blueprint ranges.""" dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() - if range_type == "blueprint": - dummy_range = DummyBlueprintRange() - delete_func = delete_blueprint_range - header_schema = BlueprintRangeHeaderSchema - else: - dummy_range = DummyDeployedRange() - delete_func = delete_deployed_range - header_schema = DeployedRangeHeaderSchema + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + BlueprintRangeHeaderSchema, "model_validate", return_value=dummy_range + ) + + result = await delete_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is not None + dummy_db.delete.assert_called_once_with(dummy_range) + mock_model_validate.assert_called_once() + + +async def test_delete_blueprint_range_denied_no_permission( + mocker: MockerFixture, +) -> None: + """Test that users without permission cannot delete blueprint ranges.""" + dummy_db = DummyDB() + dummy_range = DummyBlueprintRange() user_id = 2 dummy_range.owner_id = 1 # Different owner - dummy_range.permissions = ( - [MockPermission(user_id, "write")] if has_write_permission else [] + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + BlueprintRangeHeaderSchema, "model_validate", return_value=dummy_range ) + result = await delete_blueprint_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + dummy_db.delete.assert_not_called() + mock_model_validate.assert_not_called() + + +async def test_delete_deployed_range_with_write_permission( + mocker: MockerFixture, +) -> None: + """Test that users with write permission can delete deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [MockPermission(user_id, "write")] + dummy_db.get.return_value = dummy_range mock_model_validate = mocker.patch.object( - header_schema, "model_validate", return_value=dummy_range + DeployedRangeHeaderSchema, "model_validate", return_value=dummy_range + ) + + result = await delete_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False ) - result = await delete_func(dummy_db, range_id=1, user_id=user_id, is_admin=False) + assert result is not None + dummy_db.delete.assert_called_once_with(dummy_range) + mock_model_validate.assert_called_once() - if should_delete: - assert result is not None - dummy_db.delete.assert_called_once_with(dummy_range) - mock_model_validate.assert_called_once() - else: - assert result is None - dummy_db.delete.assert_not_called() - mock_model_validate.assert_not_called() + +async def test_delete_deployed_range_denied_no_permission( + mocker: MockerFixture, +) -> None: + """Test that users without permission cannot delete deployed ranges.""" + dummy_db = DummyDB() + dummy_range = DummyDeployedRange() + + user_id = 2 + dummy_range.owner_id = 1 # Different owner + dummy_range.permissions = [] # No permissions + + dummy_db.get.return_value = dummy_range + mock_model_validate = mocker.patch.object( + DeployedRangeHeaderSchema, "model_validate", return_value=dummy_range + ) + + result = await delete_deployed_range( + dummy_db, range_id=1, user_id=user_id, is_admin=False + ) + + assert result is None + dummy_db.delete.assert_not_called() + mock_model_validate.assert_not_called() async def test_get_deployed_range_key_with_execute_permission() -> None: