-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add S3.generatePresignedPost for HTML form based uploads #710
Add S3.generatePresignedPost for HTML form based uploads #710
Conversation
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.
Here are some initial comments. I haven't actually tested this code yet
@adam-fowler Thanks for the feedback. The test definitely won’t pass. The inputs from the AWS docs don’t really work for a reproducible output. Working on a better test case. |
You'll need to rebase 7.x.x to get this compiling again by the way |
b6f4242
to
d9e6617
Compare
I have to approach testing this differently, but I have a plan. Update coming sometime tomorrow afternoon. |
Why can't you use the example to verify you are calculating the correct signature? |
The example string to sign in their docs is a base 64 encoded JSON object, but the JSON object they used has arbitrary whitespace in it, so it would be absurd to try to make this code produce the exact same string. I can make use of the examples from the docs to test at a more granular level. If I test the getSignature method in isolation (rather than generatePresignedPost as a whole), I can supply the string to sign directly, rather than generating it. I’ll push an updated test that does that once I’m at my desk today. The other thing I tried is using Boto3 to generate known to work values from example credentials, but Python preserves order in dictionaries, which means the properties of the JSON object end up in a different order. Same problem, we’d have to make our code worse to match the output exactly, for absolutely no gain outside of the test suite. In addition to testing getSignature, I’ll test getPresignedPostCredential, and I’ll have to test that generatePresignedPost produces the right URL, and a fields dictionary and post policy that have all the right keys. If I test all of that, I’m confident that the code is well covered. I’m about halfway done with these tests, and should be able to finish this morning. Thanks for your patience. |
func getPresignedPostCredential(date: String, accessKeyId: String) async throws -> String { | ||
let region = config.region.rawValue | ||
let service = config.signingName | ||
|
||
let credential = "\(accessKeyId)/\(date)/\(region)/\(service)/aws4_request" | ||
return credential | ||
} |
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.
I think this also exists in some form in soto-core. See previous comment.
@adam-fowler Ready for another review. |
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.
Looks good
Have you tested with real assets?
I've just run swift format on everything. And will merge when all checks are complete |
I did test with real assets and credentials, but not from outside of the package until today. Submitted a small follow-up PR and have confirmed that everything works as intended once that is merged. |
Adds a generatePresignedPost method to the S3 service, similar to what is available in Boto3.
Amazon docs for this type of request live here.
Remaining work: