server icon indicating copy to clipboard operation
server copied to clipboard

PM-23358 removing phishing blocker code

Open prograhamming opened this issue 1 month ago â€ĸ 4 comments

đŸŽŸī¸ Tracking

PM-23358

📔 Objective

Phishing Blocker feature is now is frontend only and the BEEP server side code needs to be removed.

📸 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

prograhamming avatar Dec 02 '25 15:12 prograhamming

Claude finished @prograhamming's task in 1m 30s —— View job


Review Complete

The code review has been posted with detailed findings.

Summary: The PR successfully removes server-side BEEP implementation code (controllers, jobs, Azure services), but database schema and entity model cleanup remains incomplete.

Key findings:

  • Database UsePhishingBlocker field still present in Organization table and related stored procedures/views
  • Entity properties still exist across 43+ files
  • Requires rollback migration and entity model cleanup

claude[bot] avatar Dec 02 '25 15:12 claude[bot]

Logo Checkmarx One – Scan Summary & Details – bc61c948-bf42-4bd1-9936-ccd0dcb9257c

Great job! No new security vulnerabilities introduced in this pull request

github-actions[bot] avatar Dec 02 '25 15:12 github-actions[bot]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 54.38%. Comparing base (acfe0d7) to head (b530b73). :warning: Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6668      +/-   ##
==========================================
+ Coverage   54.19%   54.38%   +0.19%     
==========================================
  Files        1922     1916       -6     
  Lines       85095    84769     -326     
  Branches     7610     7579      -31     
==========================================
- Hits        46114    46105       -9     
+ Misses      37221    36903     -318     
- Partials     1760     1761       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Dec 02 '25 15:12 codecov[bot]

Deletions look good to me. Do we need any additional migrations to remove tables or cleanup any jobs?

Yeah good callout. It looks like from the old code they added a JSON blob to a Azure Storage service and nothing to the relational databases we support. The only migration script related to Phishing Blocker I can find is Vijay's migrations (completed on 11/20/2025) adding theUsePhishingBlocker field to the Organization table and view related to that table.

prograhamming avatar Dec 02 '25 16:12 prograhamming

Overall Assessment: REQUEST CHANGES

Critical Issues:

Finding 1: UsePhishingBlocker database field still present (src/Sql/dbo/Tables/Organization.sql:64) Finding 2: UsePhishingBlocker property retained in Organization entity (src/Core/AdminConsole/Entities/Organization.cs:140) Finding 3: Licensing code still references UsePhishingBlocker


Finding 1: Database Field Cleanup Required

âš ī¸ IMPORTANT: The UsePhishingBlocker field was added to the Organization table in migration 2025-11-21_00_AddUsePhishingBlockerToOrganization.sql. Since phishing blocker is now frontend-only, this database field should be removed.

Details and fix

Files requiring cleanup:

  1. SQL Schema: src/Sql/dbo/Tables/Organization.sql (line 64)
  2. Stored Procedures:
    • Organization_Create
    • Organization_Update
    • Organization_ReadAbilities
  3. Views:
    • OrganizationView.sql
    • OrganizationUserOrganizationDetailsView.sql
    • ProviderUserProviderOrganizationDetailsView.sql
  4. EF Migrations: All three database providers (MySQL, PostgreSQL, SQLite)
  5. Admin Portal: src/Admin/AdminConsole/Views/Shared/_OrganizationForm.cshtml (lines 160-161)

Required actions:

  1. Create a rollback migration script (e.g., 2025-12-XX_RemoveUsePhishingBlockerFromOrganization.sql)
  2. Drop the column: ALTER TABLE [dbo].[Organization] DROP CONSTRAINT [DF_Organization_UsePhishingBlocker]; ALTER TABLE [dbo].[Organization] DROP COLUMN [UsePhishingBlocker];
  3. Update all stored procedures to remove the @UsePhishingBlocker parameter
  4. Update all views to remove the column from SELECT statements
  5. Create corresponding Entity Framework migrations for rollback
  6. Remove the checkbox from the admin portal form

The field defaults to 0 (false), so removal should not impact existing organizations.


Finding 2: Organization Entity Properties Cleanup

âš ī¸ IMPORTANT: The UsePhishingBlocker property remains in the Organization entity and related models.

Details and fix

Locations still referencing UsePhishingBlocker:

  1. src/Core/AdminConsole/Entities/Organization.cs (line 140) - Entity property
  2. src/Core/Billing/Organizations/Models/OrganizationLicense.cs (line 146) - License model
  3. src/Core/Billing/Licenses/Services/Implementations/OrganizationLicenseClaimsFactory.cs (line 60) - Claims factory
  4. src/Core/Billing/Licenses/LicenseConstants.cs (line 47) - Constants
  5. Multiple response models, DTOs, and views across AdminConsole

Recommended approach:

Since phishing blocker is now frontend-only with no server-side functionality, remove the property entirely:

  1. Remove from Organization entity
  2. Remove from OrganizationLicense model
  3. Remove from license claims factory
  4. Remove from license constants
  5. Update Organization.UpdateFromLicense() method (line 342) to remove UsePhishingBlocker assignment
  6. Update all repository implementations, views, and queries
  7. Update test fixtures that set this property

This ensures clean separation between server and client responsibilities.


Finding 3: Feature Flag Cleanup

â™ģī¸ DEBT: The PhishingDetection feature flag constant is still defined in src/Core/Constants.cs (line 246).

Details and cleanup needed
public const string PhishingDetection = "phishing-detection";

Action required:

Before removing, verify:

  1. No client-side code (web vault, browser extensions) references this flag server-side
  2. If clients use it for frontend-only functionality, update documentation to clarify it's client-managed
  3. If completely unused, remove the constant definition

This prevents technical debt from unused constants accumulating in the codebase.


Review Summary

The PR successfully removes the server-side BEEP implementation (API controller, jobs, Azure storage services, HTTP clients, and configuration). However, the database schema and entity model still contain the UsePhishingBlocker field, which creates inconsistency.

Since the feature is now frontend-only, the server should not maintain any references to phishing blocker configuration. A complete cleanup requires:

  1. Database migration to drop the column
  2. Remove properties from entity models
  3. Remove from licensing system
  4. Verify feature flag usage

Completeness: Code deletions are thorough ✅
Database cleanup: Incomplete - UsePhishingBlocker field remains ❌
Entity model cleanup: Incomplete - Properties still present ❌
Configuration cleanup: Complete ✅
Job cleanup: Complete ✅

Please address the database and entity model cleanup to complete the server-side removal.

claude[bot] avatar Dec 15 '25 17:12 claude[bot]