[PM-11516] Initial license file refactor #5002
Open
+847
−107
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
…tains all license properties as claims
… license properties as claims
|
New Issues
Fixed Issues
|
# Conflicts: # src/Core/Constants.cs
LaunchDarkly flag references🔍 1 flag added or modified
|
r-tome
previously approved these changes
Nov 11, 2024
| var expires = subscriptionInfo.UpcomingInvoice?.Date?.AddDays(7) ?? entity.PremiumExpirationDate?.AddDays(7); | ||
| var refresh = subscriptionInfo.UpcomingInvoice?.Date ?? entity.PremiumExpirationDate; | ||
| var trial = (subscriptionInfo.Subscription?.TrialEndDate.HasValue ?? false) && | ||
| subscriptionInfo.Subscription.TrialEndDate.Value > DateTime.UtcNow; |
There was a problem hiding this comment.
You can probably leverage TimeProvider here.
There was a problem hiding this comment.
I'll note it for the next set of reactors in the next PR. In this one, I wanted to keep the logic as identical as possible to how it's working right now, just with the change of using a JWT
amorask-bitwarden
previously approved these changes
Nov 11, 2024
| claims.Add(new Claim(nameof(OrganizationLicenseConstants.BusinessName), entity.BusinessName)); | ||
| } | ||
|
|
||
| return Task.FromResult(claims); |
There was a problem hiding this comment.
❓ Out of curiosity, what's the purpose of this method returning a Task without doing anything asynchronously?
There was a problem hiding this comment.
This came out of a discussion I had with platform on the approach. The idea was to have a single interface that could be used to generate claims for any object. If we needed to have some async IO in some speculative implementation to calculate the value for a claim, then this would allow for that.
withinfocus
dismissed stale reviews from amorask-bitwarden and r-tome
via
51537ab
|
Had to do some quick hacking on this one -- please catch back up to |
# Conflicts: # src/Core/Constants.cs
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-11516
📔 Objective
This PR is the first, and most major iteration in a series of refactoring and cleaning up pull requests for our organization and user self-host licenses.
Currently, our licenses work based on an integer-based versioning system. Additionally, our cloud and self-host releases are at a minimum staggered by a couple of days, which can be even more if the self-host admins don't update immediately.
Suppose a new field is added to
OrganizationLicense, for example, and the version is incremented by one to accommodate the new field. In that case, self-host instances will break in the intervening time because they're not expecting the new field, and their resulting hash will not match the hash in the license file.To get around this, we've enabled Customer Success to generate licenses for any version current and prior using the Bitwarden Portal. While this workaround has been the status quo, we've decided to rectify this issue.
This pull request addresses the above problem by moving all current fields on
OrganizationLicenseandUserLicenseinto a new field,Tokenon each respective license. TheTokenis a string JWT containing a dictionary of claims that match the current fields, as well as a header and signature. In the above scenario, no additional versioning will be required from this point forward given that, if the self-host instance doesn't know to lookup a claim type, then it simply won't. The additional unused payload won't break the self-hosted instances ability to verify the signature using their public key.I made some attempts to minimize the number of changes in this pull request by using the existing paradigms and patterns given that licenses touch a fair number of files. In future pull requests, I'll be aiming to separate out application logic from the licenses, like
CanUseorVerifyData, into commands. Similarly, I'll be aiming to clean up or separate concerns inILicensingServiceinto commands and queries. Additionally, in a future PR I'll move billing related files to our own domain.📸 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