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.

Sign up for GitHub

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

Jump to bottom

Feature/fusion auth #13

Merged
merged 3 commits into from
Nov 1, 2024
Merged

Feature/fusion auth #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
380f719
add fusion auth
0xF6 Oct 31, 2024
799b309
Merge remote-tracking branch 'origin/master' into feature/fusion-auth
0xF6 Oct 31, 2024
16990ba
fix
0xF6 Oct 31, 2024
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
52 changes: 30 additions & 22 deletions src/Argon.Api/Argon.Api.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,42 +1,46 @@
<Project Sdk="Microsoft.NET.Sdk.Web">
<Project Sdk="Microsoft.NET.Sdk.Web">

<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
<Nullable>enable</Nullable>
<ImplicitUsings>enable</ImplicitUsings>
<DockerDefaultTargetOS>Linux</DockerDefaultTargetOS>
<UserSecretsId>b90ebea2-7ea4-447f-b92f-46da1cfd6437</UserSecretsId>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="Flurl.Http" Version="4.0.2"/>
<PackageReference Include="Flurl.Http.Newtonsoft" Version="0.9.1"/>
<PackageReference Include="MemoryPack" Version="1.21.3"/>
<PackageReference Include="MemoryPack.Generator" Version="1.21.3">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
<PackageReference Include="Flurl.Http" Version="4.0.2" />
<PackageReference Include="Flurl.Http.Newtonsoft" Version="0.9.1" />
<PackageReference Include="MemoryPack" Version="1.21.3" />
<PackageReference Include="MessagePipe" Version="1.8.1" />
<PackageReference Include="MessagePipe.Analyzer" Version="1.8.1">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.10"/>
<PackageReference Include="ActualLab.Fusion" Version="9.5.52"/>
<PackageReference Include="ActualLab.Fusion.EntityFramework.Npgsql" Version="9.5.52"/>
<PackageReference Include="ActualLab.Rpc.Server" Version="9.5.52"/>
<PackageReference Include="Argon.Sfu.Protocol" Version="1.26.0"/>
<PackageReference Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="8.0.10" />
<PackageReference Include="ActualLab.Fusion" Version="9.5.52" />
<PackageReference Include="ActualLab.Fusion.EntityFramework.Npgsql" Version="9.5.52" />
<PackageReference Include="ActualLab.Rpc.Server" Version="9.5.52" />
<PackageReference Include="Argon.Sfu.Protocol" Version="1.26.0" />
<PackageReference Include="Microsoft.EntityFrameworkCore.Design" Version="8.0.10">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="Microsoft.Orleans.Persistence.AdoNet" Version="8.2.0"/>
<PackageReference Include="Microsoft.Orleans.Runtime" Version="8.2.0"/>
<PackageReference Include="Microsoft.Orleans.Sdk" Version="8.2.0"/>
<PackageReference Include="Microsoft.Orleans.Serialization" Version="8.2.0"/>
<PackageReference Include="Microsoft.Orleans.Server" Version="8.2.0"/>
<PackageReference Include="Npgsql" Version="8.0.5"/>
<PackageReference Include="Swashbuckle.AspNetCore" Version="6.4.0"/>
<PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="8.1.2"/>
<PackageReference Include="Microsoft.Extensions.Caching.Memory" Version="8.0.1" />
<PackageReference Include="Microsoft.Orleans.Persistence.AdoNet" Version="8.2.0" />
<PackageReference Include="Microsoft.Orleans.Runtime" Version="8.2.0" />
<PackageReference Include="Microsoft.Orleans.Sdk" Version="8.2.0" />
<PackageReference Include="Microsoft.Orleans.Serialization" Version="8.2.0" />
<PackageReference Include="Microsoft.Orleans.Server" Version="8.2.0" />
<PackageReference Include="Npgsql" Version="8.0.5" />
<PackageReference Include="R3" Version="1.2.9" />
<PackageReference Include="Swashbuckle.AspNetCore" Version="6.4.0" />
<PackageReference Include="System.IdentityModel.Tokens.Jwt" Version="8.1.2" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\Argon.Contracts\Argon.Contracts.csproj"/>
<ProjectReference Include="..\ServiceDefaults\ServiceDefaults.csproj"/>
<ProjectReference Include="..\Argon.Contracts\Argon.Contracts.csproj" />
<ProjectReference Include="..\ServiceDefaults\ServiceDefaults.csproj" />
</ItemGroup>

