[PM-13446] Add 'IsMultiOrgEnterprise' column to 'Provider' table #4903

jonashendrickx · 2024-10-16T14:20:27Z

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-13446

📔 Objective

Add an IsMultiOrgEnterprise column to the Provider table that will be used to differentiate between standard MSP’s and those who are multi-organization enterprises.

📸 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 · 2024-10-16T14:29:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.74%. Comparing base (7a509d2) to head (311ec0a).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4903   +/-   ##
=======================================
  Coverage   41.74%   41.74%           
=======================================
  Files        1362     1362           
  Lines       63908    63909    +1     
  Branches     5857     5857           
=======================================
+ Hits        26678    26679    +1     
  Misses      36024    36024           
  Partials     1206     1206

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

amorask-bitwarden

Just a few things with your migration files.

amorask-bitwarden · 2024-10-16T14:25:25Z

util/Migrator/DbScripts/2024-10-16_00__AddIsMultiOrgEnterpriseColumn.sql

@@ -0,0 +1,175 @@
+-- Table
+IF OBJECT_ID('[dbo].[ClientOrganizationMigrationRecord]') IS NULL


❌ It looks like you might have forgot to update the SQL Server migration script. This is just creating an unrelated table that already exists. Yours will look more like this, but just with the new IsMultiOrgEnterprise column.

amorask-bitwarden · 2024-10-16T14:30:27Z

util/PostgresMigrations/Migrations/20241016140839_AddIsMultiOrgEnterpriseColumn.cs

+namespace Bit.PostgresMigrations.Migrations;
+
+/// <inheritdoc />
+public partial class AddIsMultiOrgEnterpriseColumn : Migration


❌ Looks like something got messed up with your EF Migration here. It appears to be rebuilding the entire database in this file. Check out your MySql migration file. That's what we want.

amorask-bitwarden · 2024-10-16T14:31:03Z

util/SqliteMigrations/Migrations/20241016140849_AddIsMultiOrgEnterpriseColumn.cs

+namespace Bit.SqliteMigrations.Migrations;
+
+/// <inheritdoc />
+public partial class AddIsMultiOrgEnterpriseColumn : Migration


❌ Same thing here as with Postgres.

github-actions · 2024-10-16T15:11:42Z

Checkmarx One – Scan Summary & Details – 695a78b1-dfa7-45a3-8a30-b40c27d23c2e

New Issues

Issue	Source File / Package	Checkmarx Insight
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Missing User Instruction	/Dockerfile: 1	A user should be specified in the dockerfile, otherwise the image will run as root
Passwords And Secrets - Generic Password	/test-database.yml: 185	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 80	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 182	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 189	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 69	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 92	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 111	Query to find passwords and secrets in infrastructure code.
Passwords And Secrets - Generic Password	/test-database.yml: 105	Query to find passwords and secrets in infrastructure code.
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile-k8s: 8	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile-k8s: 8	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 1	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 1	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 7	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 7	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 5	When installing a package, its pin version should be defined
Apt Get Install Pin Version Not Defined	/Dockerfile: 1	When installing a package, its pin version should be defined
Image Version Using 'latest'	/Dockerfile: 1	When building images, always tag them with useful tags which codify version information, intended destination (prod or test, for instance), stabili...
Unpinned Actions Full Length Commit SHA	/_move_finalization_db_scripts.yml: 103	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 111	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/cleanup-rc-branch.yml: 21	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 119	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/release.yml: 44	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 548	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 582	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 104	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 631	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 207	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 60	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/_move_finalization_db_scripts.yml: 27	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 515	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/release.yml: 63	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Healthcheck Instruction Missing	/Dockerfile: 1	Ensure that HEALTHCHECK is being used. The HEALTHCHECK instruction tells Docker how to test a container to check that it is still working
Healthcheck Instruction Missing	/Dockerfile: 1	Ensure that HEALTHCHECK is being used. The HEALTHCHECK instruction tells Docker how to test a container to check that it is still working
Healthcheck Instruction Missing	/Dockerfile: 1	Ensure that HEALTHCHECK is being used. The HEALTHCHECK instruction tells Docker how to test a container to check that it is still working
Multiple RUN, ADD, COPY, Instructions Listed	/Dockerfile-k8s: 17	Multiple commands (RUN, COPY, ADD) should be grouped in order to reduce the number of layers.
Multiple RUN, ADD, COPY, Instructions Listed	/Dockerfile: 13	Multiple commands (RUN, COPY, ADD) should be grouped in order to reduce the number of layers.
Multiple RUN, ADD, COPY, Instructions Listed	/Dockerfile: 11	Multiple commands (RUN, COPY, ADD) should be grouped in order to reduce the number of layers.
Unpinned Actions Full Length Commit SHA	/build.yml: 548	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 582	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 60	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 119	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 104	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/repository-management.yml: 111	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 631	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...
Unpinned Actions Full Length Commit SHA	/build.yml: 515	Pinning an action to a full length commit SHA is currently the only way to use an action as an immutable release. Pinning to a particular SHA helps...

Fixed Issues

Severity	Issue	Source File / Package
	CSRF	/src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 351
	CSRF	/src/Admin/AdminConsole/Controllers/OrganizationsController.cs: 351

eliykat

This seems like a perfect case for adding a new ProviderType enum. Is there a reason we can't use that, or a particular need for adding a new column instead?

amorask-bitwarden · 2024-10-17T00:32:22Z

