Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Anpl 1704 s3 folders use task queue #1204

Merged
merged 21 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
aab6dff
ANPL-1704 fix bug where task to create bucket is sent before bucket c…
michaeljcollinsuk Aug 30, 2023
b52237e
ANPL-1704 use send_task flag on UserS3Bucket
michaeljcollinsuk Aug 30, 2023
e95539b
ANPL-1704 create the UserS3Bucket in CreateS3Bucket task. Removes nee…
michaeljcollinsuk Aug 30, 2023
36f6c84
ANPL-1704 remove send_task attr from UserS3Bucket
michaeljcollinsuk Aug 30, 2023
79fa2d6
Revert "ANPL-1704 create the UserS3Bucket in CreateS3Bucket task. Rem…
michaeljcollinsuk Aug 30, 2023
6809e8c
ANPL-1704 revert to using transaction lock in the view, and sending t…
michaeljcollinsuk Aug 30, 2023
69d55b4
ANPL-1704 fix test
michaeljcollinsuk Aug 31, 2023
d106460
ANPL-1704 pass bucket owner to CreateS3Bucket task
michaeljcollinsuk Aug 31, 2023
94f3871
ANPL-1704 remove debug prints
michaeljcollinsuk Aug 31, 2023
eed4c2c
ANPL-1704 refactor task handlers
michaeljcollinsuk Aug 31, 2023
b513d87
ANPL-1704 update help text for migrating_customers command
michaeljcollinsuk Aug 31, 2023
a1598ff
WIP add comments about github error
michaeljcollinsuk Sep 1, 2023
c6bd7d1
ANPL-1704 add task to revoke user access
michaeljcollinsuk Sep 4, 2023
0071a4c
ANPL-1704 add task to revoke bucket access, not used yet
michaeljcollinsuk Sep 4, 2023
432e6cf
Fix test since changing to tasks to revoke access
michaeljcollinsuk Sep 4, 2023
6870167
ANPL-1704 enable revoke app access via a task
michaeljcollinsuk Sep 12, 2023
c43582d
ANPL-1704 use handle method on all tasks
michaeljcollinsuk Sep 12, 2023
6a310a0
ANPL-1704 add tests for new tasks to revoke access
michaeljcollinsuk Sep 12, 2023
928238f
NPL-1704 remove send_task flag to simplify the flow. Update tests to …
michaeljcollinsuk Sep 13, 2023
901a2b8
ANPL-1704 Pass the current_user when revoking access to a datasource
michaeljcollinsuk Sep 13, 2023
29c5ebb
ANPL-1704 Remove default permissions checks from task handlers
michaeljcollinsuk Sep 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions controlpanel/api/models/apps3bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -43,11 +42,6 @@ def __repr__(self):

def grant_bucket_access(self):
tasks.S3BucketGrantToApp(self, self.current_user).create_task()
# cluster.App(self.app).grant_bucket_access(
# self.s3bucket.arn,
# self.access_level,
# self.resources,
# )

def revoke_bucket_access(self):
cluster.App(self.app).revoke_bucket_access(self.s3bucket.arn)
tasks.S3BucketRevokeAppAccess(self, self.current_user).create_task()
40 changes: 21 additions & 19 deletions controlpanel/api/models/s3bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -129,24 +128,27 @@ def access_level(self, user):

def save(self, *args, **kwargs):
is_create = not self.pk

super().save(*args, **kwargs)

if is_create:
bucket_owner = kwargs.pop("bucket_owner", self.bucket_owner)

# self.cluster.create(bucket_owner)
tasks.S3BucketCreate(self, self.created_by).create_task()

# XXX created_by is always set if model is saved by the API view
if self.created_by:
UserS3Bucket.objects.create(
user=self.created_by,
current_user=self.created_by,
s3bucket=self,
is_admin=True,
access_level=UserS3Bucket.READWRITE,
)
if not is_create:
return self

tasks.S3BucketCreate(
entity=self,
user=self.created_by,
extra_data={
"bucket_owner": kwargs.pop("bucket_owner", self.bucket_owner),
}
).create_task()

