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

Enhance serialization and contracts #12

Merged
merged 9 commits into from
Oct 27, 2024
4 changes: 4 additions & 0 deletions src/Argon.Api/Argon.Api.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
<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>
<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"/>
Expand Down
2 changes: 1 addition & 1 deletion src/Argon.Api/Controllers/UsersController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public async Task<ActionResult<UserStorageDto>> Post([FromBody] UserInputDto dto
public async Task<ActionResult<string>> Authenticate([FromBody] UserInputDto dto)
{
var userManager = grainFactory.GetGrain<IUserManager>(dto.Username);
var token = await userManager.Authenticate(dto.Password);
var token = await userManager.Authorize(dto.Password);
return token;
}

Expand Down
4 changes: 2 additions & 2 deletions src/Argon.Api/Grains.Interfaces/IUserManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ public interface IUserManager : IGrainWithStringKey
[Alias("Get")]
Task<UserStorageDto> Get();

[Alias("Authenticate")]
Task<string> Authenticate(string password);
[Alias("Authorize")]
Task<string> Authorize(string password);
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

Method name mismatch between interface and usage

The verification revealed inconsistencies in the codebase:

  • The interface method is marked with [Alias("Authenticate")] but named Authorize
  • The controller endpoint is named authenticate
  • The JWT configuration uses authentication-related terminology

This indicates that:

  1. The method is indeed performing authentication (generating JWT tokens)
  2. The rename to Authorize is inconsistent with both its alias and usage
  3. The codebase maintains authentication terminology everywhere except the method name

Recommendation:

  • Revert the method name back to Authenticate to maintain consistency with its alias, endpoint name, and actual functionality
  • If authorization is needed, it should be implemented as a separate concern (as shown by the [Authorize] attribute usage in the controller)
🔗 Analysis chain

Reconsider renaming Authenticate to Authorize method.

The renaming of this method could lead to confusion as authentication and authorization are distinct security concepts:

  • Authentication verifies identity ("who you are")
  • Authorization determines permissions ("what you can do")

Since this method accepts a password and likely returns an authentication token, it appears to be performing authentication rather than authorization. Consider keeping the original Authenticate name to maintain clarity and accurate terminology.

Let's verify the impact of this rename across the codebase:


Let me gather more context about the actual implementation and usage to make a more informed decision.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining references to the old method name and verify the consistency of the change

# Test 1: Look for any remaining references to "Authenticate" that might have been missed
echo "Checking for remaining 'Authenticate' references..."
rg -l 'Authenticate'

# Test 2: Verify all calls to IUserManager use the new method name
echo "Checking usage of the new 'Authorize' method..."
rg -A 2 'IUserManager.*Authorize'

# Test 3: Check for any potential authentication/authorization related comments or documentation
echo "Checking related documentation..."
rg -l -g '*.md' -g '*.cs' 'auth[a-z]*'

Length of output: 935


Script:

#!/bin/bash
# Check the content of relevant files to understand the authentication flow

# Check the implementation in UserAuthorization.cs
echo "=== UserAuthorization.cs ==="
rg -A 10 'Authorize.*password' src/Argon.Api/Services/UserAuthorization.cs

# Check the controller implementation
echo -e "\n=== UsersController.cs ==="
rg -A 10 'auth' src/Argon.Api/Controllers/UsersController.cs

# Check JWT extension to understand token generation
echo -e "\n=== JwtExtension.cs ==="
rg -A 10 'Authenticate' src/Argon.Api/Extensions/JwtExtension.cs

# Check the full interface definition
echo -e "\n=== IUserManager.cs ==="
cat src/Argon.Api/Grains.Interfaces/IUserManager.cs

Length of output: 2493


[Alias("CreateServer")]
Task<ServerStorage> CreateServer(string name, string description);
Expand Down
15 changes: 15 additions & 0 deletions src/Argon.Api/Grains.Persistence.States/ChannelStorage.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
namespace Argon.Api.Grains.Persistence.States;

using Contracts;
using MemoryPack;

[GenerateSerializer]
Expand All @@ -16,4 +17,18 @@ public sealed partial record ChannelStorage
[Id(5)] public ServerRole AccessLevel { get; set; } = ServerRole.User;
[Id(6)] public DateTime CreatedAt { get; } = DateTime.UtcNow;
[Id(7)] public DateTime UpdatedAt { get; set; } = DateTime.UtcNow;

public static implicit operator ServerDetailsResponse(ChannelStorage channelStorage)
{
return new ServerDetailsResponse(
channelStorage.Id,
channelStorage.Name,
channelStorage.Description,
channelStorage.CreatedBy.ToString("N"),
channelStorage.ChannelType.ToString(),
channelStorage.AccessLevel.ToString(),
channelStorage.CreatedAt,
channelStorage.UpdatedAt
);
}
}
14 changes: 14 additions & 0 deletions src/Argon.Api/Grains.Persistence.States/ServerStorage.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
namespace Argon.Api.Grains.Persistence.States;

