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

[AC-1454] Migrate 2fa.directory call #71

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
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: 2 additions & 0 deletions src/Api/Startup.cs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
using Bit.Core.Entities;
using Bit.Core.Billing.Extensions;
using Bit.Core.OrganizationFeatures.OrganizationSubscriptions;
using Bit.Core.Tools;
using Bit.Core.Tools.Entities;
using Bit.Core.Vault.Entities;

Expand Down Expand Up @@ -171,6 +172,7 @@ public void ConfigureServices(IServiceCollection services)
services.AddOrganizationSubscriptionServices();
services.AddCoreLocalizationServices();
services.AddBillingCommands();
services.AddToolsServices();

// Authorization Handlers
services.AddAuthorizationHandlers();
Expand Down
45 changes: 45 additions & 0 deletions src/Api/Tools/Controllers/ReportsController.cs
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;
Copy link

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

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?

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");
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

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

style: style: Remove extra blank line before closing brace

}
10 changes: 10 additions & 0 deletions src/Api/Tools/Models/Response/InactiveTwoFactorResponseModel.cs
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; }
Copy link

Choose a reason for hiding this comment

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

style: Consider using init-only setter for immutability

Copy link

Choose a reason for hiding this comment

The 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.

}
1 change: 1 addition & 0 deletions src/Core/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link

Choose a reason for hiding this comment

The 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";

Expand Down
7 changes: 7 additions & 0 deletions src/Core/Settings/GlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Copy link

Choose a reason for hiding this comment

The 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

}
}
1 change: 1 addition & 0 deletions src/Core/Settings/IGlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,5 @@ public interface IGlobalSettings
IDomainVerificationSettings DomainVerification { get; set; }
ILaunchDarklySettings LaunchDarkly { get; set; }
string DevelopmentDirectory { get; set; }
ITwoFactorDirectorySettings TwoFactorDirectory { get; set; }
}
7 changes: 7 additions & 0 deletions src/Core/Settings/ITwoFactorDirectorySettings.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
namespace Bit.Core.Settings;

public interface ITwoFactorDirectorySettings
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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; }
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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; }
Copy link

Choose a reason for hiding this comment

The 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

}
89 changes: 89 additions & 0 deletions src/Core/Tools/Queries/GetInactiveTwoFactorQuery.cs
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);
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

The 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();
Copy link

Choose a reason for hiding this comment

The 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.

}
18 changes: 18 additions & 0 deletions src/Core/Tools/ToolsServiceCollectionExtensions.cs
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
Copy link

Choose a reason for hiding this comment

The 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>();
}
}
106 changes: 106 additions & 0 deletions test/Core.Test/Tools/Queries/GetInactiveTwoFactorQueryTests.cs
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)
Copy link

Choose a reason for hiding this comment

The 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,
Copy link

Choose a reason for hiding this comment

The 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());
Copy link

Choose a reason for hiding this comment

The 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>());
}
}