Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Content Delivery Network #47

Merged
merged 13 commits into from
Nov 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ riderModule.iml
.vs/
.DS_Store
*.user

src/Argon.Api/storage/
3 changes: 3 additions & 0 deletions src/Argon.Api/Argon.Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@
<PackageReference Include="Aspire.StackExchange.Redis" Version="9.0.0"/>
<PackageReference Include="Flurl.Http" Version="4.0.2"/>
<PackageReference Include="Flurl.Http.Newtonsoft" Version="0.9.1"/>
<PackageReference Include="Genbox.SimpleS3.Core" Version="3.2.4"/>
<PackageReference Include="Genbox.SimpleS3.Extensions.GenericS3" Version="3.2.4"/>
<PackageReference Include="Genbox.SimpleS3.Extensions.HttpClient" Version="3.2.4"/>
<PackageReference Include="MemoryPack" Version="1.21.3"/>
<PackageReference Include="MessagePipe" Version="1.8.1"/>
<PackageReference Include="MessagePipe.Analyzer" Version="1.8.1">
Expand Down
74 changes: 74 additions & 0 deletions src/Argon.Api/Controllers/FilesController.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
namespace Argon.Api.Controllers;

using ActualLab.Collections;
using Argon.Api.Features.MediaStorage.Storages;
using Contracts;
using Features.MediaStorage;
using Features.Pex;
using Grains.Interfaces;
using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Options;

public class FilesController(
IOptions<CdnOptions> cdnOptions,
IContentDeliveryNetwork cdn,
IPermissionProvider permissions,
IGrainFactory grainFactory) : ControllerBase
{
// work only when cdn\storage set local disk or in memory
[HttpGet("/files/{nsPath}/{nsId:guid}/{kind}/{shard}/{fileId}")]
public async ValueTask<IActionResult> Files(
[FromRoute] string nsPath,
[FromRoute] Guid nsId,
[FromRoute] string kind,
[FromRoute] string shard,
[FromRoute] string fileId)
{
if (cdnOptions.Value.Storage.Kind == StorageKind.GenericS3)
return BadRequest();

var ns = new StorageNameSpace(nsPath, nsId);
var assetId = AssetId.FromFileId(fileId);
var mem = DiskContentStorage.OpenFileRead(ns, assetId);

return File(mem, assetId.GetMime());
Comment on lines +34 to +36
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.");
}

}

[HttpPost("/files/server/{serverId:guid}/avatar"), Authorize(JwtBearerDefaults.AuthenticationScheme)]
public async ValueTask<IActionResult> UploadServerAvatar([FromRoute] Guid serverId, IFormFile file)
{
// TODO
if (!permissions.CanAccess("server.avatar.upload", PropertyBag.Empty.Set(serverId)))
return StatusCode(401);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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: The NullPermissionProvider needs 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 assetId = AssetId.Avatar();
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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

разве не сотрет сохраненную инфу?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image


return Ok();
Comment on lines +46 to +54
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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();

}

[HttpPost("/files/user/{userId:guid}/avatar"), Authorize(JwtBearerDefaults.AuthenticationScheme)]
public async ValueTask<IActionResult> UploadUserAvatar([FromRoute] Guid userId, IFormFile file)
{
// TODO
if (!permissions.CanAccess("user.avatar.upload", PropertyBag.Empty.Set(userId)))
return StatusCode(401);
var assetId = AssetId.Avatar();
var result = await cdn.CreateAssetAsync(StorageNameSpace.ForUser(userId), assetId, file);

if (result.HasValue)
return Ok(result);

await grainFactory.GetGrain<IUserGrain>(userId)
.UpdateUser(new UserEditInput(null, null, assetId.ToFileId()));

return Ok();
}
}
22 changes: 9 additions & 13 deletions src/Argon.Api/Controllers/MetadataController.cs
Original file line number Diff line number Diff line change
@@ -1,30 +1,26 @@
namespace Argon.Api.Controllers;

using Extensions;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;

