Skip to content

Commit

Permalink
Fix the AccessPolicy type and configure the S3_CONTROLLED_ACCESS_BUCK…
Browse files Browse the repository at this point in the history
…ET variable for testing purposes.
  • Loading branch information
Chrystinne committed Dec 3, 2024
1 parent 1dc66d5 commit b8eaa82
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 14 deletions.
15 changes: 8 additions & 7 deletions physionet-django/project/cloud/s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -711,7 +711,7 @@ def get_access_point_name_for_user_and_project(current_user, aws):
).first()
if access_point:
return access_point.name
except Exception as e:
except Exception:
return "No access point found for this user with the specified project details"

return "No access point found for this user with the specified project details"
Expand Down Expand Up @@ -879,7 +879,8 @@ def create_data_access_point_policy(
"Principal": principal_value,
"Action": ["s3:GetObject", "s3:ListBucket"],
"Resource": [
f"arn:aws:s3:us-east-1:{settings.AWS_ACCOUNT_ID}:accesspoint/{access_point_name}/object/{project_slug}/{project_version}/*",
f"arn:aws:s3:us-east-1:{settings.AWS_ACCOUNT_ID}:accesspoint/"
f"{access_point_name}/object/{project_slug}/{project_version}/*",
f"arn:aws:s3:us-east-1:{settings.AWS_ACCOUNT_ID}:accesspoint/{access_point_name}",
],
}
Expand Down Expand Up @@ -912,7 +913,7 @@ def set_data_access_point_policy(data_access_point_name, data_access_point_polic
if s3_control is None:
return
try:
response = s3_control.put_access_point_policy(
s3_control.put_access_point_policy(
AccountId=settings.AWS_ACCOUNT_ID,
Name=data_access_point_name,
Policy=data_access_point_policy,
Expand Down Expand Up @@ -1023,7 +1024,7 @@ def update_data_access_point_policy(project):
data_access_point_version = str(i + 1).zfill(2)
data_access_point_name = f"{project.slug}-v{project.version.replace('.', '-')}-{data_access_point_version}"
subset_aws_ids = aws_ids[
i * MAX_PRINCIPALS_PER_AP_POLICY : (i + 1) * MAX_PRINCIPALS_PER_AP_POLICY
i * MAX_PRINCIPALS_PER_AP_POLICY: (i + 1) * MAX_PRINCIPALS_PER_AP_POLICY
]
access_point = AWSAccessPoint.objects.filter(
name=data_access_point_name, aws__project=project
Expand Down Expand Up @@ -1120,7 +1121,7 @@ def create_s3_access_point(project, access_point_name, bucket_name, account_id):
"""
s3 = create_s3_control_client()
try:
response = s3.create_access_point(
s3.create_access_point(
AccountId=account_id,
Bucket=bucket_name,
Name=access_point_name,
Expand Down Expand Up @@ -1199,9 +1200,9 @@ def upload_project_to_S3(project):
raise Exception(f"A bucket named {bucket_name} already exists.")
except s3.exceptions.BucketAlreadyOwnedByYou:
bucket_created = False

# Set the bucket policy only if the bucket was newly created and has controlled access
if bucket_created and project.access_policy == AccessPolicy.CONTROLLED:
if bucket_created and project.access_policy == AccessPolicy.CREDENTIALED:
controlled_policy = create_controlled_bucket_policy(bucket_name)
s3.put_bucket_policy(Bucket=bucket_name, Policy=controlled_policy)

Expand Down
2 changes: 2 additions & 0 deletions physionet-django/project/modelcomponents/middleware.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,12 @@

_request_local = threading.local()


def get_current_request():
"""Retrieve the current request from thread-local storage."""
return getattr(_request_local, 'request', None)


class CurrentRequestMiddleware:
"""Middleware to store the current request in thread-local storage."""
def __init__(self, get_response):
Expand Down
12 changes: 7 additions & 5 deletions physionet-django/project/modelcomponents/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ class AWS(models.Model):

class Meta:
default_permissions = ()

def s3_uri(self):
"""
Construct the S3 URI for the project.
Expand All @@ -81,7 +81,10 @@ def s3_uri(self):
# Fetch access point name
access_point_name = get_access_point_name_for_user_and_project(current_user, self)
if access_point_name and "No " not in access_point_name:
return f's3://arn:aws:s3:us-east-1:{settings.AWS_ACCOUNT_ID}:accesspoint/{access_point_name}/{self.project.slug}/{self.project.version}/'
return (
f's3://arn:aws:s3:us-east-1:{settings.AWS_ACCOUNT_ID}:accesspoint/'
f'{access_point_name}/{self.project.slug}/{self.project.version}/'
)
else:
print(f"Error: {access_point_name}")
return None
Expand All @@ -105,12 +108,12 @@ class AWSAccessPoint(models.Model):

class AWSAccessPointUser(models.Model):
access_point = models.ForeignKey(
AWSAccessPoint,
AWSAccessPoint,
related_name='linked_users',
on_delete=models.CASCADE
)
user = models.ForeignKey(
'user.User',
'user.User',
related_name='aws_access_point_users',
on_delete=models.CASCADE
)
Expand All @@ -120,4 +123,3 @@ class Meta:

def __str__(self):
return f"User: {self.user}, Access Point: {self.access_point}"

1 change: 1 addition & 0 deletions physionet-django/project/test_s3.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
AWS_ACCOUNT_ID='123456789012',
S3_OPEN_ACCESS_BUCKET='datashare-public',
S3_SERVER_ACCESS_LOG_BUCKET='datashare-logs',
S3_CONTROLLED_ACCESS_BUCKET='datashare-protected',
)
class TestS3(TestMixin):
"""
Expand Down
8 changes: 6 additions & 2 deletions physionet-django/project/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -1938,7 +1938,7 @@ def published_project(request, project_slug, version, subdir=''):
s3_uri = project.aws.s3_uri()
else:
s3_uri = '--no-sign-request ' + project.aws.s3_uri()

context = {
'project': project,
'authors': authors,
Expand Down Expand Up @@ -2048,7 +2048,11 @@ def sign_dua(request, project_slug, version):

if request.method == 'POST' and 'agree' in request.POST:
DUASignature.objects.create(user=user, project=project)
if has_s3_credentials() and files_sent_to_S3(project) is not None and s3_bucket_has_credentialed_users(project):
if (
has_s3_credentials()
and files_sent_to_S3(project) is not None
and s3_bucket_has_credentialed_users(project)
):
update_data_access_point_policy(project)
return render(request, 'project/sign_dua_complete.html', {
'project':project})
Expand Down

0 comments on commit b8eaa82

Please sign in to comment.