[PM-27731] Updated organization licenses to save the correct values from the token #6546
Conversation
|
Claude Code is working… I'll analyze this and get back to you. |
|
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6546 +/- ##
==========================================
+ Coverage 55.26% 55.46% +0.19%
==========================================
Files 1939 1939
Lines 86224 86276 +52
Branches 7705 7706 +1
==========================================
+ Hits 47655 47854 +199
+ Misses 36777 36638 -139
+ Partials 1792 1784 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cyprain-okeke
left a comment
cyprain-okeke
left a comment
There was a problem hiding this comment.
Nice Work sir, Left some few comments
src/Core/Billing/Organizations/Commands/UpdateOrganizationLicenseCommand.cs
Show resolved
Hide resolved
test/Core.Test/Billing/Organizations/Commands/UpdateOrganizationLicenseCommandTests.cs
Show resolved
Hide resolved
# Conflicts: # src/Core/AdminConsole/Models/Data/Organizations/SelfHostedOrganizationDetails.cs # test/Core.Test/Billing/Organizations/Commands/UpdateOrganizationLicenseCommandTests.cs
| license.UseOrganizationDomains = claimsPrincipal.GetValue<bool>(OrganizationLicenseConstants.UseOrganizationDomains); | ||
| license.UseAdminSponsoredFamilies = claimsPrincipal.GetValue<bool>(OrganizationLicenseConstants.UseAdminSponsoredFamilies); | ||
| license.UseAutomaticUserConfirmation = claimsPrincipal.GetValue<bool>(OrganizationLicenseConstants.UseAutomaticUserConfirmation); | ||
| } |
There was a problem hiding this comment.
The code extracts 29 properties, but two constants appear to be missing from OrganizationLicenseConstants:
- UseDisableSmAdsForUsers
- UsePhishingBlocker. Is this intentional?
There was a problem hiding this comment.
Nope, it wasn't. Those two seem to be new additions from when I opened this PR. I'll get them added, good catch
There was a problem hiding this comment.
Good catch @cyprain-okeke , I just pushed a commit addressing this. I also added a set of unit tests to guide future developers through the process of adding a property to the organization license
cyprain-okeke
left a comment
cyprain-okeke
left a comment
There was a problem hiding this comment.
Thanks for adding the properties
r-tome
left a comment
r-tome
left a comment
There was a problem hiding this comment.
Great work, using reflection is smart!
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27731
📔 Objective
Fix a bug where self-hosted organizations updating their license files were not using the Token JWT as the authoritative source of truth.
The
UpdateOrganizationLicenseCommandwas only extracting 1 out of 42 properties from Token-based (JWT claims) licenses. The other 41 properties were read from the deserialized JSON file properties instead of the authoritative Token claims. This created an architectural inconsistency where the Token (signed by the cloud) was not the single source of truth, and stale or incorrect values in the JSON properties could override the Token claims.This PR ensures the Token JWT is the single source of truth by extracting all organization properties from claims before validation and database update. Also fixes the same issue in user license updates and adds missing properties to
SelfHostedOrganizationDetails.ToOrganization().📸 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