[ApiController]
public class MetadataController : ControllerBase
{
[HttpGet("/cfg.json"), AllowAnonymous]
public ValueTask<HeadRoutingConfig> GetHead() => new(new HeadRoutingConfig($"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}",
"api.argon.gl", "argon-f14ic5ia.livekit.cloud", [
new RegionalNode("cdn-ru1.argon.gl", "ru1"),
new RegionalNode("cdn-ru2.argon.gl", "ru1"),
new RegionalNode("cdn-as1.argon.gl", "as1")
], [
new FeatureFlag("dev.window", true),
new FeatureFlag("user.allowServerCreation", true)
]));
public ValueTask<HeadRoutingConfig> GetHead()
=> new(new HeadRoutingConfig($"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}",
"api.argon.gl", "argon-f14ic5ia.livekit.cloud", [
Comment on lines +12 to +13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider moving endpoints to configuration

The API and WebRTC endpoints are hardcoded. This could make it difficult to switch between environments (development, staging, production).

Consider moving these to configuration:

- "api.argon.gl", "argon-f14ic5ia.livekit.cloud"
+ _configuration["Endpoints:ApiEndpoint"], _configuration["Endpoints:WebRtcEndpoint"]

Committable suggestion skipped: line range outside the PR's diff.

new FeatureFlag("dev.window", true),
new FeatureFlag("user.allowServerCreation", true)
], this.HttpContext.GetRegion()));
Comment on lines +14 to +16
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Move feature flags to configuration

Feature flags are hardcoded and always enabled. This limits the ability to toggle features dynamically.

Consider using a feature management system or configuration:

- new FeatureFlag("dev.window", true),
- new FeatureFlag("user.allowServerCreation", true)
+ _featureManager.GetFeatureFlags() // or
+ _configuration.GetSection("FeatureFlags").Get<List<FeatureFlag>>()

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +11 to +16
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for region retrieval

The GetRegion() call lacks error handling. If the region cannot be determined, this could result in runtime errors.

Consider adding error handling:

public ValueTask<HeadRoutingConfig> GetHead()
{
+   var region = this.HttpContext.GetRegion() ?? "default";
+   if (string.IsNullOrEmpty(region))
+   {
+       throw new InvalidOperationException("Unable to determine region");
+   }
    return new(new HeadRoutingConfig(
        $"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}",
        "api.argon.gl",
        "argon-f14ic5ia.livekit.cloud",
        [
            new FeatureFlag("dev.window", true),
            new FeatureFlag("user.allowServerCreation", true)
-       ], this.HttpContext.GetRegion()));
+       ], region));
}
📝 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.

Suggested change
public ValueTask<HeadRoutingConfig> GetHead()
=> new(new HeadRoutingConfig($"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}",
"api.argon.gl", "argon-f14ic5ia.livekit.cloud", [
new FeatureFlag("dev.window", true),
new FeatureFlag("user.allowServerCreation", true)
], this.HttpContext.GetRegion()));
public ValueTask<HeadRoutingConfig> GetHead()
{
var region = this.HttpContext.GetRegion() ?? "default";
if (string.IsNullOrEmpty(region))
{
throw new InvalidOperationException("Unable to determine region");
}
return new(new HeadRoutingConfig(
$"{GlobalVersion.FullSemVer}.{GlobalVersion.ShortSha}",
"api.argon.gl",
"argon-f14ic5ia.livekit.cloud",
[
new FeatureFlag("dev.window", true),
new FeatureFlag("user.allowServerCreation", true)
], region));
}

}

public record HeadRoutingConfig(
string version,
string masterEndpoint,
string webRtcEndpoint,
List<RegionalNode> cdnAddresses,
List<FeatureFlag> features);

public record RegionalNode(string url, string code);
List<FeatureFlag> features,
string currentRegionCode);

public record FeatureFlag(string code, bool enabled);
5 changes: 5 additions & 0 deletions src/Argon.Api/Extensions/HttpContextExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ public static string GetRegion(this HttpContext ctx)
? ctx.Request.Headers["CF-IPCountry"].ToString()
: "unknown";

public static string GetRay(this HttpContext ctx)
=> ctx.Request.Headers.ContainsKey("CF-Ray")
? ctx.Request.Headers["CF-Ray"].ToString()
: $"{Guid.NewGuid()}";

