From aab6dff48d93a810994129896a539695a6ff6086 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 30 Aug 2023 15:08:59 +0100
Subject: [PATCH 01/21] ANPL-1704 fix bug where task to create bucket is sent
 before bucket created

---
 controlpanel/api/models/s3bucket.py       | 27 +++++++++++++----------
 controlpanel/frontend/views/datasource.py |  4 +++-
 2 files changed, 18 insertions(+), 13 deletions(-)

diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py
index 36efdfe91..56771cd6b 100644
--- a/controlpanel/api/models/s3bucket.py
+++ b/controlpanel/api/models/s3bucket.py
@@ -67,6 +67,7 @@ class Meta:
     def __init__(self, *args, **kwargs):
         """Overwrite this constructor to pass some non-field parameter"""
         self.bucket_owner = kwargs.pop("bucket_owner", cluster.AWSRoleCategory.user)
+        self.send_task = kwargs.pop("send_task", True)
         super().__init__(*args, **kwargs)
 
     def __repr__(self):
@@ -132,21 +133,23 @@ def save(self, *args, **kwargs):
 
         super().save(*args, **kwargs)
 
-        if is_create:
-            bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)
+        if not is_create:
+            return self
 
-            # self.cluster.create(bucket_owner)
+        bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)
+
+        if self.send_task:
             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,
-                )
+        # 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,
+            )
 
         return self
 
diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py
index e6ef13dfb..181c43426 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,
@@ -179,11 +179,13 @@ def form_valid(self, form):
                     name=name,
                     created_by=self.request.user,
                     is_data_warehouse=datasource_type == "warehouse",
+                    send_task=False
                 )
                 messages.success(
                     self.request,
                     f"Successfully created {name} {datasource_type} data source",
                 )
+                transaction.on_commit(tasks.S3BucketCreate(self.object, self.request.user).create_task)
         except Exception as ex:
             form.add_error("name", str(ex))
             return FormMixin.form_invalid(self, form)

From b52237eee076b963da28d02c2ee5ea0a22715ead Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 30 Aug 2023 16:26:37 +0100
Subject: [PATCH 02/21] ANPL-1704 use send_task flag on UserS3Bucket

---
 controlpanel/api/models/s3bucket.py       |  1 +
 controlpanel/api/models/users3bucket.py   | 17 +++++------------
 controlpanel/api/tasks/handlers/s3.py     | 19 ++++++++++++++-----
 controlpanel/frontend/views/datasource.py |  7 ++++++-
 4 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py
index 56771cd6b..56c570995 100644
--- a/controlpanel/api/models/s3bucket.py
+++ b/controlpanel/api/models/s3bucket.py
@@ -149,6 +149,7 @@ def save(self, *args, **kwargs):
                 s3bucket=self,
                 is_admin=True,
                 access_level=UserS3Bucket.READWRITE,
+                send_task=self.send_task
             )
 
         return self
diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py
index 4c5b87c44..e6c26a1b1 100644
--- a/controlpanel/api/models/users3bucket.py
+++ b/controlpanel/api/models/users3bucket.py
@@ -33,6 +33,7 @@ class Meta:
     def __init__(self, *args, **kwargs):
         """Overwrite this constructor to pass some non-field parameter"""
         self.current_user = kwargs.pop("current_user", None)
+        self.send_task = kwargs.pop("send_task", True)
         super().__init__(*args, **kwargs)
 
     @property
@@ -46,18 +47,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,
-        # )
+        if self.send_task:
+            print("SENDING TASK IMMEDIATELY")
+            return tasks.S3BucketGrantToUser(self, self.current_user).create_task()
+        print("NOT SENDING TASK - MUST BE SENT ELSEWHERE")
 
     def revoke_bucket_access(self):
         if self.s3bucket.is_folder:
diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index 87928514a..7f8595f42 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -33,9 +33,18 @@ class GrantUserS3BucketAccess(BaseModelTaskHandler):
     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,
-        )
+        if user_bucket.s3bucket.is_folder:
+            print(f"GRANTING {user_bucket.access_level} ACCESS TO A FOLDER FOR {user}")
+            cluster.User(user_bucket.user).grant_folder_access(
+                root_folder_path=user_bucket.s3bucket.name,
+                access_level=user_bucket.access_level,
+                paths=user_bucket.paths,
+            )
+        else:
+            print(f"GRANTING {user_bucket.access_level} ACCESS TO A BUCKET FOR {user}")
+            cluster.User(user_bucket.user).grant_bucket_access(
+                user_bucket.s3bucket.arn,
+                user_bucket.access_level,
+                user_bucket.resources,
+            )
         self.complete()
diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py
index 181c43426..1d4eb74d0 100644
--- a/controlpanel/frontend/views/datasource.py
+++ b/controlpanel/frontend/views/datasource.py
@@ -185,12 +185,17 @@ def form_valid(self, form):
                     self.request,
                     f"Successfully created {name} {datasource_type} data source",
                 )
-                transaction.on_commit(tasks.S3BucketCreate(self.object, self.request.user).create_task)
+                transaction.on_commit(self.create_tasks)
         except Exception as ex:
             form.add_error("name", str(ex))
             return FormMixin.form_invalid(self, form)
         return FormMixin.form_valid(self, form)
 
+    def create_tasks(self):
+        tasks.S3BucketCreate(self.object, self.request.user).create_task()
+        user_bucket = UserS3Bucket.objects.get(s3bucket=self.object, user=self.request.user)
+        tasks.S3BucketGrantToUser(user_bucket, self.request.user).create_task()
+
 
 class DeleteDatasource(
     OIDCLoginRequiredMixin,

From e95539b704f6c68d7228667f75d06f6597419659 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 30 Aug 2023 17:13:25 +0100
Subject: [PATCH 03/21] ANPL-1704 create the UserS3Bucket in CreateS3Bucket
 task. Removes need for transaction lock in the view

---
 controlpanel/api/models/s3bucket.py       | 20 +++-------------
 controlpanel/api/tasks/handlers/s3.py     | 16 +++++++++++++
 controlpanel/frontend/views/datasource.py | 28 ++++++++---------------
 3 files changed, 29 insertions(+), 35 deletions(-)

diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py
index 56c570995..a945030e2 100644
--- a/controlpanel/api/models/s3bucket.py
+++ b/controlpanel/api/models/s3bucket.py
@@ -133,25 +133,11 @@ def save(self, *args, **kwargs):
 
         super().save(*args, **kwargs)
 
-        if not is_create:
-            return self
-
-        bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)
-
-        if self.send_task:
+        if is_create:
+            # TODO add this to the task args
+            bucket_owner = kwargs.pop("bucket_owner", self.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,
-                send_task=self.send_task
-            )
-
         return self
 
     @atomic
diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index 7f8595f42..c58b0aeea 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -10,6 +10,22 @@ class CreateS3Bucket(BaseModelTaskHandler):
 
     def run_task(self, bucket, user, bucket_owner="APP"):
         bucket.cluster.create(owner=bucket_owner)
+
+        # create access to the bucket for the user
+        if bucket.created_by:
+            try:
+                UserS3Bucket.objects.get(
+                    user=bucket.created_by,
+                    s3bucket=bucket,
+                )
+            except UserS3Bucket.DoesNotExist:
+                UserS3Bucket.objects.create(
+                    user=bucket.created_by,
+                    s3bucket=bucket,
+                    is_admin=True,
+                    access_level=UserS3Bucket.READWRITE,
+                    current_user=user,
+                )
         self.complete()
 
 
diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py
index 1d4eb74d0..4366fe0cc 100644
--- a/controlpanel/frontend/views/datasource.py
+++ b/controlpanel/frontend/views/datasource.py
@@ -5,7 +5,6 @@
 from django.conf import settings
 from django.contrib import messages
 from django.core.exceptions import PermissionDenied
-from django.db import transaction
 from django.shortcuts import get_object_or_404
 from django.urls import reverse_lazy
 from django.views.generic.base import ContextMixin
@@ -174,28 +173,21 @@ 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",
-                    send_task=False
-                )
-                messages.success(
-                    self.request,
-                    f"Successfully created {name} {datasource_type} data source",
-                )
-                transaction.on_commit(self.create_tasks)
+            self.object = S3Bucket.objects.create(
+                name=name,
+                created_by=self.request.user,
+                is_data_warehouse=datasource_type == "warehouse",
+                send_task=True
+            )
+            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)
         return FormMixin.form_valid(self, form)
 
-    def create_tasks(self):
-        tasks.S3BucketCreate(self.object, self.request.user).create_task()
-        user_bucket = UserS3Bucket.objects.get(s3bucket=self.object, user=self.request.user)
-        tasks.S3BucketGrantToUser(user_bucket, self.request.user).create_task()
-
 
 class DeleteDatasource(
     OIDCLoginRequiredMixin,

From 36f6c84c155ff9fff9efd39192f70d1325247148 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 30 Aug 2023 17:22:35 +0100
Subject: [PATCH 04/21] ANPL-1704 remove send_task attr from UserS3Bucket

---
 controlpanel/api/models/users3bucket.py | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py
index e6c26a1b1..fe1b7b576 100644
--- a/controlpanel/api/models/users3bucket.py
+++ b/controlpanel/api/models/users3bucket.py
@@ -33,7 +33,6 @@ class Meta:
     def __init__(self, *args, **kwargs):
         """Overwrite this constructor to pass some non-field parameter"""
         self.current_user = kwargs.pop("current_user", None)
-        self.send_task = kwargs.pop("send_task", True)
         super().__init__(*args, **kwargs)
 
     @property
@@ -47,10 +46,7 @@ def __repr__(self):
         )
 
     def grant_bucket_access(self):
-        if self.send_task:
-            print("SENDING TASK IMMEDIATELY")
-            return tasks.S3BucketGrantToUser(self, self.current_user).create_task()
-        print("NOT SENDING TASK - MUST BE SENT ELSEWHERE")
+        tasks.S3BucketGrantToUser(self, self.current_user).create_task()
 
     def revoke_bucket_access(self):
         if self.s3bucket.is_folder:

From 79fa2d6ab0fca7120c2e961b02fa42c1b763ffd1 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 30 Aug 2023 17:22:45 +0100
Subject: [PATCH 05/21] Revert "ANPL-1704 create the UserS3Bucket in
 CreateS3Bucket task. Removes need for transaction lock in the view"

This reverts commit a8564b858ec0d145c71ff2d81ea884ad6c0cccce.
---
 controlpanel/api/models/s3bucket.py       | 20 +++++++++++++---
 controlpanel/api/tasks/handlers/s3.py     | 16 -------------
 controlpanel/frontend/views/datasource.py | 28 +++++++++++++++--------
 3 files changed, 35 insertions(+), 29 deletions(-)

diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py
index a945030e2..56c570995 100644
--- a/controlpanel/api/models/s3bucket.py
+++ b/controlpanel/api/models/s3bucket.py
@@ -133,11 +133,25 @@ def save(self, *args, **kwargs):
 
         super().save(*args, **kwargs)
 
-        if is_create:
-            # TODO add this to the task args
-            bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)
+        if not is_create:
+            return self
+
+        bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)
+
+        if self.send_task:
             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,
+                send_task=self.send_task
+            )
+
         return self
 
     @atomic
diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index c58b0aeea..7f8595f42 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -10,22 +10,6 @@ class CreateS3Bucket(BaseModelTaskHandler):
 
     def run_task(self, bucket, user, bucket_owner="APP"):
         bucket.cluster.create(owner=bucket_owner)
-
-        # create access to the bucket for the user
-        if bucket.created_by:
-            try:
-                UserS3Bucket.objects.get(
-                    user=bucket.created_by,
-                    s3bucket=bucket,
-                )
-            except UserS3Bucket.DoesNotExist:
-                UserS3Bucket.objects.create(
-                    user=bucket.created_by,
-                    s3bucket=bucket,
-                    is_admin=True,
-                    access_level=UserS3Bucket.READWRITE,
-                    current_user=user,
-                )
         self.complete()
 
 
diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py
index 4366fe0cc..1d4eb74d0 100644
--- a/controlpanel/frontend/views/datasource.py
+++ b/controlpanel/frontend/views/datasource.py
@@ -5,6 +5,7 @@
 from django.conf import settings
 from django.contrib import messages
 from django.core.exceptions import PermissionDenied
+from django.db import transaction
 from django.shortcuts import get_object_or_404
 from django.urls import reverse_lazy
 from django.views.generic.base import ContextMixin
@@ -173,21 +174,28 @@ def form_valid(self, form):
         datasource_type = self.request.GET.get("type")
 
         try:
-            self.object = S3Bucket.objects.create(
-                name=name,
-                created_by=self.request.user,
-                is_data_warehouse=datasource_type == "warehouse",
-                send_task=True
-            )
-            messages.success(
-                self.request,
-                f"Successfully created {name} {datasource_type} data source",
-            )
+            with transaction.atomic():
+                self.object = S3Bucket.objects.create(
+                    name=name,
+                    created_by=self.request.user,
+                    is_data_warehouse=datasource_type == "warehouse",
+                    send_task=False
+                )
+                messages.success(
+                    self.request,
+                    f"Successfully created {name} {datasource_type} data source",
+                )
+                transaction.on_commit(self.create_tasks)
         except Exception as ex:
             form.add_error("name", str(ex))
             return FormMixin.form_invalid(self, form)
         return FormMixin.form_valid(self, form)
 
