[PM-29556] Fix: changing organization plan nulls out public and private keys #6738
Conversation
- make both keys required in new DTO model - move extensions class to higher level for reuse - fix mistakes
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6738 +/- ##
==========================================
- Coverage 58.58% 54.73% -3.86%
==========================================
Files 1920 1920
Lines 85264 85226 -38
Branches 7632 7626 -6
==========================================
- Hits 49953 46645 -3308
- Misses 33465 36808 +3343
+ Partials 1846 1773 -73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…anging-organization-plan-nulls-out-public-and-private-keys
| organization.UseGroups = newPlan.HasGroups; | ||
| organization.UseDirectory = newPlan.HasDirectory; | ||
| organization.UseEvents = newPlan.HasEvents; | ||
| organization.UseTotp = newPlan.HasTotp; | ||
| organization.Use2fa = newPlan.Has2fa; | ||
| organization.UseApi = newPlan.HasApi; | ||
| organization.UseSso = newPlan.HasSso; | ||
| organization.UseOrganizationDomains = newPlan.HasOrganizationDomains; | ||
| organization.UseKeyConnector = newPlan.HasKeyConnector ? organization.UseKeyConnector : false; | ||
| organization.UseScim = newPlan.HasScim; | ||
| organization.UseResetPassword = newPlan.HasResetPassword; | ||
| organization.SelfHost = newPlan.HasSelfHost; |
There was a problem hiding this comment.
Unrelated fix: remove duplicate property assignments.
|
Claude finished @eliykat's task in 2m 1s —— View job Code Review Complete
Overall Assessment: APPROVE Reviewed security-critical fix for organization encryption key handling during plan upgrades across 18 files. The |
|
Overall Assessment: APPROVE Reviewed security-critical fix for organization encryption key handling during plan upgrades. The implementation correctly prevents existing keys from being overwritten and maintains proper atomicity. Test coverage is comprehensive. |
|
Just FYI, this will be the first thing on my todo list tomorrow. Sorry, I couldn’t get to it sooner. |
…n-nulls-out-public-and-private-keys
…n-nulls-out-public-and-private-keys
eliykat
deleted the
ac/pm-29556/server-changing-organization-plan-nulls-out-public-and-private-keys
branch
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-29556
📔 Objective
Bug
When organizations upgraded their plan, the server unconditionally overwrote the organization's public and private keys with whatever the client sent, including null values. This caused existing organizations to lose their encryption keys during plan changes.
Fix
Replaced unconditional assignment with the existing
BackfillPublicPrivateKeysextension method, which only sets keys if:This preserves the legacy migration behavior for old organizations created without keypairs while protecting existing keys from being overwritten.
Additionally, the logic in
BackfillPublicPrivateKeyswas strengthened so that both keys are updated together or not at all.Related Changes
OrganizationUpgrade,OrganizationUpdateRequest) to usePublicKeyEncryptionKeyPairDatafor keys (this is KM Team's recommended DTO)BackfillPublicPrivateKeysextension method to acceptPublicKeyEncryptionKeyPairDataso it can be used from multiple places📸 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