[PM-28616] Add flag UsePhishingBlocker to dbo.Organization #6625
Conversation
|
Claude finished @voommen-livefront's task —— View job Code Review Complete ✓I've completed a thorough review of this PR. The implementation follows the established patterns for adding organization flags and includes proper database migrations, model updates, and licensing support for self-hosted instances. Summary of ChangesThis PR adds the
Recent commits show good responsiveness to feedback:
Critical IssuesFinding 1: Missing The Location: src/Core/Billing/Organizations/Models/OrganizationLicense.cs:464 Issue: The
Required fix: // After line 427, add:
var usePhishingBlocker = claimsPrincipal.GetValue<bool>(nameof(UsePhishingBlocker));
// In the return statement (line 464), add:
useAutomaticUserConfirmation == organization.UseAutomaticUserConfirmation &&
usePhishingBlocker == organization.UsePhishingBlocker;Why this matters: Every other organization capability flag (UseAutomaticUserConfirmation, UseOrganizationDomains, UseAdminSponsoredFamilies, UseRiskInsights, etc.) is validated in this method. Omitting Minor IssuesFinding 2: Missing unit test coverage for While integration tests were updated, the Suggestion: Add a unit test in the Admin.Test project to verify the property is correctly mapped. Finding 3: Missing The constructor at OrganizationEditModel.cs:38-113 initializes from an Organization entity (line 110), but there's no corresponding constructor that accepts an Note: This is likely not required based on current architecture, but documenting for awareness. Good Practices Observed
Action Items
Testing NotesI cannot run the build or tests directly, but based on the codecov report:
Recommendation: Author should verify all tests pass locally after addressing Finding 1, particularly |
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6625 +/- ##
==========================================
+ Coverage 52.98% 56.96% +3.97%
==========================================
Files 1909 1902 -7
Lines 84828 84879 +51
Branches 7634 7633 -1
==========================================
+ Hits 44948 48348 +3400
+ Misses 38127 34705 -3422
- Partials 1753 1826 +73 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jrmccannon
left a comment
jrmccannon
left a comment
There was a problem hiding this comment.
@voommen-livefront
Usually when adding an organization feature, the feature needs to be added to the claims of the organization which will be included in the licensing file used by self-hosted instances.
If that needs to be considered for this flag, please review this guide that Admin Console has put together and also reach out to the billing team to validate the feature was added correctly.
Doing this will also break the unit tests and the doc has information on what to update to fix them.
Any questions let me know. Thanks!
| @@ -0,0 +1,378 @@ | |||
| /* Introduce new column 'UsePhishingBlocker' not nullable with default of 0 */ | |||
| ALTER TABLE [dbo].[Organization] ADD [UsePhishingBlocker] bit NOT NULL CONSTRAINT [DF_Organization_UsePhishingBlocker] default (0) | |||
There was a problem hiding this comment.
You'll need to protect this with a check to validate the table doesn't have the column already.
See an example here
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
mkincaid-bw
left a comment
mkincaid-bw
left a comment
There was a problem hiding this comment.
Minor change requested
src/Sql/dbo/Tables/Organization.sql
Outdated
| [SyncSeats] BIT NOT NULL CONSTRAINT [DF_Organization_SyncSeats] DEFAULT (0), | ||
| [UseAutomaticUserConfirmation] BIT NOT NULL CONSTRAINT [DF_Organization_UseAutomaticUserConfirmation] DEFAULT (0), | ||
| [MaxStorageGbIncreased] SMALLINT NULL, | ||
| [UsePhishingBlocker] BIT NOT NULL CONSTRAINT [DF_Organization_UsePhishingBlocker] DEFAULT (0), |
There was a problem hiding this comment.
⛏️ Data type BIT and NOT NULL clause is not vertically aligned (changing it isn't necessary but it's nice when things line up 😃).
There was a problem hiding this comment.
Formatting done.
|
|
||
|
|
||
| --Manually refresh [dbo].[OrganizationUserOrganizationDetailsView] | ||
| IF OBJECT_ID('[dbo].[OrganizationUserOrganizationDetailsView]') IS NOT NULL |
There was a problem hiding this comment.
Since you are altering these 3 views by adding the new column, there's no need to refresh the views afterwards since they will be updated/refreshed as part of the ALTER statement (it won't hurt though).
What is important is refreshing any views that reference any table that was changed that are not modified in the migration. There are 5 views total that reference the Organization table and you've modified three. The other two are:
EXEC sp_refreshview N'[dbo].[OrganizationCipherDetailsCollectionsView]';
EXEC sp_refreshview N'[dbo].[ProviderOrganizationOrganizationDetailsView]';You should also refresh any views that reference any of these 5 views. I checked and it appears that no other views reference these views, so we should be good there.
There was a problem hiding this comment.
Thank you for catching this.
There was a problem hiding this comment.
Added these two views to the scripts to be refreshed.
| SmServiceAccounts = org.SmServiceAccounts; | ||
| UseRiskInsights = org.UseRiskInsights; | ||
| UseOrganizationDomains = org.UseOrganizationDomains; | ||
| UsePhishingBlocker = org.UsePhishingBlocker; |
There was a problem hiding this comment.
The large block comment above this section states that this is deprecated and "Do not add new properties to this constructor or extend its functionality." I think maybe this bit should be removed?
Those instructions also mention needing to add your new property to the CanUse and VerifyData methods. I'm assuming you can skip CanUse but you may want to double check if you need code in VerifyData. I am not very familiar with this license code but feel free to reach out to the billing team in slack if you have any questions!
There was a problem hiding this comment.
You are right, I didn't need to add the property to the constructor.
Also, you are right, we don't need this property in VerifyData because this property does not limit or resitct the licensing in anyway. It's just a property of Organiation and if it is in a license JSON file, then it needs to be set in dbo.Organization.
I have removed the line from the constructor though.
… We have moved to claims based properties
|
Is this PR to address the incident from last week? If so, does the old code still need to be removed? |
jrmccannon
left a comment
jrmccannon
left a comment
There was a problem hiding this comment.
Looks good from AC perspective
voommen-livefront
dismissed
mkincaid-bw’s stale review
Other team members have reviewed this code
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-28616
📔 Objective
Added the
usePhishingBlockerflag to dbo.OrganizationUpdated EntityFramework & Migrations
Added a flag in the Admin pages for an organization to
Use Phishing BlockerUpdated the models and views here as well.
📸 Screenshots
UI for Admin Console
Database (MSSQL)
Phishing Blocker in API calls
⏰ 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