# created_by should always be set, but this is a failsafe
if self.created_by:
UserS3Bucket.objects.create(
user=self.created_by,
current_user=self.created_by,
s3bucket=self,
is_admin=True,
access_level=UserS3Bucket.READWRITE,
)

return self

Expand Down
23 changes: 5 additions & 18 deletions controlpanel/api/models/users3bucket.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -46,22 +45,10 @@ def __repr__(self):
)

def grant_bucket_access(self):
if self.s3bucket.is_folder:
return cluster.User(self.user).grant_folder_access(
root_folder_path=self.s3bucket.name,
access_level=self.access_level,
paths=self.paths,
)
tasks.S3BucketGrantToUser(self, self.current_user).create_task()
# cluster.User(self.user).grant_bucket_access(
# self.s3bucket.arn,
# self.access_level,
# self.resources,
# )

def revoke_bucket_access(self):
if self.s3bucket.is_folder:
return cluster.User(self.user).revoke_folder_access(
root_folder_path=self.s3bucket.name
)
cluster.User(self.user).revoke_bucket_access(self.s3bucket.arn)
# TODO when soft delete is added, this should be updated to use the user that
# has deleted the parent S3bucket to ensure we store the user that has sent the
# task in the case of cascading deletes
tasks.S3BucketRevokeUserAccess(self, self.current_user).create_task()
4 changes: 3 additions & 1 deletion controlpanel/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
7 changes: 5 additions & 2 deletions controlpanel/api/tasks/__init__.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@

from controlpanel.api.tasks.app import AppCreateRole, AppCreateAuth
# First-party/Local
from controlpanel.api.tasks.app import AppCreateAuth, AppCreateRole
from controlpanel.api.tasks.s3bucket import (
S3BucketCreate,
S3BucketGrantToUser,
S3BucketGrantToApp,
S3BucketGrantToUser,
S3BucketRevokeAppAccess,
S3BucketRevokeUserAccess,
)
3 changes: 3 additions & 0 deletions controlpanel/api/tasks/app.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
# Third-party
from django.conf import settings

# First-party/Local
from controlpanel.api.tasks.task_base import TaskBase


Expand Down
13 changes: 9 additions & 4 deletions controlpanel/api/tasks/handlers/__init__.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,18 @@
# First-party/Local
from controlpanel import celery_app

from controlpanel.api.tasks.handlers.app import CreateAppAuthSettings, CreateAppAWSRole
from controlpanel.api.tasks.handlers.s3 import (
CreateS3Bucket, GrantAppS3BucketAccess, GrantUserS3BucketAccess,
CreateS3Bucket,
GrantAppS3BucketAccess,
GrantUserS3BucketAccess,
S3BucketRevokeAppAccess,
S3BucketRevokeUserAccess,
)
from controlpanel.api.tasks.handlers.app import CreateAppAuthSettings, CreateAppAWSRole


create_app_aws_role = celery_app.register_task(CreateAppAWSRole())
create_s3bucket = celery_app.register_task(CreateS3Bucket())
grant_app_s3bucket_access = celery_app.register_task(GrantAppS3BucketAccess())
grant_user_s3bucket_access = celery_app.register_task(GrantUserS3BucketAccess())
create_app_auth_settings = celery_app.register_task(CreateAppAuthSettings())
revoke_user_s3bucket_access = celery_app.register_task(S3BucketRevokeUserAccess())
revoke_app_s3bucket_access = celery_app.register_task(S3BucketRevokeAppAccess())
22 changes: 10 additions & 12 deletions controlpanel/api/tasks/handlers/app.py
Original file line number Diff line number Diff line change
@@ -1,22 +1,21 @@
# First-party/Local
from controlpanel.api import cluster
from controlpanel.api.models import App
from controlpanel.api.models import App, User
from controlpanel.api.tasks.handlers.base import BaseModelTaskHandler


class CreateAppAuthSettings(BaseModelTaskHandler):
model = App
name = "create_app_auth_settings"
permission_required = "api.create_app"

