-
Notifications
You must be signed in to change notification settings - Fork 0
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
[AC-1454] Migrate 2fa.directory call #71
base: main
Are you sure you want to change the base?
Changes from all commits
700984e
54e2a20
6a1607b
b45cd11
12bd746
feebc50
6d5934c
54606c7
8d15389
e3d7bf0
7b5ea1e
e56925a
6bbccb3
8a33050
c7e9f5f
2322789
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,45 @@ | ||
using Bit.Api.Tools.Models.Response; | ||
using Bit.Core; | ||
using Bit.Core.Context; | ||
using Bit.Core.Services; | ||
using Bit.Core.Tools.Queries.Interfaces; | ||
using Bit.Core.Utilities; | ||
using Microsoft.AspNetCore.Authorization; | ||
using Microsoft.AspNetCore.Mvc; | ||
|
||
namespace Bit.Api.Tools.Controllers; | ||
|
||
[Route("reports")] | ||
[Authorize("Application")] | ||
public class ReportsController : Controller | ||
{ | ||
private readonly ICurrentContext _currentContext; | ||
private readonly IGetInactiveTwoFactorQuery _getInactiveTwoFactorQuery; | ||
private readonly IUserService _userService; | ||
|
||
public ReportsController(ICurrentContext currentContext, IGetInactiveTwoFactorQuery getInactiveTwoFactorQuery, IUserService userService) | ||
{ | ||
_currentContext = currentContext; | ||
_getInactiveTwoFactorQuery = getInactiveTwoFactorQuery; | ||
_userService = userService; | ||
} | ||
|
||
[HttpGet("inactive-two-factor")] | ||
[RequireFeature(FeatureFlagKeys.MigrateTwoFactorDirectory)] | ||
public async Task<InactiveTwoFactorResponseModel> GetInactiveTwoFactorAsync() | ||
{ | ||
// Premium guarded | ||
var user = await _userService.GetUserByPrincipalAsync(User); | ||
if (!user.Premium) | ||
{ | ||
throw new UnauthorizedAccessException("Premium required"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using a custom exception for premium requirements |
||
} | ||
|
||
var services = await _getInactiveTwoFactorQuery.GetInactiveTwoFactorAsync(); | ||
return new InactiveTwoFactorResponseModel() | ||
{ | ||
Services = services | ||
}; | ||
|
||
} | ||
Comment on lines
+43
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: style: Remove extra blank line before closing brace |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
using Bit.Core.Models.Api; | ||
|
||
namespace Bit.Api.Tools.Models.Response; | ||
|
||
public class InactiveTwoFactorResponseModel : ResponseModel | ||
{ | ||
public InactiveTwoFactorResponseModel() : base("inactive-two-factor") { } | ||
|
||
public IReadOnlyDictionary<string, string> Services { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using init-only setter for immutability There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Property allows write access despite being IReadOnlyDictionary. Consider making the setter private to ensure immutability. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding a documentation comment above this constant explaining its purpose, similar to other feature flags in this file |
||
|
||
public const string PM5766AutomaticTax = "PM-5766-automatic-tax"; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,6 +81,7 @@ public virtual string LicenseDirectory | |
public virtual IDomainVerificationSettings DomainVerification { get; set; } = new DomainVerificationSettings(); | ||
public virtual ILaunchDarklySettings LaunchDarkly { get; set; } = new LaunchDarklySettings(); | ||
public virtual string DevelopmentDirectory { get; set; } | ||
public virtual ITwoFactorDirectorySettings TwoFactorDirectory { get; set; } = new TwoFactorDirectorySettings(); | ||
|
||
public string BuildExternalUri(string explicitValue, string name) | ||
{ | ||
|
@@ -556,4 +557,10 @@ public class LaunchDarklySettings : ILaunchDarklySettings | |
public string FlagDataFilePath { get; set; } = "flags.json"; | ||
public Dictionary<string, string> FlagValues { get; set; } = new Dictionary<string, string>(); | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: 24 hours may be too long for cache expiration - consider reducing to prevent stale data |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
namespace Bit.Core.Settings; | ||
|
||
public interface ITwoFactorDirectorySettings | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding XML documentation comments to describe the interface's purpose |
||
{ | ||
public Uri Uri { get; set; } | ||
public int CacheExpirationHours { get; set; } | ||
Comment on lines
+5
to
+6
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider making properties read-only to prevent unintended modifications after initialization |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
using System.ComponentModel.DataAnnotations; | ||
using System.Text.Json.Serialization; | ||
|
||
namespace Bit.Core.Tools.Models.Api.Response; | ||
|
||
public class TwoFactorDirectoryTotpResponseModel | ||
{ | ||
[Required] | ||
[JsonPropertyName("domain")] | ||
public string Domain { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Domain property should be initialized to avoid null reference exceptions since it's required |
||
[JsonPropertyName("documentation")] | ||
public string Documentation { get; set; } | ||
Comment on lines
+10
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider making properties init-only since they're only populated during deserialization |
||
[JsonPropertyName("additional-domains")] | ||
public IEnumerable<string> AdditionalDomains { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using IReadOnlyCollection instead of IEnumerable for AdditionalDomains to prevent accidental modifications |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
using System.Text.Json; | ||
using Bit.Core.Exceptions; | ||
using Bit.Core.Settings; | ||
using Bit.Core.Tools.Models.Api.Response; | ||
using Bit.Core.Tools.Queries.Interfaces; | ||
using Bit.Core.Utilities; | ||
using Microsoft.Extensions.Caching.Distributed; | ||
using Microsoft.Extensions.Logging; | ||
|
||
namespace Bit.Core.Tools.Queries; | ||
|
||
public class GetInactiveTwoFactorQuery : IGetInactiveTwoFactorQuery | ||
{ | ||
private const string _cacheKey = "ReportsInactiveTwoFactor"; | ||
|
||
private readonly IDistributedCache _distributedCache; | ||
private readonly IHttpClientFactory _httpClientFactory; | ||
private readonly IGlobalSettings _globalSettings; | ||
private readonly ILogger<GetInactiveTwoFactorQuery> _logger; | ||
|
||
public GetInactiveTwoFactorQuery( | ||
IDistributedCache distributedCache, | ||
IHttpClientFactory httpClientFactory, | ||
IGlobalSettings globalSettings, | ||
ILogger<GetInactiveTwoFactorQuery> logger) | ||
{ | ||
_distributedCache = distributedCache; | ||
_httpClientFactory = httpClientFactory; | ||
_globalSettings = globalSettings; | ||
_logger = logger; | ||
} | ||
|
||
public async Task<Dictionary<string, string>> GetInactiveTwoFactorAsync() | ||
{ | ||
_distributedCache.TryGetValue(_cacheKey, out Dictionary<string, string> services); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider using TryGetValueAsync for asynchronous cache retrieval |
||
if (services != null) | ||
{ | ||
return services; | ||
} | ||
|
||
using var client = _httpClientFactory.CreateClient(); | ||
var response = | ||
await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, _globalSettings.TwoFactorDirectory.Uri)); | ||
Comment on lines
+41
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding a timeout to the HTTP request to prevent hanging on slow responses |
||
if (!response.IsSuccessStatusCode) | ||
{ | ||
_logger.LogInformation("Request to 2fa.Directory was unsuccessful: {statusCode}", response.StatusCode); | ||
throw new BadRequestException(); | ||
Comment on lines
+46
to
+47
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Log error instead of info for unsuccessful requests. Consider including more details about the error |
||
} | ||
|
||
services = new Dictionary<string, string>(); | ||
var deserializedData = ParseTwoFactorDirectoryTotpResponse(await response.Content.ReadAsStringAsync()); | ||
Comment on lines
+50
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider moving the parsing logic to a separate method for better separation of concerns |
||
|
||
foreach (var service in deserializedData.Where(service => !string.IsNullOrEmpty(service.Documentation))) | ||
{ | ||
if (service.AdditionalDomains != null) | ||
{ | ||
foreach (var additionalDomain in service.AdditionalDomains) | ||
{ | ||
// TryAdd used to prevent duplicate keys | ||
services.TryAdd(additionalDomain, service.Documentation); | ||
} | ||
} | ||
|
||
// TryAdd used to prevent duplicate keys | ||
services.TryAdd(service.Domain, service.Documentation); | ||
} | ||
|
||
await _distributedCache.SetAsync(_cacheKey, services, | ||
new DistributedCacheEntryOptions().SetAbsoluteExpiration( | ||
new TimeSpan(_globalSettings.TwoFactorDirectory.CacheExpirationHours, 0, 0))); | ||
return services; | ||
} | ||
|
||
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; | ||
} | ||
Comment on lines
+74
to
+88
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding error handling for JSON parsing exceptions |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
namespace Bit.Core.Tools.Queries.Interfaces; | ||
|
||
public interface IGetInactiveTwoFactorQuery | ||
{ | ||
Task<Dictionary<string, string>> GetInactiveTwoFactorAsync(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding XML documentation comments to describe the method's purpose and return value. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
using Bit.Core.Tools.Queries; | ||
using Bit.Core.Tools.Queries.Interfaces; | ||
using Microsoft.Extensions.DependencyInjection; | ||
|
||
namespace Bit.Core.Tools; | ||
|
||
public static class ToolsServiceCollectionExtensions | ||
{ | ||
public static void AddToolsServices(this IServiceCollection services) | ||
{ | ||
services.AddReportsQueries(); | ||
} | ||
Comment on lines
+9
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: Consider adding XML documentation comments to describe the purpose of this public method |
||
|
||
private static void AddReportsQueries(this IServiceCollection services) | ||
{ | ||
services.AddScoped<IGetInactiveTwoFactorQuery, GetInactiveTwoFactorQuery>(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
using System.Net; | ||
using System.Net.Mime; | ||
using System.Text; | ||
using System.Text.Json; | ||
using Bit.Core.Exceptions; | ||
using Bit.Core.Settings; | ||
using Bit.Core.Tools.Queries; | ||
using Bit.Test.Common.AutoFixture; | ||
using Bit.Test.Common.AutoFixture.Attributes; | ||
using Microsoft.Extensions.Caching.Distributed; | ||
using NSubstitute; | ||
using NSubstitute.ReturnsExtensions; | ||
using Xunit; | ||
using GlobalSettings = Bit.Core.Settings.GlobalSettings; | ||
|
||
namespace Bit.Core.Test.Tools.Queries; | ||
|
||
[SutProviderCustomize] | ||
public class GetInactiveTwoFactorQueryTests | ||
{ | ||
public class MockHttpMessageHandler : HttpMessageHandler | ||
{ | ||
protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, | ||
CancellationToken cancellationToken) => Send(request, cancellationToken); | ||
|
||
public virtual Task<HttpResponseMessage> Send(HttpRequestMessage request, CancellationToken token) | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
} | ||
|
||
[Theory] | ||
[BitAutoData] | ||
public async Task GetInactiveTwoFactor_FromApi_Success(SutProvider<GetInactiveTwoFactorQuery> sutProvider) | ||
{ | ||
sutProvider.GetDependency<IDistributedCache>().Get(Arg.Any<string>()).ReturnsNull(); | ||
|
||
var handler = Substitute.ForPartsOf<MockHttpMessageHandler>(); | ||
handler.Send(Arg.Any<HttpRequestMessage>(), Arg.Any<CancellationToken>()) | ||
.Returns(new HttpResponseMessage() | ||
{ | ||
StatusCode = HttpStatusCode.OK, | ||
Content = new StringContent("{}", Encoding.UTF8, MediaTypeNames.Application.Json) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. style: style: Consider testing with actual 2FA directory response data instead of empty JSON to verify parsing logic |
||
}); | ||
|
||
var client = new HttpClient(handler); | ||
sutProvider.GetDependency<IHttpClientFactory>() | ||
.CreateClient() | ||
.Returns(client); | ||
|
||
sutProvider.GetDependency<IGlobalSettings>().TwoFactorDirectory.Returns( | ||
new GlobalSettings.TwoFactorDirectorySettings() | ||
{ | ||
CacheExpirationHours = 1, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 sutProvider.Sut.GetInactiveTwoFactorAsync(); | ||
|
||
await sutProvider.GetDependency<IDistributedCache>().Received(1).SetAsync(Arg.Any<string>(), | ||
Arg.Any<byte[]>(), Arg.Any<DistributedCacheEntryOptions>()); | ||
} | ||
|
||
[Theory] | ||
[BitAutoData] | ||
public async Task GetInactiveTwoFactor_FromApi_Failure(SutProvider<GetInactiveTwoFactorQuery> sutProvider) | ||
{ | ||
sutProvider.GetDependency<IDistributedCache>().Get(Arg.Any<string>()).ReturnsNull(); | ||
|
||
var handler = Substitute.ForPartsOf<MockHttpMessageHandler>(); | ||
handler.Send(Arg.Any<HttpRequestMessage>(), Arg.Any<CancellationToken>()) | ||
.Returns(new HttpResponseMessage() | ||
{ | ||
StatusCode = HttpStatusCode.Unauthorized | ||
}); | ||
|
||
var client = new HttpClient(handler); | ||
sutProvider.GetDependency<IHttpClientFactory>() | ||
.CreateClient() | ||
.Returns(client); | ||
|
||
sutProvider.GetDependency<IGlobalSettings>().TwoFactorDirectory.Returns( | ||
new GlobalSettings.TwoFactorDirectorySettings() | ||
{ | ||
CacheExpirationHours = 1, | ||
Uri = new Uri("http://localhost") | ||
}); | ||
|
||
await Assert.ThrowsAsync<BadRequestException>(() => sutProvider.Sut.GetInactiveTwoFactorAsync()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: Throwing a BadRequestException for an Unauthorized response might not be the most appropriate exception. Consider using a more specific exception type. |
||
} | ||
|
||
[Theory] | ||
[BitAutoData] | ||
public async Task GetInactiveTwoFactor_FromCache_Success(Dictionary<string, string> dictionary, | ||
SutProvider<GetInactiveTwoFactorQuery> sutProvider) | ||
{ | ||
// Byte array needs to deserialize into dictionary object | ||
var bytes = JsonSerializer.SerializeToUtf8Bytes(dictionary); | ||
sutProvider.GetDependency<IDistributedCache>().Get(Arg.Any<string>()).Returns(bytes); | ||
|
||
await sutProvider.Sut.GetInactiveTwoFactorAsync(); | ||
|
||
await sutProvider.GetDependency<IDistributedCache>().DidNotReceive().SetAsync(Arg.Any<string>(), | ||
Arg.Any<byte[]>(), Arg.Any<DistributedCacheEntryOptions>()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: _currentContext is declared but never used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@greptileai, can I remove this?