- 
                Notifications
    
You must be signed in to change notification settings  - Fork 1.5k
 
[PM-26636] - Auto Confirm Org User Command #6488
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 14 commits
14f3e88
              4645222
              6a29377
              89b8d59
              f60f0f5
              0ba770d
              e5f07de
              5527ca6
              27ce4b8
              2069c9f
              cb81d29
              24a39d8
              a1b2bac
              7307454
              1303b22
              4bb8734
              68afb17
              aee2734
              031c080
              805eba9
              ce596b2
              6e5188f
              d6a5813
              9d47bf4
              929ac41
              File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| 
          
            
          
           | 
    @@ -10,7 +10,9 @@ | |
| using Bit.Api.Vault.AuthorizationHandlers.Collections; | ||
| using Bit.Core; | ||
| using Bit.Core.AdminConsole.Enums; | ||
| using Bit.Core.AdminConsole.Models.Data; | ||
| using Bit.Core.AdminConsole.Models.Data.Organizations.Policies; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.AutoConfirmUser; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.DeleteClaimedAccount; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.Interfaces; | ||
| using Bit.Core.AdminConsole.OrganizationFeatures.OrganizationUsers.InviteUsers; | ||
| 
          
            
          
           | 
    @@ -66,6 +68,8 @@ public class OrganizationUsersController : Controller | |
| private readonly IFeatureService _featureService; | ||
| private readonly IPricingClient _pricingClient; | ||
| private readonly IResendOrganizationInviteCommand _resendOrganizationInviteCommand; | ||
| private readonly IAutomaticallyConfirmOrganizationUserCommand _automaticallyConfirmOrganizationUserCommand; | ||
| private readonly TimeProvider _timeProvider; | ||
| private readonly IConfirmOrganizationUserCommand _confirmOrganizationUserCommand; | ||
| private readonly IRestoreOrganizationUserCommand _restoreOrganizationUserCommand; | ||
| private readonly IInitPendingOrganizationCommand _initPendingOrganizationCommand; | ||
| 
          
            
          
           | 
    @@ -97,7 +101,9 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor | |
| IRestoreOrganizationUserCommand restoreOrganizationUserCommand, | ||
| IInitPendingOrganizationCommand initPendingOrganizationCommand, | ||
| IRevokeOrganizationUserCommand revokeOrganizationUserCommand, | ||
| IResendOrganizationInviteCommand resendOrganizationInviteCommand) | ||
| IResendOrganizationInviteCommand resendOrganizationInviteCommand, | ||
| IAutomaticallyConfirmOrganizationUserCommand automaticallyConfirmOrganizationUserCommand, | ||
| TimeProvider timeProvider) | ||
| { | ||
| _organizationRepository = organizationRepository; | ||
| _organizationUserRepository = organizationUserRepository; | ||
| 
        
          
        
         | 
    @@ -122,6 +128,8 @@ public OrganizationUsersController(IOrganizationRepository organizationRepositor | |
| _featureService = featureService; | ||
| _pricingClient = pricingClient; | ||
| _resendOrganizationInviteCommand = resendOrganizationInviteCommand; | ||
| _automaticallyConfirmOrganizationUserCommand = automaticallyConfirmOrganizationUserCommand; | ||
| _timeProvider = timeProvider; | ||
| _confirmOrganizationUserCommand = confirmOrganizationUserCommand; | ||
| _restoreOrganizationUserCommand = restoreOrganizationUserCommand; | ||
| _initPendingOrganizationCommand = initPendingOrganizationCommand; | ||
| 
          
            
          
           | 
    @@ -691,6 +699,56 @@ public async Task PatchBulkEnableSecretsManagerAsync(Guid orgId, | |
| await BulkEnableSecretsManagerAsync(orgId, model); | ||
| } | ||
| 
     | 
||
| 
     | 
||
| [Authorize<ManageUsersRequirement>] | ||
| [HttpPost("{id}/auto-confirm")] | ||
| public async Task<IActionResult> AutomaticallyConfirmOrganizationUserAsync([FromRoute] Guid orgId, | ||
| [FromRoute] Guid id, | ||
| [FromBody] OrganizationUserConfirmRequestModel model) | ||
| { | ||
| if (!_featureService.IsEnabled(FeatureFlagKeys.AutomaticConfirmUsers)) | ||
| { | ||
| return NotFound(); | ||
| } | ||
| 
     | 
||
| var userId = _userService.GetProperUserId(User); | ||
| 
     | 
||
| if (userId is null || userId.Value == Guid.Empty) | ||
| { | ||
| return Unauthorized(); | ||
| } | ||
| 
     | 
||
| var result = await _automaticallyConfirmOrganizationUserCommand.AutomaticallyConfirmOrganizationUserAsync( | ||
| new AutomaticallyConfirmOrganizationUserRequest | ||
| { | ||
| OrganizationId = orgId, | ||
| OrganizationUserId = id, | ||
| Key = model.Key, | ||
| DefaultUserCollectionName = model.DefaultUserCollectionName, | ||
| PerformedBy = new StandardUser(userId.Value, await _currentContext.OrganizationOwner(orgId)), | ||
| PerformedOn = _timeProvider.GetUtcNow() | ||
| 
         There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Passing in the date from the controller level seems unexpected to me. It's not information from the request or otherwise from the API layer (like claims in   | 
||
| }); | ||
| 
     | 
||
| if (result is { IsError: true }) | ||
| { | ||
| return result.AsError switch | ||
                
       | 
||
| { | ||
| NotFoundError notFound => NotFound(notFound.Message), | ||
| BadRequestError badRequest => BadRequest(badRequest.Message), | ||
| InternalError internalError => Problem( | ||
| detail: internalError.Message, | ||
| statusCode: StatusCodes.Status500InternalServerError, | ||
| title: "An unexpected error occurred. Please try again or contact support."), | ||
| _ => Problem( | ||
| detail: result.AsError.Message, | ||
| statusCode: StatusCodes.Status500InternalServerError, | ||
| title: "An unexpected error occurred") | ||
| }; | ||
| } | ||
| 
     | 
||
| return Ok(); | ||
| } | ||
| 
     | 
||
| private async Task RestoreOrRevokeUserAsync( | ||
| Guid orgId, | ||
| Guid id, | ||
| 
          
            
          
           | 
    ||
| 
                       There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should add unit tests for this command  | 
            
| Original file line number | Diff line number | Diff line change | 
|---|---|---|
| @@ -0,0 +1,200 @@ | ||
| ๏ปฟ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.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.OrganizationFeatures.OrganizationUsers.DeleteClaimedAccount.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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ๐ญ (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 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.
This should be replaced with
[RequireFeature(FeatureFlagKeys.AutomaticConfirmUsers)]