+    def create_tasks(self):
+        tasks.S3BucketCreate(self.object, self.request.user).create_task()
+        user_bucket = UserS3Bucket.objects.get(s3bucket=self.object, user=self.request.user)
+        tasks.S3BucketGrantToUser(user_bucket, self.request.user).create_task()
+
 
 class DeleteDatasource(
     OIDCLoginRequiredMixin,

From 6809e8c1bab246975583b0d6c92456f65940c32f Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 30 Aug 2023 17:51:37 +0100
Subject: [PATCH 06/21] ANPL-1704 revert to using transaction lock in the view,
 and sending tasks when objects committed

---
 controlpanel/api/models/s3bucket.py       |  6 ++----
 controlpanel/api/models/users3bucket.py   |  9 ++++++++-
 controlpanel/frontend/views/datasource.py | 23 ++++++++++++++++++-----
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py
index 56c570995..9553d9cfd 100644
--- a/controlpanel/api/models/s3bucket.py
+++ b/controlpanel/api/models/s3bucket.py
@@ -130,18 +130,16 @@ def access_level(self, user):
 
     def save(self, *args, **kwargs):
         is_create = not self.pk
-
         super().save(*args, **kwargs)
-
         if not is_create:
             return self
 
+        # TODO add this to the task args
         bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)
-
         if self.send_task:
             tasks.S3BucketCreate(self, self.created_by).create_task()
 
-        # XXX created_by is always set if model is saved by the API view
+        # created_by should always be set, but this is a failsafe
         if self.created_by:
             UserS3Bucket.objects.create(
                 user=self.created_by,
diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py
index fe1b7b576..80bb8928d 100644
--- a/controlpanel/api/models/users3bucket.py
+++ b/controlpanel/api/models/users3bucket.py
@@ -33,6 +33,7 @@ class Meta:
     def __init__(self, *args, **kwargs):
         """Overwrite this constructor to pass some non-field parameter"""
         self.current_user = kwargs.pop("current_user", None)
+        self.send_task = kwargs.pop("send_task", True)
         super().__init__(*args, **kwargs)
 
     @property
@@ -46,7 +47,13 @@ def __repr__(self):
         )
 
     def grant_bucket_access(self):
-        tasks.S3BucketGrantToUser(self, self.current_user).create_task()
+        """
+        If send_task is False, granting permission must be handled elsewhere. This may
+        be because we are using a database transaction and want all objects to be
+        created before sending the task to grant access.
+        """
+        if self.send_task:
+            tasks.S3BucketGrantToUser(self, self.current_user).create_task()
 
     def revoke_bucket_access(self):
         if self.s3bucket.is_folder:
diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py
index 1d4eb74d0..733dc81f9 100644
--- a/controlpanel/frontend/views/datasource.py
+++ b/controlpanel/frontend/views/datasource.py
@@ -175,6 +175,7 @@ def form_valid(self, form):
 
         try:
             with transaction.atomic():
+                # create the object but dont send task yet
                 self.object = S3Bucket.objects.create(
                     name=name,
                     created_by=self.request.user,
@@ -185,16 +186,28 @@ def form_valid(self, form):
                     self.request,
                     f"Successfully created {name} {datasource_type} data source",
                 )
-                transaction.on_commit(self.create_tasks)
+                # instead wait for all objects to be committed to the database before
+                # sending task to avoid a race condition resulting in task error
+                transaction.on_commit(self.send_create_tasks)
         except Exception as ex:
             form.add_error("name", str(ex))
             return FormMixin.form_invalid(self, form)
         return FormMixin.form_valid(self, form)
 
-    def create_tasks(self):
-        tasks.S3BucketCreate(self.object, self.request.user).create_task()
-        user_bucket = UserS3Bucket.objects.get(s3bucket=self.object, user=self.request.user)
-        tasks.S3BucketGrantToUser(user_bucket, self.request.user).create_task()
+    def send_create_tasks(self):
+        """
+        Sends tasks to create the S3 bucket and grant access to the user
+        """
+        tasks.S3BucketCreate(
+            entity=self.object,
+            user=self.request.user
+        ).create_task()
+        tasks.S3BucketGrantToUser(
+            entity=UserS3Bucket.objects.get(
+                s3bucket=self.object, user=self.request.user
+            ),
+            user=self.request.user,
+        ).create_task()
 
 
 class DeleteDatasource(

From 69d55b4423c7914e2cb3df0b13823f0a36e3714b Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 31 Aug 2023 10:07:20 +0100
Subject: [PATCH 07/21] ANPL-1704 fix test

---
 tests/api/models/test_users3bucket.py | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/tests/api/models/test_users3bucket.py b/tests/api/models/test_users3bucket.py
index 8a55d89b5..9b55c124c 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,25 +46,26 @@ 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

From d1064608c27550e4dd96578e9b4fbeed91a012b9 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 31 Aug 2023 13:23:18 +0100
Subject: [PATCH 08/21] ANPL-1704 pass bucket owner to CreateS3Bucket task

---
 controlpanel/api/models/s3bucket.py       | 13 ++++++++-----
 controlpanel/api/tasks/handlers/s3.py     |  6 ++++--
 controlpanel/api/tasks/s3bucket.py        | 10 ++++++++++
 controlpanel/frontend/views/datasource.py |  5 ++++-
 tests/api/tasks/test_s3.py                | 18 +++++++++++++-----
 5 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py
index 9553d9cfd..d41c33cc4 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):
@@ -134,10 +133,14 @@ def save(self, *args, **kwargs):
         if not is_create:
             return self
 
-        # TODO add this to the task args
-        bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)
         if self.send_task:
-            tasks.S3BucketCreate(self, self.created_by).create_task()
+            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:
diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index 7f8595f42..14a83775e 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -1,5 +1,6 @@
+# First-party/Local
 from controlpanel.api import cluster
-from controlpanel.api.models import S3Bucket, UserS3Bucket, AppS3Bucket
+from controlpanel.api.models import AppS3Bucket, S3Bucket, UserS3Bucket
 from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler
 
 
@@ -8,7 +9,8 @@ class CreateS3Bucket(BaseModelTaskHandler):
     name = "create_s3bucket"
     permission_required = "api.create_s3bucket"
 
-    def run_task(self, bucket, user, bucket_owner="APP"):
+    def run_task(self, bucket, user, bucket_owner=None):
+        bucket_owner = bucket_owner or "USER"
         bucket.cluster.create(owner=bucket_owner)
         self.complete()
 
