Content Delivery Network #47
Conversation
WalkthroughThe pull request introduces several modifications across various files in the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 43
🧹 Outside diff range and nitpick comments (41)
src/Argon.Api/Grains.Interfaces/IUserGrain.cs (1)
7-13: Consider CDN-specific considerations for avatar management.Since this interface now supports avatar management through CDN:
- Consider adding documentation about avatar size limits and supported formats
- Think about implementing avatar cleanup when a user is deleted
- Consider adding methods for avatar validation and processing
src/Argon.Api/Features/MediaStorage/CdnOptions.cs (2)
8-8: Consider using a secure string or encrypted configuration for SignSecret.Storing sensitive data like signing secrets as plain string properties could pose a security risk if the configuration is logged or serialized.
Consider using Azure Key Vault or similar secure storage for the signing secret in production environments.
14-18: Follow C# naming conventions for enum values and expand error states.The enum uses uppercase naming which doesn't follow C# conventions. Additionally, consider adding more specific error states for better error handling.
Consider this improved implementation:
+/// <summary> +/// Represents possible errors during file upload operations +/// </summary> public enum UploadError { - NONE, - INTERNAL_ERROR + /// <summary> + /// No error occurred + /// </summary> + None = 0, + + /// <summary> + /// Internal server error occurred + /// </summary> + InternalError = 1, + + /// <summary> + /// Insufficient storage space + /// </summary> + InsufficientSpace = 2, + + /// <summary> + /// Invalid file format + /// </summary> + InvalidFormat = 3, + + /// <summary> + /// File size exceeds limit + /// </summary> + FileTooLarge = 4 }src/Argon.Api/Features/Pex/IPermissionProvider.cs (2)
16-19: Add XML documentation to improve API clarity.The interface lacks essential documentation about its purpose, parameters, and contract.
Consider adding documentation:
+/// <summary> +/// Provides permission checking capabilities for the application. +/// </summary> public interface IPermissionProvider { + /// <summary> + /// Determines if the current context has access to the specified scope. + /// </summary> + /// <param name="scope">The permission scope to check. Must not be null.</param> + /// <param name="bag">Additional properties used for permission evaluation.</param> + /// <returns>True if access is granted, false otherwise.</returns> + /// <exception cref="ArgumentNullException">Thrown when scope is null.</exception> public bool CanAccess(string scope, PropertyBag bag); }
1-25: Consider enhancing the permission system for CDN requirements.The current permission system might be insufficient for CDN security needs. Consider:
- Adding CDN-specific permission scopes (e.g., read, write, delete)
- Supporting resource-level permissions
- Implementing caching for permission checks
- Adding audit logging for permission decisions
Example CDN-specific permission provider:
public interface ICdnPermissionProvider : IPermissionProvider { Task<bool> CanAccessResource(string resourceId, CdnOperation operation); Task<bool> CanManageContainer(string containerId); } public enum CdnOperation { Read, Write, Delete, List }Would you like me to help design a more comprehensive permission system for CDN?
src/Argon.Api/Controllers/MetadataController.cs (1)
Line range hint
1-27: Consider splitting responsibilitiesThe MetadataController is handling multiple concerns (configuration, feature flags, region management). Consider splitting these responsibilities:
- Create separate endpoints for feature flags
- Move region-specific logic to a dedicated service
- Consider using ASP.NET Core's built-in configuration endpoints for non-sensitive settings
This will improve maintainability and make the system more modular.
src/Argon.Api/Features/MediaStorage/IContentDeliveryNetwork.cs (2)
21-23: Add XML documentation for interface methods.The abstract methods would benefit from XML documentation describing their behavior, parameters, return values, and possible error cases. This is especially important for interface contracts.
Add documentation like this:
+ /// <summary> + /// Creates a new asset in the CDN. + /// </summary> + /// <param name="ns">The storage namespace for the asset.</param> + /// <param name="asset">The unique identifier for the asset.</param> + /// <param name="file">The file stream containing the asset data.</param> + /// <returns>A Maybe containing an UploadError if the operation fails.</returns> ValueTask<Maybe<UploadError>> CreateAssetAsync(StorageNameSpace ns, AssetId asset, Stream file); + /// <summary> + /// Replaces an existing asset in the CDN. + /// </summary> + /// <param name="ns">The storage namespace for the asset.</param> + /// <param name="asset">The unique identifier for the asset to replace.</param> + /// <param name="file">The file stream containing the new asset data.</param> + /// <returns>A Maybe containing an UploadError if the operation fails.</returns> ValueTask<Maybe<UploadError>> ReplaceAssetAsync(StorageNameSpace ns, AssetId asset, Stream file); + /// <summary> + /// Generates a URL for accessing the specified asset. + /// </summary> + /// <param name="ns">The storage namespace for the asset.</param> + /// <param name="asset">The unique identifier of the asset.</param> + /// <returns>A URL string for accessing the asset.</returns> ValueTask<string> GenerateAssetUrl(StorageNameSpace ns, AssetId asset);
5-24: Consider adding validation and content type handling.From an architectural perspective, consider these enhancements:
- Add methods for content validation before upload
- Include content type handling in the interface
- Consider adding methods for asset metadata management
- Add support for bulk operations
These additions would make the CDN interface more robust and feature-complete.
src/Argon.Entry/appsettings.json (1)
17-20: Consider using environment-specific cluster IDsUsing identical values for both
ClusterIdandServiceIdmight cause issues when running multiple environments (staging/production) or during A/B deployments.Consider using environment-specific values:
"Orleans": { - "ClusterId": "argonapi", - "ServiceId": "argonapi" + "ClusterId": "argonapi-${ENVIRONMENT}", + "ServiceId": "argonapi-${ENVIRONMENT}" }src/Argon.Api/Grains.Interfaces/IServerGrain.cs (1)
Line range hint
30-34: Add input validation documentationSince all properties are now nullable, it would be helpful to document the validation rules for these fields (e.g., when are they required, any length/format constraints).
Add XML documentation to clarify the requirements:
public sealed partial record ServerInput( [property: DataMember(Order = 0), MemoryPackOrder(0), Key(0), Id(0)] + /// <summary> + /// The name of the server. While nullable, this should be provided for server creation. + /// </summary> string? Name, [property: DataMember(Order = 1), MemoryPackOrder(1), Key(1), Id(1)] + /// <summary> + /// Optional description of the server. + /// </summary> string? Description, [property: DataMember(Order = 2), MemoryPackOrder(2), Key(2), Id(2)] + /// <summary> + /// Optional URL to the server's avatar image. + /// </summary> string? AvatarUrl);src/Argon.Api/Extensions/HttpContextExtensions.cs (1)
15-18: Consider revising the fallback mechanism for GetRayThe current implementation uses a GUID as a fallback when the CF-Ray header is missing. This might be misleading as Cloudflare Ray IDs follow a different format (e.g., "6fa5789e684c4f35-LAX"). Consider using a more distinctive prefix for non-Cloudflare requests.
public static string GetRay(this HttpContext ctx) => ctx.Request.Headers.ContainsKey("CF-Ray") ? ctx.Request.Headers["CF-Ray"].ToString() - : $"{Guid.NewGuid()}"; + : $"LOCAL-{Guid.NewGuid()}";src/Argon.Api/Features/MediaStorage/DiskContentDeliveryNetwork.cs (1)
9-21: Enhance error handling and loggingThe current implementation could be improved in several ways:
- Include the namespace in the log message for better context
- Handle different types of exceptions separately (e.g., IOException, UnauthorizedAccessException)
- Consider adding debug logging for successful uploads
Here's a suggested improvement:
public async ValueTask<Maybe<UploadError>> CreateAssetAsync(StorageNameSpace ns, AssetId asset, Stream file) { try { await Storage.UploadFile(ns, asset, file); + logger.LogDebug("Successfully uploaded file '{FilePath}' to namespace '{Namespace}'", + asset.GetFilePath(), ns.ToPath()); return Maybe<UploadError>.None(); } + catch (UnauthorizedAccessException e) + { + logger.LogError(e, "Access denied while uploading file '{FilePath}' to namespace '{Namespace}'", + asset.GetFilePath(), ns.ToPath()); + return UploadError.ACCESS_DENIED; + } + catch (IOException e) + { + logger.LogError(e, "IO error while uploading file '{FilePath}' to namespace '{Namespace}'", + asset.GetFilePath(), ns.ToPath()); + return UploadError.IO_ERROR; + } catch (Exception e) { - logger.LogCritical(e, $"Failed upload file '{asset.GetFilePath()}'"); + logger.LogCritical(e, "Unexpected error uploading file '{FilePath}' to namespace '{Namespace}'", + asset.GetFilePath(), ns.ToPath()); return UploadError.INTERNAL_ERROR; } }src/Argon.Api/Features/MediaStorage/Storages/S3ContentStorage.cs (2)
6-7: Add XML documentation and validate dependencies.The public class lacks XML documentation describing its purpose and responsibilities. Additionally, constructor parameters should be validated.
+/// <summary> +/// Implements S3-based content storage functionality for managing files in an S3 bucket. +/// </summary> public class S3ContentStorage([FromKeyedServices("GenericS3:container")] IObjectClient s3Client, IOptions<StorageOptions> options) : IContentStorage { + public S3ContentStorage([FromKeyedServices("GenericS3:container")] IObjectClient s3Client, IOptions<StorageOptions> options) + { + this.s3Client = s3Client ?? throw new ArgumentNullException(nameof(s3Client)); + this.options = options ?? throw new ArgumentNullException(nameof(options)); + }
6-31: Add resilience and observability features.Consider implementing the following architectural improvements:
- Add retry policy for transient failures
- Add logging for operations
- Validate bucket existence during initialization
public class S3ContentStorage([FromKeyedServices("GenericS3:container")] IObjectClient s3Client, IOptions<StorageOptions> options) : IContentStorage { + private readonly ILogger<S3ContentStorage> _logger; + private readonly AsyncRetryPolicy _retryPolicy; + + public S3ContentStorage( + [FromKeyedServices("GenericS3:container")] IObjectClient s3Client, + IOptions<StorageOptions> options, + ILogger<S3ContentStorage> logger) + { + _logger = logger; + _retryPolicy = Policy + .Handle<InvalidOperationException>() + .WaitAndRetryAsync(3, retryAttempt => + TimeSpan.FromSeconds(Math.Pow(2, retryAttempt))); + + // Validate bucket exists + var bucketExists = await s3Client.GetBucketExistsAsync(options.Value.BucketName); + if (!bucketExists.IsSuccess) + throw new InvalidOperationException($"Bucket {options.Value.BucketName} does not exist"); + }Consider wrapping operations with retry policy and logging:
public async ValueTask UploadFile(StorageNameSpace block, AssetId assetId, Stream data) { + _logger.LogInformation("Uploading file {AssetId} to {Block}", assetId, block); + await _retryPolicy.ExecuteAsync(async () => { // ... existing upload code ... + }); + _logger.LogInformation("Successfully uploaded file {AssetId} to {Block}", assetId, block); }src/Argon.Api/Features/MediaStorage/IContentStorage.cs (2)
3-15: Add XML documentation for interface membersThe interface methods lack XML documentation, which is essential for maintaining API clarity and enabling better IntelliSense support.
Apply this diff:
public interface IContentStorage { + /// <summary> + /// Retrieves current storage statistics. + /// </summary> + /// <returns>Storage space information.</returns> ValueTask<StorageSpace> GetStorageStats(); + /// <summary> + /// Uploads a file to the specified storage namespace. + /// </summary> + /// <param name="block">Storage namespace information.</param> + /// <param name="assetId">Unique identifier for the asset.</param> + /// <param name="data">File content stream.</param> ValueTask UploadFile(StorageNameSpace block, AssetId assetId, Stream data); + /// <summary> + /// Deletes a file from the specified storage namespace. + /// </summary> + /// <param name="block">Storage namespace information.</param> + /// <param name="assetId">Unique identifier for the asset to delete.</param> ValueTask DeleteFile(StorageNameSpace block, AssetId assetId);
12-14: Consider moving storage keys to a separate constants classStorage-related constants should be moved to a dedicated class for better organization and maintainability.
Consider creating a new class:
public static class StorageConstants { public const string GenericS3StorageKey = "cdn:bucket:s3"; public const string InMemoryStorageKey = "cdn:bucket:inmemory"; public const string DiskContentStorageKey = "cdn:bucket:disk"; }src/Argon.Contracts/IUserAuthorization.cs (1)
19-26: Add XML documentation for the new record and its properties.Since this record is part of the public contract and is used for user profile updates including CDN-stored avatars, it should be well-documented.
Apply this diff to add documentation:
[MemoryPackable, Serializable, GenerateSerializer, Alias(nameof(UserEditInput))] +/// <summary> +/// Represents the input model for updating user profile information. +/// </summary> public sealed partial record UserEditInput( + /// <summary> + /// Gets the new username. Can be null if not being updated. + /// </summary> [field: Id(0)] string? Username, + /// <summary> + /// Gets the new display name. Can be null if not being updated. + /// </summary> [field: Id(1)] string? DisplayName, + /// <summary> + /// Gets the CDN identifier for the user's avatar image. Can be null if not being updated. + /// </summary> [field: Id(2)] string? AvatarId);src/Argon.Api/Features/Jwt/JwtFeature.cs (1)
Line range hint
14-24: Consider token lifetime and clock skew settingsWhile the token validation setup is generally good, there are two considerations:
- Setting
ClockSkew = TimeSpan.Zeromight be too strict for distributed systems. Consider a small tolerance (e.g., 2 minutes) to account for time differences between servers.- Token lifetime constraints are not explicitly set. Consider adding
TokenDecryptionKeyfor additional security andRequireExpirationTime = trueto enforce token expiration.var tokenValidator = new TokenValidationParameters { ValidIssuer = jwt.Issuer, ValidAudience = jwt.Audience, IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(jwt.Key)), ValidateIssuer = true, ValidateAudience = true, ValidateLifetime = true, ValidateIssuerSigningKey = true, - ClockSkew = TimeSpan.Zero + ClockSkew = TimeSpan.FromMinutes(2), + RequireExpirationTime = true };src/Argon.Api/Services/UserInteraction.cs (1)
Line range hint
1-33: Consider documenting the architectural changes.Given that this is part of a CDN-related PR, it would be helpful to document how these user grain changes relate to the broader CDN architecture. Consider adding comments or documentation explaining:
- The relationship between user management and CDN functionality
- The rationale behind the interface rename from
IUserManagertoIUserGrain- Any new responsibilities of the
IUserGraininterface related to CDNsrc/Argon.Api/Grains/UserGrain.cs (1)
Line range hint
10-65: Consider architectural improvements for robustness and performanceSeveral architectural improvements could enhance this implementation:
Error Handling:
- Consider implementing a custom exception type for user-related errors
- Add proper exception handling for concurrent updates
Performance:
- The
Get()method eagerly loads all relations including channels- Consider implementing selective loading based on the operation needs
Concurrency:
- Add optimistic concurrency control using a version/timestamp field
- Implement retry logic for concurrent updates
Would you like me to provide a detailed implementation for any of these improvements?
src/Argon.Api/Features/MediaStorage/AssetId.cs (4)
5-6: Add XML documentation to explain the file ID format.The method generates a unique identifier with a specific format. Consider adding XML documentation to explain:
- The format structure and each component
- The purpose of the reserved "00" suffix
- Why hex format is used for scope and kind
+ /// <summary> + /// Generates a unique file identifier in the format: "{guid}-{scope:X2}-{kind:X2}-00" + /// </summary> + /// <returns>A string containing the formatted file identifier with reserved suffix</returns> public string ToFileId()
8-17: Consider refactoring path generation for better maintainability.The current implementation has several areas for improvement:
- Multiple if statements could be more concise using a switch expression
- Path segments could be configuration-driven for flexibility
- Default case falls through to "temp" without explicit scope validation
Consider this more maintainable approach:
public string GetFilePath() - { - if (scope == AssetScope.ProfileAsset) - return $"profile/{assetId.ToString().Substring(0, 8)}/{ToFileId()}"; - if (scope == AssetScope.ChatAsset) - return $"chat/{assetId.ToString().Substring(0, 8)}/{ToFileId()}"; - if (scope == AssetScope.ServiceAsset) - return $"service/{assetId.ToString().Substring(0, 8)}/{ToFileId()}"; - return $"temp/{ToFileId()}"; - } + { + var prefix = scope switch + { + AssetScope.ProfileAsset => "profile", + AssetScope.ChatAsset => "chat", + AssetScope.ServiceAsset => "service", + _ => "temp" + }; + var subDir = scope != AssetScope.Unknown ? $"/{assetId:N8}" : ""; + return $"{prefix}{subDir}/{ToFileId()}"; + }
31-41: Optimize tag generation and improve type safety.The string interpolation for enum values is unnecessary and could be replaced with direct ToString calls.
public Dictionary<string, string> GetTags(StorageNameSpace @namespace) { var tags = new Dictionary<string, string> { - { nameof(AssetScope), $"{scope}" }, - { nameof(AssetKind), $"{kind}" }, - { $"Id", $"{assetId}" }, - { $"Namespace", $"{@namespace.path}:{@namespace.id}" } + { nameof(AssetScope), scope.ToString() }, + { nameof(AssetKind), kind.ToString() }, + { "Id", assetId.ToString() }, + { "Namespace", $"{@namespace.path}:{@namespace.id}" } }; return tags; }
57-73: Add XML documentation for enums and their values.The enums would benefit from proper XML documentation, especially to clarify:
- The purpose and usage of each scope
- The specific formats supported by each kind (e.g., PNG for Video, GIF for VideoNoSound)
+ /// <summary> + /// Defines the scope or category of an asset in the system. + /// </summary> public enum AssetScope : byte { + /// <summary> + /// Asset belongs to a user profile. + /// </summary> ProfileAsset, + /// <summary> + /// Asset belongs to a chat conversation. + /// </summary> ChatAsset, + /// <summary> + /// Asset belongs to a service. + /// </summary> ServiceAsset } + /// <summary> + /// Defines the type and format of an asset. + /// </summary> public enum AssetKind : byte { + /// <summary> + /// Image asset in PNG format. + /// </summary> Image, - Video, // only png - VideoNoSound, // gif + /// <summary> + /// Video asset in PNG format with sound. + /// </summary> + Video, + /// <summary> + /// Video asset in GIF format without sound. + /// </summary> + VideoNoSound, // ... (continue for other values)src/Argon.Api/Argon.Api.csproj (1)
16-17: Document CDN configuration in READMESince this is introducing CDN functionality with S3 storage support, it would be helpful to document the setup and configuration.
Consider adding the following to your README or documentation:
- Required S3 configuration settings
- Example configuration for different environments
- Usage guidelines for the CDN features
- Any limitations or considerations when using S3 storage
Would you like me to help create this documentation?
src/Argon.Api/Grains/ServerGrain.cs (2)
50-52: Consider enhancing the update logic and event emissionWhile the null-coalescing operator usage is good for preserving existing values, there are several potential improvements:
- The event payload currently includes all input properties regardless of whether they changed. Consider tracking and emitting only the actually modified properties.
- There's no validation of input values beyond null checks (e.g., name length, URL format).
Here's a suggested improvement:
+ var changes = new PropertyBag(); + if (input.Name != null && input.Name != server.Name) + { + server.Name = input.Name; + changes.Set("name", input.Name); + } + if (input.Description != null && input.Description != server.Description) + { + server.Description = input.Description; + changes.Set("description", input.Description); + } + if (input.AvatarUrl != null && input.AvatarUrl != server.AvatarUrl) + { + server.AvatarUrl = input.AvatarUrl; + changes.Set("avatarUrl", input.AvatarUrl); + } - server.Name = input.Name ?? server.Name; - server.Description = input.Description ?? server.Description; - server.AvatarUrl = input.AvatarUrl ?? server.AvatarUrl; context.Servers.Update(server); await context.SaveChangesAsync(); - await _serverEvents.Fire(new ServerModified(PropertyBag.Empty - .Set("name", input.Name) - .Set("description", input.Description) - .Set("avatarUrl", input.AvatarUrl) - )); + if (!changes.IsEmpty) + await _serverEvents.Fire(new ServerModified(changes));
Line range hint
1-140: Consider architectural improvements for better separation of concernsThe current implementation mixes several responsibilities within the grain:
- Direct Entity Framework usage
- Business logic
- Event handling
- Mapping
Consider implementing:
- Repository pattern to abstract persistence
- Domain service layer for business logic
- Unit of work pattern for transaction management
- Event aggregation for better consistency
Example repository abstraction:
public interface IServerRepository { Task<Server> GetById(Guid id); Task Update(Server server); Task Create(Server server); Task Delete(Guid id); }This would allow the grain to focus on orchestration and state management, which are its primary responsibilities in the Orleans architecture.
src/Argon.Contracts/etc/Result.cs (1)
107-107: Consider performance implications of EqualityComparerThe use of
EqualityComparer<TResult>.Default.Equalsis more computationally expensive than a null check. While this provides more consistent behavior, it could impact performance in hot paths.Consider adding a specialized fast path for reference types:
- public bool HasValue => !EqualityComparer<TResult>.Default.Equals(_value, default); + public bool HasValue => typeof(TResult).IsValueType + ? !EqualityComparer<TResult>.Default.Equals(_value, default) + : _value is not null;src/Argon.Api/Features/Orleanse/MemoryPackCodec.cs (1)
166-166: Consider streaming serialization for large CDN contentFor CDN scenarios where you might handle large media files, consider implementing streaming serialization to avoid loading entire files into memory. This could be achieved by:
- Implementing a custom StreamingBufferWriter
- Adding chunked serialization support
- Setting appropriate buffer size limits
This would help prevent potential memory pressure when handling large files through the CDN.
Would you like me to provide an example implementation of streaming serialization?
src/Argon.Api/Features/MediaStorage/YandexContentDeliveryNetwork.cs (3)
46-50: Use URL-safe Base64 encodingInstead of manually replacing characters in the Base64 string to make it URL-safe, use built-in methods that handle URL-safe encoding.
Apply this diff to simplify the encoding:
-var token = Convert.ToBase64String(hashBytes) - .Replace("\n", "") - .Replace("+", "-") - .Replace("/", "_") - .Replace("=", ""); +var token = WebEncoders.Base64UrlEncode(hashBytes);Ensure you have the appropriate
usingdirective:+using Microsoft.AspNetCore.WebUtilities;
28-29: Provide a meaningful implementation or exception messageThe
ReplaceAssetAsyncmethod currently throws aNotImplementedException. Consider implementing the method or, if not applicable, provide a more descriptive exception or remove the method altogether to prevent runtime errors.
14-26: Enhance exception handling for specific errorsCatching the general
Exceptionclass can make debugging difficult. Consider catching specific exceptions that might be thrown byStorage.UploadFile, such asIOExceptionor a custom storage exception, to provide clearer error handling.src/Argon.Entry/Program.cs (3)
18-25: Review theKeepAliveTimeoutsetting in Kestrel configurationThe
KeepAliveTimeoutis set to 400 seconds, which is significantly higher than the default value (120 seconds). This may lead to connections remaining open longer than necessary, potentially causing resource exhaustion under high load. Consider reviewing this value to ensure it aligns with your application's performance and scalability requirements.
30-32: Consider usingSystem.Text.Jsoninstead ofNewtonsoft.Jsonfor JSON serializationUnless there is a specific need for features provided by
Newtonsoft.Json, you might consider usingSystem.Text.Jsonfor JSON serialization. It offers better performance and is now the default in ASP.NET Core.
42-45: ExternalizeClusterOptionsconfiguration toappsettings.jsonThe
ClusterIdandServiceIdare hard-coded within the code. To enhance maintainability and flexibility, consider moving these configurations to theappsettings.jsonfile or environment variables. This allows for easier updates without code changes and supports different configurations for various environments.src/Argon.Api/Controllers/FilesController.cs (3)
29-30: Use a more appropriate status code for unsupported operationsReturning a
BadRequest()when the storage kind isGenericS3may not accurately reflect the issue. Consider returningStatusCode(501)(Not Implemented) orStatusCode(405)(Method Not Allowed) to indicate that this operation is not supported with the current storage configuration.Apply this diff to update the status code:
- return BadRequest(); + return StatusCode(501);
42-42: Reminder: Address the TODO commentThere's a
// TODOcomment indicating incomplete implementation. Please ensure that all necessary code is added to fully implement this method.Would you like assistance in completing this implementation or creating a GitHub issue to track this task?
44-44: UseUnauthorized()method for clarityInstead of
return StatusCode(401);, you can use the built-inUnauthorized()method to improve readability and maintain consistency with ASP.NET Core conventions.Apply this diff to enhance the response:
- return StatusCode(401); + return Unauthorized();src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs (3)
24-24: Provide a descriptive exception message for unsupported storage kindWhen throwing an
InvalidOperationException, it's helpful to include a descriptive error message to aid in debugging and improve code readability.Consider modifying the code as follows:
- StorageKind.InMemory => throw new InvalidOperationException(), + StorageKind.InMemory => throw new InvalidOperationException("In-memory storage is not supported for CDN."),
27-27: Include parameter details when throwingArgumentOutOfRangeExceptionProviding the parameter name and its invalid value in the exception enhances clarity and facilitates easier debugging.
Update the code to include parameter information:
- _ => throw new ArgumentOutOfRangeException() + _ => throw new ArgumentOutOfRangeException(nameof(bucketOptions.Kind), bucketOptions.Kind, "Unsupported storage kind.")
34-45: Simplify conditional logic usingelse ifor aswitchstatementCurrently, the
ifstatements check mutually exclusive conditions withoutelse, which could lead to unintended behavior if more storage kinds are added in the future. To enhance code clarity and ensure mutual exclusivity, consider usingelse ifor aswitchstatement.Refactored using
else if:Alternatively, using a
switchstatement:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (29)
.gitignore(1 hunks)src/Argon.Api/Argon.Api.csproj(1 hunks)src/Argon.Api/Controllers/FilesController.cs(1 hunks)src/Argon.Api/Controllers/MetadataController.cs(1 hunks)src/Argon.Api/Extensions/HttpContextExtensions.cs(1 hunks)src/Argon.Api/Features/Jwt/JwtFeature.cs(3 hunks)src/Argon.Api/Features/MediaStorage/AssetId.cs(1 hunks)src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs(1 hunks)src/Argon.Api/Features/MediaStorage/CdnOptions.cs(1 hunks)src/Argon.Api/Features/MediaStorage/DiskContentDeliveryNetwork.cs(1 hunks)src/Argon.Api/Features/MediaStorage/IContentDeliveryNetwork.cs(1 hunks)src/Argon.Api/Features/MediaStorage/IContentStorage.cs(1 hunks)src/Argon.Api/Features/MediaStorage/Storages/DiskContentStorage.cs(1 hunks)src/Argon.Api/Features/MediaStorage/Storages/S3ContentStorage.cs(1 hunks)src/Argon.Api/Features/MediaStorage/YandexContentDeliveryNetwork.cs(1 hunks)src/Argon.Api/Features/Orleanse/MemoryPackCodec.cs(1 hunks)src/Argon.Api/Features/Pex/IPermissionProvider.cs(1 hunks)src/Argon.Api/Grains.Interfaces/IServerGrain.cs(1 hunks)src/Argon.Api/Grains.Interfaces/IUserGrain.cs(1 hunks)src/Argon.Api/Grains/ServerGrain.cs(1 hunks)src/Argon.Api/Grains/SessionManager.cs(1 hunks)src/Argon.Api/Grains/UserGrain.cs(2 hunks)src/Argon.Api/Program.cs(2 hunks)src/Argon.Api/Services/UserInteraction.cs(2 hunks)src/Argon.Api/appsettings.json(1 hunks)src/Argon.Contracts/IUserAuthorization.cs(1 hunks)src/Argon.Contracts/etc/Result.cs(1 hunks)src/Argon.Entry/Program.cs(1 hunks)src/Argon.Entry/appsettings.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🔇 Additional comments (38)
src/Argon.Api/Grains.Interfaces/IUserGrain.cs (2)
6-7: LGTM! Proper Orleans grain interface setup.
The interface correctly follows the Orleans grain pattern with proper inheritance and alias attribute.
13-13: Verify the UpdateUser method parameter type change impact.
The change from UserCredentialsInput to UserEditInput aligns with the CDN functionality for avatar management. However, we should verify that all existing callers have been updated.
✅ Verification successful
Based on the results, I can generate the final response as we have comprehensive information about the UpdateUser usage across the codebase.
All UpdateUser method calls have been properly updated to use UserEditInput
The verification shows that:
- No instances of the old
UserCredentialsInputtype remain withUpdateUser - All three occurrences of
UpdateUser(in FilesController, UserGrain, and IUserGrain) use the newUserEditInputtype - The implementation in UserGrain matches the interface definition in IUserGrain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of UpdateUser to ensure they've been updated to use UserEditInput
# Search for UpdateUser method calls
echo "Searching for UpdateUser method calls:"
rg -l "UpdateUser\("
# Search for UserCredentialsInput (old type) usage with UpdateUser
echo -e "\nChecking for any remaining old UserCredentialsInput usage with UpdateUser:"
rg "UpdateUser.*UserCredentialsInput"
# Search for UserEditInput (new type) usage with UpdateUser
echo -e "\nVerifying new UserEditInput usage with UpdateUser:"
rg "UpdateUser.*UserEditInput"
Length of output: 903
src/Argon.Api/Features/MediaStorage/CdnOptions.cs (1)
9-9: Verify StorageOptions type definition.
The StorageOptions type is referenced but not defined in the provided code.
✅ Verification successful
Let me check the content of these files to locate the StorageOptions definition.
StorageOptions type is properly defined in the codebase
The StorageOptions class is defined in src/Argon.Api/Features/MediaStorage/IContentStorage.cs with the following properties:
- Kind
- BaseUrl
- Login
- Region
The type is correctly referenced and used throughout the codebase, particularly in CDN and storage-related features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for StorageOptions definition
ast-grep --pattern 'class StorageOptions {
$$$
}'
# Backup search using ripgrep
rg -l "StorageOptions"
Length of output: 420
Script:
#!/bin/bash
# Check content of files for StorageOptions definition
rg "StorageOptions" -A 5 -B 5
Length of output: 13806
src/Argon.Api/Grains/SessionManager.cs (1)
11-11: LGTM! Logger type update is consistent with refactoring
The change from ILogger<UserManager> to ILogger<UserGrain> aligns with the broader refactoring where UserManager was renamed to UserGrain.
src/Argon.Api/Controllers/MetadataController.cs (2)
3-3: LGTM: Required import for HttpContext.GetRegion() extension method
23-24: Verify removal of CDN addresses impact
The removal of CDN addresses from HeadRoutingConfig might affect existing clients. Please ensure all consumers are updated to handle the new structure.
✅ Verification successful
Let me gather more context about the HeadRoutingConfig usage and structure.
No breaking changes from CDN addresses removal
The verification shows that:
- The
HeadRoutingConfigrecord is only used in one place - theMetadataController.GetHead()endpoint - There are no references to
cdnAddressesin the codebase - The current implementation already uses the new structure with
masterEndpoint,webRtcEndpoint,features, andcurrentRegionCode
The changes appear to be already fully implemented and there are no remaining references to the old structure that need updating.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to cdnAddresses
rg -l "cdnAddresses" --type cs
# Search for usages of HeadRoutingConfig to identify potential breaking changes
ast-grep --pattern 'new HeadRoutingConfig($$$)'
Length of output: 986
Script:
#!/bin/bash
# Search for HeadRoutingConfig class definition
ast-grep --pattern 'record HeadRoutingConfig'
# Search for any tests or configurations using HeadRoutingConfig
rg -l "HeadRoutingConfig" --type cs
# Get more context around the MetadataController usage
rg -C 5 "HeadRoutingConfig" src/Argon.Api/Controllers/MetadataController.cs
Length of output: 7018
src/Argon.Api/Features/MediaStorage/IContentDeliveryNetwork.cs (1)
5-7: LGTM! Well-designed interface property.
The read-only Storage property follows good design practices by providing access to the underlying storage while preventing external modifications.
src/Argon.Entry/appsettings.json (1)
21-26:
Verify CAPTCHA configuration and document NO_CAPTCHA mode
The empty CAPTCHA credentials with "Kind": "NO_CAPTCHA" suggests a disabled state. This could pose security risks in production.
Run this script to check if there are any documentation or examples about the CAPTCHA configuration:
✅ Verification successful
NO_CAPTCHA mode is a valid configuration with proper implementation
The codebase shows a well-structured CAPTCHA system with explicit support for NO_CAPTCHA mode:
NO_CAPTCHAis a legitimate enum value inCaptchaKind.cs- When NO_CAPTCHA is selected, the system uses
NullCaptchaimplementation that always returns true - The system also supports CLOUDFLARE and YANDEX CAPTCHA providers for production use
This is a common pattern for development/testing environments and not a security risk as long as proper CAPTCHA configuration is used in production.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CAPTCHA documentation or examples
rg -i "captcha|no_captcha" --type md
fd -e md -x grep -l -i "captcha"
Length of output: 71
Script:
#!/bin/bash
# Search for CAPTCHA configuration and usage in code files
rg -i "captcha" -A 5 -B 5
# Search specifically for NO_CAPTCHA references
rg -i "no_captcha" -A 5 -B 5
# Look for CAPTCHA-related configuration classes or enums
ast-grep --pattern 'enum $_ {
$$$
NO_CAPTCHA
$$$
}'
Length of output: 12455
src/Argon.Api/Grains.Interfaces/IServerGrain.cs (2)
Line range hint 8-9: Verify validation in CreateServer implementation
Since CreateServer accepts ServerInput, ensure the implementation validates the name property before creating a server.
Let's check the implementation:
#!/bin/bash
# Description: Look for CreateServer implementation and its validation
# Expected: Find validation logic for server name
# Find the ServerGrain implementation
ast-grep --pattern 'class $_ {
$$$
Task<ServerDto> CreateServer(ServerInput input, Guid creatorId)
{
$$$
}
$$$
}'30-30:
Reconsider making the Name property nullable
Making the server name nullable could lead to data consistency issues since server names are typically required for identification and display purposes. This change might also impact existing code that assumes server names are always present.
Let's verify how server names are used across the codebase:
Consider either:
- Keeping the property non-nullable and enforcing name requirement, or
- Adding validation in the grain implementation to ensure names are provided when required
src/Argon.Api/Extensions/HttpContextExtensions.cs (1)
15-18: 🛠️ Refactor suggestion
Add validation for CF-Ray header value
The method currently accepts any value from the CF-Ray header without validation. Consider adding format validation to ensure the header contains a valid Cloudflare Ray ID.
Let's check if there are any existing validation utilities in the codebase:
public static string GetRay(this HttpContext ctx)
- => ctx.Request.Headers.ContainsKey("CF-Ray")
- ? ctx.Request.Headers["CF-Ray"].ToString()
- : $"LOCAL-{Guid.NewGuid()}";
+ {
+ if (!ctx.Request.Headers.ContainsKey("CF-Ray"))
+ return $"LOCAL-{Guid.NewGuid()}";
+
+ var rayId = ctx.Request.Headers["CF-Ray"].ToString();
+ // Cloudflare Ray IDs typically follow the format: 16 hex chars followed by hyphen and location
+ if (!System.Text.RegularExpressions.Regex.IsMatch(rayId, @"^[a-f0-9]{16}-[A-Z]{3}$"))
+ return $"INVALID-{Guid.NewGuid()}";
+
+ return rayId;
+ }src/Argon.Api/Features/MediaStorage/DiskContentDeliveryNetwork.cs (1)
26-27: 🛠️ Refactor suggestion
Improve URL generation with proper encoding and configurable caching
The current implementation has several potential issues:
- No URL encoding for namespace and asset path
- Hardcoded cache-busting parameter
- Hardcoded "/files" prefix
Consider this improved implementation:
-public ValueTask<string> GenerateAssetUrl(StorageNameSpace ns, AssetId asset)
- => new($"/files/{ns.ToPath()}/{asset.GetFilePath()}?nocache=1");
+public ValueTask<string> GenerateAssetUrl(StorageNameSpace ns, AssetId asset)
+{
+ var encodedNs = Uri.EscapeDataString(ns.ToPath());
+ var encodedPath = Uri.EscapeDataString(asset.GetFilePath());
+ // Consider injecting these values through configuration
+ const string basePrefix = "/files";
+ var timestamp = DateTimeOffset.UtcNow.ToUnixTimeSeconds();
+ return new($"{basePrefix}/{encodedNs}/{encodedPath}?v={timestamp}");
+}Let's verify if there are any special characters in existing namespaces or asset paths:
src/Argon.Api/Features/MediaStorage/IContentStorage.cs (2)
27-32: LGTM!
The enum is well-defined and follows C# naming conventions.
5-5: Verify referenced types exist in the codebase
The interface references StorageSpace type which isn't defined in this file.
✅ Verification successful
StorageSpace type is properly defined and used in the codebase
The StorageSpace type is defined as a readonly record struct in src/Argon.Api/Features/MediaStorage/CdnOptions.cs and is correctly used in the interface and its implementations:
- Interface:
IContentStorage.csdeclares the method - Implementation in
S3ContentStorage.csreturns the type - Implementation in
DiskContentStorage.csreturns the type
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for StorageSpace type definition
echo "Searching for StorageSpace type definition..."
ast-grep --pattern 'class StorageSpace' || ast-grep --pattern 'struct StorageSpace' || ast-grep --pattern 'record StorageSpace'
# Search for any usages of StorageSpace
echo "Searching for StorageSpace usages..."
rg "StorageSpace" --type csharp
Length of output: 13543
src/Argon.Contracts/IUserAuthorization.cs (2)
26-26: Consider adding validation for the AvatarId format.
Since AvatarId is likely a CDN resource identifier, it would be beneficial to ensure it follows the expected format.
Let's verify if there are any existing CDN ID format validations in the codebase:
Consider:
- Adding validation attributes if this is a REST API input model
- Implementing format validation in the business logic
- Documenting the expected format in XML comments
19-26: Verify the impact on existing user update flows.
The introduction of this new input model suggests changes in user update handling.
Let's verify the integration points:
src/Argon.Api/Features/Jwt/JwtFeature.cs (3)
3-3: LGTM!
The addition of the JWT Bearer authentication namespace is appropriate for the implementation.
26-28: LGTM!
The service registration is correctly implemented with appropriate lifetime scopes.
Line range hint 1-38: Verify JWT configuration aligns with CDN requirements
Since this JWT implementation is part of a CDN feature:
- Ensure the token lifetime aligns with your CDN caching strategy
- Verify that JWT tokens are not logged or cached by the CDN
- Consider implementing token scope validation for CDN resources
src/Argon.Api/Services/UserInteraction.cs (2)
31-31: LGTM! The grain type update is consistent.
The change from IUserManager to IUserGrain maintains consistency with the interface rename while preserving the existing functionality.
14-14: LGTM! Verify all IUserManager references are updated.
The grain type change from IUserManager to IUserGrain is consistent with the interface rename.
Let's verify all references have been updated:
✅ Verification successful
All references to IUserManager have been replaced with IUserGrain
The verification confirms that all references to IUserManager have been removed from the codebase, and IUserGrain is consistently used across the relevant files:
src/Argon.Api/Services/UserInteraction.cssrc/Argon.Api/Grains/SessionManager.cssrc/Argon.Api/Grains/UserGrain.cssrc/Argon.Api/Grains.Interfaces/IUserGrain.cssrc/Argon.Api/Controllers/FilesController.cs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to IUserManager
# This helps ensure the interface rename was complete
# Search for any remaining IUserManager references
rg "IUserManager" --type csharp
# Search for all IUserGrain references to confirm the new usage
rg "IUserGrain" --type csharp
Length of output: 937
src/Argon.Api/appsettings.json (2)
60-61: Validate CDN base URL configuration
The empty BaseUrl setting might cause issues in production. Additionally, verify if the EntryExpire duration aligns with your caching requirements.
Let's check how the BaseUrl is used in the codebase:
#!/bin/bash
# Search for BaseUrl usage in CDN-related code
rg -l "BaseUrl" | xargs ast-grep --pattern '$_.$_BaseUrl'53-54: Verify CDN storage configuration consistency
The storage configuration shows mixed settings:
Kindis set to "Disk" (local storage)Regionsuggests cloud storage (AWS S3)
Let's verify the supported storage kinds:
src/Argon.Api/Grains/UserGrain.cs (2)
10-10: LGTM! Clean grain implementation with proper dependency injection.
The class follows the Orleans grain pattern correctly and uses modern C# features effectively.
10-10: Verify breaking changes from interface modifications
The change from IUserManager to IUserGrain and updates to method signatures could impact existing consumers.
src/Argon.Api/Program.cs (3)
12-12: LGTM: Using directives properly organized
The new using directives for MediaStorage and Pex features are correctly placed with other feature imports.
Also applies to: 14-14
37-41: LGTM: Well-structured Fusion configuration
The Fusion service configuration is properly formatted with improved readability. The RPC servers are registered in a clear and maintainable way.
44-44: Verify middleware registration order
The new feature registrations look good, but let's verify the dependency order:
- CDN is correctly placed within the non-managed environment block
- Permissions are globally available as expected
Also applies to: 46-46
✅ Verification successful
Middleware registration order is correct
The CDN registration is properly ordered within the non-managed environment block:
- It's registered after authentication/authorization setup
- It depends on
IContentStoragewhich is configured during registration - No other middleware components depend on it for initialization
- Permissions are correctly placed outside the managed block as they are required globally
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any dependency requirements in the CDN and Permissions feature implementations
# Look for service dependencies in CDN implementation
rg -l "IServiceCollection.*AddContentDeliveryNetwork" | xargs rg "services\..*Add.*" -A 5
# Look for service dependencies in Permissions implementation
rg -l "IServiceCollection.*AddArgonPermissions" | xargs rg "services\..*Add.*" -A 5
# Check if these features are referenced by other middleware
rg "UseContentDeliveryNetwork|UseArgonPermissions"
Length of output: 230
Script:
#!/bin/bash
# Let's try a different approach to find the service registration and middleware usage
# Search for CDN related code
rg -A 5 -B 5 "AddContentDeliveryNetwork|ContentDeliveryNetwork"
# Search for any middleware registration patterns in Program.cs
rg -A 3 -B 3 "app\.Use|builder\.Services\.Add" src/Argon.Api/Program.cs
# Look for any extension method definitions related to CDN
ast-grep --pattern 'public static $_ AddContentDeliveryNetwork($_) { $$$ }'
Length of output: 10952
src/Argon.Api/Features/MediaStorage/AssetId.cs (2)
3-4: LGTM! Well-structured immutable type declaration.
Good use of readonly struct for an immutable value type representing asset identifiers. The primary constructor with strongly-typed parameters ensures type safety.
53-54: LGTM! Well-designed factory methods.
Good use of static factory methods for common asset creation scenarios. The methods are concise and their intent is clear.
src/Argon.Api/Argon.Api.csproj (1)
16-17: Verify SimpleS3 package versions and security
The addition of S3 packages aligns with the CDN implementation objective. Let's verify the package versions and security.
✅ Verification successful
Let me try a different approach to verify the package versions using the .NET project file and check for any potential issues.
Let me check the implementation details of the S3 storage to ensure proper integration of these packages.
SimpleS3 packages are properly integrated and used securely
The verification shows that:
- The packages are correctly integrated into the S3 storage implementation
- The code follows security best practices by using dependency injection and configuration abstraction
- The implementation properly handles authentication using
StringAccessKey - The packages are used in a focused manner, only in relevant CDN-related files
- The target framework (.NET 8.0) is compatible with the package versions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check latest versions and security advisories for SimpleS3 packages
# Check NuGet for latest versions of both packages
curl -s https://api.nuget.org/v3-flatcontainer/Genbox.SimpleS3.Core/index.json | jq -r '.versions[]' | sort -V | tail -n 1
curl -s https://api.nuget.org/v3-flatcontainer/Genbox.SimpleS3.Extensions.GenericS3/index.json | jq -r '.versions[]' | sort -V | tail -n 1
# Check GitHub for any security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NUGET, package: "Genbox.SimpleS3.Core") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 735
Script:
#!/bin/bash
# Check the target framework version and other dependencies
cat src/Argon.Api/Argon.Api.csproj
# Search for any S3-related code implementations
fd -e cs -x grep -l "SimpleS3" {}
Length of output: 3904
Script:
#!/bin/bash
# Check the S3 storage implementation
cat "./src/Argon.Api/Features/MediaStorage/Storages/S3ContentStorage.cs"
cat "./src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs"
# Check if there are any other files referencing these S3 packages
rg -l "Genbox.SimpleS3" --type cs
Length of output: 4502
src/Argon.Contracts/etc/Result.cs (2)
107-107: Verify CDN integration points
Since this PR implements CDN functionality, ensure this change aligns with CDN-related requirements, particularly for handling optional media content or configuration values.
#!/bin/bash
# Look for CDN-related code using Maybe<T>
rg -l "CDN|Content.*Delivery|Media|File" | xargs rg "Maybe<.*>"
# Check for potential configuration handling
rg -l "config|settings" -g "*.cs" | xargs rg "Maybe<.*>"107-107:
The change from null checking to equality comparison with default values represents a breaking change that could affect existing code, particularly for value types and custom types with specialized equality comparisons.
Let's verify the impact on existing usage:
Consider adding XML documentation to clarify the new behavior:
+ /// <summary>
+ /// Determines if this Maybe instance contains a value.
+ /// A value is considered present if it is not equal to the default value of type TResult.
+ /// </summary>
public bool HasValue => !EqualityComparer<TResult>.Default.Equals(_value, default);✅ Verification successful
Change is safe but needs documentation
The verification shows that the HasValue property is used safely in the codebase:
- All
Maybe<T>usages are with reference types (UploadError,RegistrationError,RealtimeToken) - The pattern of usage consistently checks
HasValuebefore accessing values None()is used to represent the absence of a value, which aligns with the new implementation
The change from null-check to EqualityComparer is actually beneficial as it provides more consistent behavior across all types. However, the change should be documented for maintainability.
+ /// <summary>
+ /// Determines if this Maybe instance contains a value by comparing it with the default value of TResult.
+ /// For reference types, this is equivalent to a null check. For value types, it compares with default(TResult).
+ /// </summary>
public bool HasValue => !EqualityComparer<TResult>.Default.Equals(_value, default);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Maybe<T> usage patterns that might be affected by the HasValue change
rg -l "Maybe<.*>" | xargs rg "HasValue" -B 2 -A 2
# Look for potential value type usage that might be affected
ast-grep --pattern 'Maybe<$_>'
Length of output: 2068
Script:
#!/bin/bash
# Let's search for actual HasValue usage patterns
rg "HasValue" -B 2 -A 2
# Look for Maybe<T> instantiations with value types
ast-grep --pattern 'new Maybe<$_>($$$)'
# Search for Maybe.None() usage to understand default value patterns
rg "Maybe.*None\(\)" -B 2 -A 2
Length of output: 4298
src/Argon.Api/Features/Orleanse/MemoryPackCodec.cs (1)
166-166: Verify CDN content serialization compatibility
Let's verify that this serialization change doesn't affect existing CDN content handling.
✅ Verification successful
No impact on CDN functionality
The MemoryPack serialization is only used for Orleans grain storage serialization and is completely separate from the CDN implementation. The CDN components (YandexContentDeliveryNetwork and DiskContentDeliveryNetwork) handle raw file streams and URL generation without any custom serialization.
- MemoryPackCodec and MemoryPackSerializer are exclusively used for Orleans persistence
- CDN implementations work directly with Stream objects for file uploads
- No serialization is involved in CDN asset handling or URL generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CDN-related usages of MemoryPackCodec
# Find CDN-related files that might use this codec
echo "Searching for CDN-related files..."
rg -l "CDN|Content.*Delivery|Storage" --type cs
# Find direct usage of MemoryPackCodec in those files
echo "Checking MemoryPackCodec usage in CDN context..."
rg -l "MemoryPackCodec|MemoryPackSerializer" --type cs
# Check for any serialization-related configuration in appsettings
echo "Checking serialization configuration..."
fd -e json -x cat {} \; | jq 'select(..|.serialization? != null)'
Length of output: 2211
Script:
#!/bin/bash
# Let's analyze the relationship between CDN and MemoryPack serialization
# Check content of MemoryPackStorageSerializer.cs as it appears in both CDN and MemoryPack searches
echo "Checking MemoryPackStorageSerializer implementation..."
cat src/Argon.Api/Features/Orleanse/MemoryPackStorageSerializer.cs
# Check OrleansExtension.cs for configuration
echo "Checking Orleans configuration..."
cat src/Argon.Api/Features/Orleanse/OrleansExtension.cs
# Check if MemoryPackCodec is used in any CDN implementation
echo "Checking CDN implementations..."
cat src/Argon.Api/Features/MediaStorage/YandexContentDeliveryNetwork.cs
cat src/Argon.Api/Features/MediaStorage/DiskContentDeliveryNetwork.cs
# Check the interface to understand the contract
echo "Checking CDN interface..."
cat src/Argon.Api/Features/MediaStorage/IContentDeliveryNetwork.cs
Length of output: 8008
src/Argon.Api/Features/MediaStorage/YandexContentDeliveryNetwork.cs (2)
7-9:
Correct the class constructor syntax
The constructor is incorrectly declared within the class declaration. In C#, constructors should be defined inside the class body. Also, attributes should be applied directly to the constructor parameters.
Apply this diff to fix the syntax error:
-public class YandexContentDeliveryNetwork([FromKeyedServices(IContentStorage.GenericS3StorageKey)] IContentStorage storage,
- ILogger<YandexContentDeliveryNetwork> logger, IOptions<CdnOptions> options)
- : IContentDeliveryNetwork
+public class YandexContentDeliveryNetwork : IContentDeliveryNetwork
+{
+ private readonly IContentStorage storage;
+ private readonly ILogger<YandexContentDeliveryNetwork> logger;
+ private readonly IOptions<CdnOptions> options;
+
+ public YandexContentDeliveryNetwork(
+ [FromKeyedServices(IContentStorage.GenericS3StorageKey)] IContentStorage storage,
+ ILogger<YandexContentDeliveryNetwork> logger,
+ IOptions<CdnOptions> options)
+ {
+ this.storage = storage;
+ this.logger = logger;
+ this.options = options;
+ }Likely invalid or redundant comment.
31-32: Verify the cast of TotalSeconds to int
Casting Config.EntryExpire.TotalSeconds (a double) to int may lead to truncation and unintended expiry times. Ensure that the expiry duration is correctly calculated.
Run the following script to check for potential issues:
✅ Verification successful
Let me gather more information about the Config class and the GenerateSignedLink method to understand the requirements better.
Let me check one more thing to understand the Config class structure and any documentation about the expected expiry time values.
Let me check the appsettings.json files to understand the typical values configured for EntryExpire.
Cast of TotalSeconds to int is safe in this context
The EntryExpire configuration is set to "01:00:00" (1 hour) in the appsettings.json files. When converting this TimeSpan's TotalSeconds (3600) to an integer, there's no risk of data loss or truncation since:
- The value is well within int32 bounds (max ~2.1 billion seconds, or ~68 years)
- It represents a whole number of seconds, so no decimal precision is lost
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the value of Config.EntryExpire.TotalSeconds and ensure correct casting.
# Assuming Config.EntryExpire.TotalSeconds is defined in code, search for its usages
rg --no-filename 'Config\.EntryExpire\.TotalSeconds' -tcs
# Check if there are any casts to int elsewhere that might indicate a pattern
rg --no-filename 'int\)\s*Config\.EntryExpire\.TotalSeconds' -tcs
Length of output: 425
Script:
#!/bin/bash
# Check Config class definition and EntryExpire property
ast-grep --pattern 'class $_ {
$$$
EntryExpire
$$$
}'
# Check GenerateSignedLink method signature and implementation
ast-grep --pattern 'GenerateSignedLink($$$)'
# Search for any other similar expiry time handling
rg 'TimeSpan.*TotalSeconds' -tcs
Length of output: 868
Script:
#!/bin/bash
# Look for Config class definition and any comments/documentation
rg -A 5 "class.*Config" -tcs
# Check for any configuration files that might set EntryExpire
fd -e json -e yaml -e yml -e xml | xargs rg -l "EntryExpire"
Length of output: 1980
Script:
#!/bin/bash
# Check the content of appsettings files focusing on EntryExpire configuration
cat src/Argon.Entry/appsettings.json src/Argon.Api/appsettings.json | jq -r '..|.EntryExpire? | select(. != null)'
# Also check for any unit tests that might verify this functionality
rg -A 3 "EntryExpire" -tcs
Length of output: 1065
src/Argon.Api/Controllers/FilesController.cs (2)
57-73:
Duplicate issues in UploadUserAvatar method
The same concerns mentioned for UploadServerAvatar apply here:
-
Address the TODO comment: Please complete the implementation.
-
Use
Unauthorized()method instead ofStatusCode(401);for clarity. -
Verify permission check parameters: Ensure
PropertyBag.Empty.Set(userId)uses the correct key. -
Handle failures from
CreateAssetAsyncappropriately: Modify the logic to handle cases where the asset creation fails before updating the user avatar.
14-19:
Incorrect class constructor syntax in FilesController
The current class definition uses an invalid syntax for a class constructor in C#. Classes cannot have constructor parameters directly in their declaration like this. Instead, you should define a constructor within the class body.
Apply this diff to fix the class constructor:
-public class FilesController(
- IOptions<CdnOptions> cdnOptions,
- IContentDeliveryNetwork cdn,
- IPermissionProvider permissions,
- IGrainFactory grainFactory) : ControllerBase
+public class FilesController : ControllerBase
+{
+ private readonly IOptions<CdnOptions> _cdnOptions;
+ private readonly IContentDeliveryNetwork _cdn;
+ private readonly IPermissionProvider _permissions;
+ private readonly IGrainFactory _grainFactory;
+
+ public FilesController(
+ IOptions<CdnOptions> cdnOptions,
+ IContentDeliveryNetwork cdn,
+ IPermissionProvider permissions,
+ IGrainFactory grainFactory)
+ {
+ _cdnOptions = cdnOptions;
+ _cdn = cdn;
+ _permissions = permissions;
+ _grainFactory = grainFactory;
+ }Likely invalid or redundant comment.
| public StorageOptions Storage { get; set; } | ||
| } | ||
|
|
||
| public readonly record struct StorageSpace(ulong total, ulong current, uint free); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve StorageSpace struct definition with validation and consistent types.
The struct uses inconsistent numeric types (ulong vs uint) and lacks validation to ensure the space calculations are correct.
Consider this improved implementation:
-public readonly record struct StorageSpace(ulong total, ulong current, uint free);
+/// <summary>
+/// Represents storage space metrics in bytes
+/// </summary>
+public readonly record struct StorageSpace
+{
+ public StorageSpace(ulong total, ulong current)
+ {
+ Total = total;
+ Current = current;
+
+ if (Current > Total)
+ throw new ArgumentException("Current space cannot exceed total space");
+ }
+
+ /// <summary>
+ /// Total storage space in bytes
+ /// </summary>
+ public ulong Total { get; }
+
+ /// <summary>
+ /// Currently used space in bytes
+ /// </summary>
+ public ulong Current { get; }
+
+ /// <summary>
+ /// Available free space in bytes
+ /// </summary>
+ public ulong Free => Total - Current;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public readonly record struct StorageSpace(ulong total, ulong current, uint free); | |
| /// <summary> | |
| /// Represents storage space metrics in bytes | |
| /// </summary> | |
| public readonly record struct StorageSpace | |
| { | |
| public StorageSpace(ulong total, ulong current) | |
| { | |
| Total = total; | |
| Current = current; | |
| if (Current > Total) | |
| throw new ArgumentException("Current space cannot exceed total space"); | |
| } | |
| /// <summary> | |
| /// Total storage space in bytes | |
| /// </summary> | |
| public ulong Total { get; } | |
| /// <summary> | |
| /// Currently used space in bytes | |
| /// </summary> | |
| public ulong Current { get; } | |
| /// <summary> | |
| /// Available free space in bytes | |
| /// </summary> | |
| public ulong Free => Total - Current; | |
| } |
| public class CdnOptions | ||
| { | ||
| public string BaseUrl { get; set; } | ||
| public TimeSpan EntryExpire { get; set; } | ||
| public bool SignUrl { get; set; } | ||
| public string SignSecret { get; set; } | ||
| public StorageOptions Storage { get; set; } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and XML documentation for the configuration properties.
The configuration class lacks input validation and documentation. Properties like BaseUrl and SignSecret should be validated to ensure they contain valid values.
Consider applying these improvements:
public class CdnOptions
{
- public string BaseUrl { get; set; }
- public TimeSpan EntryExpire { get; set; }
- public bool SignUrl { get; set; }
- public string SignSecret { get; set; }
- public StorageOptions Storage { get; set; }
+ /// <summary>
+ /// The base URL of the CDN server
+ /// </summary>
+ public string BaseUrl { get; set; } = string.Empty;
+
+ /// <summary>
+ /// The expiration time for CDN entries
+ /// </summary>
+ public TimeSpan EntryExpire { get; set; } = TimeSpan.FromHours(24);
+
+ /// <summary>
+ /// Indicates whether URLs should be signed for security
+ /// </summary>
+ public bool SignUrl { get; set; }
+
+ /// <summary>
+ /// The secret key used for signing URLs when SignUrl is enabled
+ /// </summary>
+ public string SignSecret { get; set; } = string.Empty;
+
+ /// <summary>
+ /// Storage-specific configuration options
+ /// </summary>
+ public StorageOptions Storage { get; set; } = new();
+
+ public void Validate()
+ {
+ if (string.IsNullOrWhiteSpace(BaseUrl))
+ throw new ArgumentException("BaseUrl cannot be empty", nameof(BaseUrl));
+
+ if (SignUrl && string.IsNullOrWhiteSpace(SignSecret))
+ throw new ArgumentException("SignSecret is required when SignUrl is enabled", nameof(SignSecret));
+
+ if (EntryExpire <= TimeSpan.Zero)
+ throw new ArgumentException("EntryExpire must be positive", nameof(EntryExpire));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class CdnOptions | |
| { | |
| public string BaseUrl { get; set; } | |
| public TimeSpan EntryExpire { get; set; } | |
| public bool SignUrl { get; set; } | |
| public string SignSecret { get; set; } | |
| public StorageOptions Storage { get; set; } | |
| } | |
| public class CdnOptions | |
| { | |
| /// <summary> | |
| /// The base URL of the CDN server | |
| /// </summary> | |
| public string BaseUrl { get; set; } = string.Empty; | |
| /// <summary> | |
| /// The expiration time for CDN entries | |
| /// </summary> | |
| public TimeSpan EntryExpire { get; set; } = TimeSpan.FromHours(24); | |
| /// <summary> | |
| /// Indicates whether URLs should be signed for security | |
| /// </summary> | |
| public bool SignUrl { get; set; } | |
| /// <summary> | |
| /// The secret key used for signing URLs when SignUrl is enabled | |
| /// </summary> | |
| public string SignSecret { get; set; } = string.Empty; | |
| /// <summary> | |
| /// Storage-specific configuration options | |
| /// </summary> | |
| public StorageOptions Storage { get; set; } = new(); | |
| public void Validate() | |
| { | |
| if (string.IsNullOrWhiteSpace(BaseUrl)) | |
| throw new ArgumentException("BaseUrl cannot be empty", nameof(BaseUrl)); | |
| if (SignUrl && string.IsNullOrWhiteSpace(SignSecret)) | |
| throw new ArgumentException("SignSecret is required when SignUrl is enabled", nameof(SignSecret)); | |
| if (EntryExpire <= TimeSpan.Zero) | |
| throw new ArgumentException("EntryExpire must be positive", nameof(EntryExpire)); | |
| } | |
| } |
| UserManagerService managerService, | ||
| IPasswordHashingService passwordHashingService, | ||
| ApplicationDbContext context) : Grain, ISessionManager | ||
| { | ||
| public async Task<UserDto> GetUser() => await grainFactory.GetGrain<IUserManager>(this.GetPrimaryKey()).GetUser(); | ||
| public async Task<UserDto> GetUser() => await grainFactory.GetGrain<IUserGrain>(this.GetPrimaryKey()).GetUser(); | ||
|
|
||
| public Task Logout() => throw new NotImplementedException(); |
There was a problem hiding this comment.
Implement Logout method for CDN session management
The NotImplementedException in the Logout method needs to be addressed, especially since proper session management is crucial for CDN integration to handle user-specific content and caching.
Would you like me to help implement the Logout method with proper CDN session cleanup?
| UserManagerService managerService, | ||
| IPasswordHashingService passwordHashingService, | ||
| ApplicationDbContext context) : Grain, ISessionManager | ||
| { | ||
| public async Task<UserDto> GetUser() => await grainFactory.GetGrain<IUserManager>(this.GetPrimaryKey()).GetUser(); | ||
| public async Task<UserDto> GetUser() => await grainFactory.GetGrain<IUserGrain>(this.GetPrimaryKey()).GetUser(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for grain communication
While the implementation is concise, it lacks error handling for potential grain communication failures. This is particularly important as the system scales with CDN integration.
Consider adding error handling:
- public async Task<UserDto> GetUser() => await grainFactory.GetGrain<IUserGrain>(this.GetPrimaryKey()).GetUser();
+ public async Task<UserDto> GetUser()
+ {
+ try
+ {
+ var userGrain = grainFactory.GetGrain<IUserGrain>(this.GetPrimaryKey());
+ return await userGrain.GetUser();
+ }
+ catch (Exception ex)
+ {
+ logger.LogError(ex, "Failed to retrieve user information");
+ throw new GrainCommunicationException("Unable to retrieve user information", ex);
+ }
+ }Committable suggestion skipped: line range outside the PR's diff.
| public class NullPermissionProvider : IPermissionProvider | ||
| { | ||
| public bool CanAccess(string scope, PropertyBag bag) | ||
| => true; | ||
| } |
There was a problem hiding this comment.
Security Risk: NullPermissionProvider grants unrestricted access.
The current implementation poses a significant security risk by always returning true, effectively bypassing all permission checks.
Consider this secure implementation:
public class NullPermissionProvider : IPermissionProvider
{
+ private readonly bool _isDevelopment;
+
+ public NullPermissionProvider(bool isDevelopment)
+ {
+ _isDevelopment = isDevelopment;
+ }
+
public bool CanAccess(string scope, PropertyBag bag)
- => true;
+ {
+ if (!_isDevelopment)
+ {
+ throw new InvalidOperationException(
+ "NullPermissionProvider should not be used in production.");
+ }
+ return true;
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public class NullPermissionProvider : IPermissionProvider | |
| { | |
| public bool CanAccess(string scope, PropertyBag bag) | |
| => true; | |
| } | |
| public class NullPermissionProvider : IPermissionProvider | |
| { | |
| private readonly bool _isDevelopment; | |
| public NullPermissionProvider(bool isDevelopment) | |
| { | |
| _isDevelopment = isDevelopment; | |
| } | |
| public bool CanAccess(string scope, PropertyBag bag) | |
| { | |
| if (!_isDevelopment) | |
| { | |
| throw new InvalidOperationException( | |
| "NullPermissionProvider should not be used in production."); | |
| } | |
| return true; | |
| } | |
| } |
| app.UseWebSockets(); | ||
| app.MapRpcWebSocketServer(); | ||
| app.UseSwagger(); | ||
| app.UseSwaggerUI(); | ||
| app.UseHttpsRedirection(); | ||
| app.UseAuthentication(); | ||
| app.UseAuthorization(); | ||
| app.MapControllers(); | ||
| app.MapDefaultEndpoints(); |
There was a problem hiding this comment.
Ensure middleware is in the correct order to apply HTTPS redirection before other middleware
The app.UseHttpsRedirection() middleware is currently called after app.UseSwagger() and app.UseSwaggerUI(). To ensure that all incoming HTTP requests are redirected to HTTPS before reaching other middleware components, consider moving app.UseHttpsRedirection() earlier in the pipeline.
Apply this diff to adjust the middleware ordering:
app.UseWebSockets();
app.MapRpcWebSocketServer();
+app.UseHttpsRedirection();
app.UseSwagger();
app.UseSwaggerUI();
-app.UseHttpsRedirection();
app.UseAuthentication();
app.UseAuthorization();
app.MapControllers();
app.MapDefaultEndpoints();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| app.UseWebSockets(); | |
| app.MapRpcWebSocketServer(); | |
| app.UseSwagger(); | |
| app.UseSwaggerUI(); | |
| app.UseHttpsRedirection(); | |
| app.UseAuthentication(); | |
| app.UseAuthorization(); | |
| app.MapControllers(); | |
| app.MapDefaultEndpoints(); | |
| app.UseWebSockets(); | |
| app.MapRpcWebSocketServer(); | |
| app.UseHttpsRedirection(); | |
| app.UseSwagger(); | |
| app.UseSwaggerUI(); | |
| app.UseAuthentication(); | |
| app.UseAuthorization(); | |
| app.MapControllers(); | |
| app.MapDefaultEndpoints(); |
| var result = await cdn.CreateAssetAsync(StorageNameSpace.ForServer(serverId), assetId, file); | ||
|
|
||
| if (result.HasValue) | ||
| return Ok(result); | ||
|
|
||
| await grainFactory.GetGrain<IServerGrain>(serverId) | ||
| .UpdateServer(new ServerInput(null, null, assetId.ToFileId())); | ||
|
|
||
| return Ok(); |
There was a problem hiding this comment.
Handle failures from CreateAssetAsync appropriately
If cdn.CreateAssetAsync fails and result.HasValue is false, the code proceeds to update the server avatar, which might not be appropriate since the asset was not created successfully. Consider adding error handling to address this scenario.
Apply this diff to handle the failure case:
var result = await cdn.CreateAssetAsync(StorageNameSpace.ForServer(serverId), assetId, file);
- if (result.HasValue)
- return Ok(result);
+ if (!result.HasValue)
+ return StatusCode(500, "Failed to create asset.");
await grainFactory.GetGrain<IServerGrain>(serverId)
.UpdateServer(new ServerInput(null, null, assetId.ToFileId()));
return Ok();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var result = await cdn.CreateAssetAsync(StorageNameSpace.ForServer(serverId), assetId, file); | |
| if (result.HasValue) | |
| return Ok(result); | |
| await grainFactory.GetGrain<IServerGrain>(serverId) | |
| .UpdateServer(new ServerInput(null, null, assetId.ToFileId())); | |
| return Ok(); | |
| var result = await cdn.CreateAssetAsync(StorageNameSpace.ForServer(serverId), assetId, file); | |
| if (!result.HasValue) | |
| return StatusCode(500, "Failed to create asset."); | |
| await grainFactory.GetGrain<IServerGrain>(serverId) | |
| .UpdateServer(new ServerInput(null, null, assetId.ToFileId())); | |
| return Ok(); |
| var mem = DiskContentStorage.OpenFileRead(ns, assetId); | ||
|
|
||
| return File(mem, assetId.GetMime()); |
There was a problem hiding this comment.
Add error handling for file access
DiskContentStorage.OpenFileRead(ns, assetId) might throw exceptions if the file does not exist or is inaccessible. It's important to handle these potential exceptions to prevent the application from crashing and to provide a meaningful response to the client.
Apply this diff to add error handling:
- var mem = DiskContentStorage.OpenFileRead(ns, assetId);
-
- return File(mem, assetId.GetMime());
+ try
+ {
+ var mem = DiskContentStorage.OpenFileRead(ns, assetId);
+ return File(mem, assetId.GetMime());
+ }
+ catch (FileNotFoundException)
+ {
+ return NotFound();
+ }
+ catch (Exception)
+ {
+ // Optionally log the exception
+ return StatusCode(500, "An unexpected error occurred.");
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var mem = DiskContentStorage.OpenFileRead(ns, assetId); | |
| return File(mem, assetId.GetMime()); | |
| try | |
| { | |
| var mem = DiskContentStorage.OpenFileRead(ns, assetId); | |
| return File(mem, assetId.GetMime()); | |
| } | |
| catch (FileNotFoundException) | |
| { | |
| return NotFound(); | |
| } | |
| catch (Exception) | |
| { | |
| // Optionally log the exception | |
| return StatusCode(500, "An unexpected error occurred."); | |
| } |
| { | ||
| // TODO | ||
| if (!permissions.CanAccess("server.avatar.upload", PropertyBag.Empty.Set(serverId))) | ||
| return StatusCode(401); |
There was a problem hiding this comment.
💡 Codebase verification
Based on the findings, I can now provide the final response:
Permission check implementation is consistent but needs review
The PropertyBag.Empty.Set(serverId) usage is consistent across avatar upload endpoints, but the current implementation uses NullPermissionProvider which always returns true. This effectively bypasses all permission checks, making the authorization ineffective.
src/Argon.Api/Features/Pex/IPermissionProvider.cs: TheNullPermissionProviderneeds to be replaced with a proper implementation that validates permissions.src/Argon.Api/Controllers/FilesController.cs: The permission checks for both server and user avatar uploads are currently not enforcing any real restrictions.
🔗 Analysis chain
Verify permission check parameters
Ensure that PropertyBag.Empty.Set(serverId) correctly sets the server ID with the appropriate key for the permission check. If the key is missing or incorrect, the permission validation might not work as intended.
Run the following script to verify the usage of PropertyBag.Set with the correct key:
Let me gather more context about the permission check implementation.
Let me check the implementation of PropertyBag and permission checks to ensure the pattern is consistent.
Let me check the implementation of the permission provider to verify the parameter usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of PropertyBag.Set to verify key assignments.
rg 'PropertyBag\.Empty\.Set\(\s*serverId\s*\)'
Length of output: 186
Script:
#!/bin/bash
# Search for the permission key usage and its definition
rg "server\.avatar\.upload" -A 3 -B 3
# Search for other similar permission checks
ast-grep --pattern 'permissions.CanAccess($_, $_)'
Length of output: 1083
Script:
#!/bin/bash
# Search for PropertyBag implementation
ast-grep --pattern 'class PropertyBag'
# Search for other usages of PropertyBag.Set
rg "PropertyBag\.Empty\.Set" -A 2 -B 2
# Look for permission check implementations
ast-grep --pattern 'interface IPermissions'
Length of output: 16081
Script:
#!/bin/bash
# Search for CanAccess implementation
ast-grep --pattern 'CanAccess($_, $_)'
# Search for permission provider implementations
rg "class.*PermissionProvider" -A 5
Length of output: 688
| var storageContainer = new ServiceCollection(); | ||
| var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(builder.Services); | ||
|
|
||
| coreBuilder.UseGenericS3(config => | ||
| { | ||
| config.Endpoint = options.BaseUrl; | ||
| config.RegionCode = options.Region; | ||
| config.Credentials = new StringAccessKey(options.Login, options.Password); | ||
| }); | ||
|
|
||
| var storageServices = storageContainer.BuildServiceProvider(); | ||
|
|
There was a problem hiding this comment.
Correct the service registration to ensure S3 services are properly configured
The storageContainer is instantiated but not utilized effectively. The SimpleS3 services are currently being added to builder.Services instead of storageContainer, resulting in storageServices lacking the necessary configurations. This will cause GetRequiredService<IObjectClient>() to fail when attempting to retrieve services from the empty storageServices.
Modify the code to register SimpleS3 services with storageContainer:
- var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(builder.Services);
+ var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(storageContainer);This change ensures that the SimpleS3 services are correctly added to storageContainer, and subsequently, storageServices will have the required services when built.
Additionally, verify that all service registrations and dependencies are correctly configured to prevent runtime errors.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var storageContainer = new ServiceCollection(); | |
| var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(builder.Services); | |
| coreBuilder.UseGenericS3(config => | |
| { | |
| config.Endpoint = options.BaseUrl; | |
| config.RegionCode = options.Region; | |
| config.Credentials = new StringAccessKey(options.Login, options.Password); | |
| }); | |
| var storageServices = storageContainer.BuildServiceProvider(); | |
| var storageContainer = new ServiceCollection(); | |
| var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(storageContainer); | |
| coreBuilder.UseGenericS3(config => | |
| { | |
| config.Endpoint = options.BaseUrl; | |
| config.RegionCode = options.Region; | |
| config.Credentials = new StringAccessKey(options.Login, options.Password); | |
| }); | |
| var storageServices = storageContainer.BuildServiceProvider(); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs (2)
31-47: Improve storage kind handling and service registrationThe current implementation has several areas for improvement:
- Use switch expression instead of if statements for better extensibility
- Add handling for unsupported storage kinds
- Consider using TryAdd for service registration to prevent duplicates
Consider this alternative implementation:
public static IServiceCollection AddContentDeliveryNetwork<T>(this WebApplicationBuilder builder, StorageKind keyName, StorageOptions options) where T : class, IContentDeliveryNetwork { - if (keyName == StorageKind.Disk) - { - builder.Services.AddKeyedSingleton<IContentStorage, DiskContentStorage>(IContentStorage.DiskContentStorageKey); - builder.Services.AddSingleton<IContentDeliveryNetwork, T>(); - } - if (keyName == StorageKind.GenericS3) - { - builder.Services.AddKeyedSingleton<IContentStorage, S3ContentStorage>(IContentStorage.GenericS3StorageKey); - builder.Services.AddSingleton<IContentDeliveryNetwork, T>(); - builder.AddS3Storage(keyName, options); - } + _ = keyName switch + { + StorageKind.Disk => RegisterDiskServices(), + StorageKind.GenericS3 => RegisterS3Services(), + _ => throw new ArgumentOutOfRangeException(nameof(keyName), $"Storage kind {keyName} is not supported") + }; return builder.Services; + IServiceCollection RegisterDiskServices() + { + builder.Services.TryAddKeyedSingleton<IContentStorage, DiskContentStorage>(IContentStorage.DiskContentStorageKey); + builder.Services.TryAddSingleton<IContentDeliveryNetwork, T>(); + return builder.Services; + } + + IServiceCollection RegisterS3Services() + { + builder.Services.TryAddKeyedSingleton<IContentStorage, S3ContentStorage>(IContentStorage.GenericS3StorageKey); + builder.Services.TryAddSingleton<IContentDeliveryNetwork, T>(); + builder.AddS3Storage(keyName, options); + return builder.Services; + } }
1-70: Consider improving architecture with interface segregationThe current implementation tightly couples CDN and storage implementations. Consider:
- Separating storage-specific configurations into dedicated configuration classes
- Creating separate interfaces for different storage capabilities
- Using a factory pattern for storage provider creation
This would make the system more maintainable and easier to extend with new storage types.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs(1 hunks)
🔇 Additional comments (1)
src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs (1)
1-8: LGTM: Namespace and imports are well-organized
The namespace follows the project structure, and imports are relevant to the CDN and S3 functionality.
| public static IServiceCollection AddS3Storage(this WebApplicationBuilder builder, StorageKind keyName, StorageOptions options) | ||
| { | ||
| var storageContainer = new ServiceCollection(); | ||
| var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(storageContainer); | ||
|
|
||
| coreBuilder.UseGenericS3(config => | ||
| { | ||
| config.Endpoint = options.BaseUrl; | ||
| config.RegionCode = options.Region; | ||
| config.Credentials = new StringAccessKey(options.Login, options.Password); | ||
| }); | ||
|
|
||
| var storageServices = storageContainer.BuildServiceProvider(); | ||
|
|
||
| builder.Services.AddKeyedSingleton<IServiceProvider>($"{keyName}:container", storageServices); | ||
| builder.Services.AddKeyedSingleton<IObjectClient>($"{keyName}:client", (services, o) | ||
| => services.GetRequiredKeyedService<IServiceProvider>($"{keyName}:container").GetRequiredService<IObjectClient>()); | ||
|
|
||
| return builder.Services; | ||
| } |
There was a problem hiding this comment.
Critical: Fix S3 service registration and add validation
The implementation has several critical issues:
- S3 services are registered to a separate container instead of the main one
- No validation of required S3 options
- No disposal handling for the service provider
Apply these fixes:
public static IServiceCollection AddS3Storage(this WebApplicationBuilder builder, StorageKind keyName, StorageOptions options)
{
+ if (string.IsNullOrEmpty(options.BaseUrl))
+ throw new ArgumentException("BaseUrl is required for S3 storage", nameof(options));
+ if (string.IsNullOrEmpty(options.Region))
+ throw new ArgumentException("Region is required for S3 storage", nameof(options));
+ if (string.IsNullOrEmpty(options.Login) || string.IsNullOrEmpty(options.Password))
+ throw new ArgumentException("Login and Password are required for S3 storage", nameof(options));
- var storageContainer = new ServiceCollection();
- var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(storageContainer);
+ var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(builder.Services);
coreBuilder.UseGenericS3(config =>
{
config.Endpoint = options.BaseUrl;
config.RegionCode = options.Region;
config.Credentials = new StringAccessKey(options.Login, options.Password);
});
- var storageServices = storageContainer.BuildServiceProvider();
-
- builder.Services.AddKeyedSingleton<IServiceProvider>($"{keyName}:container", storageServices);
builder.Services.AddKeyedSingleton<IObjectClient>($"{keyName}:client", (services, o)
- => services.GetRequiredKeyedService<IServiceProvider>($"{keyName}:container").GetRequiredService<IObjectClient>());
+ => services.GetRequiredService<IObjectClient>());
return builder.Services;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static IServiceCollection AddS3Storage(this WebApplicationBuilder builder, StorageKind keyName, StorageOptions options) | |
| { | |
| var storageContainer = new ServiceCollection(); | |
| var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(storageContainer); | |
| coreBuilder.UseGenericS3(config => | |
| { | |
| config.Endpoint = options.BaseUrl; | |
| config.RegionCode = options.Region; | |
| config.Credentials = new StringAccessKey(options.Login, options.Password); | |
| }); | |
| var storageServices = storageContainer.BuildServiceProvider(); | |
| builder.Services.AddKeyedSingleton<IServiceProvider>($"{keyName}:container", storageServices); | |
| builder.Services.AddKeyedSingleton<IObjectClient>($"{keyName}:client", (services, o) | |
| => services.GetRequiredKeyedService<IServiceProvider>($"{keyName}:container").GetRequiredService<IObjectClient>()); | |
| return builder.Services; | |
| } | |
| public static IServiceCollection AddS3Storage(this WebApplicationBuilder builder, StorageKind keyName, StorageOptions options) | |
| { | |
| if (string.IsNullOrEmpty(options.BaseUrl)) | |
| throw new ArgumentException("BaseUrl is required for S3 storage", nameof(options)); | |
| if (string.IsNullOrEmpty(options.Region)) | |
| throw new ArgumentException("Region is required for S3 storage", nameof(options)); | |
| if (string.IsNullOrEmpty(options.Login) || string.IsNullOrEmpty(options.Password)) | |
| throw new ArgumentException("Login and Password are required for S3 storage", nameof(options)); | |
| var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(builder.Services); | |
| coreBuilder.UseGenericS3(config => | |
| { | |
| config.Endpoint = options.BaseUrl; | |
| config.RegionCode = options.Region; | |
| config.Credentials = new StringAccessKey(options.Login, options.Password); | |
| }); | |
| builder.Services.AddKeyedSingleton<IObjectClient>($"{keyName}:client", (services, o) | |
| => services.GetRequiredService<IObjectClient>()); | |
| return builder.Services; | |
| } |
| public static IServiceCollection AddContentDeliveryNetwork(this WebApplicationBuilder builder) | ||
| { | ||
| builder.Services.Configure<CdnOptions>(builder.Configuration.GetSection("Cdn")); | ||
|
|
||
| var opt = builder.Configuration.GetSection("Cdn:Storage"); | ||
| var options = builder.Services.Configure<StorageOptions>(opt); | ||
| var bucketOptions = new StorageOptions(); | ||
| opt.Bind(bucketOptions); | ||
|
|
||
|
|
||
|
|
||
| return bucketOptions.Kind switch | ||
| { | ||
| StorageKind.InMemory => throw new InvalidOperationException(), | ||
| StorageKind.Disk => builder.AddContentDeliveryNetwork<DiskContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions), | ||
| StorageKind.GenericS3 => builder.AddContentDeliveryNetwork<YandexContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions), | ||
| _ => throw new ArgumentOutOfRangeException() | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper error handling and validation
Several improvements needed for robustness:
- Add descriptive message for InvalidOperationException
- Add null checks for configuration sections
- Validate bucketOptions after binding
Apply these changes:
public static IServiceCollection AddContentDeliveryNetwork(this WebApplicationBuilder builder)
{
+ var cdnSection = builder.Configuration.GetSection("Cdn")
+ ?? throw new InvalidOperationException("CDN configuration section is missing");
- builder.Services.Configure<CdnOptions>(builder.Configuration.GetSection("Cdn"));
+ builder.Services.Configure<CdnOptions>(cdnSection);
- var opt = builder.Configuration.GetSection("Cdn:Storage");
+ var opt = builder.Configuration.GetSection("Cdn:Storage")
+ ?? throw new InvalidOperationException("CDN storage configuration section is missing");
var options = builder.Services.Configure<StorageOptions>(opt);
var bucketOptions = new StorageOptions();
opt.Bind(bucketOptions);
+ if (string.IsNullOrEmpty(bucketOptions.BaseUrl))
+ throw new InvalidOperationException("Storage BaseUrl is required");
return bucketOptions.Kind switch
{
- StorageKind.InMemory => throw new InvalidOperationException(),
+ StorageKind.InMemory => throw new InvalidOperationException("InMemory storage is not supported for CDN"),
StorageKind.Disk => builder.AddContentDeliveryNetwork<DiskContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions),
StorageKind.GenericS3 => builder.AddContentDeliveryNetwork<YandexContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions),
_ => throw new ArgumentOutOfRangeException(nameof(bucketOptions.Kind))
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static IServiceCollection AddContentDeliveryNetwork(this WebApplicationBuilder builder) | |
| { | |
| builder.Services.Configure<CdnOptions>(builder.Configuration.GetSection("Cdn")); | |
| var opt = builder.Configuration.GetSection("Cdn:Storage"); | |
| var options = builder.Services.Configure<StorageOptions>(opt); | |
| var bucketOptions = new StorageOptions(); | |
| opt.Bind(bucketOptions); | |
| return bucketOptions.Kind switch | |
| { | |
| StorageKind.InMemory => throw new InvalidOperationException(), | |
| StorageKind.Disk => builder.AddContentDeliveryNetwork<DiskContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions), | |
| StorageKind.GenericS3 => builder.AddContentDeliveryNetwork<YandexContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions), | |
| _ => throw new ArgumentOutOfRangeException() | |
| }; | |
| } | |
| public static IServiceCollection AddContentDeliveryNetwork(this WebApplicationBuilder builder) | |
| { | |
| var cdnSection = builder.Configuration.GetSection("Cdn") | |
| ?? throw new InvalidOperationException("CDN configuration section is missing"); | |
| builder.Services.Configure<CdnOptions>(cdnSection); | |
| var opt = builder.Configuration.GetSection("Cdn:Storage") | |
| ?? throw new InvalidOperationException("CDN storage configuration section is missing"); | |
| var options = builder.Services.Configure<StorageOptions>(opt); | |
| var bucketOptions = new StorageOptions(); | |
| opt.Bind(bucketOptions); | |
| if (string.IsNullOrEmpty(bucketOptions.BaseUrl)) | |
| throw new InvalidOperationException("Storage BaseUrl is required"); | |
| return bucketOptions.Kind switch | |
| { | |
| StorageKind.InMemory => throw new InvalidOperationException("InMemory storage is not supported for CDN"), | |
| StorageKind.Disk => builder.AddContentDeliveryNetwork<DiskContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions), | |
| StorageKind.GenericS3 => builder.AddContentDeliveryNetwork<YandexContentDeliveryNetwork>(bucketOptions.Kind, bucketOptions), | |
| _ => throw new ArgumentOutOfRangeException(nameof(bucketOptions.Kind)) | |
| }; | |
| } |
| return Ok(result); | ||
|
|
||
| await grainFactory.GetGrain<IServerGrain>(serverId) | ||
| .UpdateServer(new ServerInput(null, null, assetId.ToFileId())); |
There was a problem hiding this comment.
разве не сотрет сохраненную инфу?
Summary by CodeRabbit
New Features
FilesControllerfor avatar file operations, allowing users to upload and retrieve server and user avatars.IPermissionProviderinterface.appsettings.jsonto include CDN settings and permissions management.UserEditInputfor user editing capabilities.Bug Fixes
Documentation
appsettings.jsonto reflect new sections for CDN, Orleans, and Captcha configurations.Refactor