Split LimitCollectionCreationDeletion into two database columns

[addisonbeck]

🎟️ Tracking

🧵 Jira Ticket: PM-10863

📚 Stacked PRs

1. server: Split LimitCollectionCreationDeletion into two database columns #4709
   ⬆️ YOU ARE HERE
2. server: Split Organization.LimitCollectionCreationDeletion into two separate business rules #4730
3. clients: Split Organization.LimitCollectionCreationDeletion into two separate business rules clients#11223
4. server: Turn on LimitCollectionCreationDeletionSplit for self host #4808
5. server: [PM-14821] [PM-14822] Remove LimitCollectionCreationDeletionSplit feature flag #4809
6. clients: [PM-14821] Remove LimitCollectionCreationDeletionSplit feature flag clients#11258
7. server: Drop LimitCollectionCreationDeletion from the database #4810

📔 Objective

🤳 The Bigger Picture

There is an option in the Admin Console that removes collection creation and
deletion controls from all organization members that are not in the Owner and
Admin roles. This PR stack splits this conjoined option into two separate
controls: one to block create operations and one to block delete
operations. This is a part of an effort to make these settings better align
with what customers expect it to do.

👉 This Pull Request 👍

This pull request submits an initial migration & a transition migration for
introducing a dbo.Organization.LimitCollectionCreation column and a
dbo.Organization.LimitCollectionDeletion column that will split
dbo.Organization.LimitCollectionCreationDeletion into two individual data
points.

I've include the data-syncing UPDATE script that copies the existing
column's data over to the new column as a separate script from the initial
migration. This UPDATE script seems like it might fit the criteria of a
"transition" phase migration in EDD. I think it would be fine to run with the
initial migration, but I am unsure if (quoting the contributing docs)

[the script would] be too slow or put too much load on the database, or
otherwise make it unsuitable for the Initial migration

It seems like it wouldn't be too much to me, but let me know!

The same updates have been made to the entity framework database
implementations. The entity fields are synced by a hand written migration,
and are get and set to pull from LimitCollectionCreationDeletion until
the next phase of this work that will fade that field out of use in the
server project.

🧮 Side Effects

• I deleted a test class that was meant to be ephemeral and was blocking new
  EF migrations from being properly handled by tests.
• I changed the default value of LimitCollectionCreationDeletion from
  true to false in Entity Framework. This is already the default in
  MSSQL, and I think it was missed in EF.

📸 Screenshots

⏰ 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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.81%. Comparing base (7368e57) to head (af50aac).
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4709   +/-   ##
=======================================
  Coverage   41.80%   41.81%           
=======================================
  Files        1320     1320           
  Lines       62687    62689    +2     
  Branches     5774     5774           
=======================================
+ Hits        26209    26211    +2     
  Misses      35267    35267           
  Partials     1211     1211           

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

[github-actions]

Checkmarx One – Scan Summary & Details – a414d158-691e-4598-b08d-20951f835b63

New Issues

Severity	Issue	Source File / Package	Checkmarx Insight
	Missing_HSTS_Header	/src/Admin/Startup.cs: 40	Attack Vector
	Privacy_Violation	/src/Api/Auth/Controllers/TwoFactorController.cs: 488	Attack Vector
	Privacy_Violation	/src/Api/Auth/Controllers/WebAuthnController.cs: 178	Attack Vector
	Privacy_Violation	/src/Api/Controllers/DevicesController.cs: 132	Attack Vector
	Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 863	Attack Vector
	Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 560	Attack Vector
	Privacy_Violation	/src/Api/Vault/Controllers/CiphersController.cs: 906	Attack Vector
	Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 845	Attack Vector
	Privacy_Violation	/src/Api/Auth/Controllers/AccountsController.cs: 412	Attack Vector
	Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 274	Attack Vector
	Privacy_Violation	/src/Api/Controllers/DevicesController.cs: 158	Attack Vector
	Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 365	Attack Vector
	Privacy_Violation	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 418	Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/WebAuthnController.cs: 68	Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/WebAuthnController.cs: 153	Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/WebAuthnController.cs: 85	Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 404	Attack Vector
	Log_Forging	/src/Api/Vault/Controllers/CiphersController.cs: 898	Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 855	Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 837	Attack Vector
	Log_Forging	/src/Api/Auth/Controllers/AccountsController.cs: 552	Attack Vector
	Log_Forging	/src/Api/Controllers/DevicesController.cs: 123	Attack Vector
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 254	Attack Vector
	Log_Forging	/src/Api/Controllers/DevicesController.cs: 149	Attack Vector
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 330	Attack Vector
	Log_Forging	/src/Api/AdminConsole/Controllers/OrganizationsController.cs: 393	Attack Vector

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Billing/Controllers/StripeController.cs: 176
	CSRF	/src/Billing/Controllers/StripeController.cs: 164
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 455
	CSRF	/src/Api/Auth/Controllers/AccountsController.cs: 689
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164
	Log_Forging	/src/Billing/Controllers/StripeController.cs: 164

[addisonbeck]

@r-tome @eliykat @vincentsalucci This isn't ready for merge yet. I want to get a bit further and manually test. That said: if anyone finds time for a review I'd appreciate it. I haven't done a formally processed EDD migration yet, and I do not want to miss anything. Thanks!

[eliykat]

You need to refresh OrganizationView after adding the column; but this otherwise looks right to me, including EDD considerations.

To confirm, I expect the timeline would be:

1. Merge this PR
   -- rc cut --
2. Update the code to use the new column
   -- rc cut --
3. Drop the old column

[addisonbeck]

Thanks! I added a refresh to OrganizationView on 725dbfd.

[r-tome]

LGTM!

[eliykat]

This looks good to me! Thanks for taking the time to navigate EDD issues.

[addisonbeck]

I rebased this on main to update migration script dates and keep everything in sync for branches further down the tree than this one like #4730

No functional changes were made.

[addisonbeck]

I'd also like to ignore the failing SonarCloud action run on this PR. It seems mostly upset about tests, but this is entirely database operations.

[withinfocus]

I opened up the Sonar report finding, and there's one thing listed:

https://sonarcloud.io/project/issues?severities=BLOCKER&sinceLeakPeriod=true&issueStatuses=OPEN%2CCONFIRMED&types=BUG&id=bitwarden_server&open=AZH6gLSYkzQSuElBzVms

It suggests a WHERE with your update.

[addisonbeck]

Thank you @withinfocus, that's helpful and did give me an idea. I do want to update every record on the table, so a WHERE clause isn't wrong logically. But, it cuts down the amount of records accessed to only check for rows that don't match the default value of the column being updated. That sounds like a good enough reason to add one.

@eliykat this does mark a small functional change since review. When you get to it: please give a review to the very small change on c37f77f.

[eliykat]

LGTM, also note my recent database changes in #4701 just in case you need to re-do any of your EF migrations. (You may have already done this)

[rkac-bw]

lgtm