diff --git a/controlpanel/api/tasks/s3bucket.py b/controlpanel/api/tasks/s3bucket.py
index 21b3a2286..2380a6978 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,6 +17,13 @@ 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"
diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py
index 733dc81f9..5f605a380 100644
--- a/controlpanel/frontend/views/datasource.py
+++ b/controlpanel/frontend/views/datasource.py
@@ -200,7 +200,10 @@ def send_create_tasks(self):
         """
         tasks.S3BucketCreate(
             entity=self.object,
-            user=self.request.user
+            user=self.request.user,
+            extra_data={
+                "bucket_owner": self.object.bucket_owner,
+            }
         ).create_task()
         tasks.S3BucketGrantToUser(
             entity=UserS3Bucket.objects.get(
diff --git a/tests/api/tasks/test_s3.py b/tests/api/tasks/test_s3.py
index babd83877..f7dc16432 100644
--- a/tests/api/tasks/test_s3.py
+++ b/tests/api/tasks/test_s3.py
@@ -1,11 +1,15 @@
+# Third-party
 import pytest
 from mock import patch
 from model_mommy import mommy
 
+# First-party/Local
 from controlpanel.api import cluster
-from controlpanel.api.models import S3Bucket, AppS3Bucket, UserS3Bucket
+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,
 )
 
 
@@ -29,12 +33,16 @@ def test_exception_raised_when_called_without_valid_app(
 @patch("controlpanel.api.tasks.handlers.base.BaseModelTaskHandler.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()
 
 

From 94f3871874b982d8b38d7cfbac3a2a7b201180cf Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 31 Aug 2023 15:18:08 +0100
Subject: [PATCH 09/21] ANPL-1704 remove debug prints

---
 controlpanel/api/tasks/handlers/s3.py | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index 14a83775e..a1205a409 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -36,17 +36,15 @@ class GrantUserS3BucketAccess(BaseModelTaskHandler):
 
     def run_task(self, user_bucket, user):
         if user_bucket.s3bucket.is_folder:
-            print(f"GRANTING {user_bucket.access_level} ACCESS TO A FOLDER FOR {user}")
             cluster.User(user_bucket.user).grant_folder_access(
                 root_folder_path=user_bucket.s3bucket.name,
                 access_level=user_bucket.access_level,
                 paths=user_bucket.paths,
             )
         else:
-            print(f"GRANTING {user_bucket.access_level} ACCESS TO A BUCKET FOR {user}")
             cluster.User(user_bucket.user).grant_bucket_access(
-                user_bucket.s3bucket.arn,
-                user_bucket.access_level,
-                user_bucket.resources,
+                bucket_arn=user_bucket.s3bucket.arn,
+                access_level=user_bucket.access_level,
+                path_arns=user_bucket.resources,
             )
         self.complete()

From eed4c2c750b8af86cfab18a0868f8a6a23cd7dc5 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 31 Aug 2023 16:37:17 +0100
Subject: [PATCH 10/21] ANPL-1704 refactor task handlers

---
 controlpanel/api/tasks/handlers/__init__.py |  9 +++---
 controlpanel/api/tasks/handlers/app.py      |  9 +++---
 controlpanel/api/tasks/handlers/base.py     | 31 +++++++++++--------
 controlpanel/api/tasks/handlers/s3.py       | 34 ++++++++++-----------
 4 files changed, 45 insertions(+), 38 deletions(-)

diff --git a/controlpanel/api/tasks/handlers/__init__.py b/controlpanel/api/tasks/handlers/__init__.py
index 14190b744..7300282ae 100644
--- a/controlpanel/api/tasks/handlers/__init__.py
+++ b/controlpanel/api/tasks/handlers/__init__.py
@@ -1,10 +1,11 @@
+# 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,
 )
-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())
diff --git a/controlpanel/api/tasks/handlers/app.py b/controlpanel/api/tasks/handlers/app.py
index 8b302954e..22592f669 100644
--- a/controlpanel/api/tasks/handlers/app.py
+++ b/controlpanel/api/tasks/handlers/app.py
@@ -1,3 +1,4 @@
+# First-party/Local
 from controlpanel.api import cluster
 from controlpanel.api.models import App
 from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler
@@ -14,9 +15,9 @@ def has_permission(self, user, obj=None):
 
         return user.github_api_token
 
-    def run_task(self, app, user, envs, disable_authentication, connections):
+    def handle(self, envs, disable_authentication, connections):
         for env in envs:
-            cluster.App(app, user.github_api_token).create_auth_settings(
+            cluster.App(self.object, self.user.github_api_token).create_auth_settings(
                 env_name=env,
                 disable_authentication=disable_authentication,
                 connections=connections,
@@ -29,6 +30,6 @@ class CreateAppAWSRole(BaseModelTaskHandler):
     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..cd22c7307 100644
--- a/controlpanel/api/tasks/handlers/base.py
+++ b/controlpanel/api/tasks/handlers/base.py
@@ -1,13 +1,16 @@
+# 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):
     name = None
     model = None
     permission_required = None
-
+    object = None
+    user = 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
@@ -31,7 +34,7 @@ def get_user(self, pk):
         """
         try:
             return User.objects.get(pk=pk)
-        except User.DoesNotExist as exc:
+        except User.DoesNotExist:
             # 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
@@ -58,22 +61,24 @@ def complete(self):
 
     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.
