[AC-1454] Migrate 2fa.directory call #71
Conversation
…safe, refs AC-1454
…4-2fa-directory
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
12 file(s) reviewed, 13 comment(s)
Edit PR Review Bot Settings | Greptile
| [Authorize("Application")] | ||
| public class ReportsController : Controller | ||
| { | ||
| private readonly ICurrentContext _currentContext; |
There was a problem hiding this comment.
style: _currentContext is declared but never used
| var user = await _userService.GetUserByPrincipalAsync(User); | ||
| if (!user.Premium) | ||
| { | ||
| throw new UnauthorizedAccessException("Premium required"); |
There was a problem hiding this comment.
style: Consider using a custom exception for premium requirements
| { | ||
| public InactiveTwoFactorResponseModel() : base("inactive-two-factor") { } | ||
|
|
||
| public IReadOnlyDictionary<string, string> Services { get; set; } |
There was a problem hiding this comment.
style: Consider using init-only setter for immutability
| @@ -0,0 +1,7 @@ | |||
| namespace Bit.Core.Settings; | |||
|
|
|||
| public interface ITwoFactorDirectorySettings | |||
There was a problem hiding this comment.
style: Consider adding XML documentation comments to describe the interface's purpose
| [JsonPropertyName("documentation")] | ||
| public string Documentation { get; set; } | ||
| [JsonPropertyName("additional-domains")] | ||
| public IEnumerable<string> AdditionalDomains { get; set; } |
There was a problem hiding this comment.
style: Consider using IReadOnlyCollection instead of IEnumerable for AdditionalDomains to prevent accidental modifications
| private static IEnumerable<TwoFactorDirectoryTotpResponseModel> ParseTwoFactorDirectoryTotpResponse(string json) | ||
| { | ||
| var data = new List<TwoFactorDirectoryTotpResponseModel>(); | ||
| using var jsonDocument = JsonDocument.Parse(json); | ||
| // JSON response object opens with Array notation | ||
| if (jsonDocument.RootElement.ValueKind == JsonValueKind.Array) | ||
| { | ||
| // Each nested array has two values: a floating "name" value [index: 0] and an object with desired data [index: 1] | ||
| data.AddRange(from element in jsonDocument.RootElement.EnumerateArray() | ||
| where element.ValueKind == JsonValueKind.Array && element.GetArrayLength() == 2 | ||
| select element[1].Deserialize<TwoFactorDirectoryTotpResponseModel>()); | ||
| } | ||
|
|
||
| return data; | ||
| } |
There was a problem hiding this comment.
style: Consider adding error handling for JSON parsing exceptions
|
|
||
| public interface IGetInactiveTwoFactorQuery | ||
| { | ||
| Task<Dictionary<string, string>> GetInactiveTwoFactorAsync(); |
There was a problem hiding this comment.
style: Consider adding XML documentation comments to describe the method's purpose and return value.
| public static void AddToolsServices(this IServiceCollection services) | ||
| { | ||
| services.AddReportsQueries(); | ||
| } |
There was a problem hiding this comment.
style: Consider adding XML documentation comments to describe the purpose of this public method
| sutProvider.GetDependency<IGlobalSettings>().TwoFactorDirectory.Returns( | ||
| new GlobalSettings.TwoFactorDirectorySettings() | ||
| { | ||
| CacheExpirationHours = 1, |
There was a problem hiding this comment.
logic: The cache expiration is set to 1 hour here, but the PR description mentions 24 hours. Ensure consistency across the implementation.
| Uri = new Uri("http://localhost") | ||
| }); | ||
|
|
||
| await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.GetInactiveTwoFactorAsync()); |
There was a problem hiding this comment.
logic: Throwing a BadRequestException for an Unauthorized response might not be the most appropriate exception. Consider using a more specific exception type.
There was a problem hiding this comment.
PR Summary
This pull request implements server-side caching for the 2fa.directory API calls, reducing load on the external service and improving response times for the inactive two-factor authentication report.
- Added distributed caching in
GetInactiveTwoFactorQuery.cswith configurable 24-hour expiration - Implemented premium user validation in
ReportsController.csto restrict access to paid accounts - Created custom JSON parser in
GetInactiveTwoFactorQuery.csto handle the specific 2fa.directory response format - Added comprehensive unit tests covering cache hits, API failures, and successful API responses
- Protected endpoint with feature flag
MigrateTwoFactorDirectoryfor controlled rollout
12 file(s) reviewed, 10 comment(s)
Edit PR Review Bot Settings | Greptile
|
|
||
| } |
There was a problem hiding this comment.
style: style: Remove extra blank line before closing brace
| { | ||
| public InactiveTwoFactorResponseModel() : base("inactive-two-factor") { } | ||
|
|
||
| public IReadOnlyDictionary<string, string> Services { get; set; } |
There was a problem hiding this comment.
logic: Property allows write access despite being IReadOnlyDictionary. Consider making the setter private to ensure immutability.
| @@ -115,6 +115,7 @@ public static class FeatureFlagKeys | |||
| /// flexible collections | |||
| /// </summary> | |||
| public const string FlexibleCollectionsMigration = "flexible-collections-migration"; | |||
| public const string MigrateTwoFactorDirectory = "migrate-two-factor-directory"; | |||
There was a problem hiding this comment.
style: Consider adding a documentation comment above this constant explaining its purpose, similar to other feature flags in this file
| public class TwoFactorDirectorySettings : ITwoFactorDirectorySettings | ||
| { | ||
| public Uri Uri { get; set; } = new("https://api.2fa.directory/v3/totp.json"); | ||
| public int CacheExpirationHours { get; set; } = 24; |
There was a problem hiding this comment.
style: 24 hours may be too long for cache expiration - consider reducing to prevent stale data
| public Uri Uri { get; set; } | ||
| public int CacheExpirationHours { get; set; } |
There was a problem hiding this comment.
style: Consider making properties read-only to prevent unintended modifications after initialization
| { | ||
| [Required] | ||
| [JsonPropertyName("domain")] | ||
| public string Domain { get; set; } |
There was a problem hiding this comment.
style: Domain property should be initialized to avoid null reference exceptions since it's required
| public string Domain { get; set; } | ||
| [JsonPropertyName("documentation")] | ||
| public string Documentation { get; set; } |
There was a problem hiding this comment.
style: Consider making properties init-only since they're only populated during deserialization
| using var client = _httpClientFactory.CreateClient(); | ||
| var response = | ||
| await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, _globalSettings.TwoFactorDirectory.Uri)); |
There was a problem hiding this comment.
style: Consider adding a timeout to the HTTP request to prevent hanging on slow responses
| .Returns(new HttpResponseMessage() | ||
| { | ||
| StatusCode = HttpStatusCode.OK, | ||
| Content = new StringContent("{}", Encoding.UTF8, MediaTypeNames.Application.Json) |
There was a problem hiding this comment.
style: style: Consider testing with actual 2FA directory response data instead of empty JSON to verify parsing logic
Type of change
Objective
Code changes
ReportsAPI controller with the future in mind of potentially moving around other report functions to the backend.ITwoFactorDirectorySettingsfor potential customizations to the URL and cache expirationGlobalSettingstoolsteam specific servicestoolsservicesShout-outs
Before you submit
dotnet format --verify-no-changes) (required)Greptile Summary
This pull request implements a caching mechanism for the inactive two-factor authentication report, reducing API calls to the 2fa.directory service.
ReportsControllerin/src/Api/Tools/Controllers/ReportsController.csto handle caching and retrieval of 2FA report dataGetInactiveTwoFactorQueryin/src/Core/Tools/Queries/GetInactiveTwoFactorQuery.csfor fetching and caching 2FA directory dataTwoFactorDirectorySettingsin/src/Core/Settings/GlobalSettings.csto manage API URL and cache expiration/test/Core.Test/Tools/Queries/GetInactiveTwoFactorQueryTests.csto verify caching and API interactionMigrateTwoFactorDirectoryin/src/Core/Constants.csfor controlled rollout of the new functionality