diff --git a/backend/api/signals.py b/backend/api/signals.py index 723863635..92e197635 100644 --- a/backend/api/signals.py +++ b/backend/api/signals.py @@ -1,9 +1,20 @@ +import logging +import time +import weakref + from allauth.account.signals import user_signed_up +from django.db import transaction from django.db.models.signals import pre_delete from django.dispatch import receiver from django.conf import settings from backend.api.notifier import notify_slack -from api.models import RotatingSecret, RotatingSecretCredential +from api.models import DynamicSecretLease, RotatingSecret, RotatingSecretCredential + +logger = logging.getLogger(__name__) + +# Keeps one slow or unreachable provider from pushing a delete past worker timeouts. +CASCADE_REVOKE_BUDGET_SECONDS = 20 +_cascade_revoke_state = {} CLOUD_HOSTED = settings.APP_HOST == "cloud" @@ -50,3 +61,73 @@ def _rotating_secret_pre_delete(sender, instance, **kwargs): revoke_credential(cred.id, immediate=True) except Exception: pass + + +def _cascade_revoke_budget(origin): + # One budget per delete attempt: a retry has a new origin object or transaction. + atomic_blocks = transaction.get_connection().atomic_blocks + outermost = atomic_blocks[0] if atomic_blocks else None + owners = [owner for owner in (origin, outermost) if owner is not None] + key = tuple(id(owner) for owner in owners) + state = _cascade_revoke_state.get(key) if owners else None + if state is None: + state = { + "deadline": time.monotonic() + CASCADE_REVOKE_BUDGET_SECONDS, + "unreachable_authentication_ids": set(), + } + try: + for owner in owners: + weakref.finalize(owner, _cascade_revoke_state.pop, key, None) + except TypeError: + return state + if owners: + _cascade_revoke_state[key] = state + return state + + +def _log_unrevoked_lease(lease, reason, exc_info=False): + from ee.integrations.secrets.dynamic.utils import lease_iam_username + + logger.error( + "Dynamic secret lease %s was not revoked during cascade delete (%s); " + "delete IAM user %s manually (secret %s, environment %s, credential %s, " + "expires %s)", + lease.id, + reason, + lease_iam_username(lease) or "", + lease.secret_id, + lease.secret.environment_id, + lease.secret.authentication_id, + lease.expires_at, + exc_info=exc_info, + ) + + +@receiver(pre_delete, sender=DynamicSecretLease) +def _dynamic_secret_lease_pre_delete(sender, instance, origin=None, **kwargs): + # Cascades skip DynamicSecret.delete(); a queued revoke job would find no row. + if instance.status != DynamicSecretLease.ACTIVE: + return + + from ee.integrations.secrets.dynamic.utils import ( + is_provider_unreachable_error, + revoke_lease_immediately, + ) + + budget = _cascade_revoke_budget(origin) + authentication_id = instance.secret.authentication_id + if time.monotonic() > budget["deadline"]: + _log_unrevoked_lease(instance, "revocation time budget exhausted") + return + if authentication_id in budget["unreachable_authentication_ids"]: + _log_unrevoked_lease(instance, "provider unreachable earlier in this delete") + return + + try: + # Savepoint so a swallowed DB error can't abort the cascade transaction. + with transaction.atomic(): + revoke_lease_immediately(instance) + except Exception as exc: + if is_provider_unreachable_error(exc): + budget["unreachable_authentication_ids"].add(authentication_id) + _log_unrevoked_lease(instance, "revocation failed", exc_info=True) diff --git a/backend/api/utils/access/permissions.py b/backend/api/utils/access/permissions.py index 0637db0cf..3057e481b 100644 --- a/backend/api/utils/access/permissions.py +++ b/backend/api/utils/access/permissions.py @@ -91,6 +91,29 @@ def user_can_access_environment(user_id, env_id): ).exists() +def accessible_environment_ids(user_id, **env_filters): + """Env ids the user holds keys for — one query instead of one per env.""" + EnvironmentKey = apps.get_model("api", "EnvironmentKey") + + return set( + EnvironmentKey.objects.filter( + user__user_id=user_id, + user__deleted_at=None, + deleted_at=None, + **env_filters, + ).values_list("environment_id", flat=True) + ) + + +def request_accessible_env_ids(info): + """Caller's env keys, memoized per request — nested fields resolve per row.""" + env_ids = getattr(info.context, "_accessible_env_ids", None) + if env_ids is None: + env_ids = accessible_environment_ids(info.context.user.userId) + setattr(info.context, "_accessible_env_ids", env_ids) + return env_ids + + def service_account_can_access_environment(account_id, env_id): Environment = apps.get_model("api", "Environment") EnvironmentKey = apps.get_model("api", "EnvironmentKey") diff --git a/backend/api/views/secrets.py b/backend/api/views/secrets.py index ebf157b38..223bc539d 100644 --- a/backend/api/views/secrets.py +++ b/backend/api/views/secrets.py @@ -41,6 +41,8 @@ ) from ee.integrations.secrets.dynamic.serializers import DynamicSecretSerializer from ee.integrations.secrets.dynamic.utils import ( + LEASE_CREATE_PERMISSION_ERROR, + can_create_dynamic_secret_lease, create_dynamic_secret_lease, ) from rest_framework.views import APIView @@ -278,9 +280,20 @@ def get(self, request, *args, **kwargs): service_account = request.auth["service_account_token"].service_account if include_lease: + dynamic_secrets = list(dynamic_secrets_qs) + # The CLI always requests leases; only deny when one would be minted. + if dynamic_secrets and not can_create_dynamic_secret_lease( + env, + organisation_member=request.auth.get("org_member"), + service_account=service_account, + ): + return Response( + {"error": LEASE_CREATE_PERMISSION_ERROR}, status=403 + ) + leases_by_secret_id = {} failed_leases = [] - for ds in dynamic_secrets_qs: + for ds in dynamic_secrets: try: lease, _ = create_dynamic_secret_lease( ds, @@ -334,7 +347,7 @@ def get(self, request, *args, **kwargs): "lease_id": leases_by_secret_id.get(ds.id), }, ).data - for ds in dynamic_secrets_qs + for ds in dynamic_secrets ] else: # Serialize without lease @@ -811,9 +824,20 @@ def get(self, request, *args, **kwargs): service_account = request.auth["service_account_token"].service_account if include_lease: + dynamic_secrets = list(dynamic_secrets_qs) + # The CLI always requests leases; only deny when one would be minted. + if dynamic_secrets and not can_create_dynamic_secret_lease( + env, + organisation_member=request.auth.get("org_member"), + service_account=service_account, + ): + return Response( + {"error": LEASE_CREATE_PERMISSION_ERROR}, status=403 + ) + leases_by_secret_id = {} failed_leases = [] - for ds in dynamic_secrets_qs: + for ds in dynamic_secrets: try: lease, _ = create_dynamic_secret_lease( ds, @@ -875,7 +899,7 @@ def get(self, request, *args, **kwargs): "lease_id": leases_by_secret_id.get(ds.id), }, ).data - for ds in dynamic_secrets_qs + for ds in dynamic_secrets ] else: # Serialize without lease diff --git a/backend/backend/graphene/mutations/account.py b/backend/backend/graphene/mutations/account.py index d9ab4c61a..4597d9fb1 100644 --- a/backend/backend/graphene/mutations/account.py +++ b/backend/backend/graphene/mutations/account.py @@ -77,44 +77,16 @@ def revoke_lease_now(lease): deletes the row leaves the scheduled job re-fetching a gone id, leaking the provider credential forever. """ - import django_rq - - from ee.integrations.secrets.dynamic.exceptions import LeaseAlreadyRevokedError - - if lease.secret.provider != "aws": - logger.warning( - "Unknown dynamic secret provider %s for lease %s — skipping revoke", - lease.secret.provider, - lease.id, - ) - return - - from ee.integrations.secrets.dynamic.aws.utils import ( - revoke_aws_dynamic_secret_lease, - ) + from ee.integrations.secrets.dynamic.utils import revoke_lease_immediately try: - revoke_aws_dynamic_secret_lease(lease.id, manual=True) - except LeaseAlreadyRevokedError: - pass # idempotent retry + revoke_lease_immediately(lease) except Exception: logger.exception("Failed to revoke dynamic secret lease %s", lease.id) raise GraphQLError( "Failed to revoke active dynamic credentials. Please try again." ) - if lease.cleanup_job_id: - try: - scheduler = django_rq.get_scheduler("scheduled-jobs") - scheduler.cancel(lease.cleanup_job_id) - except Exception: - # Best-effort: the orphaned job no-ops against a revoked lease. - logger.warning( - "Failed to cancel cleanup job %s for lease %s", - lease.cleanup_job_id, - lease.id, - ) - class DeleteAccountMutation(graphene.Mutation): """Permanently delete the session user's account. Type-to-confirm is diff --git a/backend/backend/graphene/mutations/environment.py b/backend/backend/graphene/mutations/environment.py index 565346466..b345bcca7 100644 --- a/backend/backend/graphene/mutations/environment.py +++ b/backend/backend/graphene/mutations/environment.py @@ -401,6 +401,10 @@ def mutate(cls, root, info, environment_id): ): raise GraphQLError("You do not have permission to delete environments") + # Environments:delete is app-wide (incl. team overrides); env keys set scope. + if not user_can_access_environment(user.userId, environment.id): + raise GraphQLError("You don't have access to this environment") + # An env that contains live rotating secrets can't be deleted without # RotatingSecrets:delete — otherwise a caller with only Environments:delete # could destroy a rotation config (and its provider creds via cascade). diff --git a/backend/ee/integrations/secrets/dynamic/aws/utils.py b/backend/ee/integrations/secrets/dynamic/aws/utils.py index a2d7eac57..9b5c4ec93 100644 --- a/backend/ee/integrations/secrets/dynamic/aws/utils.py +++ b/backend/ee/integrations/secrets/dynamic/aws/utils.py @@ -355,7 +355,7 @@ def create_access_key(username, iam_client): raise -def get_sts_client(region="us-east-1"): +def get_sts_client(region="us-east-1", config=None): aws_access_key_id = get_secret("AWS_INTEGRATION_ACCESS_KEY_ID") aws_secret_access_key = get_secret("AWS_INTEGRATION_SECRET_ACCESS_KEY") @@ -368,9 +368,10 @@ def get_sts_client(region="us-east-1"): region_name=region, aws_access_key_id=aws_access_key_id, aws_secret_access_key=aws_secret_access_key, + config=config, ) else: - sts_client = boto3.client("sts", region_name=region) + sts_client = boto3.client("sts", region_name=region, config=config) return sts_client @@ -378,12 +379,12 @@ def get_sts_client(region="us-east-1"): import boto3 -def get_iam_client(secret: DynamicSecret) -> tuple[boto3.client, dict]: +def get_iam_client(secret: DynamicSecret, config=None) -> tuple[boto3.client, dict]: """ Construct an IAM client using the given DynamicSecret's authentication config. Returns (iam_client, aws_credentials). """ - sts_client = get_sts_client() + sts_client = get_sts_client(config=config) # Determine authentication method has_role_arn = "role_arn" in secret.authentication.credentials @@ -418,6 +419,7 @@ def get_iam_client(secret: DynamicSecret) -> tuple[boto3.client, dict]: "region_name": region, "aws_access_key_id": aws_credentials["AccessKeyId"], "aws_secret_access_key": aws_credentials["SecretAccessKey"], + "config": config, } if "SessionToken" in aws_credentials: iam_client_kwargs["aws_session_token"] = aws_credentials["SessionToken"] @@ -619,6 +621,7 @@ def revoke_aws_dynamic_secret_lease( request=None, organisation_member=None, service_account=None, + client_config=None, ): """ Delete IAM user and all associated credentials. @@ -640,7 +643,7 @@ def revoke_aws_dynamic_secret_lease( logger.info(f"Revoking lease {lease.id} (manual={manual})") - iam_client, _ = get_iam_client(lease.secret) + iam_client, _ = get_iam_client(lease.secret, config=client_config) meta = { "action": "revoke", diff --git a/backend/ee/integrations/secrets/dynamic/graphene/mutations.py b/backend/ee/integrations/secrets/dynamic/graphene/mutations.py index eaa4cc008..d3f68abd8 100644 --- a/backend/ee/integrations/secrets/dynamic/graphene/mutations.py +++ b/backend/ee/integrations/secrets/dynamic/graphene/mutations.py @@ -30,6 +30,21 @@ logger = logging.getLogger(__name__) +def _get_lease_and_member(user, lease_id): + # Same error for unknown and foreign leases so ids can't be probed across orgs. + lease = DynamicSecretLease.objects.filter(id=lease_id).first() + org_member = None + if lease is not None: + org_member = OrganisationMember.objects.filter( + organisation=lease.secret.environment.app.organisation, + user=user, + deleted_at=None, + ).first() + if org_member is None: + raise GraphQLError("Lease not found") + return lease, org_member + + class DeleteDynamicSecretMutation(graphene.Mutation): class Arguments: secret_id = graphene.ID(required=True) @@ -54,6 +69,10 @@ def mutate( "You don't have permission to delete secrets in this organisation" ) + # Deleting revokes every active lease, so app access isn't enough. + if not user_can_access_environment(user.userId, secret.environment_id): + raise GraphQLError("You don't have access to this environment") + secret.delete() return DeleteDynamicSecretMutation(ok=True) @@ -85,13 +104,23 @@ def mutate( if not user_is_org_member(user.userId, org.id): raise GraphQLError("You don't have access to this organisation") - if not user_has_permission(user, "create", "Secrets", org, True, app=secret.environment.app): - raise GraphQLError("You don't have permission to create Dynamic Secrets") + app = secret.environment.app + if not user_has_permission(user, "read", "Secrets", org, True, app=app): + raise GraphQLError("You don't have permission to read secrets in this app") + + if not user_has_permission( + user, "create", "DynamicSecretLeases", org, True, app=app + ): + raise GraphQLError( + "You don't have permission to create dynamic secret leases" + ) if not user_can_access_environment(user.userId, secret.environment.id): raise GraphQLError("You don't have access to this environment") - org_member = OrganisationMember.objects.get(organisation=org, user=user) + org_member = OrganisationMember.objects.get( + organisation=org, user=user, deleted_at=None + ) # create lease lease_name = secret.name if name is None else name @@ -124,25 +153,20 @@ def mutate( user = info.context.user - lease = DynamicSecretLease.objects.get(id=lease_id) - org = lease.secret.environment.app.organisation - org_member = OrganisationMember.objects.get(organisation=org, user=user) + lease, org_member = _get_lease_and_member(user, lease_id) + env = lease.secret.environment + org = env.app.organisation # --- permission checks --- - if not user_is_org_member(user.userId, org.id): - raise GraphQLError("You don't have access to this organisation") - - if ( - lease.organisation_member is None - or lease.organisation_member.id != org_member.id - ) and not user_has_permission( - info.context.user, "update", "DynamicSecretLeases", org, True, app=lease.secret.environment.app + is_holder = lease.organisation_member_id == org_member.id + if not is_holder and not user_has_permission( + user, "update", "DynamicSecretLeases", org, True, app=env.app ): raise GraphQLError( "You cannot renew this lease as it wasn't created by you" ) - if not user_can_access_environment(user.userId, lease.secret.environment.id): + if not user_can_access_environment(user.userId, env.id): raise GraphQLError("You don't have access to this environment") lease = renew_dynamic_secret_lease( @@ -168,34 +192,28 @@ def mutate( user = info.context.user - lease = DynamicSecretLease.objects.get(id=lease_id) - org = lease.secret.environment.app.organisation - org_member = OrganisationMember.objects.get(organisation=org, user=user) + lease, org_member = _get_lease_and_member(user, lease_id) + env = lease.secret.environment + org = env.app.organisation # --- permission checks --- - if not user_is_org_member(user.userId, org.id): - raise GraphQLError("You don't have access to this organisation") - - if ( - lease.organisation_member is None - or lease.organisation_member.id != org_member.id - ) and not user_has_permission( - info.context.user, "delete", "DynamicSecretLeases", org, True, app=lease.secret.environment.app + is_holder = lease.organisation_member_id == org_member.id + if not is_holder and not user_has_permission( + user, "delete", "DynamicSecretLeases", org, True, app=env.app ): raise GraphQLError( "You cannot revoke this lease as it wasn't created by you" ) - if not user_can_access_environment(user.userId, lease.secret.environment.id): + if not user_can_access_environment(user.userId, env.id): raise GraphQLError("You don't have access to this environment") - else: - if lease.secret.provider == "aws": - revoke_aws_dynamic_secret_lease( - lease.id, - organisation_member=org_member, - manual=True, - request=info.context, - ) + if lease.secret.provider == "aws": + revoke_aws_dynamic_secret_lease( + lease.id, + organisation_member=org_member, + manual=True, + request=info.context, + ) return RevokeLeaseMutation(lease=lease) diff --git a/backend/ee/integrations/secrets/dynamic/graphene/queries.py b/backend/ee/integrations/secrets/dynamic/graphene/queries.py index ffd61302e..f19e1252f 100644 --- a/backend/ee/integrations/secrets/dynamic/graphene/queries.py +++ b/backend/ee/integrations/secrets/dynamic/graphene/queries.py @@ -7,6 +7,7 @@ from graphql import GraphQLError from api.models import DynamicSecret, App, Environment, Organisation from api.utils.access.permissions import ( + accessible_environment_ids, user_has_permission, user_can_access_app, user_can_access_environment, @@ -74,7 +75,16 @@ def resolve_dynamic_secrets( if not user_can_access_app(user.userId, app_id): raise GraphQLError("You don't have access to this app") - filters.update({"environment__app__id": app_id}) + # App access alone would leak dynamic secrets — and their leases — + # from envs the caller isn't provisioned for. + filters.update( + { + "environment__app__id": app_id, + "environment_id__in": accessible_environment_ids( + user.userId, environment__app_id=app_id + ), + } + ) return DynamicSecret.objects.filter(**filters) if env_id: @@ -85,7 +95,14 @@ def resolve_dynamic_secrets( return DynamicSecret.objects.filter(**filters) if org_id: - filters.update({"environment__app__organisation_id": org_id}) + filters.update( + { + "environment__app__organisation_id": org_id, + "environment_id__in": accessible_environment_ids( + user.userId, environment__app__organisation_id=org_id + ), + } + ) return [ ds for ds in DynamicSecret.objects.filter(**filters) diff --git a/backend/ee/integrations/secrets/dynamic/graphene/types.py b/backend/ee/integrations/secrets/dynamic/graphene/types.py index cc2084c58..1763b3df3 100644 --- a/backend/ee/integrations/secrets/dynamic/graphene/types.py +++ b/backend/ee/integrations/secrets/dynamic/graphene/types.py @@ -4,7 +4,10 @@ DynamicSecretLeaseEvent, OrganisationMember, ) -from api.utils.access.permissions import user_has_permission +from api.utils.access.permissions import ( + request_accessible_env_ids, + user_has_permission, +) import graphene from graphene_django import DjangoObjectType from graphene.types.generic import GenericScalar @@ -82,6 +85,10 @@ def resolve_max_ttl_seconds(self, info): return int(self.max_ttl.total_seconds()) if self.max_ttl else None def resolve_leases(self, info): + # Nested field: the parent may be reachable without env access. + if self.environment_id not in request_accessible_env_ids(info): + return self.leases.none() + filter = {} if not user_has_permission( info.context.user, @@ -92,7 +99,9 @@ def resolve_leases(self, info): app=self.environment.app, ): filter["organisation_member"] = OrganisationMember.objects.get( - organisation=self.environment.app.organisation, user=info.context.user + organisation=self.environment.app.organisation, + user=info.context.user, + deleted_at=None, ) return self.leases.filter(**filter).order_by("-created_at") diff --git a/backend/ee/integrations/secrets/dynamic/rest/views.py b/backend/ee/integrations/secrets/dynamic/rest/views.py index 57c815055..c981ba841 100644 --- a/backend/ee/integrations/secrets/dynamic/rest/views.py +++ b/backend/ee/integrations/secrets/dynamic/rest/views.py @@ -31,6 +31,8 @@ LeaseAlreadyRevokedError, ) from ee.integrations.secrets.dynamic.utils import ( + LEASE_CREATE_PERMISSION_ERROR, + can_create_dynamic_secret_lease, create_dynamic_secret_lease, renew_dynamic_secret_lease, ) @@ -148,6 +150,11 @@ def get(self, request, *args, **kwargs): service_account = request.auth["service_account"] if include_lease: + if not can_create_dynamic_secret_lease( + env, organisation_member=org_member, service_account=service_account + ): + return Response({"error": LEASE_CREATE_PERMISSION_ERROR}, status=403) + leases_by_secret_id = {} for ds in dynamic_secrets: try: @@ -223,16 +230,24 @@ def _get_lease_or_404(self, lease_id: str) -> DynamicSecretLease: except DynamicSecretLease.DoesNotExist: raise NotFound("Lease not found") + def _is_lease_holder(self, request, lease: DynamicSecretLease) -> bool: + # Compare like with like: member and service account ids share one id space. + if request.auth["auth_type"] == "User": + member = request.auth["org_member"] + return member is not None and lease.organisation_member_id == member.id + if request.auth["auth_type"] == "ServiceAccount": + service_account = request.auth["service_account"] + return ( + service_account is not None + and lease.service_account_id == service_account.id + ) + return False + def _assert_can_act_on_lease(self, request, lease: DynamicSecretLease, action: str): # action: "update" for renew, "delete" for revoke - account, organisation = self._get_account_and_org(request) - lease_holder = lease.organisation_member or lease.service_account - if ( - lease_holder - and hasattr(lease_holder, "id") - and lease_holder.id == getattr(account, "id", None) - ): + if self._is_lease_holder(request, lease): return + account, organisation = self._get_account_and_org(request) env = request.auth["environment"] if not user_has_permission( account, @@ -270,7 +285,7 @@ def get(self, request, *args, **kwargs): ) try: - secret = DynamicSecret.objects.get(id=secret_id) + secret = DynamicSecret.objects.get(id=secret_id, environment=env) except DynamicSecret.DoesNotExist: return Response({"error": "Secret not found"}, status=404) diff --git a/backend/ee/integrations/secrets/dynamic/utils.py b/backend/ee/integrations/secrets/dynamic/utils.py index 35130f38a..ad74efbb5 100644 --- a/backend/ee/integrations/secrets/dynamic/utils.py +++ b/backend/ee/integrations/secrets/dynamic/utils.py @@ -6,6 +6,7 @@ create_environment_folder_structure, get_environment_keys, ) +from api.utils.access.permissions import user_has_permission from api.utils.crypto import decrypt_asymmetric from api.models import DynamicSecretLease, DynamicSecretLeaseEvent from api.utils.rest import get_resolver_request_meta @@ -26,6 +27,14 @@ from graphql import GraphQLError from django.utils import timezone import django_rq +from botocore.config import Config +from botocore.exceptions import ( + ConnectionClosedError, + ConnectionError as BotoConnectionError, + ReadTimeoutError, +) +from django.db import transaction +from rq.exceptions import NoSuchJobError from rq.job import Job import logging from django.apps import apps @@ -34,6 +43,17 @@ DynamicSecret = apps.get_model("api", "DynamicSecret") +LEASE_CREATE_PERMISSION_ERROR = ( + "You don't have permission to create dynamic secret leases in this environment." +) + +# Revocations that run inside a request must fail fast when AWS is unreachable. +IN_REQUEST_REVOKE_CLIENT_CONFIG = Config( + connect_timeout=5, + read_timeout=15, + retries={"mode": "standard", "total_max_attempts": 2}, +) + def validate_key_map(key_map, provider, environment, path, dynamic_secret_id=None): provider_def = None @@ -186,6 +206,27 @@ def create_dynamic_secret( return dynamic_secret +def can_create_dynamic_secret_lease( + environment, organisation_member=None, service_account=None +): + if service_account is not None: + account, is_service_account = service_account, True + elif organisation_member is not None: + account, is_service_account = organisation_member.user, False + else: + # Legacy service tokens have no account that could hold a lease. + return False + return user_has_permission( + account, + "create", + "DynamicSecretLeases", + environment.app.organisation, + True, + is_service_account, + app=environment.app, + ) + + def create_dynamic_secret_lease( secret, lease_name=None, @@ -265,6 +306,17 @@ def renew_dynamic_secret_lease( "Dynamic secrets are only available on the Enterprise plan." ) + if not isinstance(ttl, int) or isinstance(ttl, bool) or ttl <= 0: + raise DynamicSecretError("ttl must be a positive integer (seconds)") + + if lease.revoked_at is not None: + raise LeaseAlreadyRevokedError( + "This lease has been revoked and cannot be renewed" + ) + + if lease.secret.deleted_at is not None: + raise LeaseRenewalError("This dynamic secret has been deleted") + # Check if adding this renewal would exceed max TTL current_ttl_seconds = lease.ttl.total_seconds() new_total_ttl = current_ttl_seconds + ttl @@ -292,22 +344,14 @@ def renew_dynamic_secret_lease( lease.updated_at = timezone.now() # --- reschedule cleanup job --- - scheduler = django_rq.get_scheduler("scheduled-jobs") + old_job_id = lease.cleanup_job_id - # cancel the old job if it exists - if lease.cleanup_job_id: - try: - old_job = Job.fetch(lease.cleanup_job_id, connection=scheduler.connection) - old_job.cancel() - except Exception as e: - logger.info(f"Failed to delete job: {e}") - pass - - lease.save() - - # enqueue a new revocation job + # Enqueue first (this saves the new expiry) so a failure keeps the old job. schedule_lease_revocation(lease) + if old_job_id: + cancel_scheduled_lease_job(old_job_id, lease.id) + # record renewal event ip_address, user_agent = (None, None) if request is not None: @@ -367,3 +411,75 @@ def schedule_lease_revocation(lease, immediate=False): lease.cleanup_job_id = job.id lease.save() + + +def cancel_scheduled_lease_job(job_id, lease_id): + """Best-effort removal of a lease's scheduled revocation job.""" + try: + scheduler = django_rq.get_scheduler("scheduled-jobs") + # scheduler.cancel() only unschedules; the job hash must be deleted separately. + scheduler.cancel(job_id) + Job.fetch(job_id, connection=scheduler.connection).delete( + remove_from_queue=False + ) + except NoSuchJobError: + pass + except Exception: + logger.warning( + "Failed to cancel cleanup job %s for lease %s", + job_id, + lease_id, + exc_info=True, + ) + + +def is_provider_unreachable_error(exc): + """True when a revoke failed on connectivity rather than on the lease itself.""" + while exc is not None: + if isinstance( + exc, (BotoConnectionError, ReadTimeoutError, ConnectionClosedError) + ): + return True + exc = exc.__cause__ or exc.__context__ + return False + + +def lease_iam_username(lease): + """Best-effort plaintext IAM username for logs that ask for manual revocation.""" + try: + env_pubkey, env_privkey = get_environment_keys(lease.secret.environment_id) + return decrypt_asymmetric( + lease.credentials.get("username"), env_privkey, env_pubkey + ) + except Exception: + return None + + +def revoke_lease_immediately(lease): + """Revoke a lease at the provider now and drop its scheduled revocation job. + + Raises on provider failure; an already-revoked lease is a no-op. + """ + if lease.secret.provider != "aws": + logger.warning( + "Unknown dynamic secret provider %s for lease %s, skipping revoke", + lease.secret.provider, + lease.id, + ) + return + + from ee.integrations.secrets.dynamic.aws.utils import ( + revoke_aws_dynamic_secret_lease, + ) + + try: + revoke_aws_dynamic_secret_lease( + lease.id, manual=True, client_config=IN_REQUEST_REVOKE_CLIENT_CONFIG + ) + except LeaseAlreadyRevokedError: + pass + + job_id, lease_id = lease.cleanup_job_id, lease.id + if job_id: + # On rollback the kept job finds the IAM user gone and marks the lease expired. + transaction.on_commit(lambda: cancel_scheduled_lease_job(job_id, lease_id)) diff --git a/backend/tests/api/mutations/test_delete_account.py b/backend/tests/api/mutations/test_delete_account.py index 2cecd7fc6..79597428a 100644 --- a/backend/tests/api/mutations/test_delete_account.py +++ b/backend/tests/api/mutations/test_delete_account.py @@ -5,6 +5,8 @@ from unittest.mock import MagicMock, patch, ANY from graphql import GraphQLError +from ee.integrations.secrets.dynamic.utils import IN_REQUEST_REVOKE_CLIENT_CONFIG + def _make_user(email="alice@example.com"): user = MagicMock() @@ -110,6 +112,15 @@ def test_active_scim_management_blocks(self, mock_om, mock_scim): class TestRevokeLeaseNow: + @pytest.fixture(autouse=True) + def _run_on_commit_immediately(self, monkeypatch): + # Account deletion revokes outside a transaction, where on_commit runs at once. + monkeypatch.setattr( + "ee.integrations.secrets.dynamic.utils.transaction.on_commit", + lambda fn: fn(), + ) + monkeypatch.setattr("ee.integrations.secrets.dynamic.utils.Job", MagicMock()) + def _lease(self, provider="aws"): lease = MagicMock() lease.id = "lease-1" @@ -125,7 +136,9 @@ def test_revokes_synchronously_and_cancels_job(self, mock_revoke, mock_scheduler lease = self._lease() revoke_lease_now(lease) - mock_revoke.assert_called_once_with("lease-1", manual=True) + mock_revoke.assert_called_once_with( + "lease-1", manual=True, client_config=IN_REQUEST_REVOKE_CLIENT_CONFIG + ) mock_scheduler.return_value.cancel.assert_called_once_with("job-1") @patch("django_rq.get_scheduler") @@ -156,6 +169,17 @@ def test_unknown_provider_is_skipped(self, mock_revoke, mock_scheduler): mock_revoke.assert_not_called() mock_scheduler.return_value.cancel.assert_not_called() + @patch("django_rq.get_scheduler") + @patch("ee.integrations.secrets.dynamic.aws.utils.revoke_aws_dynamic_secret_lease") + def test_cancel_failure_after_revoke_does_not_fail_deletion( + self, mock_revoke, mock_scheduler + ): + from backend.graphene.mutations.account import revoke_lease_now + + mock_scheduler.return_value.cancel.side_effect = ConnectionError("redis down") + revoke_lease_now(self._lease()) # must not raise + mock_revoke.assert_called_once() + # --------------------------------------------------------------------------- # DeleteAccountMutation diff --git a/backend/tests/api/utils/test_env_access_helpers.py b/backend/tests/api/utils/test_env_access_helpers.py new file mode 100644 index 000000000..5b6f4eea7 --- /dev/null +++ b/backend/tests/api/utils/test_env_access_helpers.py @@ -0,0 +1,31 @@ +"""Per-request memoization of the caller's environment keys.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +from api.utils.access import permissions + + +def _info(): + return SimpleNamespace(context=SimpleNamespace(user=SimpleNamespace(userId="u1"))) + + +def test_request_accessible_env_ids_queries_once_per_request(monkeypatch): + lookup = MagicMock(return_value={"env-1"}) + monkeypatch.setattr(permissions, "accessible_environment_ids", lookup) + info = _info() + + assert permissions.request_accessible_env_ids(info) == {"env-1"} + assert permissions.request_accessible_env_ids(info) == {"env-1"} + + lookup.assert_called_once_with("u1") + + +def test_request_accessible_env_ids_is_not_shared_between_requests(monkeypatch): + lookup = MagicMock(side_effect=[{"env-1"}, {"env-2"}]) + monkeypatch.setattr(permissions, "accessible_environment_ids", lookup) + + assert permissions.request_accessible_env_ids(_info()) == {"env-1"} + assert permissions.request_accessible_env_ids(_info()) == {"env-2"} + + assert lookup.call_count == 2 diff --git a/backend/tests/ee/integrations/secrets/dynamic/__init__.py b/backend/tests/ee/integrations/secrets/dynamic/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/backend/tests/ee/integrations/secrets/dynamic/test_dynamic_secret_env_scoping.py b/backend/tests/ee/integrations/secrets/dynamic/test_dynamic_secret_env_scoping.py new file mode 100644 index 000000000..f7c0b3e85 --- /dev/null +++ b/backend/tests/ee/integrations/secrets/dynamic/test_dynamic_secret_env_scoping.py @@ -0,0 +1,182 @@ +"""Dynamic secret reads and deletes are scoped to environments the caller holds +keys for; app-level access alone is not enough.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +from graphql import GraphQLError + +from api.models import OrganisationMember +from ee.integrations.secrets.dynamic.graphene import mutations, queries, types + +ACCESSIBLE_ENV_IDS = ["env-dev"] + + +def _make_info(user_id="actor-1"): + return SimpleNamespace( + context=SimpleNamespace(user=SimpleNamespace(userId=user_id)) + ) + + +@pytest.fixture +def query_mocks(monkeypatch): + mock_secret_model = MagicMock() + monkeypatch.setattr(queries, "DynamicSecret", mock_secret_model) + + mock_env_ids = MagicMock(return_value=set(ACCESSIBLE_ENV_IDS)) + monkeypatch.setattr(queries, "accessible_environment_ids", mock_env_ids) + + app = SimpleNamespace(id="app-1", organisation=SimpleNamespace(id="org-1")) + mock_app_model = MagicMock() + mock_app_model.objects.get.return_value = app + monkeypatch.setattr(queries, "App", mock_app_model) + + mock_org_model = MagicMock() + monkeypatch.setattr(queries, "Organisation", mock_org_model) + + mock_env_model = MagicMock() + mock_env_model.objects.get.return_value = SimpleNamespace(id="env-prod", app=app) + monkeypatch.setattr(queries, "Environment", mock_env_model) + + monkeypatch.setattr(queries, "user_has_permission", MagicMock(return_value=True)) + monkeypatch.setattr(queries, "user_can_access_app", MagicMock(return_value=True)) + mock_env_access = MagicMock(return_value=True) + monkeypatch.setattr(queries, "user_can_access_environment", mock_env_access) + + return SimpleNamespace( + secret_model=mock_secret_model, + env_ids=mock_env_ids, + env_access=mock_env_access, + ) + + +def _filters(query_mocks): + return query_mocks.secret_model.objects.filter.call_args.kwargs + + +def test_app_scoped_query_limits_to_environments_with_keys(query_mocks): + queries.resolve_dynamic_secrets(None, _make_info(), app_id="app-1") + + assert _filters(query_mocks) == { + "deleted_at": None, + "environment__app__id": "app-1", + "environment_id__in": set(ACCESSIBLE_ENV_IDS), + } + query_mocks.env_ids.assert_called_once_with("actor-1", environment__app_id="app-1") + + +def test_org_scoped_query_limits_to_environments_with_keys(query_mocks): + queries.resolve_dynamic_secrets(None, _make_info(), org_id="org-1") + + assert _filters(query_mocks) == { + "deleted_at": None, + "environment__app__organisation_id": "org-1", + "environment_id__in": set(ACCESSIBLE_ENV_IDS), + } + query_mocks.env_ids.assert_called_once_with( + "actor-1", environment__app__organisation_id="org-1" + ) + + +def test_env_scoped_query_still_requires_environment_access(query_mocks): + query_mocks.env_access.return_value = False + + with pytest.raises(GraphQLError, match="access to this environment"): + queries.resolve_dynamic_secrets(None, _make_info(), env_id="env-prod") + + query_mocks.secret_model.objects.filter.assert_not_called() + + +def _dynamic_secret(): + org = SimpleNamespace(id="org-1") + app = SimpleNamespace(id="app-1", organisation=org) + env = SimpleNamespace(id="env-prod", app=app) + return SimpleNamespace( + environment=env, environment_id="env-prod", leases=MagicMock() + ) + + +def test_leases_are_empty_without_environment_access(monkeypatch): + monkeypatch.setattr( + types, "request_accessible_env_ids", MagicMock(return_value={"env-dev"}) + ) + mock_permission = MagicMock(return_value=True) + monkeypatch.setattr(types, "user_has_permission", mock_permission) + secret = _dynamic_secret() + + result = types.DynamicSecretType.resolve_leases(secret, _make_info()) + + assert result is secret.leases.none.return_value + secret.leases.filter.assert_not_called() + mock_permission.assert_not_called() + + +def test_leases_are_listed_with_environment_access(monkeypatch): + monkeypatch.setattr( + types, "request_accessible_env_ids", MagicMock(return_value={"env-prod"}) + ) + monkeypatch.setattr(types, "user_has_permission", MagicMock(return_value=True)) + secret = _dynamic_secret() + + types.DynamicSecretType.resolve_leases(secret, _make_info()) + + secret.leases.filter.assert_called_once_with() + + +def test_leases_for_non_reader_use_the_active_membership(monkeypatch): + monkeypatch.setattr( + types, "request_accessible_env_ids", MagicMock(return_value={"env-prod"}) + ) + monkeypatch.setattr(types, "user_has_permission", MagicMock(return_value=False)) + active_member = SimpleNamespace(id="member-active") + + def member_get(**kwargs): + # Re-invited users keep their soft-deleted membership rows. + if "deleted_at" not in kwargs: + raise OrganisationMember.MultipleObjectsReturned + return active_member + + member_model = MagicMock() + member_model.objects.get.side_effect = member_get + monkeypatch.setattr(types, "OrganisationMember", member_model) + secret = _dynamic_secret() + + types.DynamicSecretType.resolve_leases(secret, _make_info()) + + secret.leases.filter.assert_called_once_with(organisation_member=active_member) + + +@pytest.fixture +def delete_mocks(monkeypatch): + org = SimpleNamespace(id="org-1") + app = SimpleNamespace(id="app-1", organisation=org) + secret = MagicMock(environment_id="env-prod") + secret.environment = SimpleNamespace(id="env-prod", app=app) + + mock_secret_model = MagicMock() + mock_secret_model.objects.get.return_value = secret + monkeypatch.setattr(mutations, "DynamicSecret", mock_secret_model) + monkeypatch.setattr(mutations, "user_has_permission", MagicMock(return_value=True)) + mock_env_access = MagicMock(return_value=True) + monkeypatch.setattr(mutations, "user_can_access_environment", mock_env_access) + + return SimpleNamespace(secret=secret, env_access=mock_env_access) + + +def test_delete_rejects_environment_without_access(delete_mocks): + delete_mocks.env_access.return_value = False + + with pytest.raises(GraphQLError, match="access to this environment"): + mutations.DeleteDynamicSecretMutation.mutate( + None, _make_info(), secret_id="ds-1" + ) + + delete_mocks.secret.delete.assert_not_called() + + +def test_delete_succeeds_with_environment_access(delete_mocks): + mutations.DeleteDynamicSecretMutation.mutate(None, _make_info(), secret_id="ds-1") + + delete_mocks.env_access.assert_called_once_with("actor-1", "env-prod") + delete_mocks.secret.delete.assert_called_once() diff --git a/backend/tests/ee/integrations/secrets/dynamic/test_lease_cascade_revocation.py b/backend/tests/ee/integrations/secrets/dynamic/test_lease_cascade_revocation.py new file mode 100644 index 000000000..ea66dfc0a --- /dev/null +++ b/backend/tests/ee/integrations/secrets/dynamic/test_lease_cascade_revocation.py @@ -0,0 +1,435 @@ +"""Cascade hard-deletes (environment, app, folder) bypass DynamicSecret.delete(), +so active leases must be revoked from a lease pre_delete receiver.""" + +import gc +from types import SimpleNamespace +from unittest.mock import MagicMock, patch + +import pytest +from django.db.models.deletion import Collector +from django.db.models.signals import pre_delete +from botocore.exceptions import EndpointConnectionError, ReadTimeoutError +from rq.exceptions import NoSuchJobError + +from api import signals +from api.models import DynamicSecretLease, DynamicSecretLeaseEvent, RotatingSecretEvent +from ee.integrations.secrets.dynamic import utils as dynamic_utils +from ee.integrations.secrets.dynamic.aws import utils as aws_utils +from ee.integrations.secrets.dynamic.exceptions import LeaseAlreadyRevokedError + + +def test_receiver_is_connected(): + assert pre_delete.has_listeners(DynamicSecretLease) + + +def test_lease_events_stay_fast_deletable(): + # Revocation writes lease events during pre_delete; they are only cleaned up + # because the collector fast-deletes events lazily after the signals run. + collector = Collector(using="default") + assert collector.can_fast_delete(DynamicSecretLeaseEvent) + assert collector.can_fast_delete(RotatingSecretEvent) + + +@pytest.fixture +def receiver_mocks(monkeypatch): + events = [] + + mock_revoke = MagicMock(side_effect=lambda lease: events.append("revoke")) + monkeypatch.setattr(dynamic_utils, "revoke_lease_immediately", mock_revoke) + monkeypatch.setattr( + dynamic_utils, "lease_iam_username", MagicMock(return_value="phase-user-1") + ) + + atomic = MagicMock() + atomic.return_value.__enter__.side_effect = lambda: events.append("enter") + atomic.return_value.__exit__.side_effect = lambda *exc: ( + events.append("exit"), + False, + )[1] + connection = SimpleNamespace(atomic_blocks=[]) + monkeypatch.setattr( + signals, + "transaction", + SimpleNamespace(atomic=atomic, get_connection=lambda: connection), + ) + + mock_logger = MagicMock() + monkeypatch.setattr(signals, "logger", mock_logger) + + clock = SimpleNamespace(now=1000.0) + monkeypatch.setattr(signals.time, "monotonic", lambda: clock.now) + + return SimpleNamespace( + events=events, + revoke=mock_revoke, + atomic=atomic, + connection=connection, + logger=mock_logger, + clock=clock, + ) + + +def _lease(lease_id="lease-1", status=DynamicSecretLease.ACTIVE, auth="cred-1"): + return SimpleNamespace( + id=lease_id, + status=status, + secret_id="ds-1", + expires_at="2026-01-01T00:00:00Z", + secret=SimpleNamespace(authentication_id=auth, environment_id="env-1"), + ) + + +class _Transaction: + """Stands in for the outermost atomic block a delete runs in.""" + + +class _Origin: + """Stands in for the model instance a delete was called on.""" + + +class _Delete: + def __init__(self, origin=None, outermost=None): + self.origin = origin or _Origin() + self.outermost = outermost or _Transaction() + + +def _receive(mocks, lease, delete): + mocks.connection.atomic_blocks = [delete.outermost] + signals._dynamic_secret_lease_pre_delete( + DynamicSecretLease, lease, origin=delete.origin + ) + + +def _unreachable(): + try: + raise EndpointConnectionError(endpoint_url="https://iam.amazonaws.com") + except EndpointConnectionError: + try: + raise Exception("Unexpected error deleting user") + except Exception as wrapped: + return wrapped + + +def _revoked_ids(mocks): + return [c.args[0].id for c in mocks.revoke.call_args_list] + + +def test_active_lease_is_revoked_inside_savepoint(receiver_mocks): + _receive(receiver_mocks, _lease(), _Delete()) + + assert receiver_mocks.events == ["enter", "revoke", "exit"] + receiver_mocks.logger.error.assert_not_called() + + +@pytest.mark.parametrize( + "status", [DynamicSecretLease.REVOKED, DynamicSecretLease.EXPIRED] +) +def test_finished_lease_is_skipped(receiver_mocks, status): + _receive(receiver_mocks, _lease(status=status), _Delete()) + + receiver_mocks.revoke.assert_not_called() + + +def test_revoke_failure_rolls_back_savepoint_and_does_not_block_delete(receiver_mocks): + receiver_mocks.revoke.side_effect = RuntimeError("DeleteConflict") + + _receive(receiver_mocks, _lease(), _Delete()) + + exit_args = receiver_mocks.atomic.return_value.__exit__.call_args.args + assert exit_args[0] is RuntimeError + receiver_mocks.logger.error.assert_called_once() + assert receiver_mocks.logger.error.call_args.kwargs["exc_info"] is True + + +def test_lease_specific_failure_does_not_skip_sibling_leases(receiver_mocks): + receiver_mocks.revoke.side_effect = RuntimeError("DeleteConflict") + delete = _Delete() + + _receive(receiver_mocks, _lease("lease-1", auth="cred-1"), delete) + _receive(receiver_mocks, _lease("lease-2", auth="cred-1"), delete) + + assert _revoked_ids(receiver_mocks) == ["lease-1", "lease-2"] + + +def test_unreachable_provider_is_not_retried_within_the_same_delete(receiver_mocks): + receiver_mocks.revoke.side_effect = lambda lease: (_ for _ in ()).throw( + _unreachable() + ) + delete = _Delete() + + _receive(receiver_mocks, _lease("lease-1", auth="cred-1"), delete) + _receive(receiver_mocks, _lease("lease-2", auth="cred-1"), delete) + _receive(receiver_mocks, _lease("lease-3", auth="cred-2"), delete) + + assert _revoked_ids(receiver_mocks) == ["lease-1", "lease-3"] + + +def test_revocations_stop_once_the_delete_budget_is_spent(receiver_mocks): + delete = _Delete() + + _receive(receiver_mocks, _lease("lease-1"), delete) + receiver_mocks.clock.now += signals.CASCADE_REVOKE_BUDGET_SECONDS + 1 + _receive(receiver_mocks, _lease("lease-2"), delete) + + assert _revoked_ids(receiver_mocks) == ["lease-1"] + receiver_mocks.logger.error.assert_called_once() + + +def _expire_budget_after_unreachable_attempt(mocks, lease, delete): + mocks.revoke.side_effect = lambda _: (_ for _ in ()).throw(_unreachable()) + _receive(mocks, lease, delete) + mocks.clock.now += signals.CASCADE_REVOKE_BUDGET_SECONDS + 1 + mocks.revoke.side_effect = None + + +def test_retry_of_the_same_object_in_a_new_transaction_gets_a_fresh_budget( + receiver_mocks, +): + lease, origin = _lease("lease-1"), _Origin() + _expire_budget_after_unreachable_attempt( + receiver_mocks, lease, _Delete(origin=origin) + ) + + _receive(receiver_mocks, lease, _Delete(origin=origin)) + + assert _revoked_ids(receiver_mocks) == ["lease-1", "lease-1"] + + +def test_retry_through_a_reused_atomic_decorator_gets_a_fresh_budget(receiver_mocks): + lease, outermost = _lease("lease-1"), _Transaction() + _expire_budget_after_unreachable_attempt( + receiver_mocks, lease, _Delete(outermost=outermost) + ) + + _receive(receiver_mocks, lease, _Delete(outermost=outermost)) + + assert _revoked_ids(receiver_mocks) == ["lease-1", "lease-1"] + + +def test_budget_state_is_released_with_the_delete(receiver_mocks): + existing = set(signals._cascade_revoke_state) + delete = _Delete() + _receive(receiver_mocks, _lease(), delete) + created = set(signals._cascade_revoke_state) - existing + assert created + + del delete + receiver_mocks.connection.atomic_blocks = [] + gc.collect() + + assert not created & set(signals._cascade_revoke_state) + + +def test_skipped_lease_log_identifies_what_to_revoke(receiver_mocks): + delete = _Delete() + _receive(receiver_mocks, _lease("lease-1"), delete) + receiver_mocks.clock.now += signals.CASCADE_REVOKE_BUDGET_SECONDS + 1 + + _receive(receiver_mocks, _lease("lease-2"), delete) + + args = receiver_mocks.logger.error.call_args.args + assert args[1:] == ( + "lease-2", + "revocation time budget exhausted", + "phase-user-1", + "ds-1", + "env-1", + "cred-1", + "2026-01-01T00:00:00Z", + ) + + +def test_budget_is_bounded_well_under_worker_timeouts(): + assert signals.CASCADE_REVOKE_BUDGET_SECONDS <= 30 + + +@pytest.mark.parametrize( + ("exc", "unreachable"), + [ + (EndpointConnectionError(endpoint_url="https://iam.amazonaws.com"), True), + (ReadTimeoutError(endpoint_url="https://iam.amazonaws.com"), True), + (RuntimeError("DeleteConflict"), False), + (None, False), + ], +) +def test_provider_unreachable_detection(exc, unreachable): + assert dynamic_utils.is_provider_unreachable_error(exc) is unreachable + + +def test_provider_unreachable_detection_follows_wrapped_errors(): + assert dynamic_utils.is_provider_unreachable_error(_unreachable()) + + +class TestRevokeLeaseImmediately: + @staticmethod + def _lease(provider="aws", cleanup_job_id="job-1"): + return SimpleNamespace( + id="lease-1", + secret=SimpleNamespace(provider=provider), + cleanup_job_id=cleanup_job_id, + ) + + @pytest.fixture(autouse=True) + def _scheduler(self, monkeypatch): + self.on_commit = [] + monkeypatch.setattr( + dynamic_utils.transaction, "on_commit", lambda fn: self.on_commit.append(fn) + ) + self.scheduler = MagicMock() + monkeypatch.setattr( + dynamic_utils.django_rq, "get_scheduler", lambda name: self.scheduler + ) + self.job = MagicMock() + monkeypatch.setattr(dynamic_utils, "Job", self.job) + + def _commit(self): + for fn in self.on_commit: + fn() + + @patch("ee.integrations.secrets.dynamic.aws.utils.revoke_aws_dynamic_secret_lease") + def test_revokes_with_bounded_client_and_cancels_job_on_commit(self, mock_revoke): + dynamic_utils.revoke_lease_immediately(self._lease()) + + mock_revoke.assert_called_once_with( + "lease-1", + manual=True, + client_config=dynamic_utils.IN_REQUEST_REVOKE_CLIENT_CONFIG, + ) + self.scheduler.cancel.assert_not_called() + + self._commit() + + self.scheduler.cancel.assert_called_once_with("job-1") + self.job.fetch.assert_called_once_with( + "job-1", connection=self.scheduler.connection + ) + self.job.fetch.return_value.delete.assert_called_once_with( + remove_from_queue=False + ) + + @patch("ee.integrations.secrets.dynamic.aws.utils.revoke_aws_dynamic_secret_lease") + def test_already_revoked_still_cancels_job(self, mock_revoke): + mock_revoke.side_effect = LeaseAlreadyRevokedError("already revoked") + + dynamic_utils.revoke_lease_immediately(self._lease()) + self._commit() + + self.scheduler.cancel.assert_called_once_with("job-1") + + @patch("ee.integrations.secrets.dynamic.aws.utils.revoke_aws_dynamic_secret_lease") + def test_provider_failure_raises_and_keeps_job(self, mock_revoke): + mock_revoke.side_effect = Exception("AccessDenied") + + with pytest.raises(Exception, match="AccessDenied"): + dynamic_utils.revoke_lease_immediately(self._lease()) + + assert self.on_commit == [] + + @patch("ee.integrations.secrets.dynamic.aws.utils.revoke_aws_dynamic_secret_lease") + def test_unknown_provider_is_skipped(self, mock_revoke): + dynamic_utils.revoke_lease_immediately(self._lease(provider="gcp")) + + mock_revoke.assert_not_called() + assert self.on_commit == [] + + @patch("ee.integrations.secrets.dynamic.aws.utils.revoke_aws_dynamic_secret_lease") + def test_cancel_failure_after_revoke_is_swallowed(self, mock_revoke): + self.scheduler.cancel.side_effect = ConnectionError("redis down") + + dynamic_utils.revoke_lease_immediately(self._lease()) + self._commit() + + mock_revoke.assert_called_once() + + @patch("ee.integrations.secrets.dynamic.aws.utils.revoke_aws_dynamic_secret_lease") + def test_missing_job_hash_is_ignored(self, mock_revoke): + self.job.fetch.side_effect = NoSuchJobError("gone") + + dynamic_utils.revoke_lease_immediately(self._lease()) + self._commit() + + self.scheduler.cancel.assert_called_once_with("job-1") + + +def test_bounded_client_config_is_timeout_limited(): + config = dynamic_utils.IN_REQUEST_REVOKE_CLIENT_CONFIG + + assert config.connect_timeout <= 5 + assert config.read_timeout <= 15 + assert config.retries["total_max_attempts"] <= 2 + + +@pytest.mark.parametrize("integration_key", [None, "AKIAINTEGRATION"]) +@pytest.mark.parametrize( + "config", [None, dynamic_utils.IN_REQUEST_REVOKE_CLIENT_CONFIG] +) +def test_iam_client_passes_config_to_both_clients(monkeypatch, config, integration_key): + mock_boto_client = MagicMock() + monkeypatch.setattr(aws_utils.boto3, "client", mock_boto_client) + monkeypatch.setattr( + aws_utils, + "get_aws_access_key_credentials", + MagicMock( + return_value={ + "access_key_id": "AKIAFAKE", + "secret_access_key": "fake", + "region": "us-east-1", + } + ), + ) + monkeypatch.setattr( + aws_utils, "get_secret", MagicMock(return_value=integration_key) + ) + secret = SimpleNamespace(authentication=SimpleNamespace(credentials={})) + + aws_utils.get_iam_client(secret, config=config) + + assert [c.kwargs.get("config") for c in mock_boto_client.call_args_list] == [ + config, + config, + ] + + +class _StopAfterClient(Exception): + pass + + +def test_revoke_forwards_client_config_to_iam_client(monkeypatch): + config = dynamic_utils.IN_REQUEST_REVOKE_CLIENT_CONFIG + lease = SimpleNamespace(id="lease-1", revoked_at=None, secret=object()) + monkeypatch.setattr( + aws_utils.DynamicSecretLease.objects, "get", MagicMock(return_value=lease) + ) + get_iam = MagicMock(side_effect=_StopAfterClient) + monkeypatch.setattr(aws_utils, "get_iam_client", get_iam) + + with pytest.raises(_StopAfterClient): + aws_utils.revoke_aws_dynamic_secret_lease( + "lease-1", manual=True, client_config=config + ) + + assert get_iam.call_args.kwargs["config"] is config + + +def test_lease_iam_username_decrypts_with_environment_keys(monkeypatch): + monkeypatch.setattr( + dynamic_utils, "get_environment_keys", MagicMock(return_value=("pub", "priv")) + ) + decrypt = MagicMock(return_value="phase-user-1") + monkeypatch.setattr(dynamic_utils, "decrypt_asymmetric", decrypt) + lease = SimpleNamespace( + credentials={"username": "ph:v1:enc"}, + secret=SimpleNamespace(environment_id="env-1"), + ) + + assert dynamic_utils.lease_iam_username(lease) == "phase-user-1" + decrypt.assert_called_once_with("ph:v1:enc", "priv", "pub") + + +def test_lease_iam_username_never_raises(monkeypatch): + monkeypatch.setattr( + dynamic_utils, "get_environment_keys", MagicMock(side_effect=RuntimeError) + ) + lease = SimpleNamespace(credentials={}, secret=SimpleNamespace(environment_id="e")) + + assert dynamic_utils.lease_iam_username(lease) is None diff --git a/backend/tests/ee/integrations/secrets/dynamic/test_lease_create_permission.py b/backend/tests/ee/integrations/secrets/dynamic/test_lease_create_permission.py new file mode 100644 index 000000000..fa7982016 --- /dev/null +++ b/backend/tests/ee/integrations/secrets/dynamic/test_lease_create_permission.py @@ -0,0 +1,355 @@ +"""Minting a dynamic secret lease requires DynamicSecretLeases:create for the +principal that will hold it, on every path that can create one.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock, call + +import pytest +from graphql import GraphQLError + +from api.models import OrganisationMember +from api.views import secrets as secrets_views +from ee.integrations.secrets.dynamic import utils as dynamic_utils +from ee.integrations.secrets.dynamic.graphene import mutations +from ee.integrations.secrets.dynamic.rest import views as dynamic_views + +ORG = SimpleNamespace(id="org-1") +APP = SimpleNamespace(id="app-1", organisation=ORG, sse_enabled=True) +ENV = SimpleNamespace(id="env-1", app=APP) + + +# --- helper ------------------------------------------------------------------- + + +def test_member_principal_is_checked_in_app_context(monkeypatch): + permission = MagicMock(return_value=True) + monkeypatch.setattr(dynamic_utils, "user_has_permission", permission) + member = SimpleNamespace(user=SimpleNamespace(userId="user-1")) + + assert dynamic_utils.can_create_dynamic_secret_lease( + ENV, organisation_member=member + ) + + permission.assert_called_once_with( + member.user, "create", "DynamicSecretLeases", ORG, True, False, app=APP + ) + + +def test_service_account_principal_is_checked_as_service_account(monkeypatch): + permission = MagicMock(return_value=False) + monkeypatch.setattr(dynamic_utils, "user_has_permission", permission) + service_account = SimpleNamespace(id="sa-1") + + assert not dynamic_utils.can_create_dynamic_secret_lease( + ENV, service_account=service_account + ) + + permission.assert_called_once_with( + service_account, "create", "DynamicSecretLeases", ORG, True, True, app=APP + ) + + +def test_principal_less_token_cannot_create_leases(monkeypatch): + permission = MagicMock(return_value=True) + monkeypatch.setattr(dynamic_utils, "user_has_permission", permission) + + assert not dynamic_utils.can_create_dynamic_secret_lease(ENV) + permission.assert_not_called() + + +# --- REST: /v1/secrets/dynamic/ ---------------------------------------------------- + + +@pytest.fixture +def dynamic_view(monkeypatch): + can_create = MagicMock(return_value=False) + monkeypatch.setattr(dynamic_views, "can_create_dynamic_secret_lease", can_create) + create_lease = MagicMock(return_value=(SimpleNamespace(id="lease-1"), {})) + monkeypatch.setattr(dynamic_views, "create_dynamic_secret_lease", create_lease) + + secret_qs = MagicMock() + secret_qs.exists.return_value = True + secret_qs.__iter__.return_value = iter([SimpleNamespace(id="ds-1")]) + secret_model = MagicMock() + secret_model.objects.filter.return_value = secret_qs + monkeypatch.setattr(dynamic_views, "DynamicSecret", secret_model) + monkeypatch.setattr( + dynamic_views, + "DynamicSecretSerializer", + MagicMock(return_value=SimpleNamespace(data={})), + ) + return SimpleNamespace(can_create=can_create, create_lease=create_lease) + + +def _dynamic_request(auth_type="User", lease=True): + member = SimpleNamespace(id="member-1") if auth_type == "User" else None + service_account = SimpleNamespace(id="sa-1") if auth_type != "User" else None + params = {"lease": "true"} if lease else {} + return SimpleNamespace( + GET=params, + auth={ + "auth_type": auth_type, + "org_member": member, + "service_account": service_account, + "environment": ENV, + }, + ) + + +def test_dynamic_view_refuses_lease_without_permission(dynamic_view): + response = dynamic_views.DynamicSecretsView().get(_dynamic_request()) + + assert response.status_code == 403 + assert response.data == {"error": dynamic_utils.LEASE_CREATE_PERMISSION_ERROR} + dynamic_view.create_lease.assert_not_called() + + +def test_dynamic_view_mints_lease_with_permission(dynamic_view): + dynamic_view.can_create.return_value = True + + response = dynamic_views.DynamicSecretsView().get(_dynamic_request()) + + assert response.status_code == 200 + dynamic_view.create_lease.assert_called_once() + + +def test_dynamic_view_without_lease_skips_the_check(dynamic_view): + response = dynamic_views.DynamicSecretsView().get(_dynamic_request(lease=False)) + + assert response.status_code == 200 + dynamic_view.can_create.assert_not_called() + + +def test_dynamic_view_checks_the_member_principal(dynamic_view): + request = _dynamic_request() + + dynamic_views.DynamicSecretsView().get(request) + + dynamic_view.can_create.assert_called_once_with( + ENV, organisation_member=request.auth["org_member"], service_account=None + ) + + +def test_dynamic_view_checks_the_service_account_principal(dynamic_view): + request = _dynamic_request(auth_type="ServiceAccount") + + dynamic_views.DynamicSecretsView().get(request) + + dynamic_view.can_create.assert_called_once_with( + ENV, organisation_member=None, service_account=request.auth["service_account"] + ) + + +# --- REST: /secrets/ and /v1/secrets/ -------------------------------------------- + + +@pytest.fixture +def secrets_view(monkeypatch): + monkeypatch.setattr( + secrets_views, "get_resolver_request_meta", MagicMock(return_value=(None, None)) + ) + secret_model = MagicMock() + secret_model.objects.filter.return_value.prefetch_related.return_value = [] + monkeypatch.setattr(secrets_views, "Secret", secret_model) + monkeypatch.setattr(secrets_views, "log_secret_events_bulk", MagicMock()) + monkeypatch.setattr(secrets_views, "get_environment_crypto_context", MagicMock()) + monkeypatch.setattr( + secrets_views, + "SecretSerializer", + MagicMock(return_value=SimpleNamespace(data=[])), + ) + monkeypatch.setattr( + secrets_views, + "DynamicSecretSerializer", + MagicMock(return_value=SimpleNamespace(data={})), + ) + + dynamic_qs = MagicMock() + dynamic_qs.__iter__.side_effect = lambda: iter([SimpleNamespace(id="ds-1")]) + dynamic_model = MagicMock() + dynamic_model.objects.filter.return_value = dynamic_qs + monkeypatch.setattr(secrets_views, "DynamicSecret", dynamic_model) + + can_create = MagicMock(return_value=False) + monkeypatch.setattr(secrets_views, "can_create_dynamic_secret_lease", can_create) + create_lease = MagicMock(return_value=(SimpleNamespace(id="lease-1"), {})) + monkeypatch.setattr(secrets_views, "create_dynamic_secret_lease", create_lease) + + return SimpleNamespace( + dynamic_qs=dynamic_qs, can_create=can_create, create_lease=create_lease + ) + + +def _secrets_request(view_cls, flags, service_account=None): + member = ( + None + if service_account + else SimpleNamespace(id="member-1", user=SimpleNamespace(userId="u1")) + ) + token = ( + SimpleNamespace(service_account=service_account) if service_account else None + ) + auth = { + "auth_type": "ServiceAccount" if service_account else "User", + "org_member": member, + "service_account": service_account, + "service_token": None, + "service_account_token": token, + "environment": ENV, + } + if view_cls is secrets_views.E2EESecretsView: + return SimpleNamespace(headers=dict(flags), GET={}, auth=auth) + return SimpleNamespace(headers={}, GET=dict(flags), auth=auth) + + +SECRETS_VIEWS = [secrets_views.E2EESecretsView, secrets_views.PublicSecretsView] +VIEW_IDS = ["e2ee", "public"] +WITH_LEASE = {"dynamic": "true", "lease": "true"} + + +@pytest.mark.parametrize("view_cls", SECRETS_VIEWS, ids=VIEW_IDS) +def test_secrets_view_refuses_lease_without_permission(secrets_view, view_cls): + response = view_cls().get(_secrets_request(view_cls, WITH_LEASE)) + + assert response.status_code == 403 + assert response.data == {"error": dynamic_utils.LEASE_CREATE_PERMISSION_ERROR} + secrets_view.create_lease.assert_not_called() + + +@pytest.mark.parametrize("view_cls", SECRETS_VIEWS, ids=VIEW_IDS) +def test_secrets_view_mints_lease_with_permission(secrets_view, view_cls): + secrets_view.can_create.return_value = True + + response = view_cls().get(_secrets_request(view_cls, WITH_LEASE)) + + assert response.status_code == 200 + secrets_view.create_lease.assert_called_once() + + +@pytest.mark.parametrize("view_cls", SECRETS_VIEWS, ids=VIEW_IDS) +def test_secrets_view_without_dynamic_secrets_ignores_lease_flag( + secrets_view, view_cls +): + secrets_view.dynamic_qs.__iter__.side_effect = lambda: iter([]) + + response = view_cls().get(_secrets_request(view_cls, WITH_LEASE)) + + assert response.status_code == 200 + secrets_view.can_create.assert_not_called() + + +@pytest.mark.parametrize("view_cls", SECRETS_VIEWS, ids=VIEW_IDS) +def test_secrets_view_without_lease_flag_skips_the_check(secrets_view, view_cls): + response = view_cls().get(_secrets_request(view_cls, {"dynamic": "true"})) + + assert response.status_code == 200 + secrets_view.can_create.assert_not_called() + + +@pytest.mark.parametrize("view_cls", SECRETS_VIEWS, ids=VIEW_IDS) +def test_secrets_view_checks_the_member_principal(secrets_view, view_cls): + request = _secrets_request(view_cls, WITH_LEASE) + + view_cls().get(request) + + secrets_view.can_create.assert_called_once_with( + ENV, organisation_member=request.auth["org_member"], service_account=None + ) + + +@pytest.mark.parametrize("view_cls", SECRETS_VIEWS, ids=VIEW_IDS) +def test_secrets_view_checks_the_service_account_principal(secrets_view, view_cls): + service_account = SimpleNamespace(id="sa-1") + + view_cls().get(_secrets_request(view_cls, WITH_LEASE, service_account)) + + secrets_view.can_create.assert_called_once_with( + ENV, organisation_member=None, service_account=service_account + ) + + +# --- GraphQL: LeaseDynamicSecret ---------------------------------------------- + + +@pytest.fixture +def lease_mutation(monkeypatch): + secret = SimpleNamespace(id="ds-1", name="AWS", environment=ENV) + secret_model = MagicMock() + secret_model.objects.get.return_value = secret + monkeypatch.setattr(mutations, "DynamicSecret", secret_model) + + active_member = SimpleNamespace(id="member-active") + + def member_get(**kwargs): + # Re-invited users keep their soft-deleted membership rows. + if "deleted_at" not in kwargs: + raise OrganisationMember.MultipleObjectsReturned + return active_member + + member_model = MagicMock() + member_model.objects.get.side_effect = member_get + monkeypatch.setattr(mutations, "OrganisationMember", member_model) + monkeypatch.setattr(mutations, "user_is_org_member", MagicMock(return_value=True)) + monkeypatch.setattr( + mutations, "user_can_access_environment", MagicMock(return_value=True) + ) + + granted = {("read", "Secrets"), ("create", "DynamicSecretLeases")} + permission = MagicMock( + side_effect=lambda user, action, resource, *args, **kwargs: ( + (action, resource) in granted + ) + ) + monkeypatch.setattr(mutations, "user_has_permission", permission) + + create_lease = MagicMock( + return_value=( + SimpleNamespace(id="lease-1"), + {"access_key_id": "a", "secret_access_key": "b", "username": "c"}, + ) + ) + monkeypatch.setattr(mutations, "create_dynamic_secret_lease", create_lease) + + return SimpleNamespace( + granted=granted, + permission=permission, + create_lease=create_lease, + active_member=active_member, + ) + + +USER = SimpleNamespace(userId="u1") + + +def _lease_mutation(): + info = SimpleNamespace(context=SimpleNamespace(user=USER)) + return mutations.LeaseDynamicSecret.mutate(None, info, secret_id="ds-1") + + +def test_lease_mutation_requires_lease_create_permission(lease_mutation): + lease_mutation.granted.discard(("create", "DynamicSecretLeases")) + + with pytest.raises(GraphQLError, match="create dynamic secret leases"): + _lease_mutation() + + lease_mutation.create_lease.assert_not_called() + + +def test_lease_mutation_requires_secrets_read(lease_mutation): + lease_mutation.granted.discard(("read", "Secrets")) + + with pytest.raises(GraphQLError, match="read secrets"): + _lease_mutation() + + lease_mutation.create_lease.assert_not_called() + + +def test_lease_mutation_succeeds_with_read_and_lease_create(lease_mutation): + _lease_mutation() + + lease_mutation.create_lease.assert_called_once() + assert lease_mutation.create_lease.call_args.args[3] is lease_mutation.active_member + assert lease_mutation.permission.call_args_list == [ + call(USER, "read", "Secrets", ORG, True, app=APP), + call(USER, "create", "DynamicSecretLeases", ORG, True, app=APP), + ] diff --git a/backend/tests/ee/integrations/secrets/dynamic/test_lease_ownership.py b/backend/tests/ee/integrations/secrets/dynamic/test_lease_ownership.py new file mode 100644 index 000000000..0fd368d6f --- /dev/null +++ b/backend/tests/ee/integrations/secrets/dynamic/test_lease_ownership.py @@ -0,0 +1,299 @@ +"""Lease holders can renew and revoke their own leases without the +DynamicSecretLeases permission; holder matching never crosses principal types.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +from graphql import GraphQLError +from rest_framework.exceptions import PermissionDenied + +from api.models import ( + CustomUser, + DynamicSecretLease, + OrganisationMember, + ServiceAccount, +) +from ee.integrations.secrets.dynamic.graphene import mutations +from ee.integrations.secrets.dynamic.rest import views + +ORG = SimpleNamespace(id="org-1") +APP = SimpleNamespace(id="app-1", organisation=ORG) +ENV = SimpleNamespace(id="env-1", app=APP) + +MEMBER = OrganisationMember(id="member-1", user=CustomUser(userId="user-1")) +OTHER_MEMBER = OrganisationMember(id="member-2", user=CustomUser(userId="user-2")) +SERVICE_ACCOUNT = ServiceAccount(id="sa-1") + + +def _lease(member=None, service_account=None): + return SimpleNamespace( + id="lease-1", + secret=SimpleNamespace(environment=ENV, provider="aws"), + organisation_member=member, + organisation_member_id=getattr(member, "id", None), + service_account=service_account, + service_account_id=getattr(service_account, "id", None), + ) + + +def _request(auth_type, member=None, service_account=None): + return SimpleNamespace( + data={"lease_id": "lease-1"}, + query_params={}, + auth={ + "auth_type": auth_type, + "org_member": member, + "service_account": service_account, + "environment": ENV, + }, + ) + + +@pytest.fixture +def rest(monkeypatch): + lease_model = MagicMock() + monkeypatch.setattr(views, "DynamicSecretLease", lease_model) + permission = MagicMock(return_value=False) + monkeypatch.setattr(views, "user_has_permission", permission) + renew = MagicMock() + monkeypatch.setattr(views, "renew_dynamic_secret_lease", renew) + revoke = MagicMock() + monkeypatch.setattr(views, "revoke_aws_dynamic_secret_lease", revoke) + + def call(method, request, lease): + lease_model.objects.get.return_value = lease + view = views.DynamicSecretLeaseView() + view.request = request + return getattr(view, method)(request) + + return SimpleNamespace(call=call, permission=permission, renew=renew, revoke=revoke) + + +@pytest.mark.parametrize("method", ["put", "delete"]) +def test_user_token_acts_on_own_lease_without_lease_permission(rest, method): + response = rest.call(method, _request("User", MEMBER), _lease(MEMBER)) + + assert response.status_code == 200 + rest.permission.assert_not_called() + + +def test_user_token_renew_is_attributed_to_the_member(rest): + rest.call("put", _request("User", MEMBER), _lease(MEMBER)) + + assert rest.renew.call_args.kwargs["organisation_member"] is MEMBER + assert rest.renew.call_args.kwargs["service_account"] is None + + +@pytest.mark.parametrize("method", ["put", "delete"]) +def test_user_token_needs_permission_for_other_member_lease(rest, method): + with pytest.raises(PermissionDenied): + rest.call(method, _request("User", MEMBER), _lease(OTHER_MEMBER)) + + rest.renew.assert_not_called() + rest.revoke.assert_not_called() + + +def test_user_token_does_not_hold_service_account_lease(rest): + with pytest.raises(PermissionDenied): + rest.call( + "delete", _request("User", MEMBER), _lease(service_account=SERVICE_ACCOUNT) + ) + + rest.revoke.assert_not_called() + + +def test_user_token_does_not_hold_service_account_lease_with_same_id(rest): + same_id_account = ServiceAccount(id=MEMBER.id) + + with pytest.raises(PermissionDenied): + rest.call( + "delete", _request("User", MEMBER), _lease(service_account=same_id_account) + ) + + rest.revoke.assert_not_called() + + +@pytest.mark.parametrize("method", ["put", "delete"]) +def test_service_account_acts_on_own_lease(rest, method): + request = _request("ServiceAccount", service_account=SERVICE_ACCOUNT) + + response = rest.call(method, request, _lease(service_account=SERVICE_ACCOUNT)) + + assert response.status_code == 200 + rest.permission.assert_not_called() + + +def test_service_account_does_not_hold_member_lease_with_same_id(rest): + same_id_member = OrganisationMember( + id=SERVICE_ACCOUNT.id, user=CustomUser(userId="user-3") + ) + request = _request("ServiceAccount", service_account=SERVICE_ACCOUNT) + + with pytest.raises(PermissionDenied): + rest.call("delete", request, _lease(same_id_member)) + + rest.revoke.assert_not_called() + + +def test_service_token_never_matches_a_holder(rest): + with pytest.raises(PermissionDenied): + rest.call("put", _request("Service"), _lease(service_account=SERVICE_ACCOUNT)) + + rest.renew.assert_not_called() + + +def test_non_holder_permission_is_checked_in_app_context(rest): + rest.permission.return_value = True + + rest.call("put", _request("User", MEMBER), _lease(OTHER_MEMBER)) + + rest.permission.assert_called_once_with( + MEMBER.user, "update", "DynamicSecretLeases", ORG, True, False, app=APP + ) + + +def _info(user_id="user-1"): + return SimpleNamespace(context=SimpleNamespace(user=CustomUser(userId=user_id))) + + +@pytest.fixture +def gql(monkeypatch): + active_member = SimpleNamespace(id="member-1") + removed_member = SimpleNamespace(id="member-removed") + state = SimpleNamespace(active_member=active_member, lease=_lease(active_member)) + + lease_model = MagicMock() + lease_model.objects.get.side_effect = lambda **kw: state.lease + lease_model.objects.filter.side_effect = lambda **kw: SimpleNamespace( + first=lambda: state.lease + ) + monkeypatch.setattr(mutations, "DynamicSecretLease", lease_model) + + # Re-invited users keep their soft-deleted membership rows. + def member_get(**kw): + if "deleted_at" not in kw: + raise OrganisationMember.MultipleObjectsReturned + if state.active_member is None: + raise OrganisationMember.DoesNotExist + return state.active_member + + def member_filter(**kw): + found = state.active_member if "deleted_at" in kw else removed_member + return SimpleNamespace(first=lambda: found) + + member_model = MagicMock() + member_model.DoesNotExist = OrganisationMember.DoesNotExist + member_model.objects.get.side_effect = member_get + member_model.objects.filter.side_effect = member_filter + monkeypatch.setattr(mutations, "OrganisationMember", member_model) + + permission = MagicMock(return_value=False) + monkeypatch.setattr(mutations, "user_has_permission", permission) + monkeypatch.setattr(mutations, "user_is_org_member", MagicMock(return_value=True)) + monkeypatch.setattr( + mutations, "user_can_access_environment", MagicMock(return_value=True) + ) + renew = MagicMock(return_value=state.lease) + monkeypatch.setattr(mutations, "renew_dynamic_secret_lease", renew) + revoke = MagicMock() + monkeypatch.setattr(mutations, "revoke_aws_dynamic_secret_lease", revoke) + + state.permission, state.renew, state.revoke = permission, renew, revoke + return state + + +def test_renew_mutation_holder_uses_active_membership(gql): + mutations.RenewLeaseMutation.mutate(None, _info(), lease_id="lease-1") + + assert gql.renew.call_args.kwargs["organisation_member"] is gql.active_member + gql.permission.assert_not_called() + + +def test_revoke_mutation_holder_uses_active_membership(gql): + mutations.RevokeLeaseMutation.mutate(None, _info(), lease_id="lease-1") + + assert gql.revoke.call_args.kwargs["organisation_member"] is gql.active_member + gql.permission.assert_not_called() + + +def test_renew_mutation_rejects_caller_outside_lease_org(gql): + gql.active_member = None + + with pytest.raises(GraphQLError, match="^Lease not found$"): + mutations.RenewLeaseMutation.mutate(None, _info(), lease_id="lease-1") + + gql.renew.assert_not_called() + + +def test_revoke_mutation_non_holder_needs_permission(gql): + gql.lease = _lease(SimpleNamespace(id="member-2")) + + with pytest.raises(GraphQLError, match="wasn't created by you"): + mutations.RevokeLeaseMutation.mutate(None, _info(), lease_id="lease-1") + + gql.revoke.assert_not_called() + + +@pytest.mark.parametrize( + ("mutation", "action"), + [ + (mutations.RenewLeaseMutation, "renew"), + (mutations.RevokeLeaseMutation, "revoke"), + ], +) +def test_unknown_lease_gets_the_same_error_as_a_foreign_one(gql, mutation, action): + gql.lease = None + mutations.DynamicSecretLease.objects.get.side_effect = ( + DynamicSecretLease.DoesNotExist + ) + + with pytest.raises(GraphQLError, match="^Lease not found$"): + mutation.mutate(None, _info(), lease_id="missing") + + getattr(gql, action).assert_not_called() + + +def test_renew_mutation_non_holder_needs_update_permission(gql): + gql.lease = _lease(SimpleNamespace(id="member-2")) + info = _info() + + with pytest.raises(GraphQLError, match="wasn't created by you"): + mutations.RenewLeaseMutation.mutate(None, info, lease_id="lease-1") + + gql.renew.assert_not_called() + gql.permission.assert_called_once_with( + info.context.user, "update", "DynamicSecretLeases", ORG, True, app=APP + ) + + +def test_revoke_mutation_non_holder_checks_delete_permission(gql): + gql.lease = _lease(SimpleNamespace(id="member-2")) + gql.permission.return_value = True + info = _info() + + mutations.RevokeLeaseMutation.mutate(None, info, lease_id="lease-1") + + gql.permission.assert_called_once_with( + info.context.user, "delete", "DynamicSecretLeases", ORG, True, app=APP + ) + + +@pytest.mark.parametrize( + ("mutation", "action"), + [ + (mutations.RenewLeaseMutation, "renew"), + (mutations.RevokeLeaseMutation, "revoke"), + ], +) +def test_holder_without_environment_access_is_rejected( + gql, monkeypatch, mutation, action +): + monkeypatch.setattr( + mutations, "user_can_access_environment", MagicMock(return_value=False) + ) + + with pytest.raises(GraphQLError, match="access to this environment"): + mutation.mutate(None, _info(), lease_id="lease-1") + + getattr(gql, action).assert_not_called() diff --git a/backend/tests/ee/integrations/secrets/dynamic/test_lease_renewal.py b/backend/tests/ee/integrations/secrets/dynamic/test_lease_renewal.py new file mode 100644 index 000000000..ff7191043 --- /dev/null +++ b/backend/tests/ee/integrations/secrets/dynamic/test_lease_renewal.py @@ -0,0 +1,135 @@ +"""Renewing a lease must actually move its scheduled revocation and reject +input that would shorten or resurrect it.""" + +from datetime import timedelta +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +from django.utils import timezone + +from api.models import Organisation +from ee.integrations.secrets.dynamic import utils as dynamic_utils +from ee.integrations.secrets.dynamic.exceptions import ( + DynamicSecretError, + LeaseAlreadyRevokedError, + LeaseRenewalError, +) + + +def _lease(**overrides): + org = SimpleNamespace(plan=Organisation.ENTERPRISE_PLAN) + secret = SimpleNamespace( + environment=SimpleNamespace(app=SimpleNamespace(organisation=org)), + max_ttl=timedelta(hours=24), + deleted_at=None, + provider="aws", + ) + lease = SimpleNamespace( + id="lease-1", + secret=secret, + ttl=timedelta(hours=1), + expires_at=timezone.now() + timedelta(minutes=30), + revoked_at=None, + cleanup_job_id="old-job", + updated_at=None, + ) + for key, value in overrides.items(): + setattr(lease, key, value) + return lease + + +@pytest.fixture +def renew_mocks(monkeypatch): + calls = MagicMock() + + def schedule(lease, immediate=False): + calls.schedule(lease.cleanup_job_id) + lease.cleanup_job_id = "new-job" + + monkeypatch.setattr(dynamic_utils, "schedule_lease_revocation", schedule) + monkeypatch.setattr( + dynamic_utils, + "cancel_scheduled_lease_job", + lambda job_id, lease_id: calls.cancel(job_id), + ) + monkeypatch.setattr(dynamic_utils, "DynamicSecretLeaseEvent", MagicMock()) + return calls + + +def test_renew_cancels_old_job_after_enqueueing_new_one(renew_mocks): + lease = _lease() + original_expiry = lease.expires_at + + dynamic_utils.renew_dynamic_secret_lease(lease, 600) + + assert [c[0] for c in renew_mocks.mock_calls] == ["schedule", "cancel"] + renew_mocks.cancel.assert_called_once_with("old-job") + assert lease.expires_at == original_expiry + timedelta(seconds=600) + assert lease.cleanup_job_id == "new-job" + + +def test_renew_enqueue_failure_keeps_old_job(renew_mocks, monkeypatch): + def failing_schedule(lease, immediate=False): + raise RuntimeError("redis down") + + monkeypatch.setattr(dynamic_utils, "schedule_lease_revocation", failing_schedule) + + with pytest.raises(RuntimeError): + dynamic_utils.renew_dynamic_secret_lease(_lease(), 600) + + renew_mocks.cancel.assert_not_called() + + +def test_renew_rejects_revoked_lease_before_changing_it(renew_mocks): + lease = _lease(revoked_at=timezone.now()) + original_expiry = lease.expires_at + + with pytest.raises(LeaseAlreadyRevokedError): + dynamic_utils.renew_dynamic_secret_lease(lease, 600) + + assert lease.expires_at == original_expiry + renew_mocks.schedule.assert_not_called() + + +def test_renew_rejects_lease_of_deleted_secret(renew_mocks): + lease = _lease() + lease.secret.deleted_at = timezone.now() + + with pytest.raises(LeaseRenewalError, match="deleted"): + dynamic_utils.renew_dynamic_secret_lease(lease, 600) + + renew_mocks.schedule.assert_not_called() + + +@pytest.mark.parametrize("ttl", [0, -5, "60", True, 1.5]) +def test_renew_rejects_invalid_ttl(renew_mocks, ttl): + lease = _lease() + original_expiry = lease.expires_at + + with pytest.raises(DynamicSecretError, match="positive integer"): + dynamic_utils.renew_dynamic_secret_lease(lease, ttl) + + assert lease.expires_at == original_expiry + renew_mocks.schedule.assert_not_called() + + +def test_renew_persists_new_expiry_and_job_through_real_scheduling(monkeypatch): + scheduler = MagicMock() + scheduler.enqueue_at.return_value = SimpleNamespace(id="new-job") + monkeypatch.setattr( + dynamic_utils.django_rq, "get_scheduler", lambda name: scheduler + ) + monkeypatch.setattr(dynamic_utils, "Job", MagicMock()) + monkeypatch.setattr(dynamic_utils, "DynamicSecretLeaseEvent", MagicMock()) + lease = _lease() + original_expiry = lease.expires_at + saved = [] + lease.save = lambda: saved.append((lease.expires_at, lease.cleanup_job_id)) + + dynamic_utils.renew_dynamic_secret_lease(lease, 600) + + renewed_expiry = original_expiry + timedelta(seconds=600) + assert saved[-1] == (renewed_expiry, "new-job") + assert scheduler.enqueue_at.call_args.args[0] == renewed_expiry + scheduler.cancel.assert_called_once_with("old-job") diff --git a/backend/tests/ee/integrations/secrets/dynamic/test_lease_view_scoping.py b/backend/tests/ee/integrations/secrets/dynamic/test_lease_view_scoping.py new file mode 100644 index 000000000..ec234eacd --- /dev/null +++ b/backend/tests/ee/integrations/secrets/dynamic/test_lease_view_scoping.py @@ -0,0 +1,82 @@ +"""Dynamic secret lease listing is scoped to the token's environment. + +Authentication can resolve that environment from a Secret-Id header that differs +from the secret_id being listed, so the view must bind the lookup itself. +""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest + +from api.models import DynamicSecret +from ee.integrations.secrets.dynamic.rest import views + + +def _make_request(env, secret_id): + return SimpleNamespace( + query_params={"secret_id": secret_id}, + auth={ + "auth_type": "User", + "org_member": MagicMock(id="member-1"), + "service_account": None, + "environment": env, + }, + ) + + +@pytest.fixture +def lease_view(monkeypatch): + token_env = MagicMock(id="env-token") + other_env = MagicMock(id="env-other") + secrets = { + "ds-own": SimpleNamespace(id="ds-own", environment=token_env), + "ds-other": SimpleNamespace(id="ds-other", environment=other_env), + } + + def get_secret(**lookup): + secret = secrets.get(lookup["id"]) + env = lookup.get("environment", secret.environment if secret else None) + if secret is None or secret.environment is not env: + raise DynamicSecret.DoesNotExist + return secret + + mock_secret_model = MagicMock() + mock_secret_model.DoesNotExist = DynamicSecret.DoesNotExist + mock_secret_model.objects.get.side_effect = get_secret + monkeypatch.setattr(views, "DynamicSecret", mock_secret_model) + + mock_lease_model = MagicMock() + monkeypatch.setattr(views, "DynamicSecretLease", mock_lease_model) + monkeypatch.setattr(views, "user_has_permission", MagicMock(return_value=True)) + monkeypatch.setattr( + views, + "DynamicSecretLeaseSerializer", + MagicMock(return_value=SimpleNamespace(data=[])), + ) + + return SimpleNamespace( + view=views.DynamicSecretLeaseView(), + token_env=token_env, + secrets=secrets, + lease_model=mock_lease_model, + ) + + +def test_lists_leases_for_secret_in_token_environment(lease_view): + request = _make_request(lease_view.token_env, "ds-own") + + response = lease_view.view.get(request) + + assert response.status_code == 200 + filters = lease_view.lease_model.objects.filter.call_args.kwargs + assert filters["secret"] is lease_view.secrets["ds-own"] + + +def test_returns_not_found_for_secret_in_another_environment(lease_view): + request = _make_request(lease_view.token_env, "ds-other") + + response = lease_view.view.get(request) + + assert response.status_code == 404 + lease_view.lease_model.objects.filter.assert_not_called() diff --git a/backend/tests/graphene/mutations/test_environment_delete_access.py b/backend/tests/graphene/mutations/test_environment_delete_access.py new file mode 100644 index 000000000..e0c4bf598 --- /dev/null +++ b/backend/tests/graphene/mutations/test_environment_delete_access.py @@ -0,0 +1,105 @@ +"""DeleteEnvironment requires access to the environment itself: Environments:delete +is granted app-wide, but deleting cascades to every secret and dynamic secret in it.""" + +from types import SimpleNamespace +from unittest.mock import MagicMock + +import pytest +from graphql import GraphQLError + +from backend.graphene.mutations import environment as env_mutations + + +def _make_info(user_id="actor-1"): + return SimpleNamespace( + context=SimpleNamespace(user=SimpleNamespace(userId=user_id)) + ) + + +@pytest.fixture +def delete_mocks(monkeypatch): + org = SimpleNamespace(id="org-1") + app = SimpleNamespace(id="app-1", organisation=org) + environment = MagicMock(id="env-prod", env_type="custom", app=app) + environment.name = "prod" + + mock_env_model = MagicMock() + mock_env_model.objects.get.return_value = environment + monkeypatch.setattr(env_mutations, "Environment", mock_env_model) + + mock_rotating_model = MagicMock() + mock_rotating_model.objects.filter.return_value.exists.return_value = False + monkeypatch.setattr(env_mutations, "RotatingSecret", mock_rotating_model) + + mock_permission = MagicMock(return_value=True) + monkeypatch.setattr(env_mutations, "user_has_permission", mock_permission) + mock_env_access = MagicMock(return_value=True) + monkeypatch.setattr(env_mutations, "user_can_access_environment", mock_env_access) + + monkeypatch.setattr( + env_mutations, "can_use_custom_envs", MagicMock(return_value=True) + ) + monkeypatch.setattr( + env_mutations, + "get_actor_info_from_graphql", + MagicMock(return_value=("user", "actor-1", {})), + ) + monkeypatch.setattr( + env_mutations, "get_resolver_request_meta", MagicMock(return_value=(None, None)) + ) + mock_audit = MagicMock() + monkeypatch.setattr(env_mutations, "log_audit_event", mock_audit) + + return SimpleNamespace( + environment=environment, + rotating_model=mock_rotating_model, + permission=mock_permission, + env_access=mock_env_access, + audit=mock_audit, + ) + + +def _delete(): + return env_mutations.DeleteEnvironmentMutation.mutate( + None, _make_info(), environment_id="env-prod" + ) + + +def test_delete_refuses_without_environment_access(delete_mocks): + delete_mocks.env_access.return_value = False + + with pytest.raises(GraphQLError, match="access to this environment"): + _delete() + + delete_mocks.env_access.assert_called_once_with("actor-1", "env-prod") + delete_mocks.environment.delete.assert_not_called() + delete_mocks.rotating_model.objects.filter.assert_not_called() + delete_mocks.audit.assert_not_called() + + +def test_delete_proceeds_with_environment_access(delete_mocks): + _delete() + + delete_mocks.environment.delete.assert_called_once() + + +def test_permission_is_checked_before_environment_access(delete_mocks): + delete_mocks.permission.return_value = False + + with pytest.raises(GraphQLError, match="permission to delete environments"): + _delete() + + delete_mocks.env_access.assert_not_called() + delete_mocks.environment.delete.assert_not_called() + + +def test_rotating_secret_gate_still_applies(delete_mocks): + delete_mocks.rotating_model.objects.filter.return_value.exists.return_value = True + delete_mocks.permission.side_effect = ( + lambda user, action, resource, *args, **kwargs: resource != "RotatingSecrets" + ) + + with pytest.raises(GraphQLError, match="contains rotating secrets"): + _delete() + + delete_mocks.environment.delete.assert_not_called() diff --git a/frontend/ee/components/secrets/dynamic/DeleteDynamicSecretDialog.tsx b/frontend/ee/components/secrets/dynamic/DeleteDynamicSecretDialog.tsx index adf516410..4b7916a20 100644 --- a/frontend/ee/components/secrets/dynamic/DeleteDynamicSecretDialog.tsx +++ b/frontend/ee/components/secrets/dynamic/DeleteDynamicSecretDialog.tsx @@ -90,7 +90,7 @@ export const DeleteDynamicSecretDialog = ({ secret }: { secret: DynamicSecretTyp }, [isOpen, organisation, fetchLeases, secret.id]) const activeLeases: DynamicSecretLeaseType[] = - data?.dynamicSecrets[0].leases?.filter( + data?.dynamicSecrets?.[0]?.leases?.filter( (lease: DynamicSecretLeaseType) => lease.status === ApiDynamicSecretLeaseStatusChoices.Active ) ?? [] diff --git a/frontend/ee/components/secrets/dynamic/ManageLeasesDialog.tsx b/frontend/ee/components/secrets/dynamic/ManageLeasesDialog.tsx index 8d8cc79b2..02b2619b0 100644 --- a/frontend/ee/components/secrets/dynamic/ManageLeasesDialog.tsx +++ b/frontend/ee/components/secrets/dynamic/ManageLeasesDialog.tsx @@ -46,7 +46,7 @@ export const ManageLeasesDialog = ({ secret }: { secret: DynamicSecretType }) => const resetViewLimit = () => setViewLimit(10) const removeViewLimit = () => setViewLimit(0) - const leases: DynamicSecretLeaseType[] = data?.dynamicSecrets[0].leases ?? [] + const leases: DynamicSecretLeaseType[] = data?.dynamicSecrets?.[0]?.leases ?? [] return (