using Contracts;
using MemoryPack;

[GenerateSerializer]
Expand All @@ -15,4 +16,17 @@ public sealed partial record ServerStorage
[Id(6)] public List<Guid> Channels { get; set; } = [];
[Id(3)] public DateTime CreatedAt { get; } = DateTime.UtcNow;
[Id(4)] public DateTime UpdatedAt { get; set; } = DateTime.UtcNow;

public static implicit operator ServerResponse(ServerStorage serverStorage)
{
return new ServerResponse(
serverStorage.Id,
serverStorage.Name,
serverStorage.Description,
serverStorage.AvatarUrl,
serverStorage.Channels,
serverStorage.CreatedAt,
serverStorage.UpdatedAt
);
}
}
43 changes: 37 additions & 6 deletions src/Argon.Api/Grains.Persistence.States/UserStorage.cs
Original file line number Diff line number Diff line change
@@ -1,19 +1,50 @@
namespace Argon.Api.Grains.Persistence.States;

using System.Runtime.Serialization;
using MemoryPack;
using MessagePack;

[GenerateSerializer]
[Serializable]
[MemoryPackable]
[Alias(nameof(UserStorage))]
public sealed partial record UserStorage
{
[Id(0)] public Guid Id { get; set; } = Guid.Empty;
[Id(1)] public string Username { get; set; } = string.Empty;
[Id(2)] public string Password { get; set; } = string.Empty;
[Id(5)] public string AvatarUrl { get; set; } = string.Empty;
[Id(3)] public DateTime CreatedAt { get; } = DateTime.UtcNow;
[Id(4)] public DateTime UpdatedAt { get; set; } = DateTime.UtcNow;
[property: DataMember(Order = 0)]
[property: MemoryPackOrder(0)]
[property: Key(0)]
[Id(0)]
public Guid Id { get; set; } = Guid.Empty;

[property: DataMember(Order = 1)]
[property: MemoryPackOrder(1)]
[property: Key(1)]
[Id(1)]
public string Username { get; set; } = string.Empty;

[property: DataMember(Order = 2)]
[property: MemoryPackOrder(2)]
[property: Key(2)]
[Id(2)]
public string Password { get; set; } = string.Empty;
Comment on lines +25 to +29
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security concern: Password field is being serialized.

The Password field is marked for serialization across multiple frameworks. This could lead to security vulnerabilities if the serialized data is logged, cached, or transmitted.

Consider one of these approaches:

  1. Exclude password from serialization:
-    [property: DataMember(Order = 2)]
-    [property: MemoryPackOrder(2)]
-    [property: Key(2)]
     [Id(2)]
+    [IgnoreDataMember]
+    [MemoryPackIgnore]
     public string Password { get; set; } = string.Empty;
  1. Store only the password hash instead of the plain text password.

Committable suggestion was skipped due to low confidence.


[property: DataMember(Order = 5)]
[property: MemoryPackOrder(5)]
[property: Key(5)]
[Id(5)]
public string AvatarUrl { get; set; } = string.Empty;
Comment on lines +31 to +35
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 inconsistent property ordering.

The serialization order numbers are not sequential:

