-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add Owner ID check for bucket with path when prefix is provided #5146
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
637bc2e
b793419
99cd278
53cb6f8
b88493b
0fc8706
61b69c6
e3b88b6
6e735fa
8f4e57f
9f4ea66
e42fb1a
52538e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -630,13 +630,12 @@ def _create_s3_bucket_if_it_does_not_exist(self, bucket_name, region): | |
s3 = self.s3_resource | ||
|
||
bucket = s3.Bucket(name=bucket_name) | ||
expected_bucket_owner_id = self.account_id() | ||
if bucket.creation_date is None: | ||
self.general_bucket_check_if_user_has_permission(bucket_name, s3, bucket, region, True) | ||
self.general_bucket_check_if_user_has_permission(bucket_name, s3, bucket, region, True, expected_bucket_owner_id) | ||
|
||
elif self._default_bucket_set_by_sdk: | ||
self.general_bucket_check_if_user_has_permission(bucket_name, s3, bucket, region, False) | ||
|
||
expected_bucket_owner_id = self.account_id() | ||
self.general_bucket_check_if_user_has_permission(bucket_name, s3, bucket, region, False, expected_bucket_owner_id) | ||
self.expected_bucket_owner_id_bucket_check(bucket_name, s3, expected_bucket_owner_id) | ||
|
||
def expected_bucket_owner_id_bucket_check(self, bucket_name, s3, expected_bucket_owner_id): | ||
|
@@ -649,9 +648,16 @@ def expected_bucket_owner_id_bucket_check(self, bucket_name, s3, expected_bucket | |
|
||
""" | ||
try: | ||
s3.meta.client.head_bucket( | ||
Bucket=bucket_name, ExpectedBucketOwner=expected_bucket_owner_id | ||
) | ||
if self.default_bucket_prefix: | ||
s3.meta.client.list_objects_v2( | ||
Bucket=bucket_name, | ||
Prefix=self.default_bucket_prefix, | ||
ExpectedBucketOwner=expected_bucket_owner_id | ||
) | ||
else: | ||
s3.meta.client.head_bucket( | ||
Bucket=bucket_name, ExpectedBucketOwner=expected_bucket_owner_id | ||
) | ||
except ClientError as e: | ||
error_code = e.response["Error"]["Code"] | ||
message = e.response["Error"]["Message"] | ||
|
@@ -668,7 +674,7 @@ def expected_bucket_owner_id_bucket_check(self, bucket_name, s3, expected_bucket | |
raise | ||
|
||
def general_bucket_check_if_user_has_permission( | ||
self, bucket_name, s3, bucket, region, bucket_creation_date_none | ||
self, bucket_name, s3, bucket, region, bucket_creation_date_none, expected_bucket_owner_id | ||
): | ||
"""Checks if the person running has the permissions to the bucket | ||
|
||
|
@@ -682,7 +688,14 @@ def general_bucket_check_if_user_has_permission( | |
bucket_creation_date_none (bool):Indicating whether S3 bucket already exists or not | ||
""" | ||
try: | ||
s3.meta.client.head_bucket(Bucket=bucket_name) | ||
if self.default_bucket_prefix: | ||
s3.meta.client.list_objects_v2( | ||
Bucket=bucket_name, | ||
Prefix=self.default_bucket_prefix, | ||
ExpectedBucketOwner=expected_bucket_owner_id | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this check is a bit different in this method. Looks like it just checks for general permission to the bucket rather than checking for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good callout. Updated the PR. |
||
) | ||
else: | ||
s3.meta.client.head_bucket(Bucket=bucket_name) | ||
except ClientError as e: | ||
error_code = e.response["Error"]["Code"] | ||
message = e.response["Error"]["Message"] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this extra parameter is unused