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

Fixes #16

Merged
merged 4 commits into from
Nov 3, 2024
Merged

Fixes #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
aa42545
set cond for validate password, fix swagger
0xF6 Nov 3, 2024
49ed193
fix
0xF6 Nov 3, 2024
bb2079b
fix
0xF6 Nov 3, 2024
81a8e85
fix missed ApiController
0xF6 Nov 3, 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
2 changes: 1 addition & 1 deletion src/Argon.Api/Controllers/MetadataController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
[ApiController]
public class MetadataController : ControllerBase
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 keeping the [ApiController] attribute.

Removing the [ApiController] attribute disables useful ASP.NET Core conveniences like automatic model validation, inference of [FromBody] attributes, and better error handling. Unless there's a specific reason to remove it, keeping this attribute is recommended for consistent controller behavior.

+[ApiController]
 public class MetadataController : ControllerBase
📝 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 MetadataController : ControllerBase
[ApiController]
public class MetadataController : ControllerBase

{
[Route("/cfg.json")]
[HttpGet("/cfg.json")]
[AllowAnonymous]
public ValueTask<HeadRoutingConfig> GetHead() =>
new(new HeadRoutingConfig(
Expand Down
2 changes: 1 addition & 1 deletion src/Argon.Api/Grains/UserManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ public async Task<UserStorageDto> Create(string password)
{
var username = this.GetPrimaryKeyString();
await EnsureUnique();
await managerService.Validate(username, password);
managerService.Validate(username, password);
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical: Restore asynchronous validation to prevent potential blocking

The removal of await from managerService.Validate() could lead to blocking operations in the Orleans grain, potentially impacting system performance. User validation should be asynchronous to ensure proper scalability and prevent thread pool starvation.

Apply this diff to restore asynchronous behavior:

-        managerService.Validate(username, password);
+        await managerService.Validate(username, password);
📝 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
managerService.Validate(username, password);
await managerService.Validate(username, password);

💡 Codebase verification

The Validate method is async and should be awaited

The Validate method in UserManagerService is indeed marked as async and performs multiple validation tasks using await. The current implementation in UserManager.cs should be modified to await this call:

await managerService.Validate(username, password);
🔗 Analysis chain

Verify the implementation of UserManagerService.Validate

Let's verify if the Validate method contains any async operations that would require awaiting.

Let me gather more information about the implementation of the Validate method.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for the Validate method implementation in UserManagerService
ast-grep --pattern 'class UserManagerService {
  $$$
  Validate($$$) {
    $$$
  }
  $$$
}'

# Also check for any async operations within the service
rg -l "await" -g "UserManagerService.cs"

Length of output: 177

Script:

#!/bin/bash
# Let's see the actual content of UserManagerService.cs to check the Validate method implementation
cat src/Argon.Api/Services/UserManagerService.cs

Length of output: 2863


userStore.State.Id = Guid.NewGuid();
userStore.State.Username = username;
Expand Down
12 changes: 1 addition & 11 deletions src/Argon.Api/Properties/launchSettings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,17 +14,7 @@
"dotnetRunMessages": true,
"launchBrowser": false,
"launchUrl": "swagger",
"applicationUrl": "http://localhost:5100",
"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
}
},
"https": {
"commandName": "Project",
"dotnetRunMessages": true,
"launchBrowser": false,
"launchUrl": "swagger",
"applicationUrl": "https://localhost:7206;http://localhost:5100",
"applicationUrl": "https://localhost:5100",
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Using HTTPS URL in HTTP profile may cause confusion.

While switching to HTTPS is good for security, having an HTTPS URL in a profile named "http" is counterintuitive and could lead to confusion. Consider renaming the profile to better reflect its purpose.

  "profiles": {
-   "http": {
+   "https": {
      "commandName": "Project",
      "dotnetRunMessages": true,
      "launchBrowser": false,
      "launchUrl": "swagger",
      "applicationUrl": "https://localhost:5100",

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

"environmentVariables": {
"ASPNETCORE_ENVIRONMENT": "Development"
}
Expand Down
18 changes: 8 additions & 10 deletions src/Argon.Api/Services/UserManagerService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
namespace Argon.Api.Services;

using System.Diagnostics;
using System.IdentityModel.Tokens.Jwt;
using System.Security.Claims;
using System.Text;
Expand Down Expand Up @@ -42,14 +43,15 @@ public Task<string> GenerateJwt(string username, Guid id)
return Task.FromResult(jwtToken);
}

public async Task Validate(string username, string password)
[Conditional("RELEASE")]
public void Validate(string username, string password)
{
await ValidateLength(username, nameof(username), 3, 50);
await ValidateLength(password, nameof(password), 8, 32);
await ValidatePasswordStrength(password);
ValidateLength(username, nameof(username), 3, 50);
ValidateLength(password, nameof(password), 8, 32);
ValidatePasswordStrength(password);
}

private Task ValidatePasswordStrength(string password)
private void ValidatePasswordStrength(string password)
{
if (!password.Any(char.IsDigit))
throw new Exception(
Expand All @@ -62,15 +64,11 @@ private Task ValidatePasswordStrength(string password)
if (!password.Any(char.IsLower))
throw new Exception(
"Password must contain at least one lowercase letter"); // TODO: Come up with application specific errors

return Task.CompletedTask;
}

private Task ValidateLength(string str, string obj, int min, int max)
private void ValidateLength(string str, string obj, int min, int max)
{
if (str.Length < min || str.Length > max)
throw new Exception($"{obj}: Invalid length"); // TODO: Come up with application specific errors

return Task.CompletedTask;
}
}
5 changes: 4 additions & 1 deletion src/Argon.Contracts/IUserAuthorization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@ public sealed partial record AuthorizeRequest(
[property: Key(2)]
string machineKey);

public sealed record AuthorizeResponse(
[DataContract]
[MemoryPackable(GenerateType.VersionTolerant)]
[MessagePackObject]
public sealed partial record AuthorizeResponse(
[property: DataMember(Order = 0)]
[property: MemoryPackOrder(0)]
[property: Key(0)]
Expand Down
Loading