  • Username: order 1
  • Password: order 2
  • AvatarUrl: order 5 (gap)
  • CreatedAt: order 3
  • UpdatedAt: order 4

This inconsistency could lead to issues with serialization/deserialization compatibility.

Recommend reordering to be sequential:

-    [property: DataMember(Order = 5)]
-    [property: MemoryPackOrder(5)]
-    [property: Key(5)]
+    [property: DataMember(Order = 3)]
+    [property: MemoryPackOrder(3)]
+    [property: Key(3)]
     public string AvatarUrl { get; set; } = string.Empty;

Also applies to: 19-23, 37-41, 43-47


[property: DataMember(Order = 3)]
[property: MemoryPackOrder(3)]
[property: Key(3)]
[Id(3)]
public DateTime CreatedAt { get; } = DateTime.UtcNow;

[property: DataMember(Order = 4)]
[property: MemoryPackOrder(4)]
[property: Key(4)]
[Id(4)]
public DateTime UpdatedAt { get; set; } = DateTime.UtcNow;

public static implicit operator UserStorageDto(UserStorage userStorage)
{
Expand Down
14 changes: 14 additions & 0 deletions src/Argon.Api/Grains.Persistence.States/UserStorageDto.cs
Original file line number Diff line number Diff line change
@@ -1,16 +1,30 @@
namespace Argon.Api.Grains.Persistence.States;

using Contracts;
using Mapster;
using MemoryPack;

[GenerateSerializer]
[Serializable]
[MemoryPackable]
[Alias(nameof(UserStorageDto))]
[GenerateMapper]
public sealed partial record UserStorageDto
{
[Id(0)] public Guid Id { get; set; }
[Id(1)] public string Username { get; set; } = string.Empty;
[Id(4)] public string AvatarUrl { get; set; } = string.Empty;
[Id(2)] public DateTime CreatedAt { get; set; }
[Id(3)] public DateTime UpdatedAt { get; set; }

public static implicit operator UserResponse(UserStorageDto userStorageDto)
{
return new UserResponse(
userStorageDto.Id,
userStorageDto.Username,
userStorageDto.AvatarUrl,
userStorageDto.CreatedAt,
userStorageDto.UpdatedAt
);
}
}
2 changes: 1 addition & 1 deletion src/Argon.Api/Grains/UserManager.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public async Task<UserStorageDto> Get()
return userStore.State;
}

public Task<string> Authenticate(string password)
public Task<string> Authorize(string password)
{
var match = userStore.State.Password == HashPassword(password);

Expand Down
1 change: 1 addition & 0 deletions src/Argon.Api/Program.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
builder.Services.AddControllers(opts => { opts.Filters.Add<InjectUsernameFilter>(); });
builder.Services.AddFusion(RpcServiceMode.Server, true)
.Rpc.AddServer<IUserAuthorization, UserAuthorization>()
.AddServer<IUserInteraction, UserInteractionService>()
.AddWebSocketServer(true);
builder.AddSwaggerWithAuthHeader();
builder.AddJwt();
Expand Down
6 changes: 3 additions & 3 deletions src/Argon.Api/Services/UserAuthorization.cs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
namespace Argon.Api.Services;

using Grains.Interfaces;
using Contracts;
using Grains.Interfaces;

public class UserAuthorization(IGrainFactory grainFactory) : IUserAuthorization
{
public async Task<AuthorizeResponse> AuthorizeAsync(AuthorizeRequest request)
{
// TODO machineKey
var token = await grainFactory.GetGrain<IUserManager>(request.username).Authenticate(request.password);
return new AuthorizeResponse(token, [new ServerResponse(Guid.NewGuid(), "xuita", null)]);
var token = await grainFactory.GetGrain<IUserManager>(request.username).Authorize(request.password);
return new AuthorizeResponse(token);
}
}
41 changes: 41 additions & 0 deletions src/Argon.Api/Services/UserInteractionService.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
namespace Argon.Api.Services;

using Contracts;
using Grains.Interfaces;

public class UserInteractionService(
string username, // TODO to be injected
IGrainFactory grainFactory
) : IUserInteraction
{
private readonly IUserManager userManager = grainFactory.GetGrain<IUserManager>(username);

public async Task<UserResponse> GetMe()
{
return await userManager.Get();
}

public async Task<ServerResponse> CreateServer(CreateServerRequest request)
{
return await userManager.CreateServer(request.Name, request.Description);
}

public async Task<List<ServerResponse>> GetServers()
{
return (await userManager.GetServers())
.Select(x => (ServerResponse)x)
.ToList();
}

public async Task<List<ServerDetailsResponse>> GetServerDetails(ServerDetailsRequest request)
{
return (await userManager.GetServerChannels(request.ServerId))
.Select(x => (ServerDetailsResponse)x)
.ToList();
}

public async Task<ChannelJoinResponse> JoinChannel(ChannelJoinRequest request)
{
return new ChannelJoinResponse((await userManager.JoinChannel(request.ServerId, request.ChannelId)).value);
}
}
35 changes: 18 additions & 17 deletions src/Argon.Contracts/Argon.Contracts.csproj
Original file line number Diff line number Diff line change
@@ -1,22 +1,23 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>
<PropertyGroup>
<TargetFramework>net8.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="ActualLab.Fusion" Version="9.5.52" />
<PackageReference Include="ActualLab.Fusion.Ext.Contracts" Version="9.5.52" />
<PackageReference Include="ActualLab.Generators" Version="9.5.52">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="MemoryPack.Generator" Version="1.21.3">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
</ItemGroup>
<ItemGroup>
<PackageReference Include="ActualLab.Fusion" Version="9.5.52"/>
<PackageReference Include="ActualLab.Fusion.Ext.Contracts" Version="9.5.52"/>
<PackageReference Include="ActualLab.Generators" Version="9.5.52">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
<PackageReference Include="Mapster" Version="7.4.0"/>
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Mapster integration needs configuration setup

The codebase shows minimal Mapster usage with potential issues:

  • UserStorageDto has [GenerateMapper] attribute but no corresponding generated mapper files found
  • No global Mapster configuration or registration setup detected
  • Currently relies on manual mapping via implicit operator instead of Mapster's features

To properly integrate Mapster:

  • Add Mapster.Tool package for code generation
  • Create global mapping configuration
  • Remove manual mapping in favor of generated mappers
🔗 Analysis chain

Verify Mapster usage across the codebase.

The addition of Mapster looks good. Let's verify its integration with the existing codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify Mapster usage and configuration
# Expected: Find mapping configurations and usages to ensure proper setup

# Check for Mapster configuration classes
echo "Searching for Mapster configuration..."
rg -l "IRegister|TypeAdapterConfig" 

# Check for Mapster usage
echo "Searching for Mapster usage..."
rg -l "using Mapster|\.Adapt<|\.Adapt\(" 

Length of output: 296


Script:

#!/bin/bash
# Let's examine the actual usage in the found file
echo "Checking Mapster usage in UserStorageDto.cs:"
rg -A 5 -B 5 "using Mapster|\.Adapt<|\.Adapt\(" "src/Argon.Api/Grains.Persistence.States/UserStorageDto.cs"

# Let's also check for any recent changes to this file
echo -e "\nChecking recent changes to UserStorageDto.cs:"
git log -p "src/Argon.Api/Grains.Persistence.States/UserStorageDto.cs"

# Let's verify if there are any mapping configurations in Program.cs or Startup.cs
echo -e "\nChecking for potential Mapster configuration in startup files:"
fd -e cs "Program|Startup" --exec rg -l "MapsterConfig|TypeAdapterConfig|RegisterMaps"

Length of output: 3164


Script:

#!/bin/bash
# Let's check for the GenerateMapper attribute usage and any mapping configurations
echo "Checking for Mapster configuration attributes:"
rg -l "\[GenerateMapper\]"

# Let's check for UserResponse to understand the mapping context
echo -e "\nChecking UserResponse definition:"
rg -A 10 "record UserResponse" 

# Let's check for any Mapster-related files in the solution
echo -e "\nChecking for Mapster-related files:"
fd -e cs -e cs.generated "Mapper|MapRegister|TypeAdapter"

Length of output: 1234

<PackageReference Include="MemoryPack.Generator" Version="1.21.3">
<PrivateAssets>all</PrivateAssets>
<IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
</PackageReference>
</ItemGroup>

</Project>
35 changes: 20 additions & 15 deletions src/Argon.Contracts/IUserAuthorization.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
namespace Argon.Contracts;

using System.Runtime.Serialization;
using ActualLab.Fusion;
using ActualLab.Rpc;
using MemoryPack;
using MessagePack;
Expand All @@ -11,19 +10,25 @@ public interface IUserAuthorization : IRpcService
Task<AuthorizeResponse> AuthorizeAsync(AuthorizeRequest request);
}

[DataContract, MemoryPackable(GenerateType.VersionTolerant), MessagePackObject]
[DataContract]
[MemoryPackable(GenerateType.VersionTolerant)]
[MessagePackObject]
public sealed partial record AuthorizeRequest(
[property: DataMember(Order = 0), MemoryPackOrder(0), Key(0)] string username,
[property: DataMember(Order = 1), MemoryPackOrder(1), Key(1)] string password,
[property: DataMember(Order = 2), MemoryPackOrder(2), Key(2)] string machineKey);
[property: DataMember(Order = 0)]
[property: MemoryPackOrder(0)]
[property: Key(0)]
string username,
[property: DataMember(Order = 1)]
[property: MemoryPackOrder(1)]
[property: Key(1)]
string password,
[property: DataMember(Order = 2)]
[property: MemoryPackOrder(2)]
[property: Key(2)]
string machineKey);
Comment on lines +13 to +28
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Add input validation and security measures for sensitive data