<ItemGroup>
Expand All @@ -45,4 +49,8 @@
</Content>
</ItemGroup>

<ItemGroup>
<Folder Include="Features\Jwt\" />
</ItemGroup>

</Project>
47 changes: 0 additions & 47 deletions src/Argon.Api/Extensions/JwtExtension.cs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

не надо удалять, нам это для тестов понадобится

This file was deleted.

74 changes: 74 additions & 0 deletions src/Argon.Api/Features/Jwt/JwtOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
namespace Argon.Api.Features.Jwt;

using Microsoft.AspNetCore.Authentication.JwtBearer;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.IdentityModel.Tokens;
using System.Text;
using Microsoft.Extensions.Options;
using Microsoft.AspNetCore.Authentication;
using Microsoft.Extensions.DependencyInjection.Extensions;

public record JwtOptions
{
public required string Issuer { get; set; }
public required string Audience { get; set; }
// TODO use cert in production
public required string Key { get; set; }
public required TimeSpan Expires { get; set; }

public void Deconstruct(out string issuer, out string audience, out string key)
{
audience = this.Audience;
issuer = this.Issuer;
key = this.Key;
}
Comment on lines +19 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

Fix the parameter assignment order in the Deconstruct method

The assignments in the Deconstruct method do not match the parameter order, which can lead to incorrect values when deconstructing the JwtOptions instance. Specifically, issuer and audience are assigned incorrectly.

Apply this diff to correct the assignment order:

 public void Deconstruct(out string issuer, out string audience, out string key)
 {
-    audience = this.Audience;
-    issuer = this.Issuer;
+    issuer = this.Issuer;
+    audience = this.Audience;
     key = this.Key;
 }
📝 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 void Deconstruct(out string issuer, out string audience, out string key)
{
audience = this.Audience;
issuer = this.Issuer;
key = this.Key;
}
public void Deconstruct(out string issuer, out string audience, out string key)
{
issuer = this.Issuer;
audience = this.Audience;
key = this.Key;
}

}