public static string GetClientName(this HttpContext ctx)
=> ctx.Request.Headers.ContainsKey("User-Agent")
? ctx.Request.Headers["User-Agent"].ToString()
Expand Down
15 changes: 13 additions & 2 deletions src/Argon.Api/Features/Jwt/JwtFeature.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
namespace Argon.Api.Features.Jwt;

using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.IdentityModel.Tokens;

public static class JwtFeature
Expand All @@ -10,7 +11,7 @@ public static IServiceCollection AddJwt(this WebApplicationBuilder builder)

var jwt = builder.Configuration.GetSection("Jwt").Get<JwtOptions>();

builder.Services.AddSingleton(new TokenValidationParameters
var tokenValidator = new TokenValidationParameters
{
ValidIssuer = jwt.Issuer,
ValidAudience = jwt.Audience,
Expand All @@ -20,8 +21,18 @@ public static IServiceCollection AddJwt(this WebApplicationBuilder builder)
ValidateLifetime = true,
ValidateIssuerSigningKey = true,
ClockSkew = TimeSpan.Zero
});
};

builder.Services.AddSingleton(tokenValidator);
builder.Services.AddSingleton<TokenAuthorization>();

builder.Services.AddAuthentication(options =>
{
options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme;
options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme;
})
.AddJwtBearer((options) => options.TokenValidationParameters = tokenValidator);
Comment on lines +29 to +34
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance JWT Bearer security options

While the basic JWT configuration is correct, consider adding these security enhancements:

 .AddJwtBearer((options) => 
+{
     options.TokenValidationParameters = tokenValidator;
+    options.RequireHttpsMetadata = true;
+    options.Events = new JwtBearerEvents
+    {
+        OnAuthenticationFailed = context =>
+        {
+            if (context.Exception is SecurityTokenExpiredException)
+            {
+                context.Response.Headers.Add("Token-Expired", "true");
+            }
+            return Task.CompletedTask;
+        }
+    };
+});

This will:

  1. Enforce HTTPS for all JWT-authenticated requests
  2. Add proper token expiration handling
📝 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.

Suggested change
builder.Services.AddAuthentication(options =>
{
options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme;
options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme;
})
.AddJwtBearer((options) => options.TokenValidationParameters = tokenValidator);
builder.Services.AddAuthentication(options =>
{
options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme;
options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme;
})
.AddJwtBearer((options) =>
{
options.TokenValidationParameters = tokenValidator;
options.RequireHttpsMetadata = true;
options.Events = new JwtBearerEvents
{
OnAuthenticationFailed = context =>
{
if (context.Exception is SecurityTokenExpiredException)
{
context.Response.Headers.Add("Token-Expired", "true");
}
return Task.CompletedTask;
}
};
});


return builder.Services;
}
}
73 changes: 73 additions & 0 deletions src/Argon.Api/Features/MediaStorage/AssetId.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
namespace Argon.Api.Features.MediaStorage;

public readonly struct AssetId(Guid assetId, AssetScope scope, AssetKind kind)
{
public string ToFileId()
=> $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00"; // last two zero reserved

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()}";
}

public string GetMime()
=> kind switch
{
AssetKind.Image => "image/png",
AssetKind.Video => "video/mp4",
AssetKind.VideoNoSound => "video/mp4",
AssetKind.File => "application/binary",
AssetKind.ServerContent => "application/binary",
AssetKind.ServiceContent => "application/binary",
AssetKind.Sound => "application/ogg",
_ => throw new ArgumentOutOfRangeException(nameof(kind), kind, null)
};
Comment on lines +19 to +30
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider improving MIME type handling.

Several concerns with the current implementation:

  1. Generic application/binary MIME type for multiple kinds might not be specific enough
  2. Inconsistency between comment "only png" for Video kind and its "video/mp4" MIME type
  3. Hardcoded MIME types make it difficult to modify without code changes

Consider using a configuration-driven approach and more specific MIME types:

+    private static readonly Dictionary<AssetKind, string> MimeTypes = new()
+    {
+        { AssetKind.Image, "image/png" },
+        { AssetKind.Video, "video/mp4" },
+        { AssetKind.VideoNoSound, "video/mp4" },
+        { AssetKind.File, "application/octet-stream" },
+        { AssetKind.ServerContent, "application/octet-stream" },
+        { AssetKind.ServiceContent, "application/octet-stream" },
+        { AssetKind.Sound, "application/ogg" }
+    };
+
     public string GetMime()
-        => kind switch
-        {
-            AssetKind.Image          => "image/png",
-            AssetKind.Video          => "video/mp4",
-            AssetKind.VideoNoSound   => "video/mp4",
-            AssetKind.File           => "application/binary",
-            AssetKind.ServerContent  => "application/binary",
-            AssetKind.ServiceContent => "application/binary",
-            AssetKind.Sound          => "application/ogg",
-            _                        => throw new ArgumentOutOfRangeException(nameof(kind), kind, null)
-        };
+        => MimeTypes.TryGetValue(kind, out var mime)
+            ? mime
+            : throw new ArgumentOutOfRangeException(nameof(kind), kind, null);
📝 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.

Suggested change
public string GetMime()
=> kind switch
{
AssetKind.Image => "image/png",
AssetKind.Video => "video/mp4",
AssetKind.VideoNoSound => "video/mp4",
AssetKind.File => "application/binary",
AssetKind.ServerContent => "application/binary",
AssetKind.ServiceContent => "application/binary",
AssetKind.Sound => "application/ogg",
_ => throw new ArgumentOutOfRangeException(nameof(kind), kind, null)
};
private static readonly Dictionary<AssetKind, string> MimeTypes = new()
{
{ AssetKind.Image, "image/png" },
{ AssetKind.Video, "video/mp4" },
{ AssetKind.VideoNoSound, "video/mp4" },
{ AssetKind.File, "application/octet-stream" },
{ AssetKind.ServerContent, "application/octet-stream" },
{ AssetKind.ServiceContent, "application/octet-stream" },
{ AssetKind.Sound, "application/ogg" }
};
public string GetMime()
=> MimeTypes.TryGetValue(kind, out var mime)
? mime
: throw new ArgumentOutOfRangeException(nameof(kind), kind, null);

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}" }
};
return tags;
}
public static AssetId FromFileId(string fileId)
{
if (fileId.Length != 45)
throw new InvalidOperationException("Bad file id");
var span = fileId.AsSpan();
var guid = Guid.Parse(span.Slice(0, 36));
var scope = byte.Parse(span.Slice(37, 2));
var kind = byte.Parse(span.Slice(40, 2));
return new AssetId(guid, (AssetScope)scope, (AssetKind)kind);
}
Comment on lines +42 to +51
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve FromFileId parsing robustness.

The method could benefit from better validation and constant definitions.

+    private const int FileIdLength = 45;
+    private const int GuidLength = 36;
+    private const int ScopeOffset = 37;
+    private const int ScopeLength = 2;
+    private const int KindOffset = 40;
+    private const int KindLength = 2;
+
     public static AssetId FromFileId(string fileId)
     {
-        if (fileId.Length != 45)
+        if (fileId?.Length != FileIdLength)
             throw new InvalidOperationException("Bad file id");
+
         var span  = fileId.AsSpan();
-        var guid  = Guid.Parse(span.Slice(0, 36));
-        var scope = byte.Parse(span.Slice(37, 2));
-        var kind  = byte.Parse(span.Slice(40, 2));
+        if (!Guid.TryParse(span.Slice(0, GuidLength), out var guid))
+            throw new InvalidOperationException("Invalid GUID format");
+
+        if (!byte.TryParse(span.Slice(ScopeOffset, ScopeLength), out var scope))
+            throw new InvalidOperationException("Invalid scope format");
+
+        if (!byte.TryParse(span.Slice(KindOffset, KindLength), out var kind))
+            throw new InvalidOperationException("Invalid kind format");
+
+        if (!Enum.IsDefined(typeof(AssetScope), scope))
+            throw new InvalidOperationException($"Unknown scope value: {scope}");
+
+        if (!Enum.IsDefined(typeof(AssetKind), kind))
+            throw new InvalidOperationException($"Unknown kind value: {kind}");
+
         return new AssetId(guid, (AssetScope)scope, (AssetKind)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.

Suggested change
public static AssetId FromFileId(string fileId)
{
if (fileId.Length != 45)
throw new InvalidOperationException("Bad file id");
var span = fileId.AsSpan();
var guid = Guid.Parse(span.Slice(0, 36));
var scope = byte.Parse(span.Slice(37, 2));
var kind = byte.Parse(span.Slice(40, 2));
return new AssetId(guid, (AssetScope)scope, (AssetKind)kind);
}
private const int FileIdLength = 45;
private const int GuidLength = 36;
private const int ScopeOffset = 37;
private const int ScopeLength = 2;
private const int KindOffset = 40;
private const int KindLength = 2;
public static AssetId FromFileId(string fileId)
{
if (fileId?.Length != FileIdLength)
throw new InvalidOperationException("Bad file id");
var span = fileId.AsSpan();
if (!Guid.TryParse(span.Slice(0, GuidLength), out var guid))
throw new InvalidOperationException("Invalid GUID format");
if (!byte.TryParse(span.Slice(ScopeOffset, ScopeLength), out var scope))
throw new InvalidOperationException("Invalid scope format");
if (!byte.TryParse(span.Slice(KindOffset, KindLength), out var kind))
throw new InvalidOperationException("Invalid kind format");
if (!Enum.IsDefined(typeof(AssetScope), scope))
throw new InvalidOperationException($"Unknown scope value: {scope}");
if (!Enum.IsDefined(typeof(AssetKind), kind))
throw new InvalidOperationException($"Unknown kind value: {kind}");
return new AssetId(guid, (AssetScope)scope, (AssetKind)kind);
}


public static AssetId Avatar() => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.Image);
public static AssetId VideoAvatar() => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.VideoNoSound);
}

public enum AssetScope : byte
{
ProfileAsset,
ChatAsset,
ServiceAsset
}

public enum AssetKind : byte
{
Image,
Video, // only png
VideoNoSound, // gif
File,
ServerContent,
ServiceContent,
Sound
}
74 changes: 74 additions & 0 deletions src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
namespace Argon.Api.Features.MediaStorage;

using Storages;
using Genbox.SimpleS3.Core.Abstracts.Clients;
using Genbox.SimpleS3.Core.Abstracts.Enums;
using Genbox.SimpleS3.Core.Common.Authentication;
using Genbox.SimpleS3.Core.Extensions;
using Genbox.SimpleS3.Extensions.GenericS3.Extensions;
using Genbox.SimpleS3.Core.Abstracts.Request;
using Genbox.SimpleS3.Extensions.HttpClient.Extensions;

public static class CdnFeatureExtensions
{
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()
};
}
Comment on lines +14 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add proper error handling and validation

Several improvements needed for robustness:

  1. Add descriptive message for InvalidOperationException
  2. Add null checks for configuration sections
  3. 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.

Suggested change
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))
};
}


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);
}

return builder.Services;
}


public static IServiceCollection AddS3Storage(this WebApplicationBuilder builder, StorageKind keyName, StorageOptions options)
{
var storageContainer = new ServiceCollection();
var coreBuilder = SimpleS3CoreServices.AddSimpleS3Core(storageContainer);
coreBuilder.UseHttpClient();
coreBuilder.UseGenericS3(config =>
{
config.Endpoint = options.BaseUrl;
config.RegionCode = options.Region;
config.Credentials = new StringAccessKey(options.Login, options.Password);
config.NamingMode = NamingMode.PathStyle;
});

var storageServices = storageContainer.BuildServiceProvider();

builder.Services.AddKeyedSingleton<IServiceProvider>($"{keyName}:container", storageServices);
builder.Services.AddKeyedSingleton<IObjectClient>($"{keyName}:client", (services, o)
=> storageServices.GetRequiredService<IObjectClient>());

return builder.Services;
}
}
18 changes: 18 additions & 0 deletions src/Argon.Api/Features/MediaStorage/CdnOptions.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
namespace Argon.Api.Features.MediaStorage;

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; }
}
Comment on lines +3 to +10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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));
}
}


