From 1bb86dd591b0dacf6951a0c4d657b8de0ff24684 Mon Sep 17 00:00:00 2001 From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com> Date: Fri, 15 Sep 2023 16:00:47 +0100 Subject: [PATCH] Anpl 1704 s3 folders use task queue (#1204) * ANPL-1704 fix bug where task to create bucket is sent before bucket created * ANPL-1704 pass bucket owner to CreateS3Bucket task * ANPL-1704 refactor task handlers * ANPL-1704 add task to revoke user access * ANPL-1704 add task to revoke bucket access, not used yet * ANPL-1704 enable revoke app access via a task * ANPL-1704 Remove default permissions checks from task handlers Current implementation of the permission checks are unnecessary as they are simply repeating checks that have already occurred in the views that trigger creating tasks. Permissions should instead be implemented as required, where necessary. --- controlpanel/api/models/apps3bucket.py | 10 +- controlpanel/api/models/s3bucket.py | 40 ++++---- controlpanel/api/models/users3bucket.py | 23 +---- controlpanel/api/serializers.py | 4 +- controlpanel/api/tasks/__init__.py | 7 +- controlpanel/api/tasks/app.py | 3 + controlpanel/api/tasks/handlers/__init__.py | 13 ++- controlpanel/api/tasks/handlers/app.py | 22 ++--- controlpanel/api/tasks/handlers/base.py | 91 +++++++------------ controlpanel/api/tasks/handlers/s3.py | 70 ++++++++++---- controlpanel/api/tasks/s3bucket.py | 67 +++++++++++--- controlpanel/api/tasks/task_base.py | 7 +- .../commands/migrating_customers.py | 5 +- controlpanel/frontend/views/app.py | 14 ++- controlpanel/frontend/views/datasource.py | 26 +++--- tests/api/models/test_apps3bucket.py | 17 ++-- tests/api/models/test_s3bucket.py | 22 ++--- tests/api/models/test_users3bucket.py | 34 ++++--- .../tasks/test_create_app_auth_settings.py | 15 ++- tests/api/tasks/test_create_app_aws_role.py | 25 ++--- tests/api/tasks/test_s3.py | 74 +++++++++++++-- tests/api/views/test_apps3bucket.py | 16 ++-- tests/api/views/test_users3bucket.py | 8 +- tests/frontend/views/test_datasource.py | 63 ++++++++----- 24 files changed, 404 insertions(+), 272 deletions(-) diff --git a/controlpanel/api/models/apps3bucket.py b/controlpanel/api/models/apps3bucket.py index 9d12b6582..9cd5d44d4 100644 --- a/controlpanel/api/models/apps3bucket.py +++ b/controlpanel/api/models/apps3bucket.py @@ -2,9 +2,8 @@ from django.db import models # First-party/Local -from controlpanel.api import cluster +from controlpanel.api import cluster, tasks from controlpanel.api.models.access_to_s3bucket import AccessToS3Bucket -from controlpanel.api import tasks class AppS3Bucket(AccessToS3Bucket): @@ -43,11 +42,6 @@ def __repr__(self): def grant_bucket_access(self): tasks.S3BucketGrantToApp(self, self.current_user).create_task() - # cluster.App(self.app).grant_bucket_access( - # self.s3bucket.arn, - # self.access_level, - # self.resources, - # ) def revoke_bucket_access(self): - cluster.App(self.app).revoke_bucket_access(self.s3bucket.arn) + tasks.S3BucketRevokeAppAccess(self, self.current_user).create_task() diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py index 36efdfe91..90de336f3 100644 --- a/controlpanel/api/models/s3bucket.py +++ b/controlpanel/api/models/s3bucket.py @@ -10,10 +10,9 @@ from django_extensions.db.models import TimeStampedModel # First-party/Local -from controlpanel.api import cluster, validators +from controlpanel.api import cluster, tasks, validators from controlpanel.api.models.apps3bucket import AppS3Bucket from controlpanel.api.models.users3bucket import UserS3Bucket -from controlpanel.api import tasks def s3bucket_console_url(name): @@ -129,24 +128,27 @@ def access_level(self, user): def save(self, *args, **kwargs): is_create = not self.pk - super().save(*args, **kwargs) - - if is_create: - bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner) - - # self.cluster.create(bucket_owner) - tasks.S3BucketCreate(self, self.created_by).create_task() - - # XXX created_by is always set if model is saved by the API view - if self.created_by: - UserS3Bucket.objects.create( - user=self.created_by, - current_user=self.created_by, - s3bucket=self, - is_admin=True, - access_level=UserS3Bucket.READWRITE, - ) + if not is_create: + return self + + tasks.S3BucketCreate( + entity=self, + user=self.created_by, + extra_data={ + "bucket_owner": kwargs.pop("bucket_owner", self.bucket_owner), + } + ).create_task() + + # created_by should always be set, but this is a failsafe + if self.created_by: + UserS3Bucket.objects.create( + user=self.created_by, + current_user=self.created_by, + s3bucket=self, + is_admin=True, + access_level=UserS3Bucket.READWRITE, + ) return self diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py index 4c5b87c44..7e5cfbec3 100644 --- a/controlpanel/api/models/users3bucket.py +++ b/controlpanel/api/models/users3bucket.py @@ -2,9 +2,8 @@ from django.db import models # First-party/Local -from controlpanel.api import cluster +from controlpanel.api import cluster, tasks from controlpanel.api.models.access_to_s3bucket import AccessToS3Bucket -from controlpanel.api import tasks class UserS3Bucket(AccessToS3Bucket): @@ -46,22 +45,10 @@ def __repr__(self): ) def grant_bucket_access(self): - if self.s3bucket.is_folder: - return cluster.User(self.user).grant_folder_access( - root_folder_path=self.s3bucket.name, - access_level=self.access_level, - paths=self.paths, - ) tasks.S3BucketGrantToUser(self, self.current_user).create_task() - # cluster.User(self.user).grant_bucket_access( - # self.s3bucket.arn, - # self.access_level, - # self.resources, - # ) def revoke_bucket_access(self): - if self.s3bucket.is_folder: - return cluster.User(self.user).revoke_folder_access( - root_folder_path=self.s3bucket.name - ) - cluster.User(self.user).revoke_bucket_access(self.s3bucket.arn) + # TODO when soft delete is added, this should be updated to use the user that + # has deleted the parent S3bucket to ensure we store the user that has sent the + # task in the case of cascading deletes + tasks.S3BucketRevokeUserAccess(self, self.current_user).create_task() diff --git a/controlpanel/api/serializers.py b/controlpanel/api/serializers.py index e33b21f14..ac17f4114 100644 --- a/controlpanel/api/serializers.py +++ b/controlpanel/api/serializers.py @@ -404,8 +404,10 @@ def _process_existing_env_settings(self, app_auth_settings, auth_settings_status env_data["variables"] = sorted(var_data.values(), key=lambda x: x["name"]) env_data["auth_required"] = auth_required - def _process_redundant_envs(self, app_auth_settings, auth_settings_status): + # NB. if earlier call to get app_auth_settings failed, this will have been + # passed into serializer as an empty dict. Which results in all env details + # being marked as redundant mistakenly redundant_envs = list(set(auth_settings_status.keys()) - set(app_auth_settings.keys())) for env_name in redundant_envs: diff --git a/controlpanel/api/tasks/__init__.py b/controlpanel/api/tasks/__init__.py index dd777c893..19e653ff1 100644 --- a/controlpanel/api/tasks/__init__.py +++ b/controlpanel/api/tasks/__init__.py @@ -1,7 +1,10 @@ -from controlpanel.api.tasks.app import AppCreateRole, AppCreateAuth +# First-party/Local +from controlpanel.api.tasks.app import AppCreateAuth, AppCreateRole from controlpanel.api.tasks.s3bucket import ( S3BucketCreate, - S3BucketGrantToUser, S3BucketGrantToApp, + S3BucketGrantToUser, + S3BucketRevokeAppAccess, + S3BucketRevokeUserAccess, ) diff --git a/controlpanel/api/tasks/app.py b/controlpanel/api/tasks/app.py index 0df460f9d..94468d322 100644 --- a/controlpanel/api/tasks/app.py +++ b/controlpanel/api/tasks/app.py @@ -1,4 +1,7 @@ +# Third-party from django.conf import settings + +# First-party/Local from controlpanel.api.tasks.task_base import TaskBase diff --git a/controlpanel/api/tasks/handlers/__init__.py b/controlpanel/api/tasks/handlers/__init__.py index 14190b744..454fd9094 100644 --- a/controlpanel/api/tasks/handlers/__init__.py +++ b/controlpanel/api/tasks/handlers/__init__.py @@ -1,13 +1,18 @@ +# First-party/Local from controlpanel import celery_app - +from controlpanel.api.tasks.handlers.app import CreateAppAuthSettings, CreateAppAWSRole from controlpanel.api.tasks.handlers.s3 import ( - CreateS3Bucket, GrantAppS3BucketAccess, GrantUserS3BucketAccess, + CreateS3Bucket, + GrantAppS3BucketAccess, + GrantUserS3BucketAccess, + S3BucketRevokeAppAccess, + S3BucketRevokeUserAccess, ) -from controlpanel.api.tasks.handlers.app import CreateAppAuthSettings, CreateAppAWSRole - create_app_aws_role = celery_app.register_task(CreateAppAWSRole()) create_s3bucket = celery_app.register_task(CreateS3Bucket()) grant_app_s3bucket_access = celery_app.register_task(GrantAppS3BucketAccess()) grant_user_s3bucket_access = celery_app.register_task(GrantUserS3BucketAccess()) create_app_auth_settings = celery_app.register_task(CreateAppAuthSettings()) +revoke_user_s3bucket_access = celery_app.register_task(S3BucketRevokeUserAccess()) +revoke_app_s3bucket_access = celery_app.register_task(S3BucketRevokeAppAccess()) diff --git a/controlpanel/api/tasks/handlers/app.py b/controlpanel/api/tasks/handlers/app.py index 8b302954e..accc6d68b 100644 --- a/controlpanel/api/tasks/handlers/app.py +++ b/controlpanel/api/tasks/handlers/app.py @@ -1,22 +1,21 @@ +# First-party/Local from controlpanel.api import cluster -from controlpanel.api.models import App +from controlpanel.api.models import App, User from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler class CreateAppAuthSettings(BaseModelTaskHandler): model = App name = "create_app_auth_settings" - permission_required = "api.create_app" - def has_permission(self, user, obj=None): - if not super().has_permission(user, obj): - return False + def handle(self, envs, disable_authentication, connections): + task_user = User.objects.filter(pk=self.task_user_pk).first() + if not task_user or not task_user.github_api_token: + # TODO maybe log this as something has gone wrong? + return self.complete() - return user.github_api_token - - def run_task(self, app, user, envs, disable_authentication, connections): for env in envs: - cluster.App(app, user.github_api_token).create_auth_settings( + cluster.App(self.object, task_user.github_api_token).create_auth_settings( env_name=env, disable_authentication=disable_authentication, connections=connections, @@ -27,8 +26,7 @@ def run_task(self, app, user, envs, disable_authentication, connections): class CreateAppAWSRole(BaseModelTaskHandler): model = App name = "create_app_aws_role" - permission_required = "api.create_app" - def run_task(self, app, user): - cluster.App(app).create_iam_role() + def handle(self): + cluster.App(self.object).create_iam_role() self.complete() diff --git a/controlpanel/api/tasks/handlers/base.py b/controlpanel/api/tasks/handlers/base.py index 543a7e3b0..4f1455566 100644 --- a/controlpanel/api/tasks/handlers/base.py +++ b/controlpanel/api/tasks/handlers/base.py @@ -1,13 +1,34 @@ +# Third-party from celery import Task as CeleryTask -from controlpanel.api.models import User, Task +# First-party/Local +from controlpanel.api.models import Task, User -class BaseModelTaskHandler(CeleryTask): +class BaseTaskHandler(CeleryTask): + + def complete(self): + task = Task.objects.filter(task_id=self.request.id).first() + if task: + task.completed = True + task.save() + + def run(self, *args, **kwargs): + self.handle(*args, **kwargs) + + def handle(self, *args, **kwargs): + """ + Should contain the logic to run the task, and will be called after the run + method has been successfully called. + """ + raise NotImplementedError("Task logic not implemented") + + +class BaseModelTaskHandler(BaseTaskHandler): name = None model = None - permission_required = None - + object = None + task_user_pk = None # can be applied to project settings also # these settings mean that messages are only removed from the queue (acknowledged) # when returned. if an error occurs, they remain in the queue, and will be resent @@ -24,58 +45,14 @@ def get_object(self, pk): # added back to the queue as could be due to a race condition raise exc - def get_user(self, pk): - """ - Try to find the user, then check they have the correct permission required to - run the task action. - """ - try: - return User.objects.get(pk=pk) - except User.DoesNotExist as exc: - # if the user is found, this should be a hard fail? So suggest log the error - # and then mark as complete to stop task being rerun? - return None - - def has_permission(self, user, obj=None): + def run(self, obj_pk, task_user_pk, *args, **kwargs): """ - Check that the user has permission to run the task on the given object. - Override on the subclass for further permission checks. + Default method that a celery Task object requires to be defined, and will be + called by the worker when a message is received by the queue. This will look up + the instance of the model, and store the PK of the user running the task to use + to look up the user later if required. The `handle` method is then called + with any other args and kwargs sent. """ - if not self.permission_required: - raise NotImplementedError("Must define a permission to check") - - if not user.has_perm(self.permission_required, obj=obj): - # log that the user did not have permission? - return False - - return True - - def complete(self): - task = Task.objects.filter(task_id=self.request.id).first() - if task: - task.completed = True - task.save() - - def run(self, obj_pk, user_pk, *args, **kwargs): - """ - Default message that a celery Task object requires to be defined, and will be - called by the worker when a message is received by the queue. This runs some - lookups and validates the user, and if these pass calls `run_task` which must - be defined on any subclass of BaseTaskHandler. - """ - obj = self.get_object(obj_pk) - user = self.get_user(user_pk) - if not user: - return self.complete() - - if not self.has_permission(user, obj): - return self.complete() - - self.run_task(obj, user, *args, **kwargs) - - def run_task(self, *args, **kwargs): - """ - Should contain the logic to run the task, and will be called after the run - method has been successfully called. - """ - raise NotImplementedError("Task logic not implemented") + self.object = self.get_object(obj_pk) + self.task_user_pk = task_user_pk + self.handle(*args, **kwargs) diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py index 87928514a..fd8fe922e 100644 --- a/controlpanel/api/tasks/handlers/s3.py +++ b/controlpanel/api/tasks/handlers/s3.py @@ -1,28 +1,31 @@ +# Third-party +from celery import Task as CeleryTask + +# First-party/Local from controlpanel.api import cluster -from controlpanel.api.models import S3Bucket, UserS3Bucket, AppS3Bucket -from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler +from controlpanel.api.models import App, AppS3Bucket, S3Bucket, User, UserS3Bucket +from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler, BaseTaskHandler class CreateS3Bucket(BaseModelTaskHandler): model = S3Bucket name = "create_s3bucket" - permission_required = "api.create_s3bucket" - def run_task(self, bucket, user, bucket_owner="APP"): - bucket.cluster.create(owner=bucket_owner) + def handle(self, bucket_owner=None): + bucket_owner = bucket_owner or "USER" + self.object.cluster.create(owner=bucket_owner) self.complete() class GrantAppS3BucketAccess(BaseModelTaskHandler): model = AppS3Bucket name = 'grant_app_s3bucket_access' - permission_required = 'api.create_apps3bucket' - def run_task(self, app_bucket, user): - cluster.App(app_bucket.app).grant_bucket_access( - app_bucket.s3bucket.arn, - app_bucket.access_level, - app_bucket.resources, + def handle(self): + cluster.App(self.object.app).grant_bucket_access( + self.object.s3bucket.arn, + self.object.access_level, + self.object.resources, ) self.complete() @@ -30,12 +33,43 @@ def run_task(self, app_bucket, user): class GrantUserS3BucketAccess(BaseModelTaskHandler): model = UserS3Bucket name = "grant_user_s3bucket_access" - permission_required = "api.create_users3bucket" - def run_task(self, user_bucket, user): - cluster.User(user_bucket.user).grant_bucket_access( - user_bucket.s3bucket.arn, - user_bucket.access_level, - user_bucket.resources, - ) + def handle(self): + if self.object.s3bucket.is_folder: + cluster.User(self.object.user).grant_folder_access( + root_folder_path=self.object.s3bucket.name, + access_level=self.object.access_level, + paths=self.object.paths, + ) + else: + cluster.User(self.object.user).grant_bucket_access( + bucket_arn=self.object.s3bucket.arn, + access_level=self.object.access_level, + path_arns=self.object.resources, + ) + self.complete() + + +class S3BucketRevokeUserAccess(BaseTaskHandler): + name = "revoke_user_s3bucket_access" + + def handle(self, bucket_identifier, bucket_user_pk, is_folder): + bucket_user = User.objects.get(pk=bucket_user_pk) + if is_folder: + cluster.User(bucket_user).revoke_folder_access(bucket_identifier) + else: + cluster.User(bucket_user).revoke_bucket_access(bucket_identifier) + self.complete() + + +class S3BucketRevokeAppAccess(BaseTaskHandler): + name = "revoke_app_s3bucket_access" + + def handle(self, bucket_arn, app_pk): + try: + app = App.objects.get(pk=app_pk) + except App.DoesNotExist: + # if the app doesnt exist, nothing to revoke, so mark completed + self.complete() + cluster.App(app).revoke_bucket_access(bucket_arn) self.complete() diff --git a/controlpanel/api/tasks/s3bucket.py b/controlpanel/api/tasks/s3bucket.py index 21b3a2286..c59f4070d 100644 --- a/controlpanel/api/tasks/s3bucket.py +++ b/controlpanel/api/tasks/s3bucket.py @@ -1,4 +1,7 @@ +# Third-party from django.conf import settings + +# First-party/Local from controlpanel.api.tasks.task_base import TaskBase @@ -14,34 +17,70 @@ def task_name(self): def task_description(self): return "creating s3 bucket" + def _get_args_list(self): + return [ + self.entity.id, + self.user.id if self.user else 'None', + self.extra_data.get('bucket_owner'), + ] -class S3BucketGrantToUser(TaskBase): - ENTITY_CLASS = "UserS3Bucket" + +class S3AccessMixin: + ACTION = None + ROLE = None @property def task_name(self): - return "grant_user_s3bucket_access" + return "{action}_{role}_s3bucket_access".format( + action=self.ACTION.lower(), + role=self.ROLE.lower() + ) @property def task_description(self): - return "granting access to the user" + return "{action} access to the {role}".format( + action=self.ACTION.lower(), + role=self.ROLE.lower() + ) @property def entity_description(self): return self.entity.s3bucket.name -class S3BucketGrantToApp(TaskBase): +class S3BucketGrantToUser(S3AccessMixin, TaskBase): + ENTITY_CLASS = "UserS3Bucket" + ACTION = "GRANT" + ROLE = "USER" + + +class S3BucketGrantToApp(S3AccessMixin, TaskBase): ENTITY_CLASS = "AppS3Bucket" + ACTION = "GRANT" + ROLE = "APP" - @property - def task_name(self): - return "grant_app_s3bucket_access" - @property - def task_description(self): - return "granting access to the app" +class S3BucketRevokeUserAccess(S3AccessMixin, TaskBase): + ENTITY_CLASS = "UserS3Bucket" + ACTION = "REVOKE" + ROLE = "USER" - @property - def entity_description(self): - return self.entity.s3bucket.name + def _get_args_list(self): + bucket = self.entity.s3bucket + return [ + bucket.name if bucket.is_folder else bucket.arn, + self.entity.user.pk, + bucket.is_folder, + ] + + +class S3BucketRevokeAppAccess(S3AccessMixin, TaskBase): + ENTITY_CLASS = "AppS3Bucket" + ACTION = "REVOKE" + ROLE = "APP" + + def _get_args_list(self): + return [ + self.entity.s3bucket.arn, + self.entity.app.pk, + ] diff --git a/controlpanel/api/tasks/task_base.py b/controlpanel/api/tasks/task_base.py index 96e79c7f1..8b124ed54 100644 --- a/controlpanel/api/tasks/task_base.py +++ b/controlpanel/api/tasks/task_base.py @@ -1,8 +1,13 @@ +# Standard library import uuid + +# Third-party from django.conf import settings +# First-party/Local from controlpanel.api.message_broker import ( - MessageBrokerClient, LocalMessageBrokerClient + LocalMessageBrokerClient, + MessageBrokerClient, ) from controlpanel.api.models.task import Task diff --git a/controlpanel/cli/management/commands/migrating_customers.py b/controlpanel/cli/management/commands/migrating_customers.py index 9d978b8d0..ed92e993a 100644 --- a/controlpanel/cli/management/commands/migrating_customers.py +++ b/controlpanel/cli/management/commands/migrating_customers.py @@ -10,7 +10,10 @@ class Command(BaseCommand): - help = "Copy the customers of an application from old auth client to new auth clients" + help = """Copy the customers of an application from old auth client to new auth + clients. This is idempotent, meaning that rerunning for the apps will not + problems, all copied customers will remain but any un-copied customers will be + migrated.""" DEFAULT_ENVS = ["dev", "prod"] diff --git a/controlpanel/frontend/views/app.py b/controlpanel/frontend/views/app.py index c8a271fd9..d009bcd74 100644 --- a/controlpanel/frontend/views/app.py +++ b/controlpanel/frontend/views/app.py @@ -79,12 +79,17 @@ def _get_all_app_settings(self, app): access_repo_error_msg = None github_settings_access_error_msg = None try: + # NB: if this call fails.... deployment_env_names = app_manager_ins.get_deployment_envs() except requests.exceptions.HTTPError as ex: access_repo_error_msg = ex.__str__() + github_settings_access_error_msg = ex.__str__() + # ...this is set to empty list... deployment_env_names = [] + # ...which means this will remain empty dict... deployments_settings = {} auth0_connections = app.auth0_connections_by_env() + # ...so no call to get secrets/variables is made try: for env_name in deployment_env_names: deployments_settings[env_name] = { @@ -94,7 +99,7 @@ def _get_all_app_settings(self, app): } except requests.exceptions.HTTPError as ex: github_settings_access_error_msg = ex.__str__() - + # ...knock on effect is in serializers.py these envs will be marked as redundant return deployments_settings, access_repo_error_msg, \ github_settings_access_error_msg @@ -111,6 +116,8 @@ def get_context_data(self, **kwargs): exclude_connected=True, ) + # If auth settings not returned, all envs marked redundant in the serializer. + # Should hide them instead? auth_settings, access_repo_error_msg, github_settings_access_error_msg \ = self._get_all_app_settings(app) auth0_clients_status = app.auth0_clients_status() @@ -305,6 +312,11 @@ class RevokeAppAccess(OIDCLoginRequiredMixin, PermissionRequiredMixin, DeleteVie model = AppS3Bucket permission_required = "api.remove_app_bucket" + def get_object(self, queryset=None): + obj = super().get_object(queryset=queryset) + obj.current_user = self.request.user + return obj + def get_success_url(self): messages.success(self.request, "Successfully disconnected data source") return reverse_lazy("manage-app", kwargs={"pk": self.object.app.id}) diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py index e6ef13dfb..5099089ea 100644 --- a/controlpanel/frontend/views/datasource.py +++ b/controlpanel/frontend/views/datasource.py @@ -15,7 +15,7 @@ from rules.contrib.views import PermissionRequiredMixin # First-party/Local -from controlpanel.api import cluster +from controlpanel.api import cluster, tasks from controlpanel.api.elasticsearch import bucket_hits_aggregation from controlpanel.api.models import ( IAMManagedPolicy, @@ -174,16 +174,15 @@ def form_valid(self, form): datasource_type = self.request.GET.get("type") try: - with transaction.atomic(): - self.object = S3Bucket.objects.create( - name=name, - created_by=self.request.user, - is_data_warehouse=datasource_type == "warehouse", - ) - messages.success( - self.request, - f"Successfully created {name} {datasource_type} data source", - ) + self.object = S3Bucket.objects.create( + name=name, + created_by=self.request.user, + is_data_warehouse=datasource_type == "warehouse", + ) + messages.success( + self.request, + f"Successfully created {name} {datasource_type} data source", + ) except Exception as ex: form.add_error("name", str(ex)) return FormMixin.form_invalid(self, form) @@ -294,6 +293,11 @@ class RevokeAccess(OIDCLoginRequiredMixin, PermissionRequiredMixin, DeleteView): model = UserS3Bucket permission_required = "api.destroy_users3bucket" + def get_object(self, queryset=None): + obj = super().get_object(queryset=queryset) + obj.current_user = self.request.user + return obj + def get_success_url(self): messages.success(self.request, "Successfully revoked access") return reverse_lazy("manage-datasource", kwargs={"pk": self.object.s3bucket.id}) diff --git a/tests/api/models/test_apps3bucket.py b/tests/api/models/test_apps3bucket.py index a0e0c75ec..6402c2a47 100644 --- a/tests/api/models/test_apps3bucket.py +++ b/tests/api/models/test_apps3bucket.py @@ -46,14 +46,16 @@ def test_aws_permissions(app, bucket, sqs, helpers): apps3bucket.save() messages = helpers.retrieve_messages(sqs) - helpers.validate_task_with_sqs_messages(messages, AppS3Bucket.__name__, apps3bucket.id) + helpers.validate_task_with_sqs_messages( + messages, AppS3Bucket.__name__, apps3bucket.id + ) @pytest.mark.django_db def test_delete_revoke_permissions(app, bucket): - with patch("controlpanel.api.aws.AWSRole.grant_bucket_access"), \ - patch("controlpanel.api.cluster.AWSRole.revoke_bucket_access") \ - as revoke_bucket_access_action: + with patch( + "controlpanel.api.tasks.S3BucketRevokeAppAccess" + ) as revoke_bucket_access_task: apps3bucket = mommy.make( "api.AppS3Bucket", app=app, @@ -63,7 +65,8 @@ def test_delete_revoke_permissions(app, bucket): apps3bucket.delete() - revoke_bucket_access_action.assert_called_with( - apps3bucket.iam_role_name, - bucket.arn, + revoke_bucket_access_task.assert_called_with( + apps3bucket, + None ) + revoke_bucket_access_task.return_value.create_task.assert_called() diff --git a/tests/api/models/test_s3bucket.py b/tests/api/models/test_s3bucket.py index a466c1af6..9d9d89f95 100644 --- a/tests/api/models/test_s3bucket.py +++ b/tests/api/models/test_s3bucket.py @@ -23,20 +23,16 @@ def bucket(): def test_delete_revokes_permissions(bucket): - with patch("controlpanel.api.aws.AWSRole.grant_bucket_access"), \ - patch("controlpanel.api.cluster.AWSRole.revoke_bucket_access") \ - as revoke_bucket_access_action: - users3bucket = mommy.make("api.UserS3Bucket", s3bucket=bucket) - apps3bucket = mommy.make("api.AppS3Bucket", s3bucket=bucket) - + with patch("controlpanel.api.models.AppS3Bucket.revoke_bucket_access") as app_revoke_bucket_access, \ + patch("controlpanel.api.models.UserS3Bucket.revoke_bucket_access") as user_revoke_user_bucket_access: + # link the bucket with an UserS3Bucket and AppS3Bucket + mommy.make("api.UserS3Bucket", s3bucket=bucket) + mommy.make("api.AppS3Bucket", s3bucket=bucket) + # delete the source S3Bucket bucket.delete() - - revoke_bucket_access_action.assert_has_calls( - [ - call(apps3bucket.iam_role_name, bucket.arn), - call(users3bucket.iam_role_name, bucket.arn), - ] - ) + # check that related objects revoke access methods were called + app_revoke_bucket_access.assert_called_once() + user_revoke_user_bucket_access.assert_called_once() def test_delete_marks_bucket_for_archival(bucket): diff --git a/tests/api/models/test_users3bucket.py b/tests/api/models/test_users3bucket.py index 8a55d89b5..7d8effcda 100644 --- a/tests/api/models/test_users3bucket.py +++ b/tests/api/models/test_users3bucket.py @@ -7,8 +7,8 @@ from model_mommy import mommy # First-party/Local -from controlpanel.api.models.access_to_s3bucket import AccessToS3Bucket from controlpanel.api.models import UserS3Bucket +from controlpanel.api.models.access_to_s3bucket import AccessToS3Bucket @pytest.fixture @@ -46,35 +46,33 @@ def test_aws_create_bucket(user, bucket, sqs, helpers): access_level=AccessToS3Bucket.READONLY ) messages = helpers.retrieve_messages(sqs) - helpers.validate_task_with_sqs_messages(messages, UserS3Bucket.__name__, users3bucket.id) + helpers.validate_task_with_sqs_messages( + messages, UserS3Bucket.__name__, users3bucket.id + ) @pytest.mark.django_db -@patch("controlpanel.api.cluster.AWSRole.grant_folder_access") -def test_aws_create_folder(grant_folder_access, user, bucket): +@patch("controlpanel.api.models.users3bucket.tasks") +def test_aws_create_folder(tasks, user, bucket): with patch.object( bucket.__class__, "is_folder", new_callable=PropertyMock(return_value=True) ): - user.users3buckets.create( + user_bucket = user.users3buckets.create( s3bucket=bucket, access_level=AccessToS3Bucket.READONLY, + current_user=user ) - grant_folder_access.assert_called_with( - role_name=user.iam_role_name, - root_folder_path=bucket.name, - access_level=AccessToS3Bucket.READONLY, - paths=[], + tasks.S3BucketGrantToUser.assert_called_once_with( + user_bucket, user ) + tasks.S3BucketGrantToUser.return_value.create_task.assert_called_once() @pytest.mark.django_db -def test_delete_revoke_permissions(user, bucket, users3bucket): +def test_delete_revoke_permissions(bucket, users3bucket): with patch( - "controlpanel.api.cluster.AWSRole.revoke_bucket_access" - ) as revoke_bucket_access_action: + "controlpanel.api.tasks.S3BucketRevokeUserAccess" + ) as revoke_user_access_task: users3bucket.delete() - revoke_bucket_access_action.assert_called_with( - user.iam_role_name, - bucket.arn, - ) - # TODO get policy from call and assert bucket ARN removed + revoke_user_access_task.assert_called_with(users3bucket, None) + revoke_user_access_task.return_value.create_task.assert_called() diff --git a/tests/api/tasks/test_create_app_auth_settings.py b/tests/api/tasks/test_create_app_auth_settings.py index d52fa59f3..f2f5acfce 100644 --- a/tests/api/tasks/test_create_app_auth_settings.py +++ b/tests/api/tasks/test_create_app_auth_settings.py @@ -1,8 +1,11 @@ -import pytest -from unittest.mock import patch, PropertyMock +# Standard library +from unittest.mock import PropertyMock, patch +# Third-party +import pytest from model_mommy import mommy +# First-party/Local from controlpanel.api.models import App from controlpanel.api.tasks.handlers import create_app_auth_settings @@ -21,12 +24,14 @@ def test_cluster_not_called_without_valid_app(cluster, complete, users): @pytest.mark.django_db @patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete") @patch("controlpanel.api.tasks.handlers.app.cluster") -@patch("controlpanel.api.models.user.User.github_api_token", new=PropertyMock(return_value=None)) +@patch("controlpanel.api.models.user.User.github_api_token", new=PropertyMock(return_value=None)) # noqa def test_cluster_not_called_without_github_api_token(cluster, complete, users): app = mommy.make("api.App") user = users["superuser"] - create_app_auth_settings(app.pk, user.pk) + create_app_auth_settings( + app.pk, user.pk, "envs", "disable_authentication", "connections", + ) # role should not be created cluster.App.assert_not_called() @@ -37,7 +42,7 @@ def test_cluster_not_called_without_github_api_token(cluster, complete, users): @pytest.mark.django_db @patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete") @patch("controlpanel.api.tasks.handlers.app.cluster") -@patch("controlpanel.api.models.user.User.github_api_token", new=PropertyMock(return_value="dummy-token")) +@patch("controlpanel.api.models.user.User.github_api_token", new=PropertyMock(return_value="dummy-token")) # noqa def test_valid_app_and_user(cluster, complete, users): app = mommy.make("api.App") diff --git a/tests/api/tasks/test_create_app_aws_role.py b/tests/api/tasks/test_create_app_aws_role.py index 8b719c01a..a882a9845 100644 --- a/tests/api/tasks/test_create_app_aws_role.py +++ b/tests/api/tasks/test_create_app_aws_role.py @@ -1,10 +1,16 @@ -import pytest +# Standard library from unittest.mock import patch +# Third-party +import pytest from model_mommy import mommy +# First-party/Local from controlpanel.api.models import App -from controlpanel.api.tasks.handlers import create_app_aws_role, create_app_auth_settings +from controlpanel.api.tasks.handlers import ( + create_app_auth_settings, + create_app_aws_role, +) @pytest.mark.django_db @@ -18,21 +24,6 @@ def test_cluster_not_called_without_valid_app(cluster, complete, users): complete.assert_not_called() -@pytest.mark.django_db -@patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete") -@patch("controlpanel.api.tasks.handlers.app.cluster") -def test_cluster_not_called_without_valid_user(cluster, complete, users): - app = mommy.make("api.App") - - user = users["normal_user"] - create_app_aws_role(app.pk, user.pk) - - # role should not be created - cluster.App.assert_not_called() - # task should be complete so that it does not run again, as user not valid - complete.assert_called_once() - - @pytest.mark.django_db @patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete") @patch("controlpanel.api.tasks.handlers.app.cluster") diff --git a/tests/api/tasks/test_s3.py b/tests/api/tasks/test_s3.py index babd83877..dc275accf 100644 --- a/tests/api/tasks/test_s3.py +++ b/tests/api/tasks/test_s3.py @@ -1,11 +1,16 @@ +# Third-party import pytest from mock import patch from model_mommy import mommy -from controlpanel.api import cluster -from controlpanel.api.models import S3Bucket, AppS3Bucket, UserS3Bucket +# First-party/Local +from controlpanel.api.models import AppS3Bucket, S3Bucket, UserS3Bucket from controlpanel.api.tasks.handlers import ( - create_s3bucket, grant_app_s3bucket_access, grant_user_s3bucket_access, + create_s3bucket, + grant_app_s3bucket_access, + grant_user_s3bucket_access, + revoke_app_s3bucket_access, + revoke_user_s3bucket_access, ) @@ -15,7 +20,7 @@ (grant_app_s3bucket_access, AppS3Bucket), (grant_user_s3bucket_access, UserS3Bucket), ]) -@patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete") +@patch("controlpanel.api.tasks.handlers.base.BaseTaskHandler.complete") def test_exception_raised_when_called_without_valid_app( complete, users, task_method, model ): @@ -26,15 +31,19 @@ def test_exception_raised_when_called_without_valid_app( @pytest.mark.django_db -@patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete") +@patch("controlpanel.api.tasks.handlers.base.BaseTaskHandler.complete") @patch("controlpanel.api.models.s3bucket.cluster") def test_bucket_created(cluster, complete, users): - s3bucket = mommy.make("api.S3Bucket") + s3bucket = mommy.make("api.S3Bucket", bucket_owner="APP") - create_s3bucket(s3bucket.pk, users["superuser"].pk, bucket_owner="APP") + create_s3bucket( + s3bucket.pk, users["superuser"].pk, bucket_owner=s3bucket.bucket_owner + ) cluster.S3Bucket.assert_called_once_with(s3bucket) - cluster.S3Bucket.return_value.create.assert_called_once_with(owner="APP") + cluster.S3Bucket.return_value.create.assert_called_once_with( + owner=s3bucket.bucket_owner + ) complete.assert_called_once() @@ -43,7 +52,7 @@ def test_bucket_created(cluster, complete, users): (grant_app_s3bucket_access, "api.AppS3Bucket", "App"), (grant_user_s3bucket_access, "api.UserS3Bucket", "User"), ]) -@patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.complete") +@patch("controlpanel.api.tasks.handlers.base.BaseTaskHandler.complete") @patch("controlpanel.api.tasks.handlers.s3.cluster") def test_access_granted( cluster, complete, users, task_method, model_class, cluster_class @@ -56,3 +65,50 @@ def test_access_granted( cluster_obj.assert_called_once() cluster_obj.return_value.grant_bucket_access.assert_called_once() complete.assert_called_once() + + +@pytest.mark.django_db +@pytest.mark.parametrize("bucket_name, is_folder", [ + ("example-bucket", False), + ("example-bucket/folder", True), +]) +@patch("controlpanel.api.tasks.handlers.base.BaseTaskHandler.complete") +@patch("controlpanel.api.tasks.handlers.s3.cluster") +def test_revoke_user_access(cluster, complete, bucket_name, is_folder): + user_bucket_access = mommy.make("api.UserS3Bucket", s3bucket__name=bucket_name) + s3bucket = user_bucket_access.s3bucket + bucket_identifier = s3bucket.name if is_folder else s3bucket.arn + revoke_user_s3bucket_access( + bucket_identifier=bucket_identifier, + bucket_user_pk=user_bucket_access.user.pk, + is_folder=is_folder + ) + + cluster.User.assert_called_once_with(user_bucket_access.user) + if is_folder: + cluster.User.return_value.revoke_folder_access.assert_called_once_with( + bucket_identifier + ) + else: + cluster.User.return_value.revoke_bucket_access.assert_called_once_with( + bucket_identifier + ) + + complete.assert_called_once() + + +@pytest.mark.django_db +@patch("controlpanel.api.tasks.handlers.base.BaseTaskHandler.complete") +@patch("controlpanel.api.tasks.handlers.s3.cluster") +def test_revoke_app_access(cluster, complete): + app_bucket_access = mommy.make("api.AppS3Bucket") + revoke_app_s3bucket_access( + bucket_arn=app_bucket_access.s3bucket.arn, + app_pk=app_bucket_access.app.pk, + ) + + cluster.App.assert_called_once_with(app_bucket_access.app) + cluster.App.return_value.revoke_bucket_access.assert_called_once_with( + app_bucket_access.s3bucket.arn + ) + complete.assert_called_once() diff --git a/tests/api/views/test_apps3bucket.py b/tests/api/views/test_apps3bucket.py index 8a7a45479..c61df0bac 100644 --- a/tests/api/views/test_apps3bucket.py +++ b/tests/api/views/test_apps3bucket.py @@ -60,16 +60,12 @@ def test_detail(client, apps3buckets): def test_delete(client, apps3buckets): with patch( - "controlpanel.api.aws.AWSRole.revoke_bucket_access" + "controlpanel.api.models.AppS3Bucket.revoke_bucket_access" ) as revoke_bucket_access: response = client.delete(reverse("apps3bucket-detail", (apps3buckets[1].id,))) assert response.status_code == status.HTTP_204_NO_CONTENT - revoke_bucket_access.assert_called_with( - apps3buckets[1].iam_role_name, - apps3buckets[1].s3bucket.arn, - ) - # TODO get policy document JSON from call and assert bucket ARN not present + revoke_bucket_access.assert_called() response = client.get(reverse("apps3bucket-detail", (apps3buckets[1].id,))) assert response.status_code == status.HTTP_404_NOT_FOUND @@ -85,7 +81,9 @@ def test_create(client, apps, buckets, sqs, helpers): assert response.status_code == status.HTTP_201_CREATED apps3bucket = AppS3Bucket.objects.get(app=apps[1], s3bucket=buckets[3]) messages = helpers.retrieve_messages(sqs) - helpers.validate_task_with_sqs_messages(messages, AppS3Bucket.__name__, apps3bucket.id) + helpers.validate_task_with_sqs_messages( + messages, AppS3Bucket.__name__, apps3bucket.id + ) def test_update(client, apps, apps3buckets, buckets, sqs, helpers): @@ -103,7 +101,9 @@ def test_update(client, apps, apps3buckets, buckets, sqs, helpers): assert response.data["access_level"] == data["access_level"] apps3bucket = AppS3Bucket.objects.get(app=apps[1], s3bucket=buckets[1]) messages = helpers.retrieve_messages(sqs) - helpers.validate_task_with_sqs_messages(messages, AppS3Bucket.__name__, apps3bucket.id) + helpers.validate_task_with_sqs_messages( + messages, AppS3Bucket.__name__, apps3bucket.id + ) def test_update_bad_requests(client, apps, apps3buckets, buckets): diff --git a/tests/api/views/test_users3bucket.py b/tests/api/views/test_users3bucket.py index 62610a9ef..5677b1579 100644 --- a/tests/api/views/test_users3bucket.py +++ b/tests/api/views/test_users3bucket.py @@ -62,16 +62,12 @@ def test_create(client, buckets, users, sqs, helpers): def test_delete(client, users3buckets): with patch( - "controlpanel.api.aws.AWSRole.revoke_bucket_access" + "controlpanel.api.models.UserS3Bucket.revoke_bucket_access" ) as revoke_bucket_access: response = client.delete(reverse("users3bucket-detail", (users3buckets[1].id,))) assert response.status_code == status.HTTP_204_NO_CONTENT - revoke_bucket_access.assert_called_with( - users3buckets[1].user.iam_role_name, - users3buckets[1].s3bucket.arn, - ) - # TODO get policy from call and assert bucket ARN not contained + revoke_bucket_access.assert_called() response = client.get(reverse("users3bucket-detail", (users3buckets[1].id,))) assert response.status_code == status.HTTP_404_NOT_FOUND diff --git a/tests/frontend/views/test_datasource.py b/tests/frontend/views/test_datasource.py index 4851663a5..1f43f02a0 100644 --- a/tests/frontend/views/test_datasource.py +++ b/tests/frontend/views/test_datasource.py @@ -274,7 +274,11 @@ def test_list_other_datasources_admins(client, buckets, users): assert other_datasources_admins[buckets["other"].id] == [] -def test_bucket_creator_has_readwrite_and_admin_access(client, users): +@patch("controlpanel.api.models.users3bucket.tasks.S3BucketGrantToUser") +@patch("controlpanel.api.models.s3bucket.tasks.S3BucketCreate") +def test_bucket_creator_has_readwrite_and_admin_access( + create_bucket_task, grant_user_task, client, users +): user = users["normal_user"] client.force_login(user) create(client) @@ -282,6 +286,8 @@ def test_bucket_creator_has_readwrite_and_admin_access(client, users): ub = user.users3buckets.all()[0] assert ub.access_level == UserS3Bucket.READWRITE assert ub.is_admin + create_bucket_task.assert_called_once() + grant_user_task.assert_called_once() @pytest.mark.parametrize( @@ -303,31 +309,44 @@ def test_create_get_form_class(rf, folders_enabled, datasource_type, form_class) assert view.get_form_class() == form_class +@patch("controlpanel.api.models.users3bucket.tasks.S3BucketGrantToUser") +@patch("controlpanel.api.models.s3bucket.tasks.S3BucketCreate") @patch("django.conf.settings.features.s3_folders.enabled", True) -def test_create_folders(client, users, root_folder_bucket): +@pytest.mark.parametrize("user", ["superuser", "normal_user", "other_user"]) +def test_create_folders( + create_bucket_task, grant_user_task, user, client, users, root_folder_bucket +): """ Check that all users can create a folder datasource """ - for _, user in users.items(): - client.force_login(user) - folder_name = f"test-{user.username}-folder" - response = create(client, name=folder_name) - - # redirect expected on success - assert response.status_code == 302 - assert user.users3buckets.filter( - s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" - ).exists() - - # create another folder to catch any errors updating IAM policy - folder_name = f"test-{user.username}-folder-2" - response = create(client, name=folder_name) - - # redirect expected on success - assert response.status_code == 302 - assert user.users3buckets.filter( - s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" - ).exists() + user = users[user] + client.force_login(user) + folder_name = f"test-{user.username}-folder" + response = create(client, name=folder_name) + + # redirect expected on success + assert response.status_code == 302 + assert user.users3buckets.filter( + s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" + ).exists() + # make sure tasks are sent + create_bucket_task.assert_called_once() + grant_user_task.assert_called_once() + + # create another folder to catch any errors updating IAM policy + create_bucket_task.reset_mock() + grant_user_task.reset_mock() + folder_name = f"test-{user.username}-folder-2" + response = create(client, name=folder_name) + + # redirect expected on success + assert response.status_code == 302 + assert user.users3buckets.filter( + s3bucket__name=f"{root_folder_bucket.name}/{folder_name}" + ).exists() + # tasks to create are sent again for the second folder + create_bucket_task.assert_called_once() + grant_user_task.assert_called_once() @patch("django.conf.settings.features.s3_folders.enabled", False)