def has_permission(self, user, obj=None):
if not super().has_permission(user, obj):
return False
def handle(self, envs, disable_authentication, connections):
task_user = User.objects.filter(pk=self.task_user_pk).first()
if not task_user or not task_user.github_api_token:
# TODO maybe log this as something has gone wrong?
return self.complete()

return user.github_api_token

def run_task(self, app, user, envs, disable_authentication, connections):
for env in envs:
cluster.App(app, user.github_api_token).create_auth_settings(
cluster.App(self.object, task_user.github_api_token).create_auth_settings(
env_name=env,
disable_authentication=disable_authentication,
connections=connections,
Expand All @@ -27,8 +26,7 @@ def run_task(self, app, user, envs, disable_authentication, connections):
class CreateAppAWSRole(BaseModelTaskHandler):
model = App
name = "create_app_aws_role"
permission_required = "api.create_app"

def run_task(self, app, user):
cluster.App(app).create_iam_role()
def handle(self):
cluster.App(self.object).create_iam_role()
self.complete()
91 changes: 34 additions & 57 deletions controlpanel/api/tasks/handlers/base.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,34 @@
# Third-party
from celery import Task as CeleryTask

from controlpanel.api.models import User, Task
# First-party/Local
from controlpanel.api.models import Task, User


class BaseModelTaskHandler(CeleryTask):
class BaseTaskHandler(CeleryTask):

def complete(self):
task = Task.objects.filter(task_id=self.request.id).first()
if task:
task.completed = True
task.save()

def run(self, *args, **kwargs):
self.handle(*args, **kwargs)

def handle(self, *args, **kwargs):
"""
Should contain the logic to run the task, and will be called after the run
method has been successfully called.
"""
raise NotImplementedError("Task logic not implemented")


class BaseModelTaskHandler(BaseTaskHandler):
name = None
model = None
permission_required = None

object = None
task_user_pk = None
# can be applied to project settings also
# these settings mean that messages are only removed from the queue (acknowledged)
# when returned. if an error occurs, they remain in the queue, and will be resent
Expand All @@ -24,58 +45,14 @@ def get_object(self, pk):
# added back to the queue as could be due to a race condition
raise exc

def get_user(self, pk):
"""
Try to find the user, then check they have the correct permission required to
run the task action.
"""
try:
return User.objects.get(pk=pk)
except User.DoesNotExist as exc:
# if the user is found, this should be a hard fail? So suggest log the error
# and then mark as complete to stop task being rerun?
return None

def has_permission(self, user, obj=None):
def run(self, obj_pk, task_user_pk, *args, **kwargs):
"""
Check that the user has permission to run the task on the given object.
Override on the subclass for further permission checks.
Default method that a celery Task object requires to be defined, and will be
called by the worker when a message is received by the queue. This will look up
the instance of the model, and store the PK of the user running the task to use
to look up the user later if required. The `handle` method is then called
with any other args and kwargs sent.
"""
if not self.permission_required:
raise NotImplementedError("Must define a permission to check")

if not user.has_perm(self.permission_required, obj=obj):
# log that the user did not have permission?
return False

return True

def complete(self):
task = Task.objects.filter(task_id=self.request.id).first()
if task:
task.completed = True
task.save()

def run(self, obj_pk, user_pk, *args, **kwargs):
"""
Default message that a celery Task object requires to be defined, and will be
called by the worker when a message is received by the queue. This runs some
lookups and validates the user, and if these pass calls `run_task` which must
be defined on any subclass of BaseTaskHandler.
"""
obj = self.get_object(obj_pk)
user = self.get_user(user_pk)
if not user:
return self.complete()

if not self.has_permission(user, obj):
return self.complete()

self.run_task(obj, user, *args, **kwargs)

def run_task(self, *args, **kwargs):
"""
Should contain the logic to run the task, and will be called after the run
method has been successfully called.
"""
raise NotImplementedError("Task logic not implemented")
self.object = self.get_object(obj_pk)
self.task_user_pk = task_user_pk
self.handle(*args, **kwargs)
Loading