- Notifications
You must be signed in to change notification settings - Fork1.5k
[PM-28616] Add flag UsePhishingBlocker to dbo.Organization#6625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
claudebot commentedNov 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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:varusePhishingBlocker=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 |
github-actionsbot commentedNov 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Great job! No new security vulnerabilities introduced in this pull request |
codecovbot commentedNov 21, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@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 thisguide 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*/ | |||
| ALTERTABLE [dbo].[Organization]ADD [UsePhishingBlocker]bitNOTNULLCONSTRAINT [DF_Organization_UsePhishingBlocker]default (0) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You'll need to protect this with a check to validate the table doesn't have the column already.
See an examplehere
Uh oh!
There was an error while loading.Please reload this page.
mkincaid-bw left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor change requested
src/Sql/dbo/Tables/Organization.sql Outdated
| [SyncSeats]BITNOTNULLCONSTRAINT [DF_Organization_SyncSeats]DEFAULT (0), | ||
| [UseAutomaticUserConfirmation]BITNOTNULLCONSTRAINT [DF_Organization_UseAutomaticUserConfirmation]DEFAULT (0), | ||
| [MaxStorageGbIncreased]SMALLINTNULL, | ||
| [UsePhishingBlocker]BITNOTNULLCONSTRAINT [DF_Organization_UsePhishingBlocker]DEFAULT (0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
⛏️ Data typeBIT andNOT 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Formatting done.
| --Manually refresh [dbo].[OrganizationUserOrganizationDetailsView] | ||
| IFOBJECT_ID('[dbo].[OrganizationUserOrganizationDetailsView]')ISNOTNULL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 theOrganization 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for catching this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Added these two views to the scripts to be refreshed.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| SmServiceAccounts=org.SmServiceAccounts; | ||
| UseRiskInsights=org.UseRiskInsights; | ||
| UseOrganizationDomains=org.UseOrganizationDomains; | ||
| UsePhishingBlocker=org.UsePhishingBlocker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 theCanUse andVerifyData methods. I'm assuming you can skipCanUse but you may want to double check if you need code inVerifyData. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You are right, I didn't need to add the property to the constructor.
Also, you are right, we don't need this property inVerifyData 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 indbo.Organization.
I have removed the line from the constructor though.
… We have moved to claims based properties
mkincaid-bw commentedNov 25, 2025
Is this PR to address the incident from last week? If so, does theold code still need to be removed? |
Banrion commentedNov 25, 2025 • edited by atlassianbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by atlassianbot
Uh oh!
There was an error while loading.Please reload this page.
jrmccannon left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks good from AC perspective
rkac-bw left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
lgtm
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