[PM-21411] Refactor interface for determining premium status and features #6688
Conversation
…on for checking user premium access status
…d feature flag for premium access checks. Enhanced user premium status retrieval logic and improved handling of user details based on feature flag state.
…ng users to new methods in IPremiumAccessQuery for premium access checks.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## auth/remove-2fa-user-from-premium-methods #6688 +/- ##
==============================================================================
- Coverage 57.23% 13.33% -43.91%
==============================================================================
Files 1917 1132 -785
Lines 85460 49837 -35623
Branches 7667 3936 -3731
==============================================================================
- Hits 48913 6645 -42268
- Misses 34706 43070 +8364
+ Partials 1841 122 -1719 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Nice work. I just noticed that User has a PremiumExpirationDate. Do we need to check if premium has expired? Or is the premium column automatically updated when the expiration date passes?
src/Core/Auth/UserFeatures/PremiumAccess/IPremiumAccessQuery.cs
Outdated
Show resolved
Hide resolved
| /// <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); |
There was a problem hiding this comment.
This is currently unused, I recommend removing it if we don't have a clear use case for it.
There was a problem hiding this comment.
It will eventually replace
There was a problem hiding this comment.
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 cached organization abilities and minimizes database queries. | ||
| /// </summary> | ||
| /// <param name="users">The users 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>> CanAccessPremiumBulkAsync(IEnumerable<User> users); |
There was a problem hiding this comment.
🎨 I think this is a good use case for an overload rather than adding Bulk to the name.
| // Try to get premium status without fetching User entity if possible | ||
| bool hasPersonalPremium; | ||
| if (user is User userEntity) | ||
| { | ||
| hasPersonalPremium = userEntity.Premium; | ||
| } | ||
| else if (user is OrganizationUserUserDetails orgUserDetails) | ||
| { | ||
| hasPersonalPremium = orgUserDetails.Premium.GetValueOrDefault(false); | ||
| } | ||
| else | ||
| { | ||
| // Fallback: fetch the User entity | ||
| var fetchedUser = await _userRepository.GetByIdAsync(userId.Value); | ||
| if (fetchedUser == null) | ||
| { | ||
| return false; | ||
| } | ||
| hasPersonalPremium = fetchedUser.Premium; | ||
| } |
There was a problem hiding this comment.
This is trying to do the job of your new query, which is meant to be the only implementation of this logic. Some options:
- maybe Auth were wrong to remove the
GetPremiummethod fromITwoFactorProvidersUser? It was unused, but it seems like you have a use for it here. Although if you go this way, I would do it like this:
public interface IPremiumUser
{
public bool HasPremium { get; set; } // move this to its own interface
}
public interface ITwoFactorProvidersUser : IPremiumUser // this inherits it
{
// snip
}
public interface IPremiumAccessQuery
{
Task<bool> CanAccessPremiumAsync(IPremiumUser user); // accepts the interface instead of any particular object
// snip
}
// and now in this method, you can just pass the argument along...
_premiumAccessQuery.CanAccessPremiumAsync(user);Other options:
- require the caller to pass in a
Userto begin with - just always fetch the
User, don't worry about it
There was a problem hiding this comment.
I went with the simplest option of just having the callers pass in User objects. I tried to go with the IPremiumUser solution but it was more complex than it needed to because we also needed the Id for the bulk method
| foreach (var userDetail in userDetails) | ||
| { | ||
| result.Add( | ||
| (userDetail.Id, | ||
| await TwoFactorEnabledAsync(userDetail.GetTwoFactorProviders(), | ||
| () => Task.FromResult(userDetail.HasPremiumAccess)) | ||
| ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
We shouldn't call an async method in a loop, even though it's a kind of fake async (using Task.FromResult), it's too liable to be misused. This is where I think writing a fresh implementation will help.
| // Get all org memberships for these users in one query | ||
| // Note: GetManyByManyUsersAsync only returns Accepted and Confirmed status org users | ||
| var allOrgUsers = await _organizationUserRepository.GetManyByManyUsersAsync(userIds); | ||
| var orgUsersGrouped = allOrgUsers | ||
| .Where(ou => ou.UserId.HasValue) | ||
| .GroupBy(ou => ou.UserId!.Value) | ||
| .ToDictionary(g => g.Key, g => g.ToList()); | ||
|
|
||
| var orgAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync(); | ||
|
|
||
| foreach (var user in usersList) | ||
| { | ||
| var hasPersonalPremium = user.Premium; | ||
| if (hasPersonalPremium) | ||
| { | ||
| result[user.Id] = true; | ||
| continue; | ||
| } | ||
|
|
||
| var hasPremiumFromOrg = orgUsersGrouped.TryGetValue(user.Id, out var userOrgs) && | ||
| userOrgs.Any(ou => | ||
| orgAbilities.TryGetValue(ou.OrganizationId, out var orgAbility) && | ||
| orgAbility.UsersGetPremium && | ||
| orgAbility.Enabled); | ||
|
|
||
| result[user.Id] = hasPremiumFromOrg; | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
This has a database call + cache + a nested loop to get information that ultimately comes from the database anyway.
I suggest a simpler implementation would be a new sproc for this purpose (untested):
SELECT
U.[Id],
MAX(U.[Premium]) PersonalPremium,
MAX(O.[UsersGetPremium]) OrganizationPremium
FROM
[dbo].[UserView] U
JOIN [dbo].[OrganizationUserView] OU ON U.Id = OU.UserId
JOIN [dbo].[Organization] O ON OU.OrganizationId = O.Id
WHERE
U.[Id] IN (SELECT [Id] FROM @Ids)
GROUP BY
U.[Id]And then this code could just return PersonalPremium || OrganizationPremium for each user.
I think this is better than the current sproc because it's not returning this weird UserWithCalculatedPremium object, it's very clear what its purpose is, and it's only used by this query.
There was a problem hiding this comment.
I'm using the existing UserWithCalculatedPremium for now. I'll refactor that later here on in a separate branch
There was a problem hiding this comment.
You're totally right, btw. I think initially I didn't retrieve the many users and just used the cache but for some reason this evolved and I kept that logic.
r-tome
changed the base branch from
main
to
auth/remove-2fa-user-from-premium-methods
…improved premium access checks and user detail handling. Removed obsolete feature service dependency and enhanced test coverage for new functionality.
…rloaded CanAccessPremiumAsync method. Update related methods to streamline premium access checks using the User object directly. Enhance test coverage by removing obsolete tests and ensuring proper functionality with the new method signatures.
…rDetails and User classes to clarify its usage and limitations regarding personal and organizational premium access.
…arameter with Guid for user ID in CanAccessPremiumAsync methods. Update related methods and tests to streamline premium access checks and improve clarity in method signatures.
r-tome
changed the title
… use 'PremiumAccessQuery' instead of 'PremiumAccessCacheCheck'. Adjust related XML documentation for clarity on premium access methods.
|
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
⛏️ Extra newlines at end of file
There was a problem hiding this comment.
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.
| public async Task<bool> HasPremiumAccessAsync(Guid userId) | ||
| { | ||
| var user = await _userRepository.GetCalculatedPremiumAsync(userId); | ||
| return user?.HasPremiumAccess ?? false; |
There was a problem hiding this comment.
If there is no user, something has gone wrong. I think it should throw an exception in this case.
| /// <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); |
There was a problem hiding this comment.
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).
|
|
||
| public async Task<Dictionary<Guid, bool>> HasPremiumAccessAsync(IEnumerable<Guid> userIds) | ||
| { | ||
| var usersWithPremium = await _userRepository.GetManyWithCalculatedPremiumAsync(userIds); |
There was a problem hiding this comment.
To be consistent with the other method, and with the interface for this query (which promises a key for each userId), I think this should also check and throw for missing users.
| } | ||
|
|
||
| // Has org premium if has premium access but not personal premium | ||
| return user.HasPremiumAccess && !user.Premium; |
There was a problem hiding this comment.
This is subtly different to the current implementation in UserService.HasPremiumFromOrganization: that current logic will return false if the user is not a part of any orgs, but will not return false if they have both personal premium and premium from an org. That doesn't seem right either, but it's unclear what the intent is.
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 ?
| // If there are only premium two factor options then check premium access | ||
| var onlyHasPremiumTwoFactor = enabledProviderKeys.All(TwoFactorProvider.RequiresPremium); | ||
| if (onlyHasPremiumTwoFactor) | ||
| { | ||
| return hasPremiumAccess; | ||
| } |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I think you still need to wire up the new methods here. (preferably just have the old methods call them and return the result)
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-21411
📔 Objective
Introduce
IPremiumAccessQueryto centralize premium-access checks and make the distinction clearer between having a personal premium subscription (User.Premium) and actually having access to premium features (personal subscription or org membership).This branch builds on #6684
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes