diff --git a/DEVELOPING.md b/DEVELOPING.md index 55e7616b3..2cc6a8d70 100644 --- a/DEVELOPING.md +++ b/DEVELOPING.md @@ -115,8 +115,10 @@ function if you prefer to keep your favorite shell. ## Running Tests You can run style checks using `make style_checks`. -To run the test test suite, use `make tests` (you likely need to run in the devcontainer for this to work, as it needs -some surrounding services to run). +To run the test suite, use `make tests` (you likely need to run in the devcontainer for this to work, as it needs some +surrounding services to run). +* Run a specific test e.g.: `poetry run pytest test/bases/renku_data_services/data_api/test_data_connectors.py::test_create_openbis_data_connector` +* Also run tests marked with `@pytest.mark.myskip`: `PYTEST_FORCE_RUN_MYSKIPS=1 make tests` ## Migrations diff --git a/components/renku_data_services/data_connectors/blueprints.py b/components/renku_data_services/data_connectors/blueprints.py index f5469de13..73da71222 100644 --- a/components/renku_data_services/data_connectors/blueprints.py +++ b/components/renku_data_services/data_connectors/blueprints.py @@ -1,6 +1,7 @@ """Data connectors blueprint.""" from dataclasses import dataclass +from datetime import datetime from typing import Any from sanic import Request @@ -8,7 +9,7 @@ from sanic_ext import validate from ulid import ULID -from renku_data_services import base_models +from renku_data_services import base_models, errors from renku_data_services.base_api.auth import ( authenticate, only_authenticated, @@ -31,6 +32,7 @@ DataConnectorSecretRepository, ) from renku_data_services.storage.rclone import RCloneValidator +from renku_data_services.utils.core import get_openbis_pat @dataclass(kw_only=True) @@ -310,10 +312,55 @@ async def _patch_secrets( user: base_models.APIUser, data_connector_id: ULID, body: apispec.DataConnectorSecretPatchList, + validator: RCloneValidator, ) -> JSONResponse: unsaved_secrets = validate_data_connector_secrets_patch(put=body) + data_connector = await self.data_connector_repo.get_data_connector( + user=user, data_connector_id=data_connector_id + ) + storage = data_connector.storage + provider = validator.providers[storage.storage_type] + sensitive_lookup = [o.name for o in provider.options if o.sensitive] + for secret in unsaved_secrets: + if secret.name in sensitive_lookup: + continue + raise errors.ValidationError( + message=f"The '{secret.name}' property is not marked sensitive and can not be saved in the secret " + f"storage." + ) + expiration_timestamp = None + + if storage.storage_type == "openbis": + + async def openbis_transform_session_token_to_pat() -> ( + tuple[list[models.DataConnectorSecretUpdate], datetime] + ): + if len(unsaved_secrets) == 1 and unsaved_secrets[0].name == "session_token": + if unsaved_secrets[0].value is not None: + try: + openbis_pat = await get_openbis_pat( + storage.configuration["host"], unsaved_secrets[0].value + ) + return ( + [models.DataConnectorSecretUpdate(name="session_token", value=openbis_pat[0])], + openbis_pat[1], + ) + except Exception as e: + raise errors.ProgrammingError(message=str(e)) + raise errors.ValidationError(message="The openBIS session token must be a string value.") + + raise errors.ValidationError(message="The openBIS storage has only one secret: session_token") + + ( + unsaved_secrets, + expiration_timestamp, + ) = await openbis_transform_session_token_to_pat() + secrets = await self.data_connector_secret_repo.patch_data_connector_secrets( - user=user, data_connector_id=data_connector_id, secrets=unsaved_secrets + user=user, + data_connector_id=data_connector_id, + secrets=unsaved_secrets, + expiration_timestamp=expiration_timestamp, ) return validated_json( apispec.DataConnectorSecretsList, [self._dump_data_connector_secret(secret) for secret in secrets] diff --git a/components/renku_data_services/data_connectors/db.py b/components/renku_data_services/data_connectors/db.py index 6bda29641..4b6995296 100644 --- a/components/renku_data_services/data_connectors/db.py +++ b/components/renku_data_services/data_connectors/db.py @@ -1,6 +1,7 @@ """Adapters for data connectors database classes.""" from collections.abc import AsyncIterator, Callable +from datetime import datetime from typing import TypeVar from cryptography.hazmat.primitives.asymmetric import rsa @@ -554,7 +555,11 @@ async def get_data_connector_secrets( return [secret.dump() for secret in secrets] async def patch_data_connector_secrets( - self, user: base_models.APIUser, data_connector_id: ULID, secrets: list[models.DataConnectorSecretUpdate] + self, + user: base_models.APIUser, + data_connector_id: ULID, + secrets: list[models.DataConnectorSecretUpdate], + expiration_timestamp: datetime | None, ) -> list[models.DataConnectorSecret]: """Create, update or remove data connector secrets.""" if user.id is None: @@ -598,7 +603,9 @@ async def patch_data_connector_secrets( if data_connector_secret_orm := existing_secrets_as_dict.get(name): data_connector_secret_orm.secret.update( - encrypted_value=encrypted_value, encrypted_key=encrypted_key + encrypted_value=encrypted_value, + encrypted_key=encrypted_key, + expiration_timestamp=expiration_timestamp, ) else: secret_orm = secrets_schemas.SecretORM( @@ -607,6 +614,7 @@ async def patch_data_connector_secrets( encrypted_value=encrypted_value, encrypted_key=encrypted_key, kind=SecretKind.storage, + expiration_timestamp=expiration_timestamp, ) data_connector_secret_orm = schemas.DataConnectorSecretORM( name=name, diff --git a/components/renku_data_services/migrations/versions/57facc53ae84_add_secret_expiration_timestamp.py b/components/renku_data_services/migrations/versions/57facc53ae84_add_secret_expiration_timestamp.py new file mode 100644 index 000000000..ed88aba4f --- /dev/null +++ b/components/renku_data_services/migrations/versions/57facc53ae84_add_secret_expiration_timestamp.py @@ -0,0 +1,30 @@ +"""add secret expiration timestamp + +Revision ID: 57facc53ae84 +Revises: 08ac2714e8e2 +Create Date: 2024-11-28 10:31:05.683682 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "57facc53ae84" +down_revision = "08ac2714e8e2" +branch_labels = None +depends_on = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.add_column( + "secrets", sa.Column("expiration_timestamp", sa.DateTime(timezone=True), nullable=True), schema="secrets" + ) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_column("secrets", "expiration_timestamp", schema="secrets") + # ### end Alembic commands ### diff --git a/components/renku_data_services/notebooks/api/schemas/cloud_storage.py b/components/renku_data_services/notebooks/api/schemas/cloud_storage.py index f4c561442..865cc0ae7 100644 --- a/components/renku_data_services/notebooks/api/schemas/cloud_storage.py +++ b/components/renku_data_services/notebooks/api/schemas/cloud_storage.py @@ -210,20 +210,29 @@ def get_manifest_patch( return patches def config_string(self, name: str) -> str: - """Convert configuration oblect to string representation. + """Convert configuration object to string representation. Needed to create RClone compatible INI files. """ if not self.configuration: raise ValidationError("Missing configuration for cloud storage") - - # Transform configuration for polybox or switchDrive + # TODO Use RCloneValidator.get_real_configuration(...) instead. + # Transform configuration for polybox, switchDrive or openBIS storage_type = self.configuration.get("type", "") access = self.configuration.get("provider", "") if storage_type == "polybox" or storage_type == "switchDrive": self.configuration["type"] = "webdav" self.configuration["provider"] = "" + elif storage_type == "s3" and access == "Switch": + # Switch is a fake provider we add for users, we need to replace it since rclone itself + # doesn't know it + self.configuration["provider"] = "Other" + elif storage_type == "openbis": + self.configuration["type"] = "sftp" + self.configuration["port"] = "2222" + self.configuration["user"] = "?" + self.configuration["pass"] = self.configuration.pop("session_token", self.configuration["pass"]) if access == "shared" and storage_type == "polybox": self.configuration["url"] = "https://polybox.ethz.ch/public.php/webdav/" @@ -240,10 +249,6 @@ def config_string(self, name: str) -> str: user_identifier = public_link.split("/")[-1] self.configuration["user"] = user_identifier - if self.configuration["type"] == "s3" and self.configuration.get("provider", None) == "Switch": - # Switch is a fake provider we add for users, we need to replace it since rclone itself - # doesn't know it - self.configuration["provider"] = "Other" parser = ConfigParser() parser.add_section(name) diff --git a/components/renku_data_services/secrets/db.py b/components/renku_data_services/secrets/db.py index 2fdf8fe4e..45f32d3e9 100644 --- a/components/renku_data_services/secrets/db.py +++ b/components/renku_data_services/secrets/db.py @@ -1,10 +1,10 @@ """Database repo for secrets.""" from collections.abc import AsyncGenerator, Callable, Sequence -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta from typing import cast -from sqlalchemy import delete, select +from sqlalchemy import Select, delete, or_, select from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from ulid import ULID @@ -25,11 +25,23 @@ def __init__( ) -> None: self.session_maker = session_maker + def _get_stmt(self, requested_by: APIUser) -> Select[tuple[SecretORM]]: + return ( + select(SecretORM) + .where(SecretORM.user_id == requested_by.id) + .where( + or_( + SecretORM.expiration_timestamp.is_(None), + SecretORM.expiration_timestamp > datetime.now(UTC) + timedelta(seconds=120), + ) + ) + ) + @only_authenticated async def get_user_secrets(self, requested_by: APIUser, kind: SecretKind) -> list[Secret]: """Get all user's secrets from the database.""" async with self.session_maker() as session: - stmt = select(SecretORM).where(SecretORM.user_id == requested_by.id).where(SecretORM.kind == kind) + stmt = self._get_stmt(requested_by).where(SecretORM.kind == kind) res = await session.execute(stmt) orm = res.scalars().all() return [o.dump() for o in orm] @@ -38,7 +50,7 @@ async def get_user_secrets(self, requested_by: APIUser, kind: SecretKind) -> lis async def get_secret_by_id(self, requested_by: APIUser, secret_id: ULID) -> Secret | None: """Get a specific user secret from the database.""" async with self.session_maker() as session: - stmt = select(SecretORM).where(SecretORM.user_id == requested_by.id).where(SecretORM.id == secret_id) + stmt = self._get_stmt(requested_by).where(SecretORM.id == secret_id) res = await session.execute(stmt) orm = res.scalar_one_or_none() if orm is None: @@ -66,6 +78,7 @@ async def insert_secret(self, requested_by: APIUser, secret: UnsavedSecret) -> S encrypted_value=secret.encrypted_value, encrypted_key=secret.encrypted_key, kind=secret.kind, + expiration_timestamp=secret.expiration_timestamp, ) session.add(orm) @@ -83,19 +96,26 @@ async def insert_secret(self, requested_by: APIUser, secret: UnsavedSecret) -> S @only_authenticated async def update_secret( - self, requested_by: APIUser, secret_id: ULID, encrypted_value: bytes, encrypted_key: bytes + self, + requested_by: APIUser, + secret_id: ULID, + encrypted_value: bytes, + encrypted_key: bytes, + expiration_timestamp: datetime | None, ) -> Secret: """Update a secret.""" async with self.session_maker() as session, session.begin(): - result = await session.execute( - select(SecretORM).where(SecretORM.id == secret_id).where(SecretORM.user_id == requested_by.id) - ) + result = await session.execute(self._get_stmt(requested_by).where(SecretORM.id == secret_id)) secret = result.scalar_one_or_none() if secret is None: raise errors.MissingResourceError(message=f"The secret with id '{secret_id}' cannot be found") - secret.update(encrypted_value=encrypted_value, encrypted_key=encrypted_key) + secret.update( + encrypted_value=encrypted_value, + encrypted_key=encrypted_key, + expiration_timestamp=expiration_timestamp, + ) return secret.dump() @only_authenticated @@ -103,9 +123,7 @@ async def delete_secret(self, requested_by: APIUser, secret_id: ULID) -> None: """Delete a secret.""" async with self.session_maker() as session, session.begin(): - result = await session.execute( - select(SecretORM).where(SecretORM.id == secret_id).where(SecretORM.user_id == requested_by.id) - ) + result = await session.execute(self._get_stmt(requested_by).where(SecretORM.id == secret_id)) secret = result.scalar_one_or_none() if secret is None: return None diff --git a/components/renku_data_services/secrets/models.py b/components/renku_data_services/secrets/models.py index cc7d46e8f..491a25d01 100644 --- a/components/renku_data_services/secrets/models.py +++ b/components/renku_data_services/secrets/models.py @@ -24,6 +24,7 @@ class UnsavedSecret(BaseModel): encrypted_key: bytes = Field(repr=False) modification_date: datetime = Field(default_factory=lambda: datetime.now(UTC).replace(microsecond=0), init=False) kind: SecretKind + expiration_timestamp: datetime | None = Field(default=None) class Secret(UnsavedSecret): diff --git a/components/renku_data_services/secrets/orm.py b/components/renku_data_services/secrets/orm.py index 5f82d94a1..6b872dc94 100644 --- a/components/renku_data_services/secrets/orm.py +++ b/components/renku_data_services/secrets/orm.py @@ -35,6 +35,9 @@ class SecretORM(BaseORM): encrypted_value: Mapped[bytes] = mapped_column(LargeBinary()) encrypted_key: Mapped[bytes] = mapped_column(LargeBinary()) kind: Mapped[models.SecretKind] + expiration_timestamp: Mapped[Optional[datetime]] = mapped_column( + "expiration_timestamp", DateTime(timezone=True), default=None, nullable=True + ) modification_date: Mapped[datetime] = mapped_column( "modification_date", DateTime(timezone=True), default_factory=lambda: datetime.now(UTC).replace(microsecond=0) ) @@ -51,6 +54,7 @@ def dump(self) -> models.Secret: encrypted_value=self.encrypted_value, encrypted_key=self.encrypted_key, kind=self.kind, + expiration_timestamp=self.expiration_timestamp, ) secret.modification_date = self.modification_date return secret @@ -62,12 +66,14 @@ def load(cls, secret: models.UnsavedSecret) -> "SecretORM": name=secret.name, encrypted_value=secret.encrypted_value, encrypted_key=secret.encrypted_key, - modification_date=secret.modification_date, kind=secret.kind, + expiration_timestamp=secret.expiration_timestamp, + modification_date=secret.modification_date, ) - def update(self, encrypted_value: bytes, encrypted_key: bytes) -> None: + def update(self, encrypted_value: bytes, encrypted_key: bytes, expiration_timestamp: datetime | None) -> None: """Update an existing secret.""" self.encrypted_value = encrypted_value self.encrypted_key = encrypted_key + self.expiration_timestamp = expiration_timestamp self.modification_date = datetime.now(UTC).replace(microsecond=0) diff --git a/components/renku_data_services/storage/rclone.py b/components/renku_data_services/storage/rclone.py index 069b020b7..42b8e3fe0 100644 --- a/components/renku_data_services/storage/rclone.py +++ b/components/renku_data_services/storage/rclone.py @@ -127,6 +127,66 @@ def __patch_schema_add_switch_provider(spec: list[dict[str, Any]]) -> None: ) existing_endpoint_spec["Provider"] += ",Switch" + @staticmethod + def __patch_schema_add_openbis_type(spec: list[dict[str, Any]]) -> None: + """Adds a fake type to help with setting up openBIS storage.""" + spec.append( + { + "Name": "openbis", + "Description": "openBIS", + "Prefix": "openbis", + "Options": [ + { + "Name": "host", + "Help": 'openBIS host to connect to.\n\nE.g. "openbis-eln-lims.ethz.ch".', + "Provider": "", + "Default": "", + "Value": None, + "Examples": [ + { + "Value": "openbis-eln-lims.ethz.ch", + "Help": "Public openBIS demo instance", + "Provider": "", + }, + ], + "ShortOpt": "", + "Hide": 0, + "Required": True, + "IsPassword": False, + "NoPrefix": False, + "Advanced": False, + "Exclusive": False, + "Sensitive": False, + "DefaultStr": "", + "ValueStr": "", + "Type": "string", + }, + { + "Name": "session_token", + "Help": "openBIS session token", + "Provider": "", + "Default": "", + "Value": None, + "ShortOpt": "", + "Hide": 0, + "Required": True, + "IsPassword": True, + "NoPrefix": False, + "Advanced": False, + "Exclusive": False, + "Sensitive": True, + "DefaultStr": "", + "ValueStr": "", + "Type": "string", + }, + ], + "CommandHelp": None, + "Aliases": None, + "Hide": False, + "MetadataInfo": None, + } + ) + @staticmethod def __patch_schema_remove_oauth_propeties(spec: list[dict[str, Any]]) -> None: """Removes OAuth2 fields since we can't do an oauth flow in the rclone CSI.""" @@ -288,6 +348,34 @@ def validate(self, configuration: Union["RCloneConfig", dict[str, Any]], keep_se provider.validate_config(configuration, keep_sensitive=keep_sensitive) + def validate_sensitive_data( + self, configuration: Union["RCloneConfig", dict[str, Any]], sensitive_data: dict[str, str] + ) -> None: + """Validates whether the provided sensitive data is marked as sensitive in the rclone schema.""" + sensitive_options = self.get_provider(configuration).sensitive_options + sensitive_options_name_lookup = [o.name for o in sensitive_options] + sensitive_data_counter = 0 + for key, value in sensitive_data.items(): + if len(value) > 0 and key in sensitive_options_name_lookup: + sensitive_data_counter += 1 + continue + raise errors.ValidationError(message=f"The '{key}' property is not marked as sensitive.") + + def get_real_configuration(self, configuration: Union["RCloneConfig", dict[str, Any]]) -> dict[str, Any]: + """Converts a Renku rclone configuration to a real rclone config.""" + real_config = dict(configuration) + + if real_config["type"] == "s3" and real_config.get("provider") == "Switch": + # Switch is a fake provider we add for users, we need to replace it since rclone itself + # doesn't know it + real_config["provider"] = "Other" + elif configuration["type"] == "openbis": + real_config["type"] = "sftp" + real_config["port"] = "2222" + real_config["user"] = "?" + real_config["pass"] = real_config.pop("session_token") + return real_config + async def test_connection( self, configuration: Union["RCloneConfig", dict[str, Any]], source_path: str ) -> ConnectionResult: @@ -298,7 +386,7 @@ async def test_connection( return ConnectionResult(False, str(e)) # Obscure configuration and transform if needed - obscured_config = await self.obscure_config(configuration) + obscured_config = await self.obscure_config(self.get_real_configuration(configuration)) transformed_config = self.transform_polybox_switchdriver_config(obscured_config) with tempfile.NamedTemporaryFile(mode="w+", delete=False, encoding="utf-8") as f: @@ -308,6 +396,8 @@ async def test_connection( proc = await asyncio.create_subprocess_exec( "rclone", "lsf", + "--low-level-retries=1", # Connection tests should fail fast. + "--retries=1", # Connection tests should fail fast. "--config", f.name, f"temp:{source_path}", diff --git a/components/renku_data_services/users/api.spec.yaml b/components/renku_data_services/users/api.spec.yaml index 3bb060852..db688b719 100644 --- a/components/renku_data_services/users/api.spec.yaml +++ b/components/renku_data_services/users/api.spec.yaml @@ -418,20 +418,23 @@ components: $ref: "#/components/schemas/Ulid" name: $ref: "#/components/schemas/SecretName" - modification_date: - $ref: "#/components/schemas/ModificationDate" kind: $ref: "#/components/schemas/SecretKind" + expiration_timestamp: + $ref: "#/components/schemas/ExpirationTimestamp" + modification_date: + $ref: "#/components/schemas/ModificationDate" required: - "id" - "name" - - "modification_date" - "kind" + - "modification_date" example: id: "01AN4Z79ZS5XN0F25N3DB94T4R" name: "S3-Credentials" - modification_date: "2024-01-16T11:42:05Z" kind: general + expiration_timestamp: null + modification_date: "2024-01-16T11:42:05Z" SecretPost: description: Secret metadata to be created type: object @@ -446,6 +449,8 @@ components: - $ref: "#/components/schemas/SecretKind" - default: "general" default: general + expiration_timestamp: + $ref: "#/components/schemas/ExpirationTimestamp" required: - "name" - "value" @@ -456,6 +461,8 @@ components: properties: value: $ref: "#/components/schemas/SecretValue" + expiration_timestamp: + $ref: "#/components/schemas/ExpirationTimestamp" required: - "value" SecretName: @@ -487,6 +494,13 @@ components: enum: - general - storage + ExpirationTimestamp: + description: The date and time the secret is not valid anymore (this is in any timezone) + type: string + nullable: true + format: date-time + example: "2030-11-01T17:32:28UTC+01:00" + default: null UserPreferences: type: object description: The object containing user preferences diff --git a/components/renku_data_services/users/apispec.py b/components/renku_data_services/users/apispec.py index 5e0637b51..01cf4be08 100644 --- a/components/renku_data_services/users/apispec.py +++ b/components/renku_data_services/users/apispec.py @@ -198,12 +198,17 @@ class SecretWithId(BaseAPISpec): min_length=1, pattern="^[a-zA-Z0-9_\\-.]*$", ) + kind: SecretKind + expiration_timestamp: Optional[datetime] = Field( + None, + description="The date and time the secret is not valid anymore (this is in any timezone)", + example="2030-11-01T17:32:28UTC+01:00", + ) modification_date: datetime = Field( ..., description="The date and time the secret was created or modified (this is always in UTC)", example="2023-11-01T17:32:28Z", ) - kind: SecretKind class SecretPost(BaseAPISpec): @@ -225,6 +230,11 @@ class SecretPost(BaseAPISpec): min_length=1, ) kind: SecretKind = SecretKind.general + expiration_timestamp: Optional[datetime] = Field( + None, + description="The date and time the secret is not valid anymore (this is in any timezone)", + example="2030-11-01T17:32:28UTC+01:00", + ) class SecretPatch(BaseAPISpec): @@ -237,6 +247,11 @@ class SecretPatch(BaseAPISpec): max_length=5000, min_length=1, ) + expiration_timestamp: Optional[datetime] = Field( + None, + description="The date and time the secret is not valid anymore (this is in any timezone)", + example="2030-11-01T17:32:28UTC+01:00", + ) class PinnedProjects(BaseAPISpec): diff --git a/components/renku_data_services/users/blueprints.py b/components/renku_data_services/users/blueprints.py index 74dd7971f..ad8a48e48 100644 --- a/components/renku_data_services/users/blueprints.py +++ b/components/renku_data_services/users/blueprints.py @@ -152,7 +152,11 @@ async def _get_all( secret_kind = SecretKind[query.kind.value] secrets = await self.secret_repo.get_user_secrets(requested_by=user, kind=secret_kind) secrets_json = [ - secret.model_dump(include={"name", "id", "modification_date", "kind"}, exclude_none=True, mode="json") + secret.model_dump( + include={"id", "name", "kind", "expiration_timestamp", "modification_date"}, + exclude_none=True, + mode="json", + ) for secret in secrets ] return validated_json( @@ -173,9 +177,11 @@ async def _get_one(_: Request, user: base_models.APIUser, secret_id: ULID) -> JS if not secret: raise errors.MissingResourceError(message=f"The secret with id {secret_id} cannot be found.") result = secret.model_dump( - include={"name", "id", "modification_date", "kind"}, exclude_none=True, mode="json" + include={"id", "name", "kind", "expiration_timestamp", "modification_date"}, + exclude_none=False, + mode="json", ) - return validated_json(apispec.SecretWithId, result) + return validated_json(apispec.SecretWithId, result, exclude_none=False) return "/user/secrets/", ["GET"], _get_one @@ -197,12 +203,15 @@ async def _post(_: Request, user: base_models.APIUser, body: apispec.SecretPost) encrypted_value=encrypted_value, encrypted_key=encrypted_key, kind=SecretKind[body.kind.value], + expiration_timestamp=body.expiration_timestamp, ) inserted_secret = await self.secret_repo.insert_secret(requested_by=user, secret=secret) result = inserted_secret.model_dump( - include={"name", "id", "modification_date", "kind"}, exclude_none=True, mode="json" + include={"id", "name", "kind", "expiration_timestamp", "modification_date"}, + exclude_none=False, + mode="json", ) - return validated_json(apispec.SecretWithId, result, 201) + return validated_json(apispec.SecretWithId, result, 201, exclude_none=False) return "/user/secrets", ["POST"], _post @@ -222,13 +231,19 @@ async def _patch( secret_value=body.value, ) updated_secret = await self.secret_repo.update_secret( - requested_by=user, secret_id=secret_id, encrypted_value=encrypted_value, encrypted_key=encrypted_key + requested_by=user, + secret_id=secret_id, + encrypted_value=encrypted_value, + encrypted_key=encrypted_key, + expiration_timestamp=body.expiration_timestamp, ) result = updated_secret.model_dump( - include={"name", "id", "modification_date", "kind"}, exclude_none=True, mode="json" + include={"id", "name", "kind", "expiration_timestamp", "modification_date"}, + exclude_none=False, + mode="json", ) - return validated_json(apispec.SecretWithId, result) + return validated_json(apispec.SecretWithId, result, exclude_none=False) return "/user/secrets/", ["PATCH"], _patch diff --git a/components/renku_data_services/utils/core.py b/components/renku_data_services/utils/core.py index 4d3362bd4..587f5528d 100644 --- a/components/renku_data_services/utils/core.py +++ b/components/renku_data_services/utils/core.py @@ -4,6 +4,7 @@ import os import ssl from collections.abc import Awaitable, Callable +from datetime import datetime, timedelta from typing import Any, Concatenate, ParamSpec, Protocol, TypeVar, cast import httpx @@ -90,3 +91,76 @@ async def transaction_wrapper(self: _WithSessionMaker, *args: _P.args, **kwargs: return await f(self, *args, **kwargs) return transaction_wrapper + + +def _get_url(host: str) -> str: + return f"https://{host}/openbis/openbis/rmi-application-server-v3.json" + + +async def get_openbis_session_token( + host: str, + username: str, + password: str, + timeout: int = 12, +) -> str: + """Requests an openBIS session token with the user's login credentials.""" + login = {"method": "login", "params": [username, password], "id": "2", "jsonrpc": "2.0"} + async with httpx.AsyncClient(verify=get_ssl_context(), timeout=5) as client: + response = await client.post(_get_url(host), json=login, timeout=timeout) + if response.status_code == 200: + json: dict[str, str] = response.json() + if "result" in json: + return json["result"] + raise Exception("No session token was returned. Username and password may be incorrect.") + + raise Exception("An openBIS session token related request failed.") + + +async def get_openbis_pat( + host: str, + session_id: str, + personal_access_token_session_name: str = "renku", + minimum_validity_in_days: int = 2, + timeout: int = 12, +) -> tuple[str, datetime]: + """Requests an openBIS PAT with an openBIS session ID.""" + url = _get_url(host) + + async with httpx.AsyncClient(verify=get_ssl_context(), timeout=5) as client: + get_server_information = {"method": "getServerInformation", "params": [session_id], "id": "2", "jsonrpc": "2.0"} + response = await client.post(url, json=get_server_information, timeout=timeout) + if response.status_code == 200: + json1: dict[str, dict[str, str]] = response.json() + if "error" not in json1: + personal_access_tokens_max_validity_period = int( + json1["result"]["personal-access-tokens-max-validity-period"] + ) + valid_from = datetime.now() + valid_to = valid_from + timedelta(seconds=personal_access_tokens_max_validity_period) + validity_in_days = (valid_to - valid_from).days + if validity_in_days >= minimum_validity_in_days: + create_personal_access_tokens = { + "method": "createPersonalAccessTokens", + "params": [ + session_id, + { + "@type": "as.dto.pat.create.PersonalAccessTokenCreation", + "sessionName": personal_access_token_session_name, + "validFromDate": int(valid_from.timestamp() * 1000), + "validToDate": int(valid_to.timestamp() * 1000), + }, + ], + "id": "2", + "jsonrpc": "2.0", + } + response = await client.post(url, json=create_personal_access_tokens, timeout=timeout) + if response.status_code == 200: + json2: dict[str, list[dict[str, str]]] = response.json() + return json2["result"][0]["permId"], valid_to + else: + raise Exception( + "The maximum allowed validity period of a personal access token is less than " + f"{minimum_validity_in_days} days." + ) + + raise Exception("An openBIS personal access token related request failed.") diff --git a/test/bases/renku_data_services/data_api/conftest.py b/test/bases/renku_data_services/data_api/conftest.py index 9259ab07a..9b1514282 100644 --- a/test/bases/renku_data_services/data_api/conftest.py +++ b/test/bases/renku_data_services/data_api/conftest.py @@ -371,6 +371,39 @@ async def create_data_connector_helper( return create_data_connector_helper +@pytest_asyncio.fixture +def create_openbis_data_connector(sanic_client: SanicASGITestClient, regular_user: UserInfo, user_headers): + async def create_openbis_data_connector_helper( + name: str, session_token: str, user: UserInfo | None = None, headers: dict[str, str] | None = None, **payload + ) -> Any: + user = user or regular_user + headers = headers or user_headers + dc_payload = { + "name": name, + "description": "An openBIS data connector", + "visibility": "private", + "namespace": user.namespace.slug, + "storage": { + "configuration": { + "type": "openbis", + "host": "openbis-eln-lims.ethz.ch", # Public openBIS demo instance. + "session_token": session_token, + }, + "source_path": "/", + "target_path": "my/target", + }, + "keywords": ["keyword 1", "keyword.2", "keyword-3", "KEYWORD_4"], + } + dc_payload.update(payload) + + _, response = await sanic_client.post("/api/data/data_connectors", headers=headers, json=dc_payload) + + assert response.status_code == 201, response.text + return response.json + + return create_openbis_data_connector_helper + + @pytest_asyncio.fixture async def create_data_connector_and_link_project( sanic_client, regular_user, user_headers, admin_user, admin_headers, create_data_connector diff --git a/test/bases/renku_data_services/data_api/test_data_connectors.py b/test/bases/renku_data_services/data_api/test_data_connectors.py index 457fa472d..910702483 100644 --- a/test/bases/renku_data_services/data_api/test_data_connectors.py +++ b/test/bases/renku_data_services/data_api/test_data_connectors.py @@ -2,6 +2,7 @@ from sanic_testing.testing import SanicASGITestClient from renku_data_services.users.models import UserInfo +from renku_data_services.utils.core import get_openbis_session_token from test.bases.renku_data_services.data_api.utils import merge_headers @@ -1073,6 +1074,14 @@ async def test_patch_data_connector_secrets( assert len(secrets) == 2 assert {s["name"] for s in secrets} == {"access_key_id", "secret_access_key"} + payload = [ + {"name": "not_sensitive", "value": "not_sensitive_value"}, + ] + _, response = await sanic_client.patch( + f"/api/data/data_connectors/{data_connector_id}/secrets", headers=user_headers, json=payload + ) + assert response.status_code == 422, response.json + @pytest.mark.asyncio async def test_patch_data_connector_secrets_update_secrets( @@ -1142,7 +1151,7 @@ async def test_patch_data_connector_secrets_add_and_remove_secrets( payload = [ {"name": "access_key_id", "value": "new access key id value"}, {"name": "secret_access_key", "value": None}, - {"name": "password", "value": "password"}, + {"name": "sse_kms_key_id", "value": "password"}, ] _, response = await sanic_client.patch( f"/api/data/data_connectors/{data_connector_id}/secrets", headers=user_headers, json=payload @@ -1152,7 +1161,7 @@ async def test_patch_data_connector_secrets_add_and_remove_secrets( assert response.json is not None secrets = response.json assert len(secrets) == 2 - assert {s["name"] for s in secrets} == {"access_key_id", "password"} + assert {s["name"] for s in secrets} == {"access_key_id", "sse_kms_key_id"} new_access_key_id_secret_id = next(filter(lambda s: s["name"] == "access_key_id", secrets), None) assert new_access_key_id_secret_id == access_key_id_secret_id @@ -1162,15 +1171,14 @@ async def test_patch_data_connector_secrets_add_and_remove_secrets( assert response.json is not None secrets = response.json assert len(secrets) == 2 - assert {s["name"] for s in secrets} == {"access_key_id", "password"} + assert {s["name"] for s in secrets} == {"access_key_id", "sse_kms_key_id"} # Check the associated secrets _, response = await sanic_client.get("/api/data/user/secrets", params={"kind": "storage"}, headers=user_headers) - assert response.status_code == 200 assert response.json is not None assert len(response.json) == 2 - assert {s["name"] for s in secrets} == {"access_key_id", "password"} + assert {s["name"] for s in secrets} == {"access_key_id", "sse_kms_key_id"} @pytest.mark.asyncio @@ -1210,6 +1218,51 @@ async def test_delete_data_connector_secrets( assert response.json == [], response.json +@pytest.mark.myskip(1 == 1, reason="Depends on a remote openBIS host which may not always be available.") +@pytest.mark.asyncio +async def test_create_openbis_data_connector(sanic_client, create_openbis_data_connector, user_headers) -> None: + openbis_session_token = await get_openbis_session_token( + host="openbis-eln-lims.ethz.ch", # Public openBIS demo instance. + username="observer", + password="1234", + ) + data_connector = await create_openbis_data_connector( + "openBIS data connector 1", session_token=openbis_session_token + ) + data_connector_id = data_connector["id"] + + payload = [ + {"name": "session_token", "value": openbis_session_token}, + ] + _, response = await sanic_client.patch( + f"/api/data/data_connectors/{data_connector_id}/secrets", headers=user_headers, json=payload + ) + assert response.status_code == 200, response.json + assert {s["name"] for s in response.json} == {"session_token"} + created_secret_ids = {s["secret_id"] for s in response.json} + assert len(created_secret_ids) == 1 + assert response.json[0].keys() == {"secret_id", "name"} + + +@pytest.mark.myskip(1 == 1, reason="Depends on a remote openBIS host which may not always be available.") +@pytest.mark.asyncio +async def test_create_openbis_data_connector_with_invalid_session_token( + sanic_client, create_openbis_data_connector, user_headers +) -> None: + invalid_openbis_session_token = "1234" + data_connector = await create_openbis_data_connector("openBIS data connector 1", invalid_openbis_session_token) + data_connector_id = data_connector["id"] + + payload = [ + {"name": "session_token", "value": invalid_openbis_session_token}, + ] + _, response = await sanic_client.patch( + f"/api/data/data_connectors/{data_connector_id}/secrets", headers=user_headers, json=payload + ) + assert response.status_code == 500, response.json + assert response.json["error"]["message"] == "An openBIS personal access token related request failed." + + @pytest.mark.asyncio async def test_get_project_permissions_unauthorized( sanic_client, create_data_connector, admin_headers, admin_user, user_headers diff --git a/test/bases/renku_data_services/data_api/test_secret.py b/test/bases/renku_data_services/data_api/test_secret.py index c4b132ab9..e47304247 100644 --- a/test/bases/renku_data_services/data_api/test_secret.py +++ b/test/bases/renku_data_services/data_api/test_secret.py @@ -1,6 +1,8 @@ """Tests for secrets blueprints.""" +import time from base64 import b64decode +from datetime import datetime, timedelta from typing import Any import pytest @@ -23,8 +25,10 @@ @pytest.fixture def create_secret(sanic_client: SanicASGITestClient, user_headers): - async def create_secret_helper(name: str, value: str, kind: str = "general") -> dict[str, Any]: - payload = {"name": name, "value": value, "kind": kind} + async def create_secret_helper( + name: str, value: str, kind: str = "general", expiration_timestamp: str = None + ) -> dict[str, Any]: + payload = {"name": name, "value": value, "kind": kind, "expiration_timestamp": expiration_timestamp} _, response = await sanic_client.post("/api/data/user/secrets", headers=user_headers, json=payload) @@ -46,11 +50,32 @@ async def test_create_secrets(sanic_client: SanicASGITestClient, user_headers, k assert response.status_code == 201, response.text assert response.json is not None - assert response.json.keys() == {"name", "id", "modification_date", "kind"} + assert response.json.keys() == {"id", "name", "kind", "expiration_timestamp", "modification_date"} assert response.json["name"] == "my-secret" assert response.json["id"] is not None + assert response.json["kind"] == kind + assert response.json["expiration_timestamp"] is None assert response.json["modification_date"] is not None + + +@pytest.mark.asyncio +@pytest.mark.parametrize("kind", [e.value for e in apispec.SecretKind]) +async def test_create_secrets_with_expiration_timestamps(sanic_client: SanicASGITestClient, user_headers, kind) -> None: + payload = { + "name": "my-secret-that-expires", + "value": "42", + "kind": kind, + "expiration_timestamp": "2029-12-31T23:59:59+01:00", + } + _, response = await sanic_client.post("/api/data/user/secrets", headers=user_headers, json=payload) + assert response.status_code == 201, response.text + assert response.json is not None + assert response.json.keys() == {"id", "name", "kind", "expiration_timestamp", "modification_date"} + assert response.json["name"] == "my-secret-that-expires" + assert response.json["id"] is not None assert response.json["kind"] == kind + assert response.json["expiration_timestamp"] == "2029-12-31T23:59:59+01:00" + assert response.json["modification_date"] is not None @pytest.mark.asyncio @@ -59,15 +84,36 @@ async def test_get_one_secret(sanic_client: SanicASGITestClient, user_headers, c secret = await create_secret("secret-2", "value-2") await create_secret("secret-3", "value-3") - secret_id = secret["id"] + _, response = await sanic_client.get(f"/api/data/user/secrets/{secret["id"]}", headers=user_headers) + assert response.status_code == 200, response.text + assert response.json is not None + assert response.json["name"] == secret["name"] + assert response.json["id"] == secret["id"] + assert "value" not in response.json + + +@pytest.mark.asyncio +async def test_get_one_secret_not_expired(sanic_client: SanicASGITestClient, user_headers, create_secret) -> None: + expiration_timestamp = (datetime.now() + timedelta(seconds=(120 + 15))).isoformat() + secret_1 = await create_secret("secret-1", "value-1", expiration_timestamp=expiration_timestamp) + secret_2 = await create_secret("secret-2", "value-2", expiration_timestamp="2029-12-31") - _, response = await sanic_client.get(f"/api/data/user/secrets/{secret_id}", headers=user_headers) + _, response = await sanic_client.get(f"/api/data/user/secrets/{secret_1["id"]}", headers=user_headers) + assert response.status_code == 200, response.text + assert response.json is not None + assert response.json["name"] == "secret-1" + assert response.json["id"] == secret_1["id"] + _, response = await sanic_client.get(f"/api/data/user/secrets/{secret_2["id"]}", headers=user_headers) assert response.status_code == 200, response.text assert response.json is not None assert response.json["name"] == "secret-2" - assert response.json["id"] == secret_id - assert "value" not in response.json + assert response.json["id"] == secret_2["id"] + + time.sleep(20) + + _, response = await sanic_client.get(f"/api/data/user/secrets/{secret_1["id"]}", headers=user_headers) + assert response.status_code == 404 @pytest.mark.asyncio @@ -84,6 +130,22 @@ async def test_get_all_secrets(sanic_client: SanicASGITestClient, user_headers, assert {s["name"] for s in response.json} == {"secret-1", "secret-2", "secret-3"} +@pytest.mark.asyncio +async def test_get_all_secrets_not_expired(sanic_client: SanicASGITestClient, user_headers, create_secret) -> None: + expiration_timestamp = (datetime.now() + timedelta(seconds=10)).isoformat() + await create_secret("secret-1", "value-1", expiration_timestamp=expiration_timestamp) + await create_secret("secret-2", "value-2") + await create_secret("secret-3", "value-3", expiration_timestamp="2029-12-31") + + time.sleep(15) + + _, response = await sanic_client.get("/api/data/user/secrets", headers=user_headers) + assert response.status_code == 200, response.text + assert response.json is not None + assert {s["name"] for s in response.json} == {"secret-2", "secret-3"} + assert {s["expiration_timestamp"] for s in response.json if s["name"] == "secret-3"} == {"2029-12-31T00:00:00Z"} + + @pytest.mark.asyncio async def test_get_all_secrets_filtered_by_kind(sanic_client, user_headers, create_secret) -> None: await create_secret("secret-1", "value-1") @@ -114,14 +176,10 @@ async def test_get_delete_a_secret(sanic_client: SanicASGITestClient, user_heade secret = await create_secret("secret-2", "value-2") await create_secret("secret-3", "value-3") - secret_id = secret["id"] - - _, response = await sanic_client.delete(f"/api/data/user/secrets/{secret_id}", headers=user_headers) - + _, response = await sanic_client.delete(f"/api/data/user/secrets/{secret["id"]}", headers=user_headers) assert response.status_code == 204, response.text _, response = await sanic_client.get("/api/data/user/secrets", headers=user_headers) - assert response.status_code == 200, response.text assert response.json is not None assert {s["name"] for s in response.json} == {"secret-1", "secret-3"} @@ -133,18 +191,42 @@ async def test_get_update_a_secret(sanic_client: SanicASGITestClient, user_heade secret = await create_secret("secret-2", "value-2") await create_secret("secret-3", "value-3") - secret_id = secret["id"] - payload = {"value": "new-value"} + _, response = await sanic_client.patch( + f"/api/data/user/secrets/{secret["id"]}", headers=user_headers, json={"name": "new-name", "value": "new-value"} + ) + assert response.status_code == 422 - _, response = await sanic_client.patch(f"/api/data/user/secrets/{secret_id}", headers=user_headers, json=payload) + _, response = await sanic_client.patch( + f"/api/data/user/secrets/{secret["id"]}", headers=user_headers, json={"value": "new-value"} + ) + assert response.status_code == 200, response.text + assert response.json is not None + assert response.json["id"] == secret["id"] + assert response.json["name"] == secret["name"] + assert response.json["expiration_timestamp"] is None + assert "value" not in response.json + _, response = await sanic_client.get(f"/api/data/user/secrets/{secret["id"]}", headers=user_headers) assert response.status_code == 200, response.text + assert response.json is not None + assert response.json["id"] == secret["id"] + assert response.json["name"] == secret["name"] + assert response.json["expiration_timestamp"] is None + assert "value" not in response.json - _, response = await sanic_client.get(f"/api/data/user/secrets/{secret_id}", headers=user_headers) + _, response = await sanic_client.patch( + f"/api/data/user/secrets/{secret["id"]}", + headers=user_headers, + json={"value": "newest-value", "expiration_timestamp": "2029-12-31"}, + ) + assert response.status_code == 200, response.text + _, response = await sanic_client.get(f"/api/data/user/secrets/{secret["id"]}", headers=user_headers) assert response.status_code == 200, response.text assert response.json is not None - assert response.json["id"] == secret_id + assert response.json["id"] == secret["id"] + assert response.json["name"] == secret["name"] + assert response.json["expiration_timestamp"] == "2029-12-31T00:00:00Z" assert "value" not in response.json @@ -156,15 +238,11 @@ async def test_cannot_get_another_user_secret( secret = await create_secret("secret-2", "value-2") await create_secret("secret-3", "value-3") - secret_id = secret["id"] - - _, response = await sanic_client.get(f"/api/data/user/secrets/{secret_id}", headers=admin_headers) - + _, response = await sanic_client.get(f"/api/data/user/secrets/{secret["id"]}", headers=admin_headers) assert response.status_code == 404, response.text assert "cannot be found" in response.json["error"]["message"] _, response = await sanic_client.get("/api/data/user/secrets", headers=admin_headers) - assert response.status_code == 200, response.text assert response.json == [] diff --git a/test/bases/renku_data_services/data_api/test_storage.py b/test/bases/renku_data_services/data_api/test_storage.py index 219284a76..5cf89ef59 100644 --- a/test/bases/renku_data_services/data_api/test_storage.py +++ b/test/bases/renku_data_services/data_api/test_storage.py @@ -11,6 +11,7 @@ from renku_data_services.data_api.app import register_all_handlers from renku_data_services.migrations.core import run_migrations_for_app from renku_data_services.storage.rclone import RCloneValidator +from renku_data_services.utils.core import get_openbis_session_token from test.utils import SanicReusableASGITestClient _valid_storage: dict[str, Any] = { @@ -538,7 +539,7 @@ async def test_storage_validate_connection(storage_test_client) -> None: _, res = await storage_test_client.post("/api/data/storage_schema/test_connection", data=json.dumps(body)) assert res.status_code == 422 - body = {"configuration": {"type": "s3", "provider": "AWS"}, "source_path": "doesntexistatall/"} + body = {"configuration": {"type": "s3", "provider": "AWS"}, "source_path": "does_not_exist_at_all/"} _, res = await storage_test_client.post("/api/data/storage_schema/test_connection", data=json.dumps(body)) assert res.status_code == 422 @@ -547,6 +548,39 @@ async def test_storage_validate_connection(storage_test_client) -> None: assert res.status_code == 204 +@pytest.mark.myskip(1 == 1, reason="Depends on a remote openBIS host which may not always be available.") +@pytest.mark.asyncio +async def test_openbis_storage_validate_connection(storage_test_client) -> None: + openbis_session_token = await get_openbis_session_token( + host="openbis-eln-lims.ethz.ch", # Public openBIS demo instance. + username="observer", + password="1234", + ) + storage_test_client, _ = storage_test_client + + body = { + "configuration": { + "type": "openbis", + "host": "openbis-eln-lims.ethz.ch", + "session_token": openbis_session_token, + }, + "source_path": "does_not_exist_at_all/", + } + _, res = await storage_test_client.post("/api/data/storage_schema/test_connection", data=json.dumps(body)) + assert res.status_code == 422 + + body = { + "configuration": { + "type": "openbis", + "host": "openbis-eln-lims.ethz.ch", + "session_token": openbis_session_token, + }, + "source_path": "/", + } + _, res = await storage_test_client.post("/api/data/storage_schema/test_connection", data=json.dumps(body)) + assert res.status_code == 204 + + @pytest.mark.asyncio async def test_storage_validate_error(storage_test_client) -> None: storage_test_client, _ = storage_test_client diff --git a/test/conftest.py b/test/conftest.py index fffa7941e..788c993eb 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -252,3 +252,15 @@ def only(iterable, default=None, too_long=None): raise too_long or ValueError(msg) return first_value + + +@pytest.hookimpl(tryfirst=True) +def pytest_runtest_setup(item): + mark = item.get_closest_marker(name="myskip") + if mark: + condition = next(iter(mark.args), True) + reason = mark.kwargs.get("reason") + item.add_marker( + pytest.mark.skipif(not os.getenv("PYTEST_FORCE_RUN_MYSKIPS", False) and condition, reason=reason), + append=False, + )