-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[PM-21411] Refactor interface for determining premium status and features #6688
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
base: main
Are you sure you want to change the base?
Changes from all commits
5629d6f
c09a973
f83a790
d30f3bd
377ae31
683a6cb
08e1413
e494a10
0cdadd1
192052b
2d31237
123f579
57252a7
bf0cc7a
65941d4
2a966e3
96a3615
516824a
b8ff0ea
415a0b9
303b81c
30ff3da
02c2aa8
2184296
5257dc5
97bcf41
0a42c9e
651d5e3
353a07a
da9f0a1
df7bd08
cd0b3da
245ce71
0279cb5
1415517
2bd00c2
dbb8619
2f11c13
9a68a97
1cd5749
61775fb
19627f4
6a783ff
85e0e1a
f833040
de13a7c
03b0431
125b4de
2081dd4
de42f20
33a464a
97bf24e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,16 +4,37 @@ | |
| using Bit.Core.Auth.Enums; | ||
| using Bit.Core.Auth.Models; | ||
| using Bit.Core.Auth.UserFeatures.TwoFactorAuth.Interfaces; | ||
| using Bit.Core.Billing.Premium.Queries; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Exceptions; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Services; | ||
|
|
||
| namespace Bit.Core.Auth.UserFeatures.TwoFactorAuth; | ||
|
|
||
| public class TwoFactorIsEnabledQuery(IUserRepository userRepository) : ITwoFactorIsEnabledQuery | ||
| public class TwoFactorIsEnabledQuery : ITwoFactorIsEnabledQuery | ||
| { | ||
| private readonly IUserRepository _userRepository = userRepository; | ||
| private readonly IUserRepository _userRepository; | ||
| private readonly IHasPremiumAccessQuery _hasPremiumAccessQuery; | ||
| private readonly IFeatureService _featureService; | ||
|
|
||
| public TwoFactorIsEnabledQuery( | ||
| IUserRepository userRepository, | ||
| IHasPremiumAccessQuery hasPremiumAccessQuery, | ||
| IFeatureService featureService) | ||
| { | ||
| _userRepository = userRepository; | ||
| _hasPremiumAccessQuery = hasPremiumAccessQuery; | ||
| _featureService = featureService; | ||
| } | ||
|
|
||
| public async Task<IEnumerable<(Guid userId, bool twoFactorIsEnabled)>> TwoFactorIsEnabledAsync(IEnumerable<Guid> userIds) | ||
| { | ||
| if (_featureService.IsEnabled(FeatureFlagKeys.PremiumAccessQuery)) | ||
| { | ||
| return await TwoFactorIsEnabledVNextAsync(userIds); | ||
| } | ||
|
|
||
| var result = new List<(Guid userId, bool hasTwoFactor)>(); | ||
| if (userIds == null || !userIds.Any()) | ||
| { | ||
|
|
@@ -36,6 +57,11 @@ await TwoFactorEnabledAsync(userDetail.GetTwoFactorProviders(), | |
|
|
||
| public async Task<IEnumerable<(T user, bool twoFactorIsEnabled)>> TwoFactorIsEnabledAsync<T>(IEnumerable<T> users) where T : ITwoFactorProvidersUser | ||
| { | ||
| if (_featureService.IsEnabled(FeatureFlagKeys.PremiumAccessQuery)) | ||
| { | ||
| return await TwoFactorIsEnabledVNextAsync(users); | ||
| } | ||
|
|
||
| var userIds = users | ||
| .Select(u => u.GetUserId()) | ||
| .Where(u => u.HasValue) | ||
|
|
@@ -71,13 +97,134 @@ public async Task<bool> TwoFactorIsEnabledAsync(ITwoFactorProvidersUser user) | |
| return false; | ||
| } | ||
|
|
||
| if (_featureService.IsEnabled(FeatureFlagKeys.PremiumAccessQuery)) | ||
| { | ||
| var userEntity = user as User ?? await _userRepository.GetByIdAsync(userId.Value); | ||
| if (userEntity == null) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| return await TwoFactorIsEnabledVNextAsync(userEntity); | ||
| } | ||
|
|
||
| return await TwoFactorEnabledAsync( | ||
| user.GetTwoFactorProviders(), | ||
| async () => | ||
| { | ||
| var calcUser = await _userRepository.GetCalculatedPremiumAsync(userId.Value); | ||
| return calcUser?.HasPremiumAccess ?? false; | ||
| }); | ||
| user.GetTwoFactorProviders(), | ||
| async () => | ||
| { | ||
| var calcUser = await _userRepository.GetCalculatedPremiumAsync(userId.Value); | ||
| return calcUser?.HasPremiumAccess ?? false; | ||
| }); | ||
| } | ||
|
|
||
| private async Task<IEnumerable<(Guid userId, bool twoFactorIsEnabled)>> TwoFactorIsEnabledVNextAsync(IEnumerable<Guid> userIds) | ||
| { | ||
| var result = new List<(Guid userId, bool hasTwoFactor)>(); | ||
| if (userIds == null || !userIds.Any()) | ||
| { | ||
| return result; | ||
| } | ||
|
|
||
| var users = await _userRepository.GetManyAsync([.. userIds]); | ||
|
Contributor
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. If some user IDs don't exist in the database, Suggested fix: var users = await _userRepository.GetManyAsync([.. userIds]);
if (users.Count() != userIds.Distinct().Count())
{
var foundUserIds = users.Select(u => u.Id).ToHashSet();
var missingIds = userIds.Where(id => !foundUserIds.Contains(id));
throw new NotFoundException($"Users not found: {string.Join(", ", missingIds)}");
}This is especially important since the single-user overload throws |
||
|
|
||
| // Get enabled providers for each user | ||
| var usersTwoFactorProvidersMap = users.ToDictionary(u => u.Id, GetEnabledTwoFactorProviders); | ||
|
|
||
| // Bulk fetch premium status only for users who need it (those with only premium providers) | ||
| var userIdsNeedingPremium = usersTwoFactorProvidersMap | ||
| .Where(kvp => kvp.Value.Any() && kvp.Value.All(TwoFactorProvider.RequiresPremium)) | ||
| .Select(kvp => kvp.Key) | ||
| .ToList(); | ||
|
|
||
| var premiumStatusMap = userIdsNeedingPremium.Count > 0 | ||
| ? await _hasPremiumAccessQuery.HasPremiumAccessAsync(userIdsNeedingPremium) | ||
| : new Dictionary<Guid, bool>(); | ||
|
|
||
| foreach (var user in users) | ||
| { | ||
| var userTwoFactorProviders = usersTwoFactorProvidersMap[user.Id]; | ||
|
|
||
| if (!userTwoFactorProviders.Any()) | ||
| { | ||
| result.Add((user.Id, false)); | ||
| continue; | ||
| } | ||
|
|
||
| // User has providers. If they're in the premium check map, verify premium status | ||
| var twoFactorIsEnabled = !premiumStatusMap.TryGetValue(user.Id, out var hasPremium) || hasPremium; | ||
|
Contributor
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 clarification needed This line is subtle and could be clearer: var twoFactorIsEnabled = !premiumStatusMap.TryGetValue(user.Id, out var hasPremium) || hasPremium;This means:
Consider adding a comment or refactoring for clarity: // If user wasn't checked for premium (has free providers), they're enabled
// Otherwise, they're only enabled if they have premium
var twoFactorIsEnabled = !premiumStatusMap.TryGetValue(user.Id, out var hasPremium) || hasPremium; |
||
| result.Add((user.Id, twoFactorIsEnabled)); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| private async Task<IEnumerable<(T user, bool twoFactorIsEnabled)>> TwoFactorIsEnabledVNextAsync<T>(IEnumerable<T> users) | ||
| where T : ITwoFactorProvidersUser | ||
| { | ||
| var userIds = users | ||
| .Select(u => u.GetUserId()) | ||
| .Where(u => u.HasValue) | ||
| .Select(u => u.Value) | ||
| .ToList(); | ||
|
|
||
| var twoFactorResults = await TwoFactorIsEnabledVNextAsync(userIds); | ||
|
|
||
| var result = new List<(T user, bool twoFactorIsEnabled)>(); | ||
|
|
||
| foreach (var user in users) | ||
| { | ||
| var userId = user.GetUserId(); | ||
| if (userId.HasValue) | ||
| { | ||
| var hasTwoFactor = twoFactorResults.FirstOrDefault(res => res.userId == userId.Value).twoFactorIsEnabled; | ||
|
Contributor
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. When Consider: var twoFactorResult = twoFactorResults.FirstOrDefault(res => res.userId == userId.Value);
if (twoFactorResult == default)
{
throw new NotFoundException($"Two-factor status not found for user {userId.Value}");
}
result.Add((user, twoFactorResult.twoFactorIsEnabled));This would make failures more explicit and easier to debug. |
||
| result.Add((user, hasTwoFactor)); | ||
| } | ||
| else | ||
| { | ||
| result.Add((user, false)); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| private async Task<bool> TwoFactorIsEnabledVNextAsync(User user) | ||
| { | ||
| var enabledProviders = GetEnabledTwoFactorProviders(user); | ||
|
|
||
| if (!enabledProviders.Any()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // If all providers require premium, check if user has premium access | ||
| if (enabledProviders.All(TwoFactorProvider.RequiresPremium)) | ||
| { | ||
| return await _hasPremiumAccessQuery.HasPremiumAccessAsync(user.Id); | ||
| } | ||
|
|
||
| // User has at least one non-premium provider | ||
| return true; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets all enabled two-factor provider types for a user. | ||
| /// </summary> | ||
| /// <param name="user">user with two factor providers</param> | ||
| /// <returns>list of enabled provider types</returns> | ||
| private static IList<TwoFactorProviderType> GetEnabledTwoFactorProviders(User user) | ||
| { | ||
| var providers = user.GetTwoFactorProviders(); | ||
|
|
||
| if (providers == null || providers.Count == 0) | ||
| { | ||
| return Array.Empty<TwoFactorProviderType>(); | ||
| } | ||
|
|
||
| // TODO: PM-21210: In practice we don't save disabled providers to the database, worth looking into. | ||
| return (from provider in providers | ||
| where provider.Value?.Enabled ?? false | ||
| select provider.Key).ToList(); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| ๏ปฟnamespace Bit.Core.Billing.Premium.Models; | ||
|
|
||
| /// <summary> | ||
| /// Represents user premium access status from personal subscriptions and organization memberships. | ||
| /// </summary> | ||
| public class UserPremiumAccess | ||
| { | ||
| /// <summary> | ||
| /// The unique identifier for the user. | ||
| /// </summary> | ||
| public Guid Id { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Indicates whether the user has a personal premium subscription. | ||
| /// This does NOT include premium access from organizations. | ||
| /// </summary> | ||
| public bool PersonalPremium { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Indicates whether the user has premium access through any organization membership. | ||
| /// This is true if the user is a member of at least one enabled organization that grants premium access to users. | ||
| /// </summary> | ||
| public bool OrganizationPremium { get; set; } | ||
|
|
||
| /// <summary> | ||
| /// Indicates whether the user has premium access from any source (personal subscription or organization). | ||
| /// </summary> | ||
| public bool HasPremiumAccess => PersonalPremium || OrganizationPremium; | ||
|
Contributor
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. ๐ Nice: Computed property is clear and correct The |
||
| } | ||
eliykat marked this conversation as resolved.
Show resolved
Hide resolved
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| ๏ปฟusing Bit.Core.Exceptions; | ||
| using Bit.Core.Repositories; | ||
|
|
||
| namespace Bit.Core.Billing.Premium.Queries; | ||
|
|
||
| public class HasPremiumAccessQuery : IHasPremiumAccessQuery | ||
| { | ||
| private readonly IUserRepository _userRepository; | ||
|
|
||
| public HasPremiumAccessQuery(IUserRepository userRepository) | ||
| { | ||
| _userRepository = userRepository; | ||
| } | ||
|
|
||
| public async Task<bool> HasPremiumAccessAsync(Guid userId) | ||
| { | ||
| var user = await _userRepository.GetPremiumAccessAsync(userId); | ||
| if (user == null) | ||
| { | ||
| throw new NotFoundException(); | ||
r-tome marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| return user.HasPremiumAccess; | ||
| } | ||
|
|
||
| public async Task<Dictionary<Guid, bool>> HasPremiumAccessAsync(IEnumerable<Guid> userIds) | ||
| { | ||
| var distinctUserIds = userIds.Distinct().ToList(); | ||
| var usersWithPremium = await _userRepository.GetPremiumAccessByIdsAsync(distinctUserIds); | ||
|
|
||
| if (usersWithPremium.Count() != distinctUserIds.Count) | ||
|
Contributor
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. ๐จ Minor: Use LINQ Count() for better clarity Consider using if (usersWithPremium.Count != distinctUserIds.Count)This avoids potential enumeration overhead if |
||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| return usersWithPremium.ToDictionary(u => u.Id, u => u.HasPremiumAccess); | ||
| } | ||
|
|
||
| public async Task<bool> HasPremiumFromOrganizationAsync(Guid userId) | ||
| { | ||
| var user = await _userRepository.GetPremiumAccessAsync(userId); | ||
| if (user == null) | ||
| { | ||
| throw new NotFoundException(); | ||
cyprain-okeke marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| return user.OrganizationPremium; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| ๏ปฟnamespace Bit.Core.Billing.Premium.Queries; | ||
|
|
||
| /// <summary> | ||
| /// Centralized query for checking if users have premium access through personal subscriptions or organizations. | ||
| /// Note: Different from User.Premium which only checks personal subscriptions. | ||
| /// </summary> | ||
| public interface IHasPremiumAccessQuery | ||
| { | ||
| /// <summary> | ||
| /// Checks if a user has premium access (personal or organization). | ||
| /// </summary> | ||
| /// <param name="userId">The user ID to check</param> | ||
| /// <returns>True if user can access premium features</returns> | ||
| Task<bool> HasPremiumAccessAsync(Guid userId); | ||
|
|
||
| /// <summary> | ||
| /// Checks premium access for multiple users. | ||
| /// </summary> | ||
| /// <param name="userIds">The user IDs to check</param> | ||
| /// <returns>Dictionary mapping user IDs to their premium access status</returns> | ||
| Task<Dictionary<Guid, bool>> HasPremiumAccessAsync(IEnumerable<Guid> userIds); | ||
|
Contributor
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. ๐ Documentation improvement: Specify exception behavior The interface documentation should specify what happens when users aren't found. Currently the implementation throws /// <summary>
/// Checks premium access for multiple users.
/// </summary>
/// <param name="userIds">The user IDs to check</param>
/// <returns>Dictionary mapping user IDs to their premium access status</returns>
/// <exception cref="NotFoundException">Thrown when any of the requested user IDs are not found</exception>
Task<Dictionary<Guid, bool>> HasPremiumAccessAsync(IEnumerable<Guid> userIds); |
||
|
|
||
| /// <summary> | ||
| /// Checks if a user belongs to any organization that grants premium (enabled org with UsersGetPremium). | ||
| /// Returns true regardless of personal subscription. Useful for UI decisions like showing subscription options. | ||
| /// </summary> | ||
| /// <param name="userId">The user ID to check</param> | ||
| /// <returns>True if user is in any organization that grants premium</returns> | ||
| Task<bool> HasPremiumFromOrganizationAsync(Guid userId); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.