[PM-28747] Storage limit bypass for enforce organization ownership policy #6759

shane-melton · 2025-12-18T23:25:05Z

🎟️ Tracking

📔 Objective

Check the organization policies for the OrganizationDataOwnership during share operations and skip the storage limit checks when the policy is enabled. This reduces frictions for users that have attachments that need to be transferred to an enforcing organization during the migration period. Organization administrators will need to deal with any storage exceedance afterwards by either removing attachments or increasing their storage plan limit.

⏰ Reminders before review

Contributor guidelines followed
All formatters and local linters executed and passed
Written new unit and / or integration tests where applicable
Protected functional changes with optionality (feature flags)
Used internationalization (i18n) for all UI strings
CI builds passed
Communicated to DevOps any deployment requirements
Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 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

…hip policy is enabled

claude · 2025-12-18T23:26:04Z

Claude finished @shane-melton's task in 2m 34s —— View job

Code Review Complete

I've completed a thorough review of the PR changes. Here are my findings:

Critical Issues

Missing Test Coverage for Disabled Feature Flag

Location: CipherServiceTests.cs:1367
The storage bypass is conditional on the MigrateMyVaultToMyItems feature flag, but no tests verify behavior when this flag is disabled
This is critical because the feature flag is the primary security control preventing unintended bypasses
Recommendation: Add a test where policy is enabled, storage is at limit, but feature flag is false - should fail with "Not enough storage available"

Technical Debt

Duplicate Code in OrganizationDataOwnershipPolicyRequirement

Location: OrganizationDataOwnershipPolicyRequirement.cs:71-82
The new EnforcedByOrg() method is identical to existing RequiresDefaultCollectionOnConfirm() method
Both methods: return _policyDetails.Any(p => p.OrganizationId == organizationId);
Recommendation: Either reuse existing method or add comments explaining why these semantically different concepts currently share implementation

Missing Edge Cases

No Test for UsePolicies = False

Location: CipherServiceTests.cs:1367
Code checks organization.UsePolicies before evaluating policy (line 1032 in CipherService.cs)
No test verifies that storage limits are still enforced when UsePolicies = false
Recommendation: Add test ensuring organizations without policies enabled don't get storage bypass

Questions

Audit Logging Consideration

Location: CipherService.cs:1008
Should storage limit bypasses be explicitly logged for compliance/security monitoring?
This would help admins track why storage usage increased and provide audit trail
May not be necessary if existing Cipher_Shared event is sufficient

Positive Findings

✅ Excellent test coverage for both new and legacy policy systems
✅ Proper security controls: feature flag, UsePolicies check, policy enforcement
✅ Zero-knowledge architecture compliance maintained
✅ Clear documentation about feature flag cleanup
✅ Appropriate gating with multiple validation layers

Response to author's comment: The EnforcedByOrg() implementation looks correct for checking the specific organization. Consider whether this could reuse the existing RequiresDefaultCollectionOnConfirm() method since they're identical.

...ganizationFeatures/Policies/PolicyRequirements/OrganizationDataOwnershipPolicyRequirement.cs

github-actions · 2025-12-18T23:40:47Z

Checkmarx One – Scan Summary & Details – 0ae1b1a1-2e98-404e-8b38-df0598bef300

New Issues (4)

Checkmarx found the following issues in this Pull Request

Issue	Source File / Package	Checkmarx Insight
CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1170	details Method at line 1170 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... `ID: 4UDljLoh3qbhad4nJM8LQ9pL%2FGI%3D` Attack Vector
CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1060	details Method at line 1060 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... `ID: jnvq22%2FQYRjw84jOY5Le%2Fc416uw%3D` Attack Vector
CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1519	details Method at line 1519 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... `ID: YVmBT2WuUfyoEtbvwW331vijyJE%3D` Attack Vector
CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1395	details Method at line 1395 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... `ID: aoxG%2FPmW%2BQ2Rj1FXBPSshGt%2B0F0%3D` Attack Vector

