-
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: auth/remove-2fa-user-from-premium-methods
Are you sure you want to change the base?
Changes from all commits
f83a790
d30f3bd
377ae31
683a6cb
08e1413
e494a10
0cdadd1
192052b
2d31237
123f579
57252a7
bf0cc7a
65941d4
2a966e3
96a3615
516824a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
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. I think you still need to wire up the new methods here. (preferably just have the old methods call them and return the result) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,13 +4,24 @@ | |
| 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.Repositories; | ||
|
|
||
| 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; | ||
|
|
||
| public TwoFactorIsEnabledQuery( | ||
| IUserRepository userRepository, | ||
| IHasPremiumAccessQuery hasPremiumAccessQuery) | ||
| { | ||
| _userRepository = userRepository; | ||
| _hasPremiumAccessQuery = hasPremiumAccessQuery; | ||
| } | ||
|
|
||
| public async Task<IEnumerable<(Guid userId, bool twoFactorIsEnabled)>> TwoFactorIsEnabledAsync(IEnumerable<Guid> userIds) | ||
| { | ||
|
|
@@ -72,12 +83,113 @@ public async Task<bool> TwoFactorIsEnabledAsync(ITwoFactorProvidersUser user) | |
| } | ||
|
|
||
| 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; | ||
| }); | ||
| } | ||
|
|
||
| public 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]); | ||
| var premiumStatus = await _hasPremiumAccessQuery.HasPremiumAccessAsync(userIds); | ||
|
|
||
| foreach (var user in users) | ||
| { | ||
| var twoFactorProviders = user.GetTwoFactorProviders(); | ||
| var hasPremiumAccess = premiumStatus.GetValueOrDefault(user.Id, false); | ||
| var twoFactorIsEnabled = TwoFactorIsEnabled(twoFactorProviders, hasPremiumAccess); | ||
|
|
||
| result.Add((user.Id, twoFactorIsEnabled)); | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| public 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; | ||
| result.Add((user, hasTwoFactor)); | ||
| } | ||
| else | ||
| { | ||
| result.Add((user, false)); | ||
| } | ||
| } | ||
|
|
||
| return result; | ||
| } | ||
|
|
||
| public async Task<bool> TwoFactorIsEnabledVNextAsync(User user) | ||
| { | ||
| var providers = user.GetTwoFactorProviders(); | ||
| var hasPremium = await _hasPremiumAccessQuery.HasPremiumAccessAsync(user.Id); | ||
|
|
||
| return TwoFactorIsEnabled(providers, hasPremium); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Checks to see what kind of two-factor is enabled. | ||
| /// Synchronous version used when premium access status is already known. | ||
| /// </summary> | ||
| /// <param name="providers">dictionary of two factor providers</param> | ||
| /// <param name="hasPremiumAccess">whether the user has premium access</param> | ||
| /// <returns>true if the user has two factor enabled; false otherwise</returns> | ||
| private static bool TwoFactorIsEnabled( | ||
| Dictionary<TwoFactorProviderType, TwoFactorProvider> providers, | ||
| bool hasPremiumAccess) | ||
| { | ||
| // If there are no providers, then two factor is not enabled | ||
| if (providers == null || providers.Count == 0) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Get all enabled providers | ||
| // TODO: PM-21210: In practice we don't save disabled providers to the database, worth looking into. | ||
| var enabledProviderKeys = from provider in providers | ||
| where provider.Value?.Enabled ?? false | ||
| select provider.Key; | ||
|
|
||
| // If no providers are enabled then two factor is not enabled | ||
| if (!enabledProviderKeys.Any()) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // If there are only premium two factor options then check premium access | ||
| var onlyHasPremiumTwoFactor = enabledProviderKeys.All(TwoFactorProvider.RequiresPremium); | ||
| if (onlyHasPremiumTwoFactor) | ||
| { | ||
| return hasPremiumAccess; | ||
| } | ||
|
Comment on lines
+184
to
+189
Member
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. I think this was previously done to avoid the db call, which was passed in as a delegate to defer that work. But now we're doing our database call upfront, I think this whole method could be simplified to: return providers != null && providers.Values.Any(p => p.Enabled);which means you probably don't need the private method at all. I think that's a great simplification of this logic. That said, Auth might have greater insight into any performance requirements here re: that database call (and this logic generally). |
||
|
|
||
| // The user has at least one non-premium two factor option | ||
| return true; | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
|
Member
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. It's so simple now ๐ |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| ๏ปฟusing Bit.Core.Repositories; | ||
|
|
||
| namespace Bit.Core.Billing.Premium.Queries; | ||
|
|
||
| /// <summary> | ||
| /// Query for checking premium access status for users using the existing stored procedure | ||
| /// that calculates premium access from personal subscriptions and organization memberships. | ||
| /// </summary> | ||
| 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.GetCalculatedPremiumAsync(userId); | ||
|
Check warning on line 20 in src/Core/Billing/Premium/Queries/HasPremiumAccessQuery.cs
|
||
| return user?.HasPremiumAccess ?? false; | ||
|
Member
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 there is no |
||
| } | ||
|
|
||
| public async Task<Dictionary<Guid, bool>> HasPremiumAccessAsync(IEnumerable<Guid> userIds) | ||
| { | ||
| var usersWithPremium = await _userRepository.GetManyWithCalculatedPremiumAsync(userIds); | ||
|
Check warning on line 26 in src/Core/Billing/Premium/Queries/HasPremiumAccessQuery.cs
|
||
|
Member
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. To be consistent with the other method, and with the interface for this query (which promises a key for each |
||
| return usersWithPremium.ToDictionary(u => u.Id, u => u.HasPremiumAccess); | ||
| } | ||
|
|
||
| public async Task<bool> HasPremiumFromOrganizationAsync(Guid userId) | ||
| { | ||
| var user = await _userRepository.GetCalculatedPremiumAsync(userId); | ||
|
Check warning on line 32 in src/Core/Billing/Premium/Queries/HasPremiumAccessQuery.cs
|
||
| if (user == null) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Has org premium if has premium access but not personal premium | ||
| return user.HasPremiumAccess && !user.Premium; | ||
|
Member
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. This is subtly different to the current implementation in That said, I have no idea why we need this logic: it's only synced to clients but doesn't seem to be used there either. I would be interested to know if we could remove it. Any ideas @amorask-bitwarden ? |
||
| } | ||
| } | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,41 @@ | ||||
| ๏ปฟnamespace Bit.Core.Billing.Premium.Queries; | ||||
|
|
||||
| /// <summary> | ||||
| /// Query for checking premium access status for users. | ||||
| /// This is the centralized location for determining if a user can access premium features | ||||
| /// (either through personal subscription or organization membership). | ||||
| /// | ||||
| /// <para> | ||||
| /// <strong>Note:</strong> This is different from checking User.Premium, which only indicates | ||||
| /// personal subscription status. Use these methods to check actual premium feature access. | ||||
| /// </para> | ||||
| /// </summary> | ||||
| public interface IHasPremiumAccessQuery | ||||
| { | ||||
| /// <summary> | ||||
| /// Checks if a user has access to premium features (personal subscription or organization). | ||||
| /// This is the definitive way to check premium access for a single user. | ||||
| /// </summary> | ||||
| /// <param name="userId">The user ID to check for premium access</param> | ||||
| /// <returns>True if user can access premium features; false otherwise</returns> | ||||
| Task<bool> HasPremiumAccessAsync(Guid userId); | ||||
|
|
||||
| /// <summary> | ||||
| /// Checks if a user has access to premium features through organization membership only. | ||||
| /// This is useful for determining the source of premium access (personal vs organization). | ||||
| /// </summary> | ||||
| /// <param name="userId">The user ID to check for organization premium access</param> | ||||
| /// <returns>True if user has premium access through any organization; false otherwise</returns> | ||||
| Task<bool> HasPremiumFromOrganizationAsync(Guid userId); | ||||
|
Comment on lines
+23
to
+29
Member
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. This is currently unused, I recommend removing it if we don't have a clear use case for it.
Contributor
Author
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. It will eventually replace
Member
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. Does it need to be wired up in that method though? It's not called from anywhere at the moment (as far as I can see). |
||||
|
|
||||
| /// <summary> | ||||
| /// Checks if multiple users have access to premium features (optimized bulk operation). | ||||
| /// Uses existing stored procedure that calculates premium from personal subscriptions and organizations. | ||||
| /// </summary> | ||||
| /// <param name="userIds">The user IDs to check for premium access</param> | ||||
| /// <returns>Dictionary mapping user IDs to their premium access status (personal or through organization)</returns> | ||||
| Task<Dictionary<Guid, bool>> HasPremiumAccessAsync(IEnumerable<Guid> userIds); | ||||
| } | ||||
|
|
||||
|
|
||||
|
|
||||
|
Member
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. These methods are no longer obsolete, because your new query is using it. Although you may still want to mention that this is for internal use only, and devs should use the query instead. All of that said - I think this sproc could be simplified per my last review, and now is a good time to do it. (i.e. by making a new sproc used by the new code, then deleting this when you clean up your feature flag). I'll leave it up to you though. It could also be a separate ticket. |
Uh oh!
There was an error while loading. Please reload this page.