public static class JwtFeature
{
public static IServiceCollection AddJwt(this WebApplicationBuilder builder)
{
builder.Services.Configure<JwtOptions>(builder.Configuration.GetSection("Jwt"));
builder.Services.AddKeyedSingleton(JwtBearerDefaults.AuthenticationScheme,
(services, _) =>
{
var options = services.GetRequiredService<IOptions<JwtOptions>>();
var (issuer, audience, key) = options.Value;
return new TokenValidationParameters
{
ValidateIssuer = true,
ValidateAudience = true,
ValidateIssuerSigningKey = true,
ValidIssuer = issuer,
ValidAudience = audience,
IssuerSigningKey = new SymmetricSecurityKey(Encoding.UTF8.GetBytes(key)),
ClockSkew = TimeSpan.Zero
};
});

builder.Services.AddAuthentication(options =>
{
options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme;
options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme;
options.DefaultScheme = JwtBearerDefaults.AuthenticationScheme;
}).AddJwtBearer(o =>
{
o.Events = new JwtBearerEvents
{
OnMessageReceived = ctx =>
{
if (ctx.Request.Headers.TryGetValue("x-argon-token", out var value))
{
ctx.Token = value;
return Task.CompletedTask;
}

ctx.Response.StatusCode = 401;
return Task.CompletedTask;
}
Comment on lines +59 to +69
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid setting the response status code in the OnMessageReceived event

Modifying the response status code within the OnMessageReceived event is unnecessary and can interfere with the authentication middleware's default behavior. The middleware will automatically handle unauthorized responses when authentication fails.

Apply this diff to remove the manual status code assignment:

 OnMessageReceived = ctx =>
 {
     if (ctx.Request.Headers.TryGetValue("x-argon-token", out var value))
     {
         ctx.Token = value;
         return Task.CompletedTask;
     }

-    ctx.Response.StatusCode = 401;
     return Task.CompletedTask;
 }
📝 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
OnMessageReceived = ctx =>
{
if (ctx.Request.Headers.TryGetValue("x-argon-token", out var value))
{
ctx.Token = value;
return Task.CompletedTask;
}
ctx.Response.StatusCode = 401;
return Task.CompletedTask;
}
OnMessageReceived = ctx =>
{
if (ctx.Request.Headers.TryGetValue("x-argon-token", out var value))
{
ctx.Token = value;
return Task.CompletedTask;
}
return Task.CompletedTask;
}

🛠️ Refactor suggestion

Consider supporting the standard Authorization header alongside the custom header

To improve interoperability and adhere to HTTP standards, consider extracting the token from the standard Authorization header in addition to the custom x-argon-token header.

Update the OnMessageReceived event to check both headers:

 OnMessageReceived = ctx =>
 {
+    if (ctx.Request.Headers.TryGetValue("Authorization", out var authHeader))
+    {
+        var token = authHeader.ToString().Split(' ').Last();
+        ctx.Token = token;
+        return Task.CompletedTask;
+    }

     if (ctx.Request.Headers.TryGetValue("x-argon-token", out var value))
     {
         ctx.Token = value;
         return Task.CompletedTask;
     }

     return Task.CompletedTask;
 }
📝 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
OnMessageReceived = ctx =>
{
if (ctx.Request.Headers.TryGetValue("x-argon-token", out var value))
{
ctx.Token = value;
return Task.CompletedTask;
}
ctx.Response.StatusCode = 401;
return Task.CompletedTask;
}
OnMessageReceived = ctx =>
{
if (ctx.Request.Headers.TryGetValue("Authorization", out var authHeader))
{
var token = authHeader.ToString().Split(' ').Last();
ctx.Token = token;
return Task.CompletedTask;
}
if (ctx.Request.Headers.TryGetValue("x-argon-token", out var value))
{
ctx.Token = value;
return Task.CompletedTask;
}
ctx.Response.StatusCode = 401;
return Task.CompletedTask;
}

};
});
return builder.Services;
}
}
57 changes: 57 additions & 0 deletions src/Argon.Api/Features/Rpc/FusionAuthorizationMiddleware.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
namespace Argon.Api.Features.Rpc;

using ActualLab.Rpc.Infrastructure;
using ActualLab.Rpc;
using MemoryPack;
using Microsoft.Extensions.Caching.Distributed;
using ActualLab;
using ActualLab.Reflection;
using Grains;
using Grains.Persistence.States;
using Microsoft.AspNetCore.Authorization;
using Orleans;

public class FusionAuthorizationMiddleware(IServiceProvider Services, IGrainFactory GrainFactory) : RpcInboundMiddleware(Services)
{
public AsyncLocal<string> Token = new AsyncLocal<string>();
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove unused field Token

The field Token is declared but never used in the class. Removing unused code improves maintainability and readability.

Apply this diff to remove the unused field:

-public AsyncLocal<string> Token = new AsyncLocal<string>();
📝 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 AsyncLocal<string> Token = new AsyncLocal<string>();

public override async Task OnBeforeCall(RpcInboundCall call)
{
var existAttribute = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Count != 0;
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Use Any() method for checking attribute existence

Improve readability by using Any() instead of comparing Count != 0.

Apply this diff to refactor the code:

-var existAttribute = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Count != 0;
+var authorizeAttributeExists = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Any();
📝 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 existAttribute = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Count != 0;
var authorizeAttributeExists = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Any();


if (!existAttribute)
{
await base.OnBeforeCall(call);
return;
}

var grain = GrainFactory.GetGrain<IFusionSession>(call.Context.Peer.Id);

Comment on lines +27 to +28
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 null checks for call.Context and call.Context.Peer

To prevent potential NullReferenceException, ensure that call.Context and call.Context.Peer are not null before accessing Id.

Consider adding null checks:

if (call.Context?.Peer?.Id == null)
{
    call.Cancel();
    return;
}

var grain = GrainFactory.GetGrain<IFusionSession>(call.Context.Peer.Id);

var state = await grain.GetState();
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 exceptions when calling grain.GetState()

Ensure that any exceptions thrown by grain.GetState() are properly handled to maintain robustness.

Consider wrapping the call in a try-catch block:

FusionSession state;
try
{
    state = await grain.GetState();
}
catch (Exception ex)
{
    // Handle exception appropriately, e.g., log the error and cancel the call
    call.Cancel();
    return;
}

if (state.IsAuthorized)
{
await base.OnBeforeCall(call);
return;
}

call.Cancel();
return;
}
}