Fixed Issues (75)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	~~Stored_XSS~~	/src/SharedWeb/Health/HealthCheckServiceExtensions.cs: 61
	~~Stored_XSS~~	/util/Server/Startup.cs: 57
	~~CSRF~~	/src/Api/Public/Controllers/CollectionsController.cs: 90
	~~CSRF~~	/src/Api/Public/Controllers/CollectionsController.cs: 90
	~~CSRF~~	/src/Api/Controllers/CollectionsController.cs: 176
	~~CSRF~~	/src/Api/Controllers/CollectionsController.cs: 176
	~~CSRF~~	/src/Api/Controllers/CollectionsController.cs: 176
	~~CSRF~~	/src/Api/Controllers/CollectionsController.cs: 176
	~~CSRF~~	/src/Api/Public/Controllers/CollectionsController.cs: 90
	~~CSRF~~	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 145
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 431
	~~CSRF~~	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 173
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 507
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 72
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/OrganizationBillingVNextController.cs: 108
	~~CSRF~~	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 270
	~~CSRF~~	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 217
	~~CSRF~~	/src/Api/Dirt/Controllers/OrganizationReportsController.cs: 173
	~~CSRF~~	/src/Api/AdminConsole/Controllers/GroupsController.cs: 289
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 270
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 375
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 366
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 366
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/OrganizationBillingVNextController.cs: 96
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/ProviderBillingVNextController.cs: 82
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/AccountBillingVNextController.cs: 60
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/OrganizationBillingVNextController.cs: 50
	~~CSRF~~	/src/Api/Billing/Controllers/VNext/ProviderBillingVNextController.cs: 40
	~~CSRF~~	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 145
	~~CSRF~~	/src/Api/KeyManagement/Controllers/AccountsKeyManagementController.cs: 105
	~~CSRF~~	/src/Api/Vault/Controllers/SecurityTaskController.cs: 66
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 700
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 364
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 211
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 620
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 186
	~~CSRF~~	/src/Api/Auth/Controllers/EmergencyAccessController.cs: 173
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 643
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 391
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 120
	~~CSRF~~	/src/Api/NotificationCenter/Controllers/NotificationsController.cs: 67
	~~CSRF~~	/src/Api/NotificationCenter/Controllers/NotificationsController.cs: 61
	~~CSRF~~	/src/Api/AdminConsole/Controllers/GroupsController.cs: 138
	~~CSRF~~	/src/Api/AdminConsole/Controllers/GroupsController.cs: 166
	~~CSRF~~	/src/Api/AdminConsole/Controllers/GroupsController.cs: 166
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 395
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 395
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 537
	~~CSRF~~	/src/Api/Controllers/CollectionsController.cs: 208
	~~CSRF~~	/src/Api/Vault/Controllers/CiphersController.cs: 717
	~~CSRF~~	/src/Api/Vault/Controllers/CiphersController.cs: 717
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 308
	~~CSRF~~	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 136
	~~CSRF~~	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 97
	~~CSRF~~	/bitwarden_license/src/Scim/Controllers/v2/GroupsController.cs: 87
	~~CSRF~~	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 208
	~~CSRF~~	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 183
	~~CSRF~~	/src/Api/AdminConsole/Controllers/ProviderUsersController.cs: 202
	~~CSRF~~	/src/Api/Vault/Controllers/CiphersController.cs: 300
	~~CSRF~~	/src/Api/Auth/Controllers/AccountsController.cs: 561
	~~CSRF~~	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 171
	~~CSRF~~	/src/Api/AdminConsole/Public/Controllers/MembersController.cs: 171
	~~CSRF~~	/src/Api/AdminConsole/Public/Controllers/GroupsController.cs: 164
	~~CSRF~~	/src/Api/AdminConsole/Controllers/OrganizationUsersController.cs: 308
	~~CSRF~~	/src/Api/Auth/Controllers/WebAuthnController.cs: 153
	~~SSL_Verification_Bypass~~	/src/Core/Platform/Mail/Delivery/MailKitSmtpMailDeliveryService.cs: 84
	~~Use_Of_Hardcoded_Password~~	/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs: 115
	~~Use_Of_Hardcoded_Password~~	/src/Core/Constants.cs: 207
	~~Use_Of_Hardcoded_Password~~	/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs: 62
	~~Use_Of_Hardcoded_Password~~	/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs: 58
	~~Use_Of_Hardcoded_Password~~	/src/Identity/IdentityServer/RequestValidators/SendAccess/SendAccessConstants.cs: 26
	~~Use_Of_Hardcoded_Password~~	/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs: 43
	~~Use_Of_Hardcoded_Password~~	/src/Core/KeyManagement/Sends/SendPasswordHasherServiceCollectionExtensions.cs: 14
	~~Use_Of_Hardcoded_Password~~	/src/Core/Constants.cs: 238
	~~Use_Of_Hardcoded_Password~~	/util/Seeder/Factories/UserSeeder.cs: 16

