Implement opting out of S3 Express session auth via config variables#3631
Implement opting out of S3 Express session auth via config variables#3631aemous wants to merge 27 commits intoboto:developfrom
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3631 +/- ##
===========================================
- Coverage 92.71% 92.62% -0.10%
===========================================
Files 68 68
Lines 15561 15664 +103
===========================================
+ Hits 14428 14509 +81
- Misses 1133 1155 +22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
One high level comment (got a few more comments to come): Your PR description says that there's a new client configuration setting, but this doesn't work: We never added it to the Config class; how are users expected to set this in code? |
@SamRemis As we discussed offline, it is set in code-config via the S3 config. import boto3
from botocore.config import Config
my_config = Config(
s3 = {
'disable_s3_express_session_auth': True
}
)
client = boto3.client('s3', config=my_config)
# client = boto3.client('s3') |
|
Thank you, you're right. I wasn't familiar with the code path for service specific configs and was expecting an update in config.py |
| from tests import ClientHTTPStubber | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
I appreciate the use of pytest.mark.parametrize here, but I think it may make more sense to split this into two more readable test cases. One where we assert that s3express auth is disabled, and one where we assert that it's not.
We can use class constants for reusable variables like the method responses, bucket name, and date. Then use a pytest fixture for the datetime.
We should also be adding an assertion on len(http_stubber.requests) for the number of requests on the stubbed client (expect one for the non-session auth test and two for the one with session auth). We could potentially even assert the URL of the first request using stubber.requests[0].url.endswith('?session'), but that's relying on the URL of the API not changing.
I'm asking for a refactoring of the whole class, so happy to chat more about this if you disagree or find any of it confusing.
There was a problem hiding this comment.
Published a revision.
SamRemis
left a comment
There was a problem hiding this comment.
Looks good, thanks Ahmed :)
…ion setting name.
…est coverage to the config hierarchy.
…n computing whether to disable S3 session auth.
Description of changes:
disable_s3_express_session_authclient configuration setting, or the newAWS_S3_DISABLE_EXPRESS_SESSION_AUTHenvironment variable, or thedisable_s3_express_session_authS3 shared configuration setting.Description of tests:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.