public readonly record struct StorageSpace(ulong total, ulong current, uint free);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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 enum UploadError
{
NONE,
INTERNAL_ERROR
}
28 changes: 28 additions & 0 deletions src/Argon.Api/Features/MediaStorage/DiskContentDeliveryNetwork.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
namespace Argon.Api.Features.MediaStorage;

using Contracts;

public class DiskContentDeliveryNetwork([FromKeyedServices(IContentStorage.DiskContentStorageKey)] IContentStorage storage,
ILogger<YandexContentDeliveryNetwork> logger) : IContentDeliveryNetwork
{
public IContentStorage Storage { get; } = storage;
Comment on lines +5 to +8
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix logger type and encapsulation issues

  1. The logger is incorrectly typed as ILogger<YandexContentDeliveryNetwork> but should be ILogger<DiskContentDeliveryNetwork>.
  2. The Storage property should be private as it's an implementation detail.

Apply this diff to fix these issues:

-public class DiskContentDeliveryNetwork([FromKeyedServices(IContentStorage.DiskContentStorageKey)] IContentStorage storage,
-    ILogger<YandexContentDeliveryNetwork> logger) : IContentDeliveryNetwork
+public class DiskContentDeliveryNetwork([FromKeyedServices(IContentStorage.DiskContentStorageKey)] IContentStorage storage,
+    ILogger<DiskContentDeliveryNetwork> logger) : IContentDeliveryNetwork
 {
-    public IContentStorage Storage { get; } = storage;
+    private readonly IContentStorage Storage = storage;
📝 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.

Suggested change
public class DiskContentDeliveryNetwork([FromKeyedServices(IContentStorage.DiskContentStorageKey)] IContentStorage storage,
ILogger<YandexContentDeliveryNetwork> logger) : IContentDeliveryNetwork
{
public IContentStorage Storage { get; } = storage;
public class DiskContentDeliveryNetwork([FromKeyedServices(IContentStorage.DiskContentStorageKey)] IContentStorage storage,
ILogger<DiskContentDeliveryNetwork> logger) : IContentDeliveryNetwork
{
private readonly IContentStorage Storage = storage;

public async ValueTask<Maybe<UploadError>> CreateAssetAsync(StorageNameSpace ns, AssetId asset, Stream file)
{
try
{
await Storage.UploadFile(ns, asset, file);
return Maybe<UploadError>.None();
}
catch (Exception e)
{
logger.LogCritical(e, $"Failed upload file '{asset.GetFilePath()}'");
return UploadError.INTERNAL_ERROR;
}
}

public ValueTask<Maybe<UploadError>> ReplaceAssetAsync(StorageNameSpace ns, AssetId asset, Stream file)
=> throw new NotImplementedException();
Comment on lines +23 to +24
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Implement ReplaceAssetAsync method

The method is currently throwing NotImplementedException. This is a critical issue as it breaks the IContentDeliveryNetwork interface contract and could cause runtime errors.

Consider implementing the method similar to CreateAssetAsync, but with additional steps to handle existing asset replacement:

-public ValueTask<Maybe<UploadError>> ReplaceAssetAsync(StorageNameSpace ns, AssetId asset, Stream file)
-    => throw new NotImplementedException();
+public async ValueTask<Maybe<UploadError>> ReplaceAssetAsync(StorageNameSpace ns, AssetId asset, Stream file)
+{
+    try
+    {
+        // TODO: Consider adding a check if the asset exists before replacing
+        await Storage.UploadFile(ns, asset, file);
+        logger.LogDebug("Successfully replaced file '{FilePath}' in namespace '{Namespace}'", 
+            asset.GetFilePath(), ns.ToPath());
+        return Maybe<UploadError>.None();
+    }
+    catch (Exception e)
+    {
+        logger.LogCritical(e, "Failed to replace file '{FilePath}' in namespace '{Namespace}'",
+            asset.GetFilePath(), ns.ToPath());
+        return UploadError.INTERNAL_ERROR;
+    }
+}

Committable suggestion skipped: line range outside the PR's diff.


public ValueTask<string> GenerateAssetUrl(StorageNameSpace ns, AssetId asset)
=> new($"/files/{ns.ToPath()}/{asset.GetFilePath()}?nocache=1");
}
Loading