codecov · 2025-12-18T23:49:42Z

Codecov Report

❌ Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 54.70%. Comparing base (e6c97bd) to head (4289d94).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ents/OrganizationDataOwnershipPolicyRequirement.cs	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6759      +/-   ##
==========================================
- Coverage   54.71%   54.70%   -0.01%     
==========================================
  Files        1925     1926       +1     
  Lines       85671    85728      +57     
  Branches     7670     7678       +8     
==========================================
+ Hits        46876    46900      +24     
- Misses      37011    37044      +33     
  Partials     1784     1784

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

...ganizationFeatures/Policies/PolicyRequirements/OrganizationDataOwnershipPolicyRequirement.cs

src/Core/Vault/Services/Implementations/CipherService.cs

eliykat

Nicely done.

shane-melton added 2 commits December 18, 2025 14:59

[PM-28747] Bypass storage limit when enforce organization data owners…

a652f8f

…hip policy is enabled

[PM-28747] Unit tests for storage limit enforcement

b5dff08

shane-melton requested review from a team as code owners December 18, 2025 23:25

shane-melton requested review from eliykat and nikwithak December 18, 2025 23:25

shane-melton commented Dec 18, 2025

View reviewed changes

...ganizationFeatures/Policies/PolicyRequirements/OrganizationDataOwnershipPolicyRequirement.cs Outdated Show resolved Hide resolved

[PM-28747] Add feature flag check

408b08a

eliykat requested changes Dec 19, 2025

View reviewed changes

...ganizationFeatures/Policies/PolicyRequirements/OrganizationDataOwnershipPolicyRequirement.cs Outdated Show resolved Hide resolved

src/Core/Vault/Services/Implementations/CipherService.cs Outdated Show resolved Hide resolved

shane-melton added 2 commits December 22, 2025 10:23

[PM-28747] Simplify ignore storage limits policy enforcement

1f690c6

[PM-28747] Add additional test cases

4289d94

nikwithak approved these changes Dec 22, 2025

View reviewed changes

shane-melton requested a review from eliykat December 23, 2025 17:09

eliykat approved these changes Dec 26, 2025

View reviewed changes

shane-melton merged commit 3b5bb76 into main Dec 29, 2025
83 of 84 checks passed

shane-melton deleted the vault/pm-28747/storage-limit-bypass branch December 29, 2025 17:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-28747] Storage limit bypass for enforce organization ownership policy #6759

[PM-28747] Storage limit bypass for enforce organization ownership policy #6759

Uh oh!

shane-melton commented Dec 18, 2025 •

edited by atlassian bot

Loading

Uh oh!

claude bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

github-actions bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

codecov bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

eliykat left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

[PM-28747] Storage limit bypass for enforce organization ownership policy #6759

[PM-28747] Storage limit bypass for enforce organization ownership policy #6759

Uh oh!

Conversation

shane-melton commented Dec 18, 2025 • edited by atlassian bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

⏰ Reminders before review

🦮 Reviewer guidelines

Uh oh!

claude bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Complete

Critical Issues

Technical Debt

Missing Edge Cases

Questions

Positive Findings

Uh oh!

Uh oh!

github-actions bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

eliykat left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

shane-melton commented Dec 18, 2025 •

edited by atlassian bot

Loading

claude bot commented Dec 18, 2025 •

edited

Loading

github-actions bot commented Dec 18, 2025 •

edited

Loading

codecov bot commented Dec 18, 2025 •

edited

Loading