Based on the codebase analysis:

  1. The AuthorizeRequest record handles sensitive data without any validation
  2. The UserAuthorization service directly passes the raw password to IUserManager without validation
  3. No logging protection or sensitive data handling mechanisms are present

Required changes:

  • Add data validation attributes to the record:
[Required, StringLength(100)]
string username,
[Required, StringLength(256)]
string password,
[Required]
string machineKey
  • Implement request validation in UserAuthorization.AuthorizeAsync
  • Add sensitive data protection for logging
🔗 Analysis chain

Add input validation and security measures.

The record handles sensitive data (username, password) but lacks input validation. Consider:

  1. Adding validation attributes (e.g., Required, StringLength) to prevent empty or oversized inputs
  2. Implementing password complexity requirements
  3. Adding [SensitiveData] or similar attributes to ensure proper handling in logs
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if sensitive data is properly handled across the codebase
# Look for potential logging of AuthorizeRequest
rg -A 5 "AuthorizeRequest.*log|log.*AuthorizeRequest"

# Check for any validation attributes usage in similar contexts
ast-grep --pattern 'attribute { name: /Required|StringLength|MinLength/ }'

Length of output: 3585


Script:

#!/bin/bash
# Let's check for any existing validation attributes or security measures in the codebase
rg -A 3 "\[(?:Required|StringLength|MinLength|MaxLength|RegularExpression|SensitiveData)\]"

