[PM-26636] - Auto Confirm Org User Command #6488
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Utilities.v2; | ||
| using Bit.Core.AdminConsole.Utilities.v2.Results; | ||
| using Bit.Core.Models.Api; | ||
| using Microsoft.AspNetCore.Mvc; | ||
|
|
||
| namespace Bit.Api.AdminConsole.Controllers; | ||
|
|
||
| public abstract class BaseAdminConsoleController : Controller | ||
| { | ||
| protected static IResult Handle<T>(CommandResult<T> commandResult) => | ||
| commandResult.Match<IResult>( | ||
| error => error switch | ||
| { | ||
| BadRequestError badRequest => TypedResults.BadRequest(new ErrorResponseModel(badRequest.Message)), | ||
| NotFoundError notFound => TypedResults.NotFound(new ErrorResponseModel(notFound.Message)), | ||
| InternalError internalError => TypedResults.Json( | ||
| new ErrorResponseModel(internalError.Message), | ||
| statusCode: StatusCodes.Status500InternalServerError), | ||
| _ => TypedResults.Json( | ||
| new ErrorResponseModel(commandResult.AsError.Message), | ||
|
||
| statusCode: StatusCodes.Status500InternalServerError | ||
| ) | ||
| }, | ||
| _ => TypedResults.NoContent() | ||
|
||
| ); | ||
|
|
||
| protected static IResult Handle(BulkCommandResult commandResult) => | ||
| commandResult.Result.Match<IResult>( | ||
| error => error switch | ||
| { | ||
| NotFoundError notFoundError => TypedResults.NotFound(new ErrorResponseModel(notFoundError.Message)), | ||
| _ => TypedResults.BadRequest(new ErrorResponseModel(error.Message)) | ||
| }, | ||
| _ => TypedResults.NoContent() | ||
| ); | ||
|
Comment on lines
+27
to
+35
There was a problem hiding this comment. This is not generally how we'd handle a I see that you're currently using this on the (non-bulk) |
||
|
|
||
| protected static IResult Handle(CommandResult commandResult) => | ||
|
There was a problem hiding this comment. Same feedback as above for There was a problem hiding this comment. For this one, its a Also, for this one, would the NoContent be more accurate as its a 200 code (success) but gives the additional information of telling the client that there's no body to look for? (I know we don't do this else where but is a good API practice.) I'll change for consistency if that's better. |
||
| commandResult.Match<IResult>( | ||
| error => error switch | ||
| { | ||
| BadRequestError badRequest => TypedResults.BadRequest(new ErrorResponseModel(badRequest.Message)), | ||
| NotFoundError notFound => TypedResults.NotFound(new ErrorResponseModel(notFound.Message)), | ||
| InternalError internalError => TypedResults.Json( | ||
| new ErrorResponseModel(internalError.Message), | ||
| statusCode: StatusCodes.Status500InternalServerError), | ||
| _ => TypedResults.Json( | ||
| new ErrorResponseModel(commandResult.AsError.Message), | ||
| statusCode: StatusCodes.Status500InternalServerError | ||
| ) | ||
| }, | ||
| _ => TypedResults.NoContent() | ||
| ); | ||
| } | ||
|
There was a problem hiding this comment. We should add unit tests for this command |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,201 @@ | ||
| ๏ปฟusing Bit.Core.AdminConsole.Entities; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.DeleteClaimedAccount; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.Policies; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.Policies.PolicyRequirements; | ||
| using Bit.Core.AdminConsole.Utilities.v2.Results; | ||
| using Bit.Core.Entities; | ||
| using Bit.Core.Enums; | ||
| using Bit.Core.Models.Data; | ||
| using Bit.Core.Platform.Push; | ||
| using Bit.Core.Repositories; | ||
| using Bit.Core.Services; | ||
| using Microsoft.Extensions.Logging; | ||
| using OneOf.Types; | ||
| using CommandResult = Bit.Core.AdminConsole.Utilities.v2.Results.CommandResult; | ||
|
|
||
| namespace Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.AutoConfirmUser; | ||
|
|
||
| public class AutomaticallyConfirmOrganizationUserCommand(IOrganizationUserRepository organizationUserRepository, | ||
| IOrganizationRepository organizationRepository, | ||
| IAutomaticallyConfirmOrganizationUsersValidator validator, | ||
| IEventService eventService, | ||
| IMailService mailService, | ||
| IUserRepository userRepository, | ||
| IPushRegistrationService pushRegistrationService, | ||
| IDeviceRepository deviceRepository, | ||
| IPushNotificationService pushNotificationService, | ||
| IFeatureService featureService, | ||
| IPolicyRequirementQuery policyRequirementQuery, | ||
| ICollectionRepository collectionRepository, | ||
| ILogger<AutomaticallyConfirmOrganizationUserCommand> logger) : IAutomaticallyConfirmOrganizationUserCommand | ||
| { | ||
| public async Task<CommandResult> AutomaticallyConfirmOrganizationUserAsync(AutomaticallyConfirmOrganizationUserRequest request) | ||
| { | ||
| var requestData = await RetrieveDataAsync(request); | ||
|
|
||
| if (requestData.IsError) return requestData.AsError; | ||
|
||
|
|
||
| var validatedData = await validator.ValidateAsync(requestData.AsSuccess); | ||
|
|
||
| if (validatedData.IsError) return validatedData.AsError; | ||
|
|
||
| var validatedRequest = validatedData.Request; | ||
|
|
||
| var successfulConfirmation = await organizationUserRepository.ConfirmOrganizationUserAsync(validatedRequest.OrganizationUser); | ||
|
|
||
| if (!successfulConfirmation) return new None(); | ||
|
||
|
|
||
| return await validatedRequest.ToCommandResultAsync() | ||
| .MapAsync(CreateDefaultCollectionsAsync) | ||
| .MapAsync(LogOrganizationUserConfirmedEventAsync) | ||
| .MapAsync(SendConfirmedOrganizationUserEmailAsync) | ||
| .MapAsync(DeleteDeviceRegistrationAsync) | ||
| .MapAsync(PushSyncOrganizationKeysAsync) | ||
| .ToResultAsync(); | ||
|
Comment on lines
+58
to
+64
There was a problem hiding this comment. It's worth considering error handling here. You do handle all errors, but then each private method here is returning an I tend to think that all these methods here should log but otherwise swallow errors. We've already confirmed the user, we should do as many side effects that we can. (This should be noted in a comment.) |
||
| } | ||
|
|
||
| private async Task<CommandResult<AutomaticallyConfirmOrganizationUserRequestData>> CreateDefaultCollectionsAsync( | ||
| AutomaticallyConfirmOrganizationUserRequestData request) | ||
| { | ||
| try | ||
| { | ||
| if (!featureService.IsEnabled(FeatureFlagKeys.CreateDefaultLocation) | ||
|
There was a problem hiding this comment. Extract all these conditions into a private method |
||
| || string.IsNullOrWhiteSpace(request.DefaultUserCollectionName) | ||
| || !(await policyRequirementQuery.GetAsync<OrganizationDataOwnershipPolicyRequirement>(request.UserId)) | ||
| .RequiresDefaultCollectionOnConfirm(request.Organization.Id)) | ||
| { | ||
| return request; | ||
| } | ||
|
|
||
| await collectionRepository.CreateAsync( | ||
| new Collection | ||
| { | ||
| OrganizationId = request.Organization.Id, | ||
| Name = request.DefaultUserCollectionName, | ||
| Type = CollectionType.DefaultUserCollection | ||
| }, | ||
| groups: null, | ||
| [new CollectionAccessSelection | ||
| { | ||
| Id = request.OrganizationUser.Id, | ||
| Manage = true | ||
| }]); | ||
|
Comment on lines
+80
to
+92
There was a problem hiding this comment. ๐ญ (non-blocking) Thoughts on making this an idempotent repository method as well? e.g. |
||
|
|
||
| return request; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Failed to create default collection for user."); | ||
| return new CommandResult<AutomaticallyConfirmOrganizationUserRequestData>(new FailedToCreateDefaultCollection()); | ||
| } | ||
| } | ||
|
|
||
| private async Task<CommandResult<AutomaticallyConfirmOrganizationUserRequestData>> PushSyncOrganizationKeysAsync(AutomaticallyConfirmOrganizationUserRequestData request) | ||
| { | ||
| try | ||
| { | ||
| await pushNotificationService.PushSyncOrgKeysAsync(request.UserId); | ||
| return request; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Failed to push organization keys."); | ||
| return new FailedToPushOrganizationSyncKeys(); | ||
| } | ||
| } | ||
|
|
||
| private async Task<CommandResult<AutomaticallyConfirmOrganizationUserRequestData>> LogOrganizationUserConfirmedEventAsync( | ||
| AutomaticallyConfirmOrganizationUserRequestData request) | ||
| { | ||
| try | ||
| { | ||
| await eventService.LogOrganizationUserEventAsync(request.OrganizationUser, | ||
| EventType.OrganizationUser_AutomaticallyConfirmed, | ||
| request.PerformedOn.UtcDateTime); | ||
| return request; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Failed to log OrganizationUser_AutomaticallyConfirmed event."); | ||
| return new FailedToWriteToEventLog(); | ||
| } | ||
| } | ||
|
|
||
| private async Task<CommandResult<AutomaticallyConfirmOrganizationUserRequestData>> SendConfirmedOrganizationUserEmailAsync( | ||
| AutomaticallyConfirmOrganizationUserRequestData request) | ||
| { | ||
| try | ||
| { | ||
| var user = await userRepository.GetByIdAsync(request.UserId); | ||
|
|
||
| if (user is null) return new UserNotFoundError(); | ||
|
|
||
| await mailService.SendOrganizationConfirmedEmailAsync(request.Organization.Name, | ||
| user.Email, | ||
| request.OrganizationUser.AccessSecretsManager); | ||
|
|
||
| return request; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Failed to send OrganizationUserConfirmed."); | ||
| return new FailedToSendConfirmedUserEmail(); | ||
| } | ||
| } | ||
|
|
||
| private async Task<CommandResult<AutomaticallyConfirmOrganizationUserRequestData>> DeleteDeviceRegistrationAsync( | ||
| AutomaticallyConfirmOrganizationUserRequestData request) | ||
| { | ||
| try | ||
| { | ||
| var devices = (await deviceRepository.GetManyByUserIdAsync(request.UserId)) | ||
| .Where(d => !string.IsNullOrWhiteSpace(d.PushToken)) | ||
| .Select(d => d.Id.ToString()); | ||
|
|
||
| await pushRegistrationService.DeleteUserRegistrationOrganizationAsync(devices, request.Organization.Id.ToString()); | ||
| return request; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Failed to delete device registration."); | ||
| return new FailedToDeleteDeviceRegistration(); | ||
| } | ||
| } | ||
|
|
||
| private async Task<CommandResult<AutomaticallyConfirmOrganizationUserRequestData>> RetrieveDataAsync( | ||
| AutomaticallyConfirmOrganizationUserRequest request) | ||
| { | ||
| var organizationUser = await GetOrganizationUserAsync(request.OrganizationUserId); | ||
|
|
||
| if (organizationUser.IsError) return organizationUser.AsError; | ||
|
|
||
| var organization = await GetOrganizationAsync(request.OrganizationId); | ||
|
|
||
| if (organization.IsError) return organization.AsError; | ||
|
|
||
| return new AutomaticallyConfirmOrganizationUserRequestData | ||
|
||
| { | ||
| OrganizationUser = organizationUser.AsSuccess, | ||
| Organization = organization.AsSuccess, | ||
| PerformedBy = request.PerformedBy, | ||
| Key = request.Key, | ||
| DefaultUserCollectionName = request.DefaultUserCollectionName, | ||
| PerformedOn = request.PerformedOn | ||
| }; | ||
| } | ||
|
|
||
| private async Task<CommandResult<OrganizationUser>> GetOrganizationUserAsync(Guid organizationUserId) | ||
| { | ||
| var organizationUser = await organizationUserRepository.GetByIdAsync(organizationUserId); | ||
|
|
||
| return organizationUser is { UserId: not null } ? organizationUser : new UserNotFoundError(); | ||
| } | ||
|
|
||
| private async Task<CommandResult<Organization>> GetOrganizationAsync(Guid organizationId) | ||
| { | ||
| var organization = await organizationRepository.GetByIdAsync(organizationId); | ||
|
|
||
| return organization is not null ? organization : new OrganizationNotFound(); | ||
| } | ||
| } | ||
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.
๐ Thanks for starting this!