+        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
+        and store the user and instance of the model, and validate the user can run the
+        task for the instance. If these lookups and validation passes, the `handle`
+        method is called, with any other args and kwargs sent.
+        The handle method be defined on any subclass of BaseModelTaskHandler.
         """
-        obj = self.get_object(obj_pk)
-        user = self.get_user(user_pk)
-        if not user:
+        self.user = self.get_user(user_pk)
+        if not self.user:
             return self.complete()
 
-        if not self.has_permission(user, obj):
+        self.object = self.get_object(obj_pk)
+        if not self.has_permission(self.user, self.object):
             return self.complete()
 
-        self.run_task(obj, user, *args, **kwargs)
+        self.handle(*args, **kwargs)
 
-    def run_task(self, *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.
diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index a1205a409..8dec3360c 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -9,9 +9,9 @@ class CreateS3Bucket(BaseModelTaskHandler):
     name = "create_s3bucket"
     permission_required = "api.create_s3bucket"
 
-    def run_task(self, bucket, user, bucket_owner=None):
+    def handle(self, bucket_owner=None):
         bucket_owner = bucket_owner or "USER"
-        bucket.cluster.create(owner=bucket_owner)
+        self.object.cluster.create(owner=bucket_owner)
         self.complete()
 
 
@@ -20,11 +20,11 @@ class GrantAppS3BucketAccess(BaseModelTaskHandler):
     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()
 
@@ -34,17 +34,17 @@ class GrantUserS3BucketAccess(BaseModelTaskHandler):
     name = "grant_user_s3bucket_access"
     permission_required = "api.create_users3bucket"
 
-    def run_task(self, user_bucket, user):
-        if user_bucket.s3bucket.is_folder:
-            cluster.User(user_bucket.user).grant_folder_access(
-                root_folder_path=user_bucket.s3bucket.name,
-                access_level=user_bucket.access_level,
-                paths=user_bucket.paths,
+    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(user_bucket.user).grant_bucket_access(
-                bucket_arn=user_bucket.s3bucket.arn,
-                access_level=user_bucket.access_level,
-                path_arns=user_bucket.resources,
+            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()

From b513d877e6d33de6b538a8f8836958818c068d7b Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 31 Aug 2023 16:40:07 +0100
Subject: [PATCH 11/21] ANPL-1704 update help text for migrating_customers
 command

---
 controlpanel/cli/management/commands/migrating_customers.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

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"]
 

From a1598ff4766c8990eb992542a7e8ed0ec4dd6eb6 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Fri, 1 Sep 2023 09:42:22 +0100
Subject: [PATCH 12/21] WIP add comments about github error

---
 controlpanel/api/serializers.py    | 4 +++-
 controlpanel/frontend/views/app.py | 9 ++++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

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/frontend/views/app.py b/controlpanel/frontend/views/app.py
index c8a271fd9..015864244 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()

From c6bd7d1cb72281d7b438093dc47a6a588835493b Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Mon, 4 Sep 2023 13:03:18 +0100
Subject: [PATCH 13/21] ANPL-1704 add task to revoke user access

---
 controlpanel/api/models/users3bucket.py     |  9 +----
 controlpanel/api/tasks/__init__.py          |  6 ++-
 controlpanel/api/tasks/app.py               |  3 ++
 controlpanel/api/tasks/handlers/__init__.py |  2 +
 controlpanel/api/tasks/handlers/base.py     | 38 +++++++++--------
 controlpanel/api/tasks/handlers/s3.py       | 21 ++++++++--
 controlpanel/api/tasks/s3bucket.py          | 45 ++++++++++++++-------
 controlpanel/api/tasks/task_base.py         |  7 +++-
 8 files changed, 88 insertions(+), 43 deletions(-)

diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py
index 80bb8928d..78ee79fe8 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):
@@ -56,8 +55,4 @@ def grant_bucket_access(self):
             tasks.S3BucketGrantToUser(self, self.current_user).create_task()
 
     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)
+        tasks.S3BucketRevokeUserAccess(self).create_task()
diff --git a/controlpanel/api/tasks/__init__.py b/controlpanel/api/tasks/__init__.py
index dd777c893..efb5bc95c 100644
--- a/controlpanel/api/tasks/__init__.py
+++ b/controlpanel/api/tasks/__init__.py
@@ -1,7 +1,9 @@
 
-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,
+    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 7300282ae..6eeeada39 100644
--- a/controlpanel/api/tasks/handlers/__init__.py
+++ b/controlpanel/api/tasks/handlers/__init__.py
@@ -5,6 +5,7 @@
     CreateS3Bucket,
     GrantAppS3BucketAccess,
     GrantUserS3BucketAccess,
+    S3BucketRevokeUserAccess,
 )
 
 create_app_aws_role = celery_app.register_task(CreateAppAWSRole())
@@ -12,3 +13,4 @@
 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())
diff --git a/controlpanel/api/tasks/handlers/base.py b/controlpanel/api/tasks/handlers/base.py
index cd22c7307..a57ca6676 100644
--- a/controlpanel/api/tasks/handlers/base.py
+++ b/controlpanel/api/tasks/handlers/base.py
@@ -5,7 +5,26 @@
 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
@@ -53,12 +72,6 @@ def has_permission(self, user, obj=None):
 
         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 method that a celery Task object requires to be defined, and will be
@@ -69,18 +82,11 @@ def run(self, obj_pk, user_pk, *args, **kwargs):
         The handle method be defined on any subclass of BaseModelTaskHandler.
         """
         self.user = self.get_user(user_pk)
-        if not self.user:
+        if user_pk and not self.user:
             return self.complete()
 
         self.object = self.get_object(obj_pk)
-        if not self.has_permission(self.user, self.object):
+        if self.user and not self.has_permission(self.user, self.object):
             return self.complete()
 
         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")
diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index 8dec3360c..2ffcf8eb5 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -1,7 +1,10 @@
+# Third-party
+from celery import Task as CeleryTask
+
 # First-party/Local
 from controlpanel.api import cluster
-from controlpanel.api.models import AppS3Bucket, S3Bucket, UserS3Bucket
-from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler
+from controlpanel.api.models import AppS3Bucket, S3Bucket, User, UserS3Bucket
+from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler, BaseTaskHandler
 
 
 class CreateS3Bucket(BaseModelTaskHandler):
@@ -32,7 +35,7 @@ def handle(self):
 class GrantUserS3BucketAccess(BaseModelTaskHandler):
     model = UserS3Bucket
     name = "grant_user_s3bucket_access"
-    permission_required = "api.create_users3bucket"
+    permission_required = "api.update_users3bucket"
 
     def handle(self):
         if self.object.s3bucket.is_folder:
@@ -48,3 +51,15 @@ def handle(self):
                 path_arns=self.object.resources,
             )
         self.complete()
+
+
+class S3BucketRevokeUserAccess(BaseTaskHandler):
+    name = "revoke_user_s3bucket_access"
+
+    def run(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()
diff --git a/controlpanel/api/tasks/s3bucket.py b/controlpanel/api/tasks/s3bucket.py
index 2380a6978..bde1cdf3f 100644
--- a/controlpanel/api/tasks/s3bucket.py
+++ b/controlpanel/api/tasks/s3bucket.py
@@ -25,33 +25,50 @@ def _get_args_list(self):
         ]
 
 
-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,
+        ]
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
 

From 0071a4c4e51268da4cab7586c1580c45e2fa1989 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Mon, 4 Sep 2023 14:40:51 +0100
Subject: [PATCH 14/21] ANPL-1704 add task to revoke bucket access, not used
 yet

---
 controlpanel/api/models/apps3bucket.py      |  2 ++
 controlpanel/api/tasks/__init__.py          |  1 +
 controlpanel/api/tasks/handlers/__init__.py |  2 ++
 controlpanel/api/tasks/handlers/s3.py       | 15 ++++++++++++++-
 controlpanel/api/tasks/s3bucket.py          | 12 ++++++++++++
 5 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/controlpanel/api/models/apps3bucket.py b/controlpanel/api/models/apps3bucket.py
index 9d12b6582..42edfa2cd 100644
--- a/controlpanel/api/models/apps3bucket.py
+++ b/controlpanel/api/models/apps3bucket.py
@@ -50,4 +50,6 @@ def grant_bucket_access(self):
         # )
 
     def revoke_bucket_access(self):
+        # TODO enable using the task when ready
+        # tasks.S3BucketRevokeAppAccess(self).create_task()
         cluster.App(self.app).revoke_bucket_access(self.s3bucket.arn)
diff --git a/controlpanel/api/tasks/__init__.py b/controlpanel/api/tasks/__init__.py
index efb5bc95c..19e653ff1 100644
--- a/controlpanel/api/tasks/__init__.py
+++ b/controlpanel/api/tasks/__init__.py
@@ -5,5 +5,6 @@
     S3BucketCreate,
     S3BucketGrantToApp,
     S3BucketGrantToUser,
+    S3BucketRevokeAppAccess,
     S3BucketRevokeUserAccess,
 )
diff --git a/controlpanel/api/tasks/handlers/__init__.py b/controlpanel/api/tasks/handlers/__init__.py
index 6eeeada39..454fd9094 100644
--- a/controlpanel/api/tasks/handlers/__init__.py
+++ b/controlpanel/api/tasks/handlers/__init__.py
@@ -5,6 +5,7 @@
     CreateS3Bucket,
     GrantAppS3BucketAccess,
     GrantUserS3BucketAccess,
+    S3BucketRevokeAppAccess,
     S3BucketRevokeUserAccess,
 )
 
@@ -14,3 +15,4 @@
 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/s3.py b/controlpanel/api/tasks/handlers/s3.py
index 2ffcf8eb5..a54687a54 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -3,7 +3,7 @@
 
 # First-party/Local
 from controlpanel.api import cluster
-from controlpanel.api.models import AppS3Bucket, S3Bucket, User, UserS3Bucket
+from controlpanel.api.models import App, AppS3Bucket, S3Bucket, User, UserS3Bucket
 from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler, BaseTaskHandler
 
 
@@ -63,3 +63,16 @@ def run(self, bucket_identifier, bucket_user_pk, is_folder):
         else:
             cluster.User(bucket_user).revoke_bucket_access(bucket_identifier)
         self.complete()
+
+
+class S3BucketRevokeAppAccess(BaseTaskHandler):
+    name = "revoke_app_s3bucket_access"
+
+    def run(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 bde1cdf3f..c59f4070d 100644
--- a/controlpanel/api/tasks/s3bucket.py
+++ b/controlpanel/api/tasks/s3bucket.py
@@ -72,3 +72,15 @@ def _get_args_list(self):
             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,
+        ]

From 432e6cf4557d4b2c10f1cb8c3852d1a92e6c25aa Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Mon, 4 Sep 2023 15:50:07 +0100
Subject: [PATCH 15/21] Fix test since changing to tasks to revoke access

---
 tests/api/models/test_s3bucket.py     | 22 +++++++++-------------
 tests/api/models/test_users3bucket.py | 11 ++++-------
 tests/api/views/test_users3bucket.py  |  8 ++------
 3 files changed, 15 insertions(+), 26 deletions(-)

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 9b55c124c..364e89ee8 100644
--- a/tests/api/models/test_users3bucket.py
+++ b/tests/api/models/test_users3bucket.py
@@ -71,11 +71,8 @@ def test_aws_create_folder(tasks, user, bucket):
 @pytest.mark.django_db
 def test_delete_revoke_permissions(user, 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)
+        revoke_user_access_task.return_value.create_task.assert_called()
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

From 6870167610737d8aa64919729e1615d9023eed6e Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Tue, 12 Sep 2023 16:17:24 +0100
Subject: [PATCH 16/21] ANPL-1704 enable revoke app access via a task

---
 controlpanel/api/models/apps3bucket.py |  8 +++-----
 tests/api/models/test_apps3bucket.py   | 14 ++++++++------
 tests/api/views/test_apps3bucket.py    | 16 ++++++++--------
 3 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/controlpanel/api/models/apps3bucket.py b/controlpanel/api/models/apps3bucket.py
index 42edfa2cd..824a74c00 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):
@@ -50,6 +49,5 @@ def grant_bucket_access(self):
         # )
 
     def revoke_bucket_access(self):
-        # TODO enable using the task when ready
-        # tasks.S3BucketRevokeAppAccess(self).create_task()
-        cluster.App(self.app).revoke_bucket_access(self.s3bucket.arn)
+        tasks.S3BucketRevokeAppAccess(self).create_task()
+        # cluster.App(self.app).revoke_bucket_access(self.s3bucket.arn)
diff --git a/tests/api/models/test_apps3bucket.py b/tests/api/models/test_apps3bucket.py
index a0e0c75ec..acfdc1b1c 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:
+            patch("controlpanel.api.tasks.S3BucketRevokeAppAccess") \
+            as revoke_bucket_access_task:
         apps3bucket = mommy.make(
             "api.AppS3Bucket",
             app=app,
@@ -63,7 +65,7 @@ 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
         )
+        revoke_bucket_access_task.return_value.create_task.assert_called()
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):

From c43582dc60880e46e674488968f5d6821eb703d3 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Tue, 12 Sep 2023 16:36:03 +0100
Subject: [PATCH 17/21] ANPL-1704 use handle method on all tasks

---
 controlpanel/api/tasks/handlers/s3.py | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/controlpanel/api/tasks/handlers/s3.py b/controlpanel/api/tasks/handlers/s3.py
index a54687a54..2c30d0dcf 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -56,7 +56,7 @@ def handle(self):
 class S3BucketRevokeUserAccess(BaseTaskHandler):
     name = "revoke_user_s3bucket_access"
 
-    def run(self, bucket_identifier, bucket_user_pk, is_folder):
+    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)
@@ -68,7 +68,7 @@ def run(self, bucket_identifier, bucket_user_pk, is_folder):
 class S3BucketRevokeAppAccess(BaseTaskHandler):
     name = "revoke_app_s3bucket_access"
 
-    def run(self, bucket_arn, app_pk):
+    def handle(self, bucket_arn, app_pk):
         try:
             app = App.objects.get(pk=app_pk)
         except App.DoesNotExist:

From 6a310a0ced06c245e6e5d740e53a29ab383cbab3 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Tue, 12 Sep 2023 17:06:00 +0100
Subject: [PATCH 18/21] ANPL-1704 add tests for new tasks to revoke access

---
 tests/api/tasks/test_s3.py | 56 +++++++++++++++++++++++++++++++++++---
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/tests/api/tasks/test_s3.py b/tests/api/tasks/test_s3.py
index f7dc16432..dc275accf 100644
--- a/tests/api/tasks/test_s3.py
+++ b/tests/api/tasks/test_s3.py
@@ -4,12 +4,13 @@
 from model_mommy import mommy
 
 # First-party/Local
