Allow for bulk processing new login device requests #4064
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ac/addison/ac-2301/service-bulk-device-approval-endpoint-api #4064 +/- ##
================================================================================================
+ Coverage 38.35% 38.50% +0.15%
================================================================================================
Files 1210 1211 +1
Lines 58712 58879 +167
Branches 5586 5608 +22
================================================================================================
+ Hits 22518 22674 +156
- Misses 35152 35154 +2
- Partials 1042 1051 +9 ☔ View full report in Codecov by Sentry. |
|
New Issues
Fixed Issues
|
In order to facilitate a command method that can update many auth requests at one time a new model must be defined that accepts valid input for the command's needs. To achieve this a new file has been created at `Core/AdminConsole/OrganizationAuth/Models/OrganizationAuthRequestUpdateCommandModel.cs` that contains a class of the same name. It's properties match those that need to come from any calling API request models to fulfill the request.
addisonbeck
force-pushed
the
ac/addison/ac-2301/bulk-device-approval-service
branch
2 times, most recently
from
7f4b13b
to
f952e19
Compare
Calling API functions of the `UpdateOrganizationAuthRequestCommand` need a function that can accept many auth request response objects and process them as approved or denied. To achieve this a new function has been added to `IUpdateOrganizationAuthRequestCommand` called `UpdateManyAsync()` that accepts an `IEnumberable<OrganizationAuthRequest>` and returns a `Task`. Implementations of this interface method will be used to bulk process auth requests as approved or denied.
To facilitate a bulk device login request approval workflow in the admin console `UpdateOrganizationAuthRequestCommand` needs to be updated to include an `UpdateMany()` method. It should accept a list of `OrganizationAuthRequestUpdateCommandModel` objects, perform some simple data validation checks, and then pass those along to `AuthRequestRepository` for updating in the database. This commit stubs out this method for the purpose of writing unit tests. At this stage the method throws a `NotImplementedException()`. It will be expand after writing assertions.
The updates to `UpdateOrganizationAuthRequestCommand` require a new direct dependency on `IAuthRequestRepository`. This commit simply registers this dependency in the `UpdateOrganizationAuthRequest` constructor for use in unit tests and the `UpdateManyAsync()` implementation.
addisonbeck
force-pushed
the
ac/addison/ac-2301/bulk-device-approval-service
branch
5 times, most recently
from
640de72
to
a1777e5
Compare
addisonbeck
changed the base branch from
main
to
ac/addison/ac-2301/service-bulk-device-approval-endpoint-api
addisonbeck
force-pushed
the
ac/addison/ac-2301/bulk-device-approval-service
branch
15 times, most recently
from
896e8b5
to
3a17788
Compare
addisonbeck
force-pushed
the
ac/addison/ac-2301/bulk-device-approval-service
branch
8 times, most recently
from
8b8099c
to
1a754f5
Compare
addisonbeck
force-pushed
the
ac/addison/ac-2301/bulk-device-approval-service
branch
from
1a754f5
to
603cc6d
Compare
| Task UpdateAsync(Guid requestId, Guid userId, bool requestApproved, string encryptedUserKey); | ||
| Task UpdateManyAsync(Guid organizationId, IEnumerable<OrganizationAuthRequestUpdateCommandModel> authRequestUpdates); |
There was a problem hiding this comment.
Suggestion: this could be an overload of UpdateAsync. Avoid the naming problems altogether 😁
There was a problem hiding this comment.
Renamed to UpdateAsync as suggested on 188af1f
| @@ -0,0 +1,8 @@ | |||
| namespace Bit.Core.AdminConsole.OrganizationAuth.Models; | |||
|
|
|||
| public class OrganizationAuthRequestUpdateCommandModel | |||
There was a problem hiding this comment.
I don't think there's any need to repeat Command in this class name. Usually request, response, and data models have suffixes, domain models don't. e.g. OrganizationAuthRequestUpdateModel.
EDIT: in fact, I'm not even sure we use the Model suffix, except in the public api. OrganizationAuthRequestUpdate?
There was a problem hiding this comment.
Renamed to OrganizationAuthRequestUpdate as suggested on 0a0216f
| var processedAuthRequests = new List<T>(); | ||
| authRequestsToProcess = FilterOutSpentAuthRequests(authRequestsToProcess); | ||
| authRequestsToProcess = FilterOutExpiredAuthRequests(authRequestsToProcess); | ||
| authRequestsToProcess = FilterOutAuthRequestsWithNoUpdates(authRequestsToProcess, updates); | ||
| authRequestsToProcess = FilterOutAuthRequestsThatDoNotMatchOrganizationId(authRequestsToProcess, organizationId); |
There was a problem hiding this comment.
This loses the nice chained linq syntax, and in general I think it's better to encapsulate the single rather than the many. I suggest encapsulating the callback only, e.g.
var processedAuthRequests = authRequestsToProcess
.Where(notExpired)
.Where(notSpent) // is there a more descriptive term than spent?
.Where(matchesOrganizationId(orgId))
// etcalthough then that suggests to me that maybe these belong on the model, because most of them are just about validating model state. eg. authRequestsToProcess.Where(a => !a.expired)
There was a problem hiding this comment.
I liked this feedback, but felt like I had to go in a slightly more complicated direction than just using the UpdateAuthRequest model. That model is meant to just be an input model, and adding a bunch of logic to it that is also dependent on the unprocessed and processed auth request seemed out of scope for its intended purpose.
BUT I did make a new model, or really two. AuthRequestUpdateProcessor, and BatchAuthRequestUpdateProcesssor. These take in everything needed to process the auth request and handle approving it, denying it, running validation checks, and declaring what a processed auth request is capable of doing like saving itself and sending notifications. They have no direct dependencies on other services, and just consume and manipulate data and the occasional callback function.
These leaves the Command as a buffer between the external dependencies of processing an auth request (all the other repos and services that are used) and what processing an auth request means to it's data (the processors). It also features some sick method chaining.
Implemented on ac451cb
| public T ApproveAuthRequest<T>(T authRequestToApprove, string Key) where T : AuthRequest | ||
| { | ||
| if (string.IsNullOrWhiteSpace(Key)) | ||
| { | ||
| _logger.LogError($"An auth request with id {authRequestToApprove.Id} was approved, but no key was provided. This auth request can not be approved."); | ||
| return authRequestToApprove; | ||
| } | ||
| authRequestToApprove.Key = Key; | ||
| authRequestToApprove.Approved = true; | ||
| authRequestToApprove.ResponseDate = DateTime.UtcNow; | ||
| return authRequestToApprove; | ||
| } | ||
|
|
||
| public T DenyAuthRequest<T>(T authRequestToDeny) where T : AuthRequest | ||
| { | ||
| authRequestToDeny.Approved = false; | ||
| authRequestToDeny.ResponseDate = DateTime.UtcNow; | ||
| return authRequestToDeny; | ||
| } |
There was a problem hiding this comment.
These are just about updating model state, they could be moved to the model. e.g. request.Approve(key) / request.Deny().
There was a problem hiding this comment.
See #4064 (comment) as a reference for all "put-this-in-a-model" talk.
|
|
||
| public TimeSpan FetchRequestExpirationTimespan() | ||
| { | ||
| return _globalSettings.PasswordlessAuth.AdminRequestExpiration; |
There was a problem hiding this comment.
I suggest this could be a property rather than a method.
There was a problem hiding this comment.
This was removed on ac451cb and just called directly.
| return await _userRepository.GetByIdAsync(userId); | ||
| } | ||
|
|
||
| public async Task<bool> PushManyTrustedDeviceEmails<T>(IEnumerable<T> authRequests) where T : AuthRequest |
There was a problem hiding this comment.
Unused return value? Only seems to be used in tests, but in that case it's better to assert the call to the mailService dependency.
There was a problem hiding this comment.
Removed on ac451cb, but to explain: this was unused, except for in tests. I was having trouble asserting the mail service was called. I also sort of converting voids into things that return status codes.
| return authRequest.RequestDeviceType.GetType() | ||
| .GetMember(authRequest.RequestDeviceType.ToString()) | ||
| .FirstOrDefault()? | ||
| // This unknown case can't be unit tested without adding an enum | ||
| // with no display attribute. Faith and trust are required! | ||
| .GetCustomAttribute<DisplayAttribute>()?.Name ?? "Unknown Device Type"; |
There was a problem hiding this comment.
Probably sounding like a broken record here. But this could be moved the model 😅 it's only concerned with parsing the RequestDeviceType property.
There was a problem hiding this comment.
See #4064 (comment) as a reference for all "put-this-in-a-model" talk.
| { | ||
| return null; | ||
| } | ||
| return await _organizationUserRepository.GetByOrganizationAsync(authRequest.OrganizationId.Value, authRequest.UserId); |
There was a problem hiding this comment.
There's a bulk method you can use - GetManyAsync(authRequests.Select(r => r.OrganizationUserId).
There was a problem hiding this comment.
Implemented on b03ea39. I also added some post release work for creating these sorts of bulk methods for the other auth request events (emails and push notifications). Those are still done one at a time in a loop for now.
| _logger.LogError($"An organization user was not found while processing auth request {authRequest.Id}. Event logs can not be posted for this request."); | ||
| return; | ||
| } | ||
| await _eventService.LogOrganizationUserEventAsync(organizationUser, CalculateOrganizationAuthRequestProcessingEventLogType(authRequest)); |
There was a problem hiding this comment.
There's also a bulk overload for this.
There was a problem hiding this comment.
Implemented on b03ea39
There was a problem hiding this comment.
I see you're still working on tests, so this might be premature, but: I think these tests are too granular. I think the biggest risk here is that the tests would not stand up to refactoring; they express how the command is implemented, not what it does. So any internal refactor would immediately break them, reducing their usefulness in guarding against regressions.
Different devs do take different approaches here, I am pretty strongly on the "only test public interfaces" side of things, but I know not everyone is. So I won't hold it up over that. But I would like to see at least some tests that test the public interface only, and assert all the expected behavior resulting from that call.
There was a problem hiding this comment.
All these new methods are only used once so I'd prefer if their logic would be inside UpdateManyAsync, that would make it easier to read. Just add a comment for each section.
addisonbeck
force-pushed
the
ac/addison/ac-2301/service-bulk-device-approval-endpoint-api
branch
5 times, most recently
from
fab5fde
to
cea0589
Compare
addisonbeck
force-pushed
the
ac/addison/ac-2301/bulk-device-approval-service
branch
7 times, most recently
from
5942db0
to
e8887a6
Compare
addisonbeck
force-pushed
the
ac/addison/ac-2301/bulk-device-approval-service
branch
from
e8887a6
to
b03ea39
Compare
Type of change
Objective
To facilitate a new bulk device login request in the admin console server
logic needs to be written that can bulk approve and deny new device login
requests safely. This logic will need to accept a list of ids, keys, and
approval states and process those requests to apply the keys and approval
states.
Code changes
See the commits tab
References
database functionality needed to bulk update
AuthRequesttable records.UpdateManyAsynccommand written here.