-
Notifications
You must be signed in to change notification settings - Fork 870
Implement Presigned Post URLs #3902
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
Conversation
6c58f61
to
dd78028
Compare
2625b99
to
8a43d25
Compare
0dcb789
to
509c246
Compare
7a5fda0
to
2ee4790
Compare
|
||
// Add the AWS signature fields | ||
response.Fields[S3Constants.PostFormDataObjectKey] = request.Key ?? ""; | ||
response.Fields[S3Constants.PostFormDataPolicy] = signedPolicy.Policy; |
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.
please note that S3Constants.PostFormDataPolicy]
is Policy
with a capital P
. All of the other fields are lowercase but i was wanting to re-use this existing field. It doesnt matter if its lowercase or uppercase when sending to s3 though
@@ -51,6 +51,7 @@ | |||
<Reference Include="System.ComponentModel.DataAnnotations" /> | |||
<Reference Include="System.IO.Compression" /> | |||
<Reference Include="System.Web" /> | |||
<Reference Include="System.Net.Http"/> |
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.
ive been trying to figure out why i need System.Net.Http here if i already have it defined in the S3 integration tests. if anyone knows let me know. without this added here i get
C:\codebuild\tmp\output\src17698009\src\aws-sdk-net\sdk\test\Services\S3\IntegrationTests\CreatePresignedPostTests.cs(303,35): error CS1069: The type name 'ByteArrayContent' could not be found in the namespace 'System.Net.Http'. This type has been forwarded to assembly 'System.Net.Http, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' Consider adding a reference to that assembly. [C:\codebuild\tmp\output\src17698009\src\aws-sdk-net\sdk\test\IntegrationTests\AWSSDK.IntegrationTests.NetFramework.csproj]
@@ -0,0 +1,18 @@ | |||
{ | |||
"core": { | |||
"updateMinimum": true, |
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.
Do you need the Core section here if the only service being changed is S3? Also, would it make sense to make this a Minor
change for S3 since it's a new feature?
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.
im updating code in the generator folder which is why i added core. but i guess you are saying generator is not part of core in this case
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 can update to minor
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.
It's not, we normally include Core
only if the generator change would impact all (or multiple) services. But in this case your change is S3 only.
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.
fixed in 20bb0e5
/// </summary> | ||
public CreatePresignedPostRequest() | ||
{ | ||
Expires = AWSSDKUtils.CorrectedUtcNow.AddHours(1); |
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.
Why are we defaulting to an hour? Seems like this should be null requiring users to decide what the expiration should be and give it to us.
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.
thats what javascript does https://github.com/aws/aws-sdk-js-v3/blob/main/packages/s3-presigned-post/src/createPresignedPost.ts#L43
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.
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.
Okay then make sure the documentation for the Expires
property says this defaults to an hour from when the CreatePresignedPostRequest
instance was created.
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.
updated comment in bcd54bc
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.
Pull Request Overview
This PR implements S3 presigned POST URL functionality for the AWS SDK for .NET, enabling browser-based file uploads directly to S3 without requiring server-side processing. The implementation includes comprehensive support for policy conditions, form fields, and AWS signature authentication.
- Adds complete presigned POST URL support with three condition types: exact match, starts-with, and content-length-range
- Implements proper AWS signature v4 authentication with policy document generation and validation
- Provides both synchronous and asynchronous APIs with comprehensive error handling and input validation
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
S3PostCondition.cs | New model classes for policy conditions with factory methods and JSON serialization |
CreatePresignedPostRequest.cs | Request model with bucket name, key, expiration, fields, and conditions |
CreatePresignedPostResponse.cs | Response model containing URL and form fields for POST requests |
AmazonS3Client.Extensions.cs | Core implementation with policy generation, signing, and endpoint resolution |
EndpointResolver.tt | Template update to handle CreatePresignedPostRequest in endpoint resolution |
Various test files | Comprehensive unit and integration tests covering all functionality |
sdk/test/Services/S3/IntegrationTests/AWSSDK.IntegrationTests.S3.NetFramework.csproj
Show resolved
Hide resolved
generator/ServiceClientGeneratorLib/Generators/Endpoints/EndpointResolver.cs
Show resolved
Hide resolved
throw new AmazonS3Exception("Credentials must be specified, cannot call method anonymously"); | ||
|
||
// Resolve credentials asynchronously | ||
var immutableCredentials = await credentials.GetCredentialsAsync().ConfigureAwait(false); |
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.
Why are we resolving this credential if we aren't using it later?
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.
let me double check this
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 updated in 22687c7 to use the resolved credentials. good catch
{ | ||
ValidateCreatePresignedPostRequest(request); | ||
|
||
var credentials = Config.DefaultAWSCredentials ?? DefaultIdentityResolverConfiguration.ResolveDefaultIdentity<AWSCredentials>(); |
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.
Same S3Express
credentials comment.
doing one final dry run before merging |
Description
Motivation and Context
Create Presigned Post
#1901 & DOTNET-8098.Testing
Types of changes
Checklist
License