[PM-16684] Integrate Pricing Service behind FF #5276
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Many instances of StaticStore use are just to get the ProductTierType of a PlanType, but this can be derived from the PlanType itself without having to fetch the entire plan.
|
New Issues (17)Checkmarx found the following issues in this Pull Request
Fixed Issues (26)Great job! The following issues were fixed in this Pull Request
|
amorask-bitwarden
changed the title
withinfocus
reviewed
Jan 16, 2025
|
The tweaks make sense to me -- on to true team review though. |
r-tome
reviewed
Jan 20, 2025
| message: $"Request to the Pricing Service failed with status {response.StatusCode}"); | ||
| } | ||
|
|
||
| private static async Task<T> DeserializeAsync<T>(HttpContent content) |
There was a problem hiding this comment.
This method could be replaced by ReadFromJsonAsync in System.Net.Http.Json
There was a problem hiding this comment.
Great call! Replaced in 8d60635.
| return new PlanAdapter(response); | ||
| if (response.IsSuccessStatusCode) | ||
| { | ||
| var plan = await DeserializeAsync<PlanDTO>(response.Content); |
There was a problem hiding this comment.
Suggested change
| var plan = await DeserializeAsync<PlanDTO>(response.Content); | |
| var plan = await response.Content.ReadFromJsonAsync<PlanDTO>(); |
There was a problem hiding this comment.
Addressed in 8d60635
r-tome
previously approved these changes
Jan 24, 2025
r-tome
approved these changes
Jan 27, 2025
jonashendrickx
approved these changes
Jan 28, 2025
cturnbull-bitwarden
approved these changes
Jan 28, 2025
|
Ephemeral Environment Details: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16684
📔 Objective
The primary goal of this PR is to replace invocations of the
StaticStore.Plansproperty andStaticStore.GetPlanmethod with invocations of the newly addedPricingClient.The Pricing Client
The
PricingClientis a simple wrapper over an HTTP Client that sends requests to the upcoming Bitwarden Pricing Service, an API responsible for managing Bitwarden's plan information.The key lines in the
PricingClientfor the purpose of this PR are:In effect, until the Pricing Service is actually stood up and the feature flag is enabled, we haven't really made any change to the data source we're using to get plan information. Additionally, we can maintain the same exact contract as we had with the
StaticStorebecause ultimately, responses from the Pricing Service will be mapped to theStaticStore.Planrecord type which is woven throughout the codebase. We are, however, changing the way retrieving this plan information is managed.Plan Retrieval
There are two major changes to retrieving plans within the platform that are addressed in this PR:
Our retrieval contracts have changed from
PlanType -> PlantoPlanType -> Task<Plan>since, at the point the feature flag is enabled, plan retrieval will include a cache request (not implemented yet), a network request or both. This has wide-ranging effects on the platform which consistently fell into a habit of retrieving plans in areas not ideal for data retrieval such as within several layers of nested models. This PR attempts to address this as efficiently as possible with the least potential disruptions to the platform; much of this will be identified as moving plan retrieval up to the closest asynchronous point of execution and then passing thePlandown through the model chain. It's not perfect, but it's what we've got without a large refactoring of our reliance on heavy and logic-containing models.Because the Pricing Service will be an internally managed Bitwarden service, we will no longer support retrieving plans on Self-Host instances. This is, in my opinion at least, correct. Plans are our own domain construct used to facilitate the purchase of subscriptions and grant specific feature access to our
Organizationentities. On the cloud side, our license generation is dependent on these plans, which is fine because we can retrieve plans on cloud. On the Self-Host side, the organization is created directly from the license itself, so there should be no need to have a detailed understanding of the plan that led to the licenses generation (outside of a few pieces of data for display purposes).Many places that were potential Self-Host usages were just retrieving a
Planfor the sake of getting theProductTier. But we don't need to do that asProductTiercan be derived directly fromPlanType.Considerations & Follow-Ups
Although I tried to remove every
StaticStoreinvocation, there are a few spots that remain problematic.The MaxProjectsQuery can be invoked in the Self-Host path and relies on plan information. I opened https://bitwarden.atlassian.net/browse/PM-17122 to address moving the information being garnered from the plan in the query to the
OrganizationLicenseitself.The IsAddOnSubscriptionItem utility in the
StaticStoreis used very deep within nested models in a Billing response. I've opened https://bitwarden.atlassian.net/browse/PM-16844 to move this simplebooldata out of our own domain and into Stripe as metadata attached to ourPriceentities.In the current platform, we handle an issue where the DLLs running on our users' Self-Host instances can be outdated, leading to new plan types that exist in cloud not existing on the Self-Host instance. We manage this by ensuring the
PlanTypeattached to theOrganizationLicenseexists in theStaticStorerunning on the user's instance. If it doesn't, we throw an exception. This PR removes that check because Self-Host instances can't invoke the Pricing Service, so this issue will ultimately reappear. I'd argue, however, that this check is unnecessary. Since the user purchased the organization subscription against Cloud, it's valid and the features assigned to the organization should be respected. It doesn't matter whether or not the new plan type exists on the Self-Host instance. The only place I can identify this being an issue is on theUpdateLicenseComponentwhere we check theProductTierTypeof the organization. However, I think this can be managed by introducing aProductTierType.Unkownand flipping this check to just assert the organization is Enterprise tier. (Ticket pending for this)Throughout the course of my testing, I also uncovered at least 3 areas that are invokable from a Self-Host instance, but can attempt to update a subscription, which should not be allowed:
📸 Screenshots
⏰ 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