-from controlpanel.api import cluster
 from controlpanel.api.models import AppS3Bucket, S3Bucket, UserS3Bucket
 from controlpanel.api.tasks.handlers import (
     create_s3bucket,
     grant_app_s3bucket_access,
     grant_user_s3bucket_access,
+    revoke_app_s3bucket_access,
+    revoke_user_s3bucket_access,
 )
 
 
@@ -19,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
 ):
@@ -30,7 +31,7 @@ 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", bucket_owner="APP")
@@ -51,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
@@ -64,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()

From 928238fb6e40a90ea1d8cadb5818aa2d23992ea1 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 13 Sep 2023 16:59:35 +0100
Subject: [PATCH 19/21] NPL-1704 remove send_task flag to simplify the flow.
 Update tests to check tasks are sent when creating datasources

---
 controlpanel/api/models/s3bucket.py       | 17 +++---
 controlpanel/api/models/users3bucket.py   |  9 +---
 controlpanel/frontend/views/datasource.py | 42 ++++-----------
 tests/frontend/views/test_datasource.py   | 63 +++++++++++++++--------
 4 files changed, 58 insertions(+), 73 deletions(-)

diff --git a/controlpanel/api/models/s3bucket.py b/controlpanel/api/models/s3bucket.py
index d41c33cc4..90de336f3 100644
--- a/controlpanel/api/models/s3bucket.py
+++ b/controlpanel/api/models/s3bucket.py
@@ -66,7 +66,6 @@ class Meta:
     def __init__(self, *args, **kwargs):
         """Overwrite this constructor to pass some non-field parameter"""
         self.bucket_owner = kwargs.pop("bucket_owner", cluster.AWSRoleCategory.user)
-        self.send_task = kwargs.pop("send_task", True)
         super().__init__(*args, **kwargs)
 
     def __repr__(self):
@@ -133,14 +132,13 @@ def save(self, *args, **kwargs):
         if not is_create:
             return self
 
-        if self.send_task:
-            tasks.S3BucketCreate(
-                entity=self,
-                user=self.created_by,
-                extra_data={
-                    "bucket_owner": kwargs.pop("bucket_owner", self.bucket_owner),
-                }
-            ).create_task()
+        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:
@@ -150,7 +148,6 @@ def save(self, *args, **kwargs):
                 s3bucket=self,
                 is_admin=True,
                 access_level=UserS3Bucket.READWRITE,
-                send_task=self.send_task
             )
 
         return self
diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py
index 78ee79fe8..2832058d8 100644
--- a/controlpanel/api/models/users3bucket.py
+++ b/controlpanel/api/models/users3bucket.py
@@ -32,7 +32,6 @@ class Meta:
     def __init__(self, *args, **kwargs):
         """Overwrite this constructor to pass some non-field parameter"""
         self.current_user = kwargs.pop("current_user", None)
-        self.send_task = kwargs.pop("send_task", True)
         super().__init__(*args, **kwargs)
 
     @property
@@ -46,13 +45,7 @@ def __repr__(self):
         )
 
     def grant_bucket_access(self):
-        """
-        If send_task is False, granting permission must be handled elsewhere. This may
-        be because we are using a database transaction and want all objects to be
-        created before sending the task to grant access.
-        """
-        if self.send_task:
-            tasks.S3BucketGrantToUser(self, self.current_user).create_task()
+        tasks.S3BucketGrantToUser(self, self.current_user).create_task()
 
     def revoke_bucket_access(self):
         tasks.S3BucketRevokeUserAccess(self).create_task()
diff --git a/controlpanel/frontend/views/datasource.py b/controlpanel/frontend/views/datasource.py
index 5f605a380..77e552cf0 100644
--- a/controlpanel/frontend/views/datasource.py
+++ b/controlpanel/frontend/views/datasource.py
@@ -174,44 +174,20 @@ def form_valid(self, form):
         datasource_type = self.request.GET.get("type")
 
         try:
-            with transaction.atomic():
-                # create the object but dont send task yet
-                self.object = S3Bucket.objects.create(
-                    name=name,
-                    created_by=self.request.user,
-                    is_data_warehouse=datasource_type == "warehouse",
-                    send_task=False
-                )
-                messages.success(
-                    self.request,
-                    f"Successfully created {name} {datasource_type} data source",
-                )
-                # instead wait for all objects to be committed to the database before
-                # sending task to avoid a race condition resulting in task error
-                transaction.on_commit(self.send_create_tasks)
+            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)
         return FormMixin.form_valid(self, form)
 
-    def send_create_tasks(self):
-        """
-        Sends tasks to create the S3 bucket and grant access to the user
-        """
-        tasks.S3BucketCreate(
-            entity=self.object,
-            user=self.request.user,
-            extra_data={
-                "bucket_owner": self.object.bucket_owner,
-            }
-        ).create_task()
-        tasks.S3BucketGrantToUser(
-            entity=UserS3Bucket.objects.get(
-                s3bucket=self.object, user=self.request.user
-            ),
-            user=self.request.user,
-        ).create_task()
-
 
 class DeleteDatasource(
     OIDCLoginRequiredMixin,
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)

From 901a2b8da6420821248077f936ead79aacc6f448 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Wed, 13 Sep 2023 17:31:18 +0100
Subject: [PATCH 20/21] ANPL-1704 Pass the current_user when revoking access to
 a datasource

---
 controlpanel/api/models/apps3bucket.py    | 8 +-------
 controlpanel/api/models/users3bucket.py   | 5 ++++-
 controlpanel/frontend/views/app.py        | 5 +++++
 controlpanel/frontend/views/datasource.py | 5 +++++
 tests/api/models/test_apps3bucket.py      | 9 +++++----
 tests/api/models/test_users3bucket.py     | 4 ++--
 6 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/controlpanel/api/models/apps3bucket.py b/controlpanel/api/models/apps3bucket.py
index 824a74c00..9cd5d44d4 100644
--- a/controlpanel/api/models/apps3bucket.py
+++ b/controlpanel/api/models/apps3bucket.py
@@ -42,12 +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):
-        tasks.S3BucketRevokeAppAccess(self).create_task()
-        # cluster.App(self.app).revoke_bucket_access(self.s3bucket.arn)
+        tasks.S3BucketRevokeAppAccess(self, self.current_user).create_task()
diff --git a/controlpanel/api/models/users3bucket.py b/controlpanel/api/models/users3bucket.py
index 2832058d8..7e5cfbec3 100644
--- a/controlpanel/api/models/users3bucket.py
+++ b/controlpanel/api/models/users3bucket.py
@@ -48,4 +48,7 @@ def grant_bucket_access(self):
         tasks.S3BucketGrantToUser(self, self.current_user).create_task()
 
     def revoke_bucket_access(self):
-        tasks.S3BucketRevokeUserAccess(self).create_task()
+        # 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/frontend/views/app.py b/controlpanel/frontend/views/app.py
index 015864244..d009bcd74 100644
--- a/controlpanel/frontend/views/app.py
+++ b/controlpanel/frontend/views/app.py
@@ -312,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 77e552cf0..5099089ea 100644
--- a/controlpanel/frontend/views/datasource.py
+++ b/controlpanel/frontend/views/datasource.py
@@ -293,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 acfdc1b1c..6402c2a47 100644
--- a/tests/api/models/test_apps3bucket.py
+++ b/tests/api/models/test_apps3bucket.py
@@ -53,9 +53,9 @@ def test_aws_permissions(app, bucket, sqs, helpers):
 
 @pytest.mark.django_db
 def test_delete_revoke_permissions(app, bucket):
-    with patch("controlpanel.api.aws.AWSRole.grant_bucket_access"), \
-            patch("controlpanel.api.tasks.S3BucketRevokeAppAccess") \
-            as revoke_bucket_access_task:
+    with patch(
+        "controlpanel.api.tasks.S3BucketRevokeAppAccess"
+    ) as revoke_bucket_access_task:
         apps3bucket = mommy.make(
             "api.AppS3Bucket",
             app=app,
@@ -66,6 +66,7 @@ def test_delete_revoke_permissions(app, bucket):
         apps3bucket.delete()
 
         revoke_bucket_access_task.assert_called_with(
-            apps3bucket
+            apps3bucket,
+            None
         )
         revoke_bucket_access_task.return_value.create_task.assert_called()
diff --git a/tests/api/models/test_users3bucket.py b/tests/api/models/test_users3bucket.py
index 364e89ee8..7d8effcda 100644
--- a/tests/api/models/test_users3bucket.py
+++ b/tests/api/models/test_users3bucket.py
@@ -69,10 +69,10 @@ def test_aws_create_folder(tasks, user, bucket):
 
 
 @pytest.mark.django_db
-def test_delete_revoke_permissions(user, bucket, users3bucket):
+def test_delete_revoke_permissions(bucket, users3bucket):
     with patch(
         "controlpanel.api.tasks.S3BucketRevokeUserAccess"
     ) as revoke_user_access_task:
         users3bucket.delete()
-        revoke_user_access_task.assert_called_with(users3bucket)
+        revoke_user_access_task.assert_called_with(users3bucket, None)
         revoke_user_access_task.return_value.create_task.assert_called()

From 29c5ebbc9c19ea2187f901c2f71e62eb8364cc91 Mon Sep 17 00:00:00 2001
From: Michael Collins <15347726+michaeljcollinsuk@users.noreply.github.com>
Date: Thu, 14 Sep 2023 12:09:11 +0100
Subject: [PATCH 21/21] 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/tasks/handlers/app.py        | 17 +++----
 controlpanel/api/tasks/handlers/base.py       | 46 +++----------------
 controlpanel/api/tasks/handlers/s3.py         |  3 --
 .../tasks/test_create_app_auth_settings.py    | 15 ++++--
 tests/api/tasks/test_create_app_aws_role.py   | 25 ++++------
 5 files changed, 31 insertions(+), 75 deletions(-)

diff --git a/controlpanel/api/tasks/handlers/app.py b/controlpanel/api/tasks/handlers/app.py
index 22592f669..accc6d68b 100644
--- a/controlpanel/api/tasks/handlers/app.py
+++ b/controlpanel/api/tasks/handlers/app.py
@@ -1,23 +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
-
-        return user.github_api_token
 
     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()
+
         for env in envs:
-            cluster.App(self.object, self.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,
@@ -28,7 +26,6 @@ def handle(self, envs, disable_authentication, connections):
 class CreateAppAWSRole(BaseModelTaskHandler):
     model = App
     name = "create_app_aws_role"
-    permission_required = "api.create_app"
 
     def handle(self):
         cluster.App(self.object).create_iam_role()
diff --git a/controlpanel/api/tasks/handlers/base.py b/controlpanel/api/tasks/handlers/base.py
index a57ca6676..4f1455566 100644
--- a/controlpanel/api/tasks/handlers/base.py
+++ b/controlpanel/api/tasks/handlers/base.py
@@ -27,9 +27,8 @@ def handle(self, *args, **kwargs):
 class BaseModelTaskHandler(BaseTaskHandler):
     name = None
     model = None
-    permission_required = None
     object = None
-    user = 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
@@ -46,47 +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:
-            # 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):
-        """
-        Check that the user has permission to run the task on the given object.
-        Override on the subclass for further permission checks.
-        """
-        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 run(self, obj_pk, user_pk, *args, **kwargs):
+    def run(self, obj_pk, task_user_pk, *args, **kwargs):
         """
         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
-        and store the user and instance of the model, and validate the user can run the
-        task for the instance. If these lookups and validation passes, the `handle`
-        method is called, with any other args and kwargs sent.
-        The handle method be defined on any subclass of BaseModelTaskHandler.
+        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.
         """
-        self.user = self.get_user(user_pk)
-        if user_pk and not self.user:
-            return self.complete()
-
         self.object = self.get_object(obj_pk)
-        if self.user and not self.has_permission(self.user, self.object):
-            return self.complete()
-
+        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 2c30d0dcf..fd8fe922e 100644
--- a/controlpanel/api/tasks/handlers/s3.py
+++ b/controlpanel/api/tasks/handlers/s3.py
@@ -10,7 +10,6 @@
 class CreateS3Bucket(BaseModelTaskHandler):
     model = S3Bucket
     name = "create_s3bucket"
-    permission_required = "api.create_s3bucket"
 
     def handle(self, bucket_owner=None):
         bucket_owner = bucket_owner or "USER"
@@ -21,7 +20,6 @@ def handle(self, bucket_owner=None):
 class GrantAppS3BucketAccess(BaseModelTaskHandler):
     model = AppS3Bucket
     name = 'grant_app_s3bucket_access'
-    permission_required = 'api.create_apps3bucket'
 
     def handle(self):
         cluster.App(self.object.app).grant_bucket_access(
@@ -35,7 +33,6 @@ def handle(self):
 class GrantUserS3BucketAccess(BaseModelTaskHandler):
     model = UserS3Bucket
     name = "grant_user_s3bucket_access"
-    permission_required = "api.update_users3bucket"
 
     def handle(self):
         if self.object.s3bucket.is_folder:
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")