# Check for any password-related validation or security measures
rg -A 5 "password.*validation|validate.*password"

# Look for logging configurations or sensitive data handling patterns
rg -A 3 "ILogger|LoggerFactory|SensitiveData|SecureString"

# Check for any existing validation in the authentication flow
ast-grep --pattern 'class { name: /.*Auth.*/ }'

Length of output: 1560


Script:

#!/bin/bash
# Let's check the actual implementation of authentication/authorization
rg -A 10 "class.*UserAuthorization"

# Check for any existing validation in the request handling
rg -A 10 "AuthorizeRequest.*async"

# Look for any security-related configurations
rg -A 5 "security|authentication|authorization" src/Argon.Api/Program.cs src/Argon.Api/Startup.cs

# Check if there's any request validation middleware
rg -A 5 "middleware.*validation|validate.*request"

Length of output: 1047



public sealed partial record ServerResponse(
[property: DataMember(Order = 0), MemoryPackOrder(0), Key(0)] Guid serverId,
[property: DataMember(Order = 1), MemoryPackOrder(1), Key(1)] string serverName,
[property: DataMember(Order = 2), MemoryPackOrder(2), Key(2)] string? avatarFileId
);

public sealed partial record AuthorizeResponse(
[property: DataMember(Order = 0), MemoryPackOrder(0), Key(0)] string token,
[property: DataMember(Order = 1), MemoryPackOrder(1), Key(1)] List<ServerResponse> servers);
public sealed record AuthorizeResponse(
[property: DataMember(Order = 0)]
[property: MemoryPackOrder(0)]
[property: Key(0)]
string token);
Loading