PM-20574 & PM-20575 Adding Risk Insight Report tables, repositories, and migrations #5839

prograhamming · 2025-05-20T03:57:57Z

🎟️ Tracking

PM-20574

PM-20575

📔 Objective

📸 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

… tools

…skinsights

codecov · 2025-05-20T04:03:42Z

Codecov Report

Attention: Patch coverage is 34.82143% with 73 lines in your changes missing coverage. Please review.

Project coverage is 50.69%. Comparing base (7b3e2a8) to head (3f9ffbb).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...r/Dirt/RiskInsightCriticalApplicationRepository.cs	0.00%	15 Missing ⚠️
...ructure.Dapper/Dirt/RiskInsightReportRepository.cs	0.00%	15 Missing ⚠️
...tories/RiskInsightCriticalApplicationRepository.cs	0.00%	11 Missing ⚠️
...k/Dirt/Repositories/RiskInsightReportRepository.cs	0.00%	11 Missing ⚠️
...rc/Core/Dirt/Reports/Entities/RiskInsightReport.cs	0.00%	9 Missing ⚠️
...Reports/Entities/RiskInsightCriticalApplication.cs	0.00%	8 Missing ⚠️
.../Dirt/PasswordHealthReportApplicationRepository.cs	0.00%	1 Missing ⚠️
...work/Dirt/Models/RiskInsightCriticalApplication.cs	83.33%	1 Missing ⚠️
...e.EntityFramework/Dirt/Models/RiskInsightReport.cs	83.33%	1 Missing ⚠️
...ories/PasswordHealthReportApplicationRepository.cs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5839      +/-   ##
==========================================
+ Coverage   47.58%   50.69%   +3.11%     
==========================================
  Files        1645     1660      +15     
  Lines       74658    74829     +171     
  Branches     6714     6716       +2     
==========================================
+ Hits        35523    37935    +2412     
+ Misses      37675    35376    -2299     
- Partials     1460     1518      +58

☔ 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.

github-actions · 2025-05-20T04:06:38Z

Checkmarx One – Scan Summary & Details – 75f0daef-5c91-472d-b2fb-14c7d491f743

Fixed Issues (2)

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

Severity	Issue	Source File / Package
	~~CSRF~~	/src/Api/Tools/Controllers/SendsController.cs: 196
	~~CSRF~~	/src/Api/Tools/Controllers/SendsController.cs: 205

withinfocus

Bit of a 💣 I realize but hopefully easy to refactor.

withinfocus · 2025-05-20T17:28:45Z

src/Sql/Dirt/dbo/Tables/RiskInsightReport.sql

@@ -0,0 +1,13 @@
+CREATE TABLE [dbo].[RiskInsightReport] (


⚠️ I said this in the beginning of this very large project so it may have been lost at this point, but "Risk Insights" is a product name and really shouldn't be in structured code. The original database design documents and suggestions explicitly kept it out too (and it changed names a couple times). These are generally-speaking "security task reports", "task reports", or just "reports".

withinfocus · 2025-05-20T17:29:37Z

src/Sql/Dirt/dbo/Tables/RiskInsightCriticalApplication.sql

@@ -0,0 +1,12 @@
+CREATE TABLE [dbo].[RiskInsightCriticalApplication] (


⚠️ Same thread as report naming.

voommen-livefront

Just minor changes - if you don't think we need an index with OrganizationId + Date - no worries, we can request them when we need it.
We are missing MySql Migrations - at this time, I only spotted empty migration files.

voommen-livefront · 2025-05-20T19:35:06Z

src/Sql/Dirt/dbo/Tables/RiskInsightCriticalApplication.sql

+    [CreationDate]             DATETIME2 (7)    NOT NULL,
+    [RevisionDate]             DATETIME2 (7)    NOT NULL,
+    CONSTRAINT [PK_RiskInsightCriticalApplication] PRIMARY KEY CLUSTERED ([Id] ASC),
+    CONSTRAINT [FK_RiskInsightCriticalApplication_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])


Suggested change

CONSTRAINT [FK_RiskInsightCriticalApplication_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])

CONSTRAINT [FK_RiskInsightCriticalApplication_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE

This is necessary because, there is an option to delete an Organization and, when an org is deleted, it should delete these records as well.

Guidance here is for direct children of Organization to use our deletion proc for SQL Server e.g. https://github.com/bitwarden/server/blob/main/src/Sql/dbo/Stored%20Procedures/Organization_DeleteById.sql#L87 -- this results in less computational pressure on the DB when an organization is actually deleted. Change that proc and make a new one that's called there that deletes by organization ID.

For EF however, ensure that the entity type configuration does indicate this; we let self-hosters utilize the cascaded delete as their environments will not be under the pressures ours has.

voommen-livefront · 2025-05-20T19:36:18Z

src/Sql/Dirt/dbo/Tables/RiskInsightReport.sql

+    [CreationDate]             DATETIME2 (7)    NOT NULL,
+    [RevisionDate]             DATETIME2 (7)    NOT NULL,
+    CONSTRAINT [PK_RiskInsightReport] PRIMARY KEY CLUSTERED ([Id] ASC),
+    CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])


Suggested change

CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])

CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE

This is needed because when an organization is deleted, it must delete records in this table as well.

voommen-livefront · 2025-05-20T19:37:59Z

util/Migrator/DbScripts/2025-05-16-00_RiskInsightReport.sql

+    [CreationDate]             DATETIME2 (7)    NOT NULL,
+    [RevisionDate]             DATETIME2 (7)    NOT NULL,
+    CONSTRAINT [PK_RiskInsightReport] PRIMARY KEY CLUSTERED ([Id] ASC),
+    CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])


Suggested change

CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])

CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE

just repeating what was mentioned earlier

voommen-livefront · 2025-05-20T19:41:55Z

util/MySqlMigrations/Migrations/20250520034740_2025-05-16-00_RiskInsightReport.sql.cs

We may not need this file

voommen-livefront · 2025-05-20T19:42:20Z

...ySqlMigrations/Migrations/20250520034928_2025-05-16-01_RiskInsightCriticalApplication.sql.cs

We may not need this migration as it is empty.

voommen-livefront · 2025-05-20T19:45:46Z

...gresMigrations/Migrations/20250520034935_2025-05-16-01_RiskInsightCriticalApplication.sql.cs

Empty migration, we can safely remove this file

voommen-livefront · 2025-05-20T19:46:41Z

...liteMigrations/Migrations/20250520034931_2025-05-16-01_RiskInsightCriticalApplication.sql.cs

Empty migration, can be safely removed.

prograhamming added 4 commits May 16, 2025 10:50

PM-20574 fixing namespaces on reporting work that got moved over from…

e6da387

… tools

PM-20574 adding tables, stored procedures, and migration files

1b6adf4

PM-20574 adding dapper and ef repos and migrations

9d5835f

Merge branch 'main' into dirt/pm-20574/database_tables_and_scripts_ri…

7d9d14c

…skinsights

prograhamming requested review from a team as code owners May 20, 2025 03:57

PM-20574 merging in main

3f9ffbb

withinfocus requested changes May 20, 2025

View reviewed changes

voommen-livefront requested changes May 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PM-20574 & PM-20575 Adding Risk Insight Report tables, repositories, and migrations #5839

PM-20574 & PM-20575 Adding Risk Insight Report tables, repositories, and migrations #5839

prograhamming commented May 20, 2025 •

edited by jira bot

Loading

codecov bot commented May 20, 2025 •

edited

Loading

github-actions bot commented May 20, 2025 •

edited

Loading

withinfocus left a comment

withinfocus May 20, 2025

withinfocus May 20, 2025

voommen-livefront left a comment

voommen-livefront May 20, 2025

withinfocus May 20, 2025

voommen-livefront May 20, 2025

voommen-livefront May 20, 2025

voommen-livefront May 20, 2025

voommen-livefront May 20, 2025

voommen-livefront May 20, 2025

voommen-livefront May 20, 2025

		@@ -0,0 +1,12 @@
		CREATE TABLE [dbo].[RiskInsightCriticalApplication] (

	CONSTRAINT [FK_RiskInsightCriticalApplication_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])
	CONSTRAINT [FK_RiskInsightCriticalApplication_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE

	CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id])
	CONSTRAINT [FK_RiskInsightReport_Organization] FOREIGN KEY ([OrganizationId]) REFERENCES [dbo].[Organization] ([Id]) ON DELETE CASCADE

PM-20574 & PM-20575 Adding Risk Insight Report tables, repositories, and migrations #5839

Are you sure you want to change the base?

PM-20574 & PM-20575 Adding Risk Insight Report tables, repositories, and migrations #5839

Conversation

prograhamming commented May 20, 2025 • edited by jira bot Loading

🎟️ Tracking

📔 Objective

📸 Screenshots

⏰ Reminders before review

🦮 Reviewer guidelines

codecov bot commented May 20, 2025 • edited Loading

Codecov Report

github-actions bot commented May 20, 2025 • edited Loading

withinfocus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voommen-livefront left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prograhamming commented May 20, 2025 •

edited by jira bot

Loading

codecov bot commented May 20, 2025 •

edited

Loading

github-actions bot commented May 20, 2025 •

edited

Loading