Skip to content
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

Merged

Conversation

nicksloan
Copy link
Contributor

@nicksloan nicksloan commented Mar 29, 2024

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:

  • Add error handling
  • Add documentation
  • Add tests

Copy link
Member

@adam-fowler adam-fowler left a 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

Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Tests/SotoTests/Services/S3/S3ExtensionTests.swift Outdated Show resolved Hide resolved
@nicksloan
Copy link
Contributor Author

@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.

@adam-fowler
Copy link
Member

You'll need to rebase 7.x.x to get this compiling again by the way

@nicksloan nicksloan force-pushed the feat/s3-generatePresignedPost branch from b6f4242 to d9e6617 Compare April 1, 2024 14:12
@nicksloan
Copy link
Contributor Author

I have to approach testing this differently, but I have a plan. Update coming sometime tomorrow afternoon.

@adam-fowler
Copy link
Member

Why can't you use the example to verify you are calculating the correct signature?

@nicksloan
Copy link
Contributor Author

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.

@nicksloan nicksloan marked this pull request as ready for review April 2, 2024 13:23
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Tests/SotoTests/Services/S3/S3ExtensionTests.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Comment on lines 226 to 232
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
}
Copy link
Contributor Author

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.

Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
Sources/Soto/Extensions/S3/S3+presignedPost.swift Outdated Show resolved Hide resolved
@nicksloan
Copy link
Contributor Author

@adam-fowler Ready for another review.

Copy link
Member

@adam-fowler adam-fowler left a 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?

@adam-fowler
Copy link
Member

I've just run swift format on everything. And will merge when all checks are complete

@adam-fowler adam-fowler merged commit 7b0437b into soto-project:7.x.x Apr 5, 2024
6 checks passed
@nicksloan
Copy link
Contributor Author

Looks good Have you tested with real assets?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants