From e8bcb11f326ab706260875cc89403b9a484de005 Mon Sep 17 00:00:00 2001 From: maliming Date: Mon, 1 Jun 2026 13:54:43 +0800 Subject: [PATCH] Address Copilot review on AWS S3-compatible provider - Skip container wiring in test module when AWS credentials are absent - Dispose AmazonS3Client in test cleanup - Validate Region or ServiceURL early in DefaultAmazonS3ClientFactory - Normalize trailing slash in ServiceURL test assertions - Clarify Region and ServiceURL coupling in XML docs and aws.md --- .../infrastructure/blob-storing/aws.md | 2 +- .../Aws/AwsBlobProviderConfiguration.cs | 14 +++++++++---- .../Aws/DefaultAmazonS3ClientFactory.cs | 6 ++++++ .../Aws/AbpBlobStoringAwsTestModule.cs | 11 +++++++++- .../Aws/DefaultAmazonS3ClientFactory_Tests.cs | 20 +++++++++---------- 5 files changed, 36 insertions(+), 17 deletions(-) diff --git a/docs/en/framework/infrastructure/blob-storing/aws.md b/docs/en/framework/infrastructure/blob-storing/aws.md index cc7274c99b..b419521bed 100644 --- a/docs/en/framework/infrastructure/blob-storing/aws.md +++ b/docs/en/framework/infrastructure/blob-storing/aws.md @@ -65,7 +65,7 @@ Configure(options => * **UseTemporaryFederatedCredentials** (bool): Use [federated user temporary credentials](https://docs.aws.amazon.com/AmazonS3/latest/dev/AuthUsingTempFederationToken.html) to access AWS services, default : `false`. * **ProfileName** (string): The [name of the profile](https://docs.aws.amazon.com/sdk-for-net/v3/developer-guide/net-dg-config-creds.html) to get credentials from. * **ProfilesLocation** (string): The path to the aws credentials file to look at. -* **Region** (string): The system name of the service. +* **Region** (string): The system name of the AWS region (e.g., `us-east-1`). **Required** for real AWS S3. Optional when `ServiceURL` is configured for an S3-compatible service; some services accept any value (or `auto` for Cloudflare R2). * **ServiceURL** (string): Custom service URL for S3-compatible APIs (e.g., MinIO, DigitalOcean Spaces, Cloudflare R2). If not specified, the default AWS S3 service URL will be used based on the region. When using S3-compatible services, this should point to your service endpoint (e.g., `https://minio.example.com:9000`). The AWS SDK automatically appends a trailing slash to the configured value. * **DisablePayloadSigning** (bool): Default `false`. When set to `true`, the provider sends `x-amz-content-sha256: UNSIGNED-PAYLOAD` on `PutObject` requests instead of the streaming chunked signature (`STREAMING-AWS4-HMAC-SHA256-PAYLOAD`) that the AWS SDK v4 uses by default. Required for Cloudflare R2 and other S3-compatible services that do not implement streaming signing. The endpoint must be HTTPS when this option is enabled. Leave as `false` for real AWS S3. * **Policy** (string): An IAM policy in JSON format that you want to use as an inline session policy. diff --git a/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/AwsBlobProviderConfiguration.cs b/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/AwsBlobProviderConfiguration.cs index 6f27ee2835..fff08dc42b 100644 --- a/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/AwsBlobProviderConfiguration.cs +++ b/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/AwsBlobProviderConfiguration.cs @@ -57,6 +57,11 @@ public class AwsBlobProviderConfiguration set => _containerConfiguration.SetConfiguration(AwsBlobProviderConfigurationNames.Policy, value); } + /// + /// The system name of the AWS region (e.g., us-east-1). Required for real AWS S3. + /// Optional when is configured for an S3-compatible service; + /// some services accept any value (or auto for Cloudflare R2). + /// public string? Region { get => _containerConfiguration.GetConfigurationOrDefault(AwsBlobProviderConfigurationNames.Region); set => _containerConfiguration.SetConfiguration(AwsBlobProviderConfigurationNames.Region, value); @@ -64,8 +69,8 @@ public class AwsBlobProviderConfiguration /// /// Custom service URL for S3-compatible APIs (e.g., MinIO, DigitalOcean Spaces, Cloudflare R2). - /// If not specified, the default AWS S3 service URL will be used based on the region. - /// Note: The AWS SDK automatically appends a trailing slash to the configured value. + /// When omitted, becomes required and the default AWS S3 endpoint for that + /// region is used. The AWS SDK automatically appends a trailing slash to the configured value. /// public string? ServiceURL { get => _containerConfiguration.GetConfigurationOrDefault(AwsBlobProviderConfigurationNames.ServiceURL); @@ -73,11 +78,12 @@ public class AwsBlobProviderConfiguration } /// - /// When true, payload signing is disabled on PutObject (and similar) requests so the SDK sends + /// When true, payload signing is disabled on PutObject upload requests so the SDK sends /// x-amz-content-sha256: UNSIGNED-PAYLOAD instead of the streaming chunked signature /// (STREAMING-AWS4-HMAC-SHA256-PAYLOAD) that AWS SDK v4 uses by default. Required for /// Cloudflare R2 and other S3-compatible services that do not implement streaming signing. - /// Default: false (keep AWS SDK defaults for real AWS S3). + /// The endpoint must be HTTPS when this option is enabled (AWS SDK rejects unsigned payloads + /// over HTTP). Default: false (keep AWS SDK defaults for real AWS S3 and MinIO). /// public bool DisablePayloadSigning { get => _containerConfiguration.GetConfigurationOrDefault(AwsBlobProviderConfigurationNames.DisablePayloadSigning, false); diff --git a/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory.cs b/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory.cs index 8e8ffa4e3a..7d3d0b3869 100644 --- a/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory.cs +++ b/framework/src/Volo.Abp.BlobStoring.Aws/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory.cs @@ -30,6 +30,12 @@ public class DefaultAmazonS3ClientFactory : IAmazonS3ClientFactory, ITransientDe public virtual async Task GetAmazonS3Client( AwsBlobProviderConfiguration configuration) { + if (configuration.Region.IsNullOrWhiteSpace() && configuration.ServiceURL.IsNullOrWhiteSpace()) + { + throw new AbpException( + $"Either {nameof(AwsBlobProviderConfiguration.Region)} or {nameof(AwsBlobProviderConfiguration.ServiceURL)} must be configured on {nameof(AwsBlobProviderConfiguration)}."); + } + var region = !configuration.Region.IsNullOrWhiteSpace() ? RegionEndpoint.GetBySystemName(configuration.Region) : null; diff --git a/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/AbpBlobStoringAwsTestModule.cs b/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/AbpBlobStoringAwsTestModule.cs index 8b9b1e3485..8ef4000e61 100644 --- a/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/AbpBlobStoringAwsTestModule.cs +++ b/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/AbpBlobStoringAwsTestModule.cs @@ -56,6 +56,15 @@ public class AbpBlobStoringAwsTestModule : AbpModule var configuration = context.Services.GetConfiguration(); var accessKeyId = configuration["Aws:AccessKeyId"]; var secretAccessKey = configuration["Aws:SecretAccessKey"]; + + // No credentials configured (e.g., CI without user secrets) → skip container wiring. + // `BlobContainerConfiguration.SetConfiguration` rejects nulls, so any attempt to resolve + // a container with null values from configuration would throw at runtime. + if (string.IsNullOrEmpty(accessKeyId) || string.IsNullOrEmpty(secretAccessKey)) + { + return; + } + var region = configuration["Aws:Region"]; var serviceUrl = configuration["Aws:ServiceURL"]; var disablePayloadSigning = bool.TryParse(configuration["Aws:DisablePayloadSigning"], out var dps) && dps; @@ -109,7 +118,7 @@ public class AbpBlobStoringAwsTestModule : AbpModule try { - var amazonS3Client = await context.ServiceProvider.GetRequiredService() + using var amazonS3Client = await context.ServiceProvider.GetRequiredService() .GetAmazonS3Client(_configuration); if (!await AmazonS3Util.DoesS3BucketExistV2Async(amazonS3Client, _actualContainerName)) diff --git a/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory_Tests.cs b/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory_Tests.cs index fbd51dd038..e4886f602b 100644 --- a/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory_Tests.cs +++ b/framework/test/Volo.Abp.BlobStoring.Aws.Tests/Volo/Abp/BlobStoring/Aws/DefaultAmazonS3ClientFactory_Tests.cs @@ -1,4 +1,5 @@ using System.Threading.Tasks; +using Amazon.Runtime; using Amazon.S3; using Microsoft.Extensions.DependencyInjection; using Shouldly; @@ -35,8 +36,8 @@ public class DefaultAmazonS3ClientFactory_Tests : AbpBlobStoringAwsTestBase // Assert s3Client.ShouldNotBeNull(); - s3Client.Config.ServiceURL.ShouldBe(serviceUrl + "/"); // AWS SDK automatically appends trailing slash - ((AmazonS3Config)s3Client.Config).ForcePathStyle.ShouldBeTrue(); // Should be enabled for S3-compatible services + s3Client.Config.ServiceURL?.TrimEnd('/').ShouldBe(serviceUrl); + ((AmazonS3Config)s3Client.Config).ForcePathStyle.ShouldBeTrue(); } [Fact] @@ -81,8 +82,8 @@ public class DefaultAmazonS3ClientFactory_Tests : AbpBlobStoringAwsTestBase // Assert s3Client.ShouldNotBeNull(); - s3Client.Config.ServiceURL.ShouldBe("https://minio.example.com:9000/"); // AWS SDK automatically appends trailing slash - ((AmazonS3Config)s3Client.Config).ForcePathStyle.ShouldBeTrue(); // Should be enabled for S3-compatible services + s3Client.Config.ServiceURL?.TrimEnd('/').ShouldBe("https://minio.example.com:9000"); + ((AmazonS3Config)s3Client.Config).ForcePathStyle.ShouldBeTrue(); } [Fact] @@ -105,12 +106,9 @@ public class DefaultAmazonS3ClientFactory_Tests : AbpBlobStoringAwsTestBase // Assert s3Client.ShouldNotBeNull(); var config = (AmazonS3Config)s3Client.Config; - config.ServiceURL.ShouldBe("https://r2.cloudflarestorage.com/"); + config.ServiceURL?.TrimEnd('/').ShouldBe("https://r2.cloudflarestorage.com"); config.ForcePathStyle.ShouldBeTrue(); - - // Verify checksum properties are set for S3-compatible services (required for Cloudflare R2) - // We just verify they are not null/default, indicating they have been set - config.RequestChecksumCalculation.ShouldNotBe(default); - config.ResponseChecksumValidation.ShouldNotBe(default); + config.RequestChecksumCalculation.ShouldBe(RequestChecksumCalculation.WHEN_REQUIRED); + config.ResponseChecksumValidation.ShouldBe(ResponseChecksumValidation.WHEN_REQUIRED); } -} \ No newline at end of file +}