Feature/fusion auth #13
Conversation
WalkthroughThe pull request introduces several changes to the Argon API project, primarily focusing on JWT authentication and authorization enhancements. Key modifications include the addition of new properties and package references in the project file, the introduction of new classes and methods for JWT configuration, and the implementation of middleware for RPC authorization. Additionally, the launch settings have been updated to prevent automatic browser launching. These changes aim to improve the project's structure and functionality without altering existing features. Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range and nitpick comments (7)
src/Argon.Api/Grains.Persistence.States/FusionSession.cs (2)
10-14: Consider adding validation and documentation.A few suggestions to improve the robustness of this class:
- Add XML documentation to describe the purpose of the class and its properties
- Consider adding validation to ensure Id is not Guid.Empty after initialization
Here's a suggested improvement:
+ /// <summary> + /// Represents a session state for Fusion authentication. + /// </summary> public partial class FusionSession { + /// <summary> + /// Gets or sets the unique identifier for the session. + /// </summary> [Id(0)] public required Guid Id { get; set; } = Guid.Empty; + /// <summary> + /// Gets or sets whether the session is authorized. + /// </summary> [Id(1)] public required bool IsAuthorized { get; set; } }
10-14: Consider implementing immutability for security.Since this class is used for session management and authorization, consider:
- Making properties immutable after initialization
- Implementing proper access control on setters
- Adding audit logging for state changes
Here's a suggested approach:
public partial class FusionSession { - [Id(0)] public required Guid Id { get; set; } = Guid.Empty; - [Id(1)] public required bool IsAuthorized { get; set; } + [Id(0)] public required Guid Id { get; init; } = Guid.Empty; + [Id(1)] private bool isAuthorized; + + [Id(1)] + public bool IsAuthorized + { + get => isAuthorized; + set + { + // Add audit logging here + isAuthorized = value; + } + } }src/Argon.Api/Properties/launchSettings.json (1)
Documentation needed for browser launch settings
The change to disable automatic browser launch (
"launchBrowser": false) in the launch settings is not documented anywhere in the repository. Since this affects the local development workflow, especially for authentication testing, it should be documented in a README.md or similar guide.
- Create a README.md file in the repository root
- Document the manual steps needed to test the API and authentication flows
- Explain why automatic browser launch is disabled by default
🔗 Analysis chain
Line range hint
1-41: Consider documenting this change in README or contributing guidelines.Since this changes the default developer experience, it would be helpful to document why browsers won't auto-launch and how developers should test the authentication flows.
Let me check if there are any documentation files in other common locations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this behavior change is documented rg -l "browser.*launch|launch.*browser" README.md docs/ || echo "No documentation found about browser launch behavior"Length of output: 268
Script:
#!/bin/bash # Look for documentation files in common locations fd -t f -e mdLength of output: 26
src/Argon.Api/Program.cs (1)
Line range hint
18-47: Consider adding integration tests and deployment documentation.This PR introduces significant authentication changes with multiple components:
- JWT authentication
- RPC authorization middleware
- WebSocket communication
- Service context management
To ensure reliability:
- Add integration tests covering the authentication flow
- Document the JWT configuration requirements for different environments
- Include WebSocket endpoint security considerations in deployment docs
Would you like assistance in creating integration test templates or deployment documentation?
src/Argon.Api/Grains/FusionGrain.cs (2)
35-36: Confirm necessity of[Alias("AuthorizeAsync")]attributeApplying
[Alias("AuthorizeAsync")]to theAuthorizeAsyncmethod may be redundant if the method name hasn't changed. Ensure that this attribute is necessary for your application's serialization or versioning requirements.If the attribute is not required, consider removing it to simplify the code:
- [Alias("AuthorizeAsync")] ValueTask<bool> AuthorizeAsync(string token);
38-39: Confirm necessity of[Alias("GetState")]attributeSimilar to the previous comment, verify if the
[Alias("GetState")]attribute is needed for theGetStatemethod. If not required, removing it can reduce complexity.Consider this change:
- [Alias("GetState")] ValueTask<FusionSession> GetState();src/Argon.Api/Features/Jwt/JwtOptions.cs (1)
15-15: Address the TODO: Use a certificate for signing in productionThe comment indicates the need to replace the symmetric key with a certificate in the production environment. Using asymmetric keys enhances security by safeguarding the private key and facilitating key rotation without downtime.
Would you like assistance in implementing certificate-based signing for JWT tokens?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
src/Argon.Api/Argon.Api.csproj(2 hunks)src/Argon.Api/Extensions/JwtExtension.cs(0 hunks)src/Argon.Api/Features/Jwt/JwtOptions.cs(1 hunks)src/Argon.Api/Features/Rpc/FusionAuthorizationMiddleware.cs(1 hunks)src/Argon.Api/Grains.Persistence.States/FusionSession.cs(1 hunks)src/Argon.Api/Grains/FusionGrain.cs(1 hunks)src/Argon.Api/Program.cs(4 hunks)src/Argon.Api/Properties/launchSettings.json(3 hunks)
💤 Files with no reviewable changes (1)
- src/Argon.Api/Extensions/JwtExtension.cs
🔇 Additional comments (12)
src/Argon.Api/Grains.Persistence.States/FusionSession.cs (2)
1-5: LGTM!
The namespace structure and imports are well-organized and follow C# conventions.
6-9: Verify the necessity of multiple serialization attributes.
The class uses three different serialization approaches. While each serves a purpose, please verify if all are required:
[GenerateSerializer]for Orleans[Serializable]for .NET[MemoryPackable]for MemoryPack
✅ Verification successful
Let me gather more information about the usage of these serialization attributes in the codebase.
Based on the gathered information, I can now provide the final response:
Multiple serialization attributes are correctly used and necessary
The combination of serialization attributes is a consistent pattern across all persistence state classes in the codebase and serves specific purposes in the Orleans-based architecture:
[GenerateSerializer]is required for Orleans serialization (Microsoft.Orleans.Sdk package)[MemoryPackable]is used for high-performance serialization (MemoryPack package is referenced and used project-wide)[Serializable]is needed for .NET serialization compatibility with Orleans[Alias]provides consistent type mapping across the Orleans cluster
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if these serialization attributes are consistently used across similar state classes
# Look for other state classes to verify the pattern
rg -l "class.*State|class.*Session" src/
Length of output: 99
Script:
#!/bin/bash
# Search for usage of serialization attributes across the codebase
rg -B 2 -A 1 "\[(GenerateSerializer|Serializable|MemoryPackable|Alias)\]" src/
Length of output: 5861
Script:
#!/bin/bash
# Look for Orleans configuration to understand serialization setup
rg -l "AddMemoryGrainStorage|UseOrleansSerializer|MemoryPack" src/
Length of output: 964
Script:
#!/bin/bash
# Check project file for serialization-related package references
fd -e csproj . src/ -x cat {}
Length of output: 5417
src/Argon.Api/Properties/launchSettings.json (1)
15-15: LGTM! Consistent browser launch settings across profiles.
The change to disable automatic browser launch across all profiles is appropriate for an authentication-focused PR, as it allows for more controlled testing of auth flows.
Also applies to: 25-25, 34-34
src/Argon.Api/Program.cs (3)
1-9: LGTM! Import statements are properly organized.
The new imports are relevant to the JWT authentication and RPC features being added.
27-33: Verify authorization middleware sequence.
The FusionAuthorizationMiddleware is added to the RPC pipeline, but we should ensure it runs after authentication.
Let's check the middleware implementation:
#!/bin/bash
# Description: Verify authorization middleware implementation
# Look for proper authentication checks in the middleware
# Check FusionAuthorizationMiddleware implementation
ast-grep --pattern 'class FusionAuthorizationMiddleware {
$$$
}'
# Look for authentication checks
rg "IAuthenticationService|AuthenticateAsync" --type cs43-44: Verify secure WebSocket configuration.
WebSocket support is enabled for RPC communication. Ensure secure WebSocket (wss://) is enforced in production.
Let's check the WebSocket security configuration:
✅ Verification successful
WebSocket security is properly configured
The application has appropriate security measures in place:
- HTTPS redirection is enforced via
UseHttpsRedirection() - Authentication and authorization are properly configured before WebSocket middleware
- WebSocket setup follows the secure middleware pipeline order
This ensures WebSocket connections will be upgraded only over HTTPS (WSS), providing secure communication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify WebSocket security settings
# Look for WebSocket security configuration
# Check for WebSocket security settings
rg "UseWebSockets|MapRpcWebSocketServer" -A 5 --type cs
# Look for SSL/TLS configuration
rg "wss://|UseHttpsRedirection|RequireHttpsMetadata" --type cs
Length of output: 221
Script:
#!/bin/bash
# Let's try a different approach to verify WebSocket security configuration
# Check for HTTPS/TLS configuration in Program.cs and Startup.cs
fd -e cs "Program|Startup" --exec rg -l "UseHttps|UseSsl|RequireHttps|UseHsts"
# Look for any WebSocket configuration including security settings
rg -i "websocket.*secure|secure.*websocket|wss://" --type cs
# Check for Kestrel server configuration which might enforce HTTPS
rg "Kestrel" -A 5 --type cs
# Look for configuration settings in appsettings files
fd "appsettings" --exec cat {}
Length of output: 1380
Script:
#!/bin/bash
# Let's check the specific HTTPS configuration in Program.cs since it was flagged
cat ./src/Argon.Api/Program.cs
# Also look for any middleware configuration that might enforce HTTPS
rg "UseMiddleware|app\.Use" -A 3 --type cs
Length of output: 2333
src/Argon.Api/Argon.Api.csproj (4)
15-19: LGTM! MessagePipe configuration is correct.
The analyzer is properly configured with appropriate asset inclusions.
52-54: LGTM! Good feature organization.
The addition of a dedicated JWT feature folder follows good organizational practices.
20-38: Verify package version compatibility.
The package versions look consistent within families (Orleans, EF Core). However, let's verify there are no potential conflicts.
#!/bin/bash
# Check for potential package conflicts or inconsistencies
rg -g "*.csproj" "PackageReference.*Version" | sort | uniq -c | sort -nr8-8: LGTM! Ensure secrets are properly managed.
The addition of UserSecretsId is appropriate for managing sensitive JWT configuration.
Let's verify if the secrets are properly configured:
✅ Verification successful
Let me verify if there are any secrets configuration files or documentation in the repository.
Secrets configuration is properly set up and follows best practices
The codebase shows a well-structured JWT configuration setup:
- JWT settings are properly externalized in appsettings.json files
- Sensitive values like JWT key are configured through user secrets (via the UserSecretsId)
- JWT configuration is properly injected via environment variables in AppHost
- The implementation follows security best practices with proper token validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's a secrets.json template or documentation
fd -g "*secrets*.json" -g "*secrets*.md" -g "*secrets*.example*"
Length of output: 4812
Script:
#!/bin/bash
# Check for appsettings files and documentation about secrets
fd -g "appsettings*.json" -g "*jwt*.json" -g "*auth*.json" -g "README*"
# Check for any documentation mentioning secrets or configuration
rg -i "user secrets|jwt config|authentication config" -g "!*.{dll,pdb,cache}"
Length of output: 363
Script:
#!/bin/bash
# Fix the fd command syntax and check for configuration files
fd -e json -e md . | grep -iE 'appsettings|jwt|auth|readme'
# Look for JWT configuration in C# files
rg -i "jwt|authentication" -g "*.cs" -A 2
Length of output: 9503
src/Argon.Api/Grains/FusionGrain.cs (2)
7-10:
Fix syntax error in class declaration: Invalid constructor syntax
The class FusionGrain uses an invalid syntax for constructor parameters in its declaration. In C#, classes do not support primary constructors (parameters after the class name). Instead, you should define a constructor inside the class and initialize any dependencies there.
Apply this diff to correct the class declaration and constructor:
-public class FusionGrain(
- [PersistentState("sessions", "OrleansStorage")]
- IPersistentState<FusionSession> sessionStorage,
- TokenValidationParameters JwtParameters) : Grain, IFusionSession
-{
+public class FusionGrain : Grain, IFusionSession
+{
+ private readonly IPersistentState<FusionSession> _sessionStorage;
+ private readonly TokenValidationParameters _jwtParameters;
+
+ public FusionGrain(
+ [PersistentState("sessions", "OrleansStorage")] IPersistentState<FusionSession> sessionStorage,
+ TokenValidationParameters jwtParameters)
+ {
+ _sessionStorage = sessionStorage;
+ _jwtParameters = jwtParameters;
+ }Likely invalid or redundant comment.
33-40: Verify the usage of [Alias] attributes in the interface
The [Alias] attribute applied to the interface methods may not have the intended effect. In Orleans, method aliases are typically used for versioning and interoperability, but they need to be correctly configured.
Run the following script to check if the [Alias] attribute is properly defined and used:
✅ Verification successful
Let me verify if the Orleans.Core package is properly referenced and if the Alias attribute is correctly imported.
Let me check for the specific Orleans namespace that contains the Alias attribute and its usage pattern in the codebase.
Let me check for the actual implementation of the FusionGrain and its interface to verify the Alias attribute usage pattern.
Based on the gathered evidence, I can now provide a final response:
The [Alias] attribute usage in IFusionSession is consistent with Orleans patterns
The usage of [Alias] attributes in the IFusionSession interface follows the same pattern as seen in other Orleans grain interfaces in the codebase (e.g., IServerManager, IUserManager, IChannelManager). The attribute is consistently used for method naming across the entire codebase, and the implementation in FusionGrain correctly implements the interface with these aliased methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usages of the [Alias] attribute and verify its correctness.
# Expect: The [Alias] attribute should be defined and properly applied to the interface methods.
rg '\[Alias\(".*"\)\]' -A 2
Length of output: 5426
Script:
#!/bin/bash
# Check for Orleans.Core package reference and using statements
rg -l "Orleans.Core" --type csproj
rg "using Orleans" -A 3
Length of output: 1025
Script:
#!/bin/bash
# Search for all Orleans using statements and Alias-related code
rg "using Orleans" --type cs
rg "using Orleans\." --type cs
rg "class.*Alias" --type cs
Length of output: 562
Script:
#!/bin/bash
# Search for FusionGrain implementation and related code
rg -A 5 "class FusionGrain"
rg -B 2 -A 5 "interface IFusionSession"
rg "using Orleans\.Aliases" --type cs
rg "using Orleans\.CodeGeneration" --type cs
Length of output: 1086
| await app.WarpUp<ApplicationDbContext>().RunAsync(); |
There was a problem hiding this comment.
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.
| await app.WarpUp<ApplicationDbContext>().RunAsync(); | |
| await app.WarmUp<ApplicationDbContext>().RunAsync(); |
| @@ -11,6 +15,7 @@ | |||
|
|
|||
| var builder = WebApplication.CreateBuilder(args); | |||
|
|
|||
| builder.AddJwt(); | |||
There was a problem hiding this comment.
💡 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-tokenis used instead of the standardAuthorization: Bearerheader
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
| public async ValueTask<FusionSession> GetState() | ||
| { | ||
| await sessionStorage.ReadStateAsync(); | ||
| return sessionStorage.State; | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| sessionStorage.State.Id = Guid.Parse(jwt.Id); | ||
| sessionStorage.State.IsAuthorized = true; |
There was a problem hiding this comment.
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.
| 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; | ||
| await sessionStorage.WriteStateAsync(); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| var grain = GrainFactory.GetGrain<IFusionSession>(peerId); | ||
|
|
||
| return grain.GetState(); |
There was a problem hiding this comment.
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 AsyncLocal<string> Token = new AsyncLocal<string>(); | ||
| public override async Task OnBeforeCall(RpcInboundCall call) | ||
| { | ||
| var existAttribute = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Count != 0; |
There was a problem hiding this comment.
🛠️ 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.
| var existAttribute = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Count != 0; | |
| var authorizeAttributeExists = call.MethodDef.Method.GetAttributes<AuthorizeAttribute>(true, true).Any(); |
|
|
||
| public class FusionAuthorizationMiddleware(IServiceProvider Services, IGrainFactory GrainFactory) : RpcInboundMiddleware(Services) | ||
| { | ||
| public AsyncLocal<string> Token = new AsyncLocal<string>(); |
There was a problem hiding this comment.
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.
| public AsyncLocal<string> Token = new AsyncLocal<string>(); |
| public void Deconstruct(out string issuer, out string audience, out string key) | ||
| { | ||
| audience = this.Audience; | ||
| issuer = this.Issuer; | ||
| key = this.Key; | ||
| } |
There was a problem hiding this comment.
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.
| 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; | |
| } |
| 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; | ||
| } |
There was a problem hiding this comment.
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.
| 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.
| 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; | |
| } |
There was a problem hiding this comment.
не надо удалять, нам это для тестов понадобится
Summary by CodeRabbit
Release Notes
New Features
JwtOptionsrecord and middleware for managing authorization in RPC calls.FusionAuthorizationMiddlewarefor enhanced authorization handling.FusionGrainfor session management and authorization processes.Improvements
Bug Fixes