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

@@ -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()

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
 

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()

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:

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,
 )

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
 
 

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())

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()

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)