S3: Cross-account access for buckets #6333
Conversation
0d5abc2 to
3431432
Compare
1b2d472 to
e7c8835
Compare
cadb2f4 to
17d6a7b
Compare
72e2654 to
c71caae
Compare
c71caae to
7210f8c
Compare
6723f42 to
5408413
Compare
| @@ -39,10 +45,20 @@ class TestS3FileHandleClosures(TestCase): | |||
| def setUp(self) -> None: | |||
| if settings.TEST_SERVER_MODE: | |||
| raise SkipTest("No point in testing ServerMode, we're not using boto3") | |||
| self.s3 = s3model.S3Backend("us-west-1", "1234") | |||
There was a problem hiding this comment.
This had to be changed because now we make references to s3_backends from with the S3Backend class
5408413 to
f6e7547
Compare
9759f23 to
7014e45
Compare
7014e45 to
adf2bc7
Compare
moto/s3/models.py
Outdated
| @@ -1645,6 +1647,13 @@ def create_bucket(self, bucket_name: str, region_name: str) -> FakeBucket: | |||
| def list_buckets(self) -> List[FakeBucket]: | |||
| return list(self.buckets.values()) | |||
|
|
|||
| def get_bucket_global(self, bucket_name: str) -> FakeBucket: | |||
There was a problem hiding this comment.
Does it make sense to just rewrite get_bucket to include this? I think that would make the cross-account logic available for all methods.
If that's not possible, or it introduces complexity somewhere else, I'd be happy to merge this as is - just trying to understand the pros/cons of this approach 🙂
There was a problem hiding this comment.
I was being cautious by having separate methods.
Changed in 329af1c, everythings seems to be working fine 👍
Codecov Report
@@ Coverage Diff @@
## master #6333 +/- ##
==========================================
- Coverage 96.27% 96.16% -0.11%
==========================================
Files 792 794 +2
Lines 77221 77540 +319
==========================================
+ Hits 74341 74567 +226
- Misses 2880 2973 +93
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
This is now part of moto >= 4.1.11.dev21 |
| except KeyError: | ||
| raise MissingBucket(bucket=bucket_name) | ||
|
|
||
| if bucket_name in s3_backends.bucket_accounts: |
There was a problem hiding this comment.
Hi @viren-nadkarni! I realize it's been a while.. but what whas the reasoning behind this part?
I guess it made sense to me at the time, but looking at it again, I wonder why we don't just throw an AccessDenied error.
There was a problem hiding this comment.
Hi @bblommers, it's there to access a bucket that belongs to a different account
There was a problem hiding this comment.
Right. When I looked at it earlier, an AccessDenied error felt more intuitive.
But thinking about it a little more, all our cross-account is allowed by default, so this implementation makes more sense. Thanks @viren-nadkarni
This PR is the intial attempt to introduce cross-account support to S3.
The scope is limited to:
GetObject,PutObject,ListObjectsCurrently, no IAM/ACL/permission checks are performed when cross-account access happens. This could be tackled gradually given how complex the S3 service is.