[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 @@
## main #6688 +/- ##
==========================================
+ Coverage 53.64% 57.52% +3.88%
==========================================
Files 1926 1926
Lines 85711 85855 +144
Branches 7686 7707 +21
==========================================
+ Hits 45978 49390 +3412
+ Misses 37961 34611 -3350
- Partials 1772 1854 +82 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
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).
src/Core/Auth/UserFeatures/PremiumAccess/IPremiumAccessQuery.cs
Outdated
Show resolved
Hide resolved
src/Core/Auth/UserFeatures/TwoFactorAuth/TwoFactorIsEnabledQuery.cs
Outdated
Show resolved
Hide resolved
src/Core/Auth/UserFeatures/TwoFactorAuth/TwoFactorIsEnabledQuery.cs
Outdated
Show resolved
Hide resolved
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.
| /// <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).
| } | ||
|
|
||
| // 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 ?
There was a problem hiding this comment.
I have updated the new implementation to match the logic in UserService.
HasPremiumFromOrganization is used through SyncController for billing logic.
src/Core/Auth/UserFeatures/TwoFactorAuth/TwoFactorIsEnabledQuery.cs
Outdated
Show resolved
Hide resolved
trmartin4
dismissed
amorask-bitwarden’s stale review
The base branch was changed.
| var userEntity = user as User ?? await _userRepository.GetByIdAsync(userId.Value); | ||
| if (userEntity == null) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
I think this should throw - there should be a valid User in the database.
…tatus # Conflicts: # src/Core/Constants.cs # src/Core/Services/IUserService.cs
…g and improve test clarity * Replaced fully qualified exception references with simplified ones. * Refactored test setup to use individual user variables for better readability. * Ensured assertions reflect the updated user variable structure.
…istent users * Updated TwoFactorIsEnabledQuery to throw NotFoundException when a user is not found instead of returning false. * Added a new unit test to verify that the NotFoundException is thrown when a user is not found while premium access query is enabled.
@eliykat the Premium field is automatically set to false. For example in cloud instances its done via a Stripe event https://github.com/bitwarden/server/blob/main/src/Billing/Services/Implementations/SubscriptionUpdatedHandler.cs |
* Updated CanAccessPremium and HasPremiumFromOrganization methods to clarify usage with the new premium access query. * Integrated IHasPremiumAccessQuery into UserService for improved premium access handling based on feature flag. * Adjusted method documentation to reflect changes in premium access logic.
* Modified Obsolete attribute messages for GetManyWithCalculatedPremiumAsync and GetCalculatedPremiumAsync to indicate that callers should use the new methods when the 'PremiumAccessQuery' feature flag is enabled. * Enhanced documentation to improve clarity regarding premium access handling.
…mium access methods * Modified Obsolete attribute messages for GetManyWithCalculatedPremiumAsync and GetCalculatedPremiumAsync in IUserRepository to indicate these methods will be removed in a future version. * Updated Obsolete attribute message for HasPremiumFromOrganization in IUserService to reflect the same deprecation notice.
* Consolidated user ID retrieval logic to avoid redundancy. * Ensured consistent handling of user ID checks for premium access queries. * Improved code readability by reducing duplicate code blocks.
r-tome
requested review from
BTreston,
Patrick-Pimentel-Bitwarden,
amorask-bitwarden,
cyprain-okeke and
eliykat
|
Claude finished @r-tome's task —— View job Code Review CompleteTasks
Code Review SummaryOverall Assessment: ✅ APPROVE This PR introduces a well-architected solution for centralizing premium access checks with proper feature gating, comprehensive test coverage, and backward compatibility. The implementation correctly distinguishes between personal premium subscriptions and actual premium feature access (personal or organizational). Key Findings by CategorySecurity ✓ PASS
Correctness ✓ PASS
Database Changes ✓ PASSView (
Stored Procedure (
Index Update:
Migration Script:
Feature Flag Implementation ✓ PASS
Test Coverage ✓ EXCELLENTUnit Tests:
Integration Tests:
Code Quality ✓ PASS
Breaking Changes ✓ NONE
Observations & Suggestions🎨 SUGGESTED: Consider OrganizationUser index optimizationLocation: The view uses a correlated subquery on WHERE OU.[UserId] = U.[Id]
AND O.[UsersGetPremium] = 1
AND O.[Enabled] = 1Consider adding a composite index on Impact: Could improve query performance when checking premium access for many users. 🎨 SUGGESTED: EF implementation could use raw SQL queryLocation: The EF implementation uses var users = await dbContext.Users
.Where(x => ids.Contains(x.Id))
.Include(u => u.OrganizationUsers)
.ThenInclude(ou => ou.Organization)
.ToListAsync();Consider using a raw SQL query to directly query the Current approach works correctly but loads more data than necessary. ❓ QUESTION: Migration script dateLocation: The migration script is dated 2025-12-09. Is this intentional, or should it match today's actual date? The commit history shows it was renamed from an earlier date per commit ❓ QUESTION: Obsolete method removal timelineLocation: Multiple files with The PR marks several methods as obsolete with clear migration guidance:
Is there a follow-up ticket to remove these methods after the feature flag is enabled and stable? This would complete the refactoring. 💭 OBSERVATION: NotFoundException behavior in bulk queryLocation: if (usersWithPremium.Count() != distinctUserIds.Count)
{
throw new NotFoundException();
}This throws Current behavior is consistent with single-user method, so this is just an observation. ♻️ DEBT: TODO comment in production codeLocation: // TODO: PM-21210: In practice we don't save disabled providers to the database, worth looking into.This TODO appears in both the old and new code paths. Consider creating/linking the Jira ticket PM-21210 or removing if not actionable. Architecture & Design ✓ EXCELLENTThe PR demonstrates excellent architectural decisions:
Statistics
Recommendation✅ APPROVE - This PR is ready to merge pending any discussion on the suggestions above. The implementation is solid with:
The suggestions are optimizations that could be addressed in follow-up work if needed. |
cyprain-okeke
left a comment
cyprain-okeke
left a comment
There was a problem hiding this comment.
Nice Work, I just have some few clarifcations
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-21411
📔 Objective
Introduce
IHasPremiumAccessQueryto 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 new query uses a new database view (
UserPremiumAccessView) and a new stored procedure (User_ReadPremiumAccessByIds) to efficiently check premium status in bulk.The implementation is gated behind the
PremiumAccessQueryfeature flag.⏰ 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