public class FusionServiceContext(IGrainFactory GrainFactory) : IFusionServiceContext
{
public ValueTask<FusionSession> GetSessionState()
{
var current = RpcInboundContext.GetCurrent();
var peerId = current.Peer.Id;
Comment on lines +45 to +46
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure current and current.Peer are not null

To avoid a NullReferenceException, check that current and current.Peer are not null before accessing Peer.Id.

Modify the code as follows:

var current = RpcInboundContext.GetCurrent();
if (current?.Peer?.Id == null)
{
    // Handle null case appropriately, e.g., throw an exception or return a default value
    throw new InvalidOperationException("Current RPC context or peer ID is not available.");
}

var peerId = current.Peer.Id;


var grain = GrainFactory.GetGrain<IFusionSession>(peerId);

return grain.GetState();
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 exceptions from grain.GetState()

Ensure that exceptions thrown by grain.GetState() are appropriately handled to maintain application stability.

Consider adding exception handling:

try
{
    return await grain.GetState();
}
catch (Exception ex)
{
    // Handle exception, e.g., log the error and throw a custom exception
    throw new InvalidOperationException("Failed to retrieve session state.", ex);
}

}
}

public interface IFusionServiceContext
{
ValueTask<FusionSession> GetSessionState();
}
14 changes: 14 additions & 0 deletions src/Argon.Api/Grains.Persistence.States/FusionSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
namespace Argon.Api.Grains.Persistence.States;

using Argon.Sfu;
using MemoryPack;

[GenerateSerializer]
[Serializable]
[MemoryPackable]
[Alias(nameof(FusionSession))]
public partial class FusionSession
{
[Id(0)] public required Guid Id { get; set; } = Guid.Empty;
[Id(1)] public required bool IsAuthorized { get; set; }
}
40 changes: 40 additions & 0 deletions src/Argon.Api/Grains/FusionGrain.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
namespace Argon.Api.Grains;

using System.IdentityModel.Tokens.Jwt;
using Microsoft.IdentityModel.Tokens;
using Persistence.States;

public class FusionGrain(
[PersistentState("sessions", "OrleansStorage")]
IPersistentState<FusionSession> sessionStorage,
TokenValidationParameters JwtParameters) : Grain, IFusionSession
{
public async ValueTask<bool> AuthorizeAsync(string token)
{
var tokenHandler = new JwtSecurityTokenHandler();
tokenHandler.ValidateToken(token, JwtParameters, out SecurityToken validatedToken);
var jwt = (JwtSecurityToken)validatedToken;

sessionStorage.State.Id = Guid.Parse(jwt.Id);
sessionStorage.State.IsAuthorized = true;
Comment on lines +18 to +19
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 invalid GUIDs when parsing JWT ID

When converting jwt.Id to a Guid, using Guid.Parse can throw an exception if the Id claim is not a valid GUID string. To prevent this, use Guid.TryParse and handle the scenario where parsing fails.

Apply this diff to safely parse the JWT ID:

-    sessionStorage.State.Id = Guid.Parse(jwt.Id);
+    if (Guid.TryParse(jwt.Id, out var sessionId))
+    {
+        _sessionStorage.State.Id = sessionId;
+    }
+    else
+    {
+        // Invalid GUID in JWT ID
+        return false;
+    }

Committable suggestion was skipped due to low confidence.

await sessionStorage.WriteStateAsync();
return true;
}
Comment on lines +12 to +22
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 exception handling in AuthorizeAsync to handle token validation errors

In the AuthorizeAsync method, the call to ValidateToken can throw exceptions (e.g., SecurityTokenException, ArgumentException) if the token is invalid or malformed. It's important to handle these exceptions to prevent the grain from crashing and to provide a proper response when token validation fails.

Consider wrapping the token validation in a try-catch block and returning false if validation fails:

 public async ValueTask<bool> AuthorizeAsync(string token)
 {
     var tokenHandler = new JwtSecurityTokenHandler();
-    tokenHandler.ValidateToken(token, JwtParameters, out SecurityToken validatedToken);
+    SecurityToken validatedToken;
+    try
+    {
+        tokenHandler.ValidateToken(token, _jwtParameters, out validatedToken);
+    }
+    catch (Exception)
+    {
+        // Token validation failed
+        return false;
+    }
     var jwt = (JwtSecurityToken)validatedToken;
-    sessionStorage.State.Id = Guid.Parse(jwt.Id);
-    sessionStorage.State.IsAuthorized = true;
-    await sessionStorage.WriteStateAsync();
+    if (Guid.TryParse(jwt.Id, out var sessionId))
+    {
+        _sessionStorage.State.Id = sessionId;
+        _sessionStorage.State.IsAuthorized = true;
+        await _sessionStorage.WriteStateAsync();
+        return true;
+    }
+    else
+    {
+        // Invalid GUID in JWT ID
+        return false;
+    }
-    return true;
 }

This change also incorporates safe parsing of jwt.Id using Guid.TryParse to handle cases where the Id claim might not be a valid GUID.

Committable suggestion was skipped due to low confidence.


public async ValueTask<FusionSession> GetState()
{
await sessionStorage.ReadStateAsync();
return sessionStorage.State;
}
Comment on lines +24 to +28
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Optimize state retrieval in GetState

In the GetState method, you are calling ReadStateAsync() every time the method is invoked. If the state is already loaded and hasn't changed, this may be unnecessary and could introduce latency.

Consider checking if the state is already loaded before reading it again:

 public async ValueTask<FusionSession> GetState()
 {
-    await sessionStorage.ReadStateAsync();
+    if (!_sessionStorage.RecordExists)
+    {
+        await _sessionStorage.ReadStateAsync();
+    }
     return _sessionStorage.State;
 }

This change checks if the state exists before reading it, which can improve performance by avoiding unnecessary I/O operations.

Committable suggestion was skipped due to low confidence.


}


public interface IFusionSession : IGrainWithGuidKey
{
[Alias("AuthorizeAsync")]
ValueTask<bool> AuthorizeAsync(string token);

[Alias("GetState")]
ValueTask<FusionSession> GetState();
}
11 changes: 9 additions & 2 deletions src/Argon.Api/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
using ActualLab.Fusion;
using ActualLab.Fusion.Extensions;
using ActualLab.Rpc;
using ActualLab.Rpc.Infrastructure;
using ActualLab.Rpc.Server;
using Argon.Api.Entities;
using Argon.Api.Extensions;
using Argon.Api.Features.Jwt;
using Argon.Api.Features.Rpc;
using Argon.Api.Filters;
using Argon.Api.Migrations;
using Argon.Api.Services;
Expand All @@ -11,6 +15,7 @@

var builder = WebApplication.CreateBuilder(args);

builder.AddJwt();
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Security concern: JWT implementation needs improvements

The JWT configuration has several security considerations that need to be addressed:

  • There's a TODO comment indicating temporary use of a string key instead of certificates: // TODO use cert in production
  • The JWT configuration uses a symmetric key with HS256 algorithm, which may not be the best choice for all scenarios
  • The token validation parameters are properly configured with all necessary validations enabled, but the key management needs improvement
  • Custom header x-argon-token is used instead of the standard Authorization: Bearer header

Recommendations:

  • Replace the symmetric key with asymmetric keys (certificates) as noted in the TODO
  • Consider using RS256 instead of HS256 for better security in distributed systems
  • Implement proper key rotation mechanisms
  • Use standard Authorization header instead of custom header unless there's a specific requirement
