Remove ImmutableCredentials property from IRequestContext #3589
Conversation
generator/ServiceClientGeneratorLib/Generators/Endpoints/EndpointResolver.partial.cs
Show resolved
Hide resolved
|
|
||
| // credentials would be null in the case of anonymous users getting public resources from S3 | ||
| if (immutableCredentials == null && requestContext.Signer.RequiresCredentials) | ||
| if (requestContext.Identity == null && requestContext.Signer.RequiresCredentials) |
There was a problem hiding this comment.
Anonymous calls will pass this condition, since the identity will be an AnonymousAWSCredentials instance, and I think it is the right approach as we shouldn't add extra corner cases for AnonymousAWSCredentials and they will have a NullSigner which can be called at the end of this method.
| @@ -201,26 +201,6 @@ public void TestSerializingExceptions() | |||
| } | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| [TestCategory("General")] | |||
| public void TestAnonymousCredentialsGetThroughPipeline() | |||
There was a problem hiding this comment.
This test case isn't valid anymore since AnonymousAWSCredentials cannot be used for actions that require credentials.
There was a problem hiding this comment.
FYI @afroz429 (I think this may impact PowerShell - I recall seeing code that explicitly set anonymous credentials for specific Cmdlets)
| @@ -201,26 +201,6 @@ public void TestSerializingExceptions() | |||
| } | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| [TestCategory("General")] | |||
| public void TestAnonymousCredentialsGetThroughPipeline() | |||
There was a problem hiding this comment.
FYI @afroz429 (I think this may impact PowerShell - I recall seeing code that explicitly set anonymous credentials for specific Cmdlets)
| @@ -113,7 +113,7 @@ private static string CheckRegionAndUpdateCache(AmazonS3Uri requestedBucketUri, | |||
| private static string GetHeadBucketPreSignedUrl(string bucketName, IRequestContext requestContext) | |||
| { | |||
| // all buckets accessible via USEast1 | |||
| using (var s3Client = GetUsEast1ClientFromCredentials(requestContext.ImmutableCredentials)) | |||
| using (var s3Client = GetUsEast1ClientFromCredentials(requestContext.ClientConfig.DefaultAWSCredentials)) | |||
There was a problem hiding this comment.
The other places you changed use requestContext.Identity as AWSCredentials and awsCredentials.GetCredentials().
Is using ClientConfig.DefaultAWSCredentials instead here intentional?
There was a problem hiding this comment.
Yep it is intentional. Here we are creating a client this way
And we cannot just set the identity, since it will be overridden anyway in the request pipeline, we need to set the credentials if we was provided by the user, otherwise we will leave it to the AuthResolver to figure it out.
There was a problem hiding this comment.
Ack. It'd be good to include a comment, it's easy to miss why DefaultAWSCredentials is being used if you don't have more context.
| @@ -67,9 +67,9 @@ public void TestRetriesOnException<T>(T exception) where T : AmazonServiceExcept | |||
| .GetValue(client, null) | |||
| as RuntimePipeline; | |||
| // Setup STS failures | |||
| var credentialsRetriever = new Mock<CredentialsRetriever>(); | |||
There was a problem hiding this comment.
I just noticed this class is using the FallbackCredentialsFactory (same for https://github.com/aws/aws-sdk-net/blob/sra-identity-auth/sdk/test/UnitTests/Custom/Runtime/Credentials/FallbackFactoryTest.cs).
We should probably update our tests to use the DefaultAWSCredentialsIdentityResolver instead (as a separate PR - not as part of this one).
generator/ServiceClientGeneratorLib/Generators/Endpoints/EndpointResolver.partial.cs
Show resolved
Hide resolved
| @@ -24,11 +31,25 @@ namespace Amazon.Runtime.Internal.Auth | |||
| /// the service to authenticate the SDK customer's identity. | |||
| /// </para> | |||
| /// </summary> | |||
| #pragma warning disable CA1040 // Avoid empty interfaces | |||
| public interface ISigner | |||
There was a problem hiding this comment.
Should add /// comments to the other public methods since you added them to RequiresCredentials.
There was a problem hiding this comment.
@muhammad-othman There's some text in the SRA book (for the Signer interface) if you want to use that: Signs the provided HTTP request. The provided identity is used for signing, and an exception will be thrown if the identity is not compatible with this signer.
I also noticed the existing comment for RequiresCredentials mentions immutable credentials (which we're trying to remove). May as well remove the <see cref="ImmutableCredentials"/> from there.
Description
ImmutableCredentialsproperty fromIRequestContext.requestContext.ImmutableCredentialsare updated to userequest.Identityinstead.ISignerinterface to include the methods and properties needed for its usages.AbstractAWSSignerstart using the newISignerinterface instead.Motivation and Context
DOTNET-7845Testing
DRY_RUN-93591562-46d9-4349-a75e-07822865d20a