This seems like a perfect case for adding a new ProviderType enum. Is there a reason we can't use that, or a particular need for adding a new column instead?

Hey @eliykat, we primarily decided against this because Multi-Enterprise Organizations are a subset of MSPs. Their functionality will be almost exactly the same as MSPs, but they will have different plan options during setup in the Bitwarden Portal. As such, it was my estimation that they don't actually represent a new provider type functionality-wise, but just an MSP that needs an indicator for a marginal functionality difference. This also allows us to re-use all of the existing Consolidated Billing functionality without having to add a new Enum check throughout the platform.

eliykat · 2024-10-17T02:19:34Z

I think I have two main concerns about this:

MSP has a semantic meaning in this domain: it's a managed service provider business. That is just a different thing than a single enterprise with multiple Bitwarden organizations. Calling a multi-org provider something it's not (an MSP) and then relying on a different property to define what it actually is, is confusing and a likely source of tech debt and bugs in the future. (See also - the Provider Information page in the Bitwarden Portal describes it as a type - I think the code should reflect the domain more closely.)
This design allows invalid combinations: you could have a Reseller provider type with IsMultiOrgEnterprise enabled, which (from what I can tell) would be an invalid combination. If another provider type is added in the future, is it a new ProviderType enum or another bool column? Either allows additional invalid combinations which shouldn't be possible just as a matter of data / schema design. This is exactly why we added ProviderType previously.

I admit I am not familiar with the consolidated billing code, but being able to have a simple enum check seems better to me than enum && bool, or an implicit assumption that "MSP includes multi-org enterprises unless stated otherwise".

…nterprise-column-to-Provider-table

amorask-bitwarden · 2024-10-17T09:42:01Z

I think I have two main concerns about this:

MSP has a semantic meaning in this domain: it's a managed service provider business. That is just a different thing than a single enterprise with multiple Bitwarden organizations. Calling a multi-org provider something it's not (an MSP) and then relying on a different property to define what it actually is, is confusing and a likely source of tech debt and bugs in the future. (See also - the Provider Information page in the Bitwarden Portal describes it as a type - I think the code should reflect the domain more closely.)

This design allows invalid combinations: you could have a Reseller provider type with IsMultiOrgEnterprise enabled, which (from what I can tell) would be an invalid combination. If another provider type is added in the future, is it a new ProviderType enum or another bool column? Either allows additional invalid combinations which shouldn't be possible just as a matter of data / schema design. This is exactly why we added ProviderType previously.

I admit I am not familiar with the consolidated billing code, but being able to have a simple enum check seems better to me than enum && bool, or an implicit assumption that "MSP includes multi-org enterprises unless stated otherwise".

Understood, that seems valid and reasonable. We'll adjust the approach here accordingly. @jonashendrickx hold off on additional work here as we'll close this ticket and re-work the next one as the starting point.

[PM-13446] Add 'IsMultiOrgEnterprise' column to 'Provider' table

0e1fef1

jonashendrickx requested review from a team as code owners October 16, 2024 14:20

jonashendrickx requested a review from eliykat October 16, 2024 14:20

amorask-bitwarden requested changes Oct 16, 2024

View reviewed changes

eliykat reviewed Oct 17, 2024

View reviewed changes

jonashendrickx added 5 commits October 17, 2024 09:31

Correct SQL Server migration script

ffe8b3f

Merge branch 'refs/heads/main' into PM-13446-Database-Add-IsMultiOrgE…

9e21c79

…nterprise-column-to-Provider-table

revert

2837e0b

finally good

f7d2ce2

fix

311ec0a

amorask-bitwarden closed this Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PM-13446] Add 'IsMultiOrgEnterprise' column to 'Provider' table #4903

[PM-13446] Add 'IsMultiOrgEnterprise' column to 'Provider' table #4903

jonashendrickx commented Oct 16, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

amorask-bitwarden left a comment

amorask-bitwarden Oct 16, 2024

amorask-bitwarden Oct 16, 2024

amorask-bitwarden Oct 16, 2024

github-actions bot commented Oct 16, 2024 •

edited

Loading

eliykat left a comment

amorask-bitwarden commented Oct 17, 2024

eliykat commented Oct 17, 2024 •

edited

Loading

amorask-bitwarden commented Oct 17, 2024

		@@ -0,0 +1,175 @@
		-- Table
		IF OBJECT_ID('[dbo].[ClientOrganizationMigrationRecord]') IS NULL

[PM-13446] Add 'IsMultiOrgEnterprise' column to 'Provider' table #4903

[PM-13446] Add 'IsMultiOrgEnterprise' column to 'Provider' table #4903

Conversation

jonashendrickx commented Oct 16, 2024

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented Oct 16, 2024 • edited Loading

Codecov Report

amorask-bitwarden left a comment

Choose a reason for hiding this comment

amorask-bitwarden Oct 16, 2024

Choose a reason for hiding this comment

amorask-bitwarden Oct 16, 2024

Choose a reason for hiding this comment

amorask-bitwarden Oct 16, 2024

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2024 • edited Loading

New Issues

Fixed Issues

eliykat left a comment

Choose a reason for hiding this comment

amorask-bitwarden commented Oct 17, 2024

eliykat commented Oct 17, 2024 • edited Loading

amorask-bitwarden commented Oct 17, 2024

codecov bot commented Oct 16, 2024 •

edited

Loading

github-actions bot commented Oct 16, 2024 •

edited

Loading

eliykat commented Oct 17, 2024 •

edited

Loading