🔗 Analysis chain

Verify JWT configuration implementation.

The JWT configuration is added, but we should verify the security settings in the implementation.

Let's check the JWT configuration implementation:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify JWT configuration implementation
# Look for JWT configuration settings like token validation parameters, 
# key management, and security settings

# Check JWT options configuration
ast-grep --pattern 'class JwtOptions {
  $$$
}'

# Check for secure key management
rg -l "SecurityKey|SigningKey|IssuerSigningKey" --type cs

Length of output: 221

Script:

#!/bin/bash
# Let's try a different approach to find JWT-related configurations

# Search for JWT-related files and configurations
rg -l "JWT|JsonWebToken" --type cs

# Look for AddJwt extension method implementation
ast-grep --pattern 'AddJwt($$$) {
  $$$
}'

# Search for token validation parameters
rg "TokenValidationParameters" -A 5 --type cs

# Look for authentication/JWT middleware setup
rg "UseAuthentication|UseJwtBearer" --type cs

Length of output: 1233

Script:

#!/bin/bash
# Let's examine the JWT configuration implementation in more detail

# Check the complete JwtOptions class implementation
cat src/Argon.Api/Features/Jwt/JwtOptions.cs

# Look for the AddJwt extension method implementation
rg -A 10 "AddJwt" --type cs

# Check for any security key configuration
rg "SecurityKey|SigningKey|IssuerSigningKey" -A 3 --type cs

Length of output: 6852

builder.AddServiceDefaults();
builder.AddRedisOutputCache("cache");
builder.AddRabbitMQClient("rmq");
Expand All @@ -19,12 +24,13 @@
builder.Services.AddFusion(RpcServiceMode.Server, true)
.Rpc.AddServer<IUserAuthorization, UserAuthorization>()
.AddServer<IUserInteraction, UserInteractionService>()
.AddInboundMiddleware<FusionAuthorizationMiddleware>()
.AddWebSocketServer(true);
builder.AddSwaggerWithAuthHeader();
builder.AddJwt();
builder.Services.AddAuthorization();
builder.AddSelectiveForwardingUnit();
builder.Services.AddTransient<UserManagerService>();
builder.Services.AddTransient<IFusionServiceContext, FusionServiceContext>();
builder.AddOrleans();
var app = builder.Build();
app.UseSwagger();
Expand All @@ -34,7 +40,8 @@
app.UseAuthorization();
app.MapControllers();
app.MapDefaultEndpoints();
app.UseWebSockets();
app.MapRpcWebSocketServer();
var buildTime = File.GetLastWriteTimeUtc(typeof(Program).Assembly.Location);
app.MapGet("/", () => new { buildTime });
await app.WarpUp<ApplicationDbContext>().RunAsync();
await app.WarpUp<ApplicationDbContext>().RunAsync();
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 typo in method name.

There appears to be a typo in the method name: WarpUp should be WarmUp.

Apply this fix:

-await app.WarpUp<ApplicationDbContext>().RunAsync();
+await app.WarmUp<ApplicationDbContext>().RunAsync();
📝 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
await app.WarpUp<ApplicationDbContext>().RunAsync();
await app.WarmUp<ApplicationDbContext>().RunAsync();

6 changes: 3 additions & 3 deletions src/Argon.Api/Properties/launchSettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
"http": {
"commandName": "Project",
"dotnetRunMessages": true,
"launchBrowser": true,
"launchBrowser": false,
"launchUrl": "swagger",
"applicationUrl": "http://localhost:5100",
"environmentVariables": {
Expand All @@ -22,7 +22,7 @@
"https": {
"commandName": "Project",
"dotnetRunMessages": true,
"launchBrowser": true,
"launchBrowser": false,
"launchUrl": "swagger",
"applicationUrl": "https://localhost:7206;http://localhost:5100",
"environmentVariables": {
Expand All @@ -31,7 +31,7 @@
},
"IIS Express": {
"commandName": "IISExpress",
"launchBrowser": true,
"launchBrowser": false,
"launchUrl": "swagger",
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
Expand Down