[PM-26376] Emergency Access Delete Command #6857
Conversation
…UserFeatures space.
…single responsibility.
|
Claude finished @ike-kottlowski's task in 2m 55s —— View job Code Review: ApprovedPR: PM-26376 Emergency Access Delete Command This PR implements a delete command for Emergency Access records following the CQRS pattern, enabling organization policy enforcement for un-enrolling users from the emergency access feature. SummaryThe implementation is well-structured with appropriate separation of concerns:
Changes Reviewed
Previously Raised Issues - All Addressed
Code Quality Highlights
No new issues identified. LGTM! Automated code review by Claude |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
src/Core/Auth/UserFeatures/EmergencyAccess/Commands/DeleteEmergencyAccessCommand.cs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6857 +/- ##
==========================================
+ Coverage 56.08% 60.06% +3.97%
==========================================
Files 1971 1972 +1
Lines 87090 87174 +84
Branches 7759 7766 +7
==========================================
+ Hits 48841 52357 +3516
+ Misses 36441 32920 -3521
- Partials 1808 1897 +89 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
src/Core/Auth/UserFeatures/EmergencyAccess/Commands/DeleteEmergencyAccessCommand.cs
Outdated
Show resolved
Hide resolved
test/Core.Test/Auth/UserFeatures/EmergencyAccess/DeleteEmergencyAccessCommandTests.cs
Show resolved
Hide resolved
ike-kottlowski
requested review from
Patrick-Pimentel-Bitwarden,
dani-garcia and
quexten
| SET @BatchSize = @@ROWCOUNT | ||
|
|
||
| END | ||
| END |
There was a problem hiding this comment.
❓ 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..
| var (grantorEmails, granteeEmails) = await DeleteEmergencyAccessAsync(emergencyAccessDetails); | ||
|
|
||
| // Send notification email to grantor | ||
| await SendEmergencyAccessRemoveGranteesEmailAsync(grantorEmails, granteeEmails); ; |
There was a problem hiding this comment.
⛏️ Double semicolon
| var emergencyAccessDetails = await _emergencyAccessRepository.GetManyDetailsByGrantorIdAsync(grantorId); | ||
|
|
||
| // if there is nothing return an empty array and do not send an email | ||
| if (emergencyAccessDetails == null || emergencyAccessDetails.Count == 0) |
There was a problem hiding this comment.
⛏️ Unnecessary null check. You have a test to make sure that no results comes back as an empty list anywho, emergencyAccessDetails can't ever be null. There are 2 places in this file this check is done and can be removed.
Patrick-Pimentel-Bitwarden
left a comment
Patrick-Pimentel-Bitwarden
left a comment
There was a problem hiding this comment.
Couple of small nitpicks, nothing blocking 👍
|
Converting to draft to handle bumping User Revision date and addressing other feedback. |
…ts to check assert grantees revision dates are updated
…dated as well as proper deletion of emergency access
ike-kottlowski
dismissed stale reviews from quexten and Patrick-Pimentel-Bitwarden
via
b151415
ike-kottlowski
requested review from
Patrick-Pimentel-Bitwarden,
mkincaid-bw and
quexten
🎟️ 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
DeleteManyByIdstored procedure that batches, if need be, the records for deletion.Things to note:
GranteeIdwe loop over the grantor emails sending one at a time, so we don't accidentlyccall the grantors for a given grantee.nullablefor classes that felt like low hanging fruit, which is why other teams are tagged.⏰ 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