[PM-26376] Emergency Access Delete Command

[ike-kottlowski]

🎟️ Tracking

PM-26376

📔 Objective

On our path to accomplishing the CQRS ADR we are taking a pit stop at the Emergency Access feature. This work is to enable the Admin Console team to enforce an Organization policy that will un-enroll users from the emergency access feature.

I have added a simple DeleteManyById stored procedure that batches, if need be, the records for deletion.

Things to note:

1. When removing by GranteeId we loop over the grantor emails sending one at a time, so we don't accidently cc all the grantors for a given grantee.
2. I enabled nullable for classes that felt like low hanging fruit, which is why other teams are tagged.

⏰ 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

[claude]

Claude finished @ike-kottlowski's task in 2m 48s —— View job

---

Code Review: PM-26376 Emergency Access Delete Command

Reviewing PR #6857 - Re-review following latest commit (d603a87)

Summary

This PR implements a DeleteEmergencyAccessCommand following the CQRS pattern to support bulk deletion of emergency access records. This enables the Admin Console team to enforce organization policies that un-enroll users from the emergency access feature.

Previous Findings Status

Finding	Status
Empty recipient when no records exist	Resolved (early returns at lines 37-40, 56-59)
Email privacy exposure (multiple grantors see each other's emails)	Resolved (individual emails per grantor, lines 93-105)
Missing test coverage for DeleteAllByGranteeIdAsync	Resolved (tests at lines 166-251)

Latest Commit Review (d603a87)

The latest commit removes the explicit BEGIN TRANSACTION/COMMIT TRANSACTION wrapper from the stored procedure EmergencyAccess_DeleteManyById. This is appropriate because:

• Each DELETE TOP(@BatchSize) statement is atomic by itself
• The batching pattern exists to avoid lock escalation, not for transactional integrity
• Removing the transaction wrapper reduces overhead and potential deadlock surface

Implementation Quality

Security

• Properly validates ownership before deletion (line 19-22)
• No sensitive data exposed in logs or error messages
• Uses parameterized stored procedure calls

Correctness

• Early returns prevent email operations when no records exist
• Individual emails sent to each grantor to preserve privacy
• HashSet used to deduplicate email addresses

Test Coverage

• Unit tests cover all three public methods
• Edge cases tested (no records, single record, multiple records)
• Integration test validates database deletion behavior

Minor Observations (Non-blocking)

1. Line 45: Double semicolon (;;) - cosmetic only, no functional impact

---

Files Reviewed: 21 changed files across command, interface, repository, mail templates, SQL, and tests

Status: All previous issues resolved. PR is ready for merge.

---

Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 90.32258% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.02%. Comparing base (93e2c97) to head (f609c2c).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
src/Core/Auth/Models/Data/EmergencyAccessNotify.cs	0.00%	3 Missing ⚠️
...ncyAccess/Commands/DeleteEmergencyAccessCommand.cs	94.54%	0 Missing and 3 partials ⚠️
src/Core/Platform/Mail/HandlebarsMailService.cs	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6857      +/-   ##
==========================================
+ Coverage   56.05%   60.02%   +3.96%     
==========================================
  Files        1967     1969       +2     
  Lines       86901    87007     +106     
  Branches     7737     7749      +12     
==========================================
+ Hits        48714    52222    +3508     
+ Misses      36386    32899    -3487     
- Partials     1801     1886      +85     

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

Checkmarx One – Scan Summary & Details – 0a44d132-16cc-46d5-9773-a445505d4188

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

[mkincaid-bw]

❓ Does the account revision date need to be bumped after the deletes? Some of our other "DeleteMany" procs do a revision bump and some don't..