From 5dc6013e378e7d6362834d9b9972fe08c1a653d6 Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Tue, 10 Aug 2021 12:16:10 -0400 Subject: [PATCH] Provider qa feedback (#1501) * Title case buttons * Throw if provider tries to add a non-business organization * Allow only one admin OR owner roll in a free org per user Boolean operators were not properly assocated and ownership of an org was precluding confirmation into any other organization * Limit email length * Require email domain with top level domain * Do not allow email domains to end in invalid characters * Fix free org tests --- .../src/CommCore/Services/ProviderService.cs | 15 +++++ .../Provider/ProviderSetupInvite.html.hbs | 2 +- .../ProviderOrganizationCreateRequestModel.cs | 1 + .../Implementations/OrganizationService.cs | 4 +- .../Utilities/StrictEmailAddressAttribute.cs | 8 ++- .../Services/OrganizationServiceTests.cs | 60 +++++++++++++++++-- .../StrictEmailAddressAttributeTests.cs | 38 ++++++------ 7 files changed, 103 insertions(+), 25 deletions(-) diff --git a/bitwarden_license/src/CommCore/Services/ProviderService.cs b/bitwarden_license/src/CommCore/Services/ProviderService.cs index 88c60546a5..c4d9521015 100644 --- a/bitwarden_license/src/CommCore/Services/ProviderService.cs +++ b/bitwarden_license/src/CommCore/Services/ProviderService.cs @@ -21,6 +21,8 @@ namespace Bit.CommCore.Services { public class ProviderService : IProviderService { + public static PlanType[] ProviderDisllowedOrganizationTypes = new[] { PlanType.Free, PlanType.FamiliesAnnually, PlanType.FamiliesAnnually2019 }; + private readonly IDataProtector _dataProtector; private readonly IMailService _mailService; private readonly IEventService _eventService; @@ -380,6 +382,9 @@ namespace Bit.CommCore.Services throw new BadRequestException("Organization already belongs to a provider."); } + var organization = await _organizationRepository.GetByIdAsync(organizationId); + ThrowOnInvalidPlanType(organization.PlanType); + var providerOrganization = new ProviderOrganization { ProviderId = providerId, @@ -394,6 +399,8 @@ namespace Bit.CommCore.Services public async Task CreateOrganizationAsync(Guid providerId, OrganizationSignup organizationSignup, string clientOwnerEmail, User user) { + ThrowOnInvalidPlanType(organizationSignup.Plan); + var (organization, _) = await _organizationService.SignUpAsync(organizationSignup, true); var providerOrganization = new ProviderOrganization @@ -487,5 +494,13 @@ namespace Bit.CommCore.Services var confirmedOwnersIds = confirmedOwners.Select(u => u.Id); return confirmedOwnersIds.Except(providerUserIds).Any(); } + + private void ThrowOnInvalidPlanType(PlanType requestedType) + { + if (ProviderDisllowedOrganizationTypes.Contains(requestedType)) + { + throw new BadRequestException($"Providers cannot manage organizations with the requested plan type ({requestedType}). Only Teams and Enterprise accounts are allowed."); + } + } } } diff --git a/src/Core/MailTemplates/Handlebars/Provider/ProviderSetupInvite.html.hbs b/src/Core/MailTemplates/Handlebars/Provider/ProviderSetupInvite.html.hbs index cc17a73bbc..eea775e0d4 100644 --- a/src/Core/MailTemplates/Handlebars/Provider/ProviderSetupInvite.html.hbs +++ b/src/Core/MailTemplates/Handlebars/Provider/ProviderSetupInvite.html.hbs @@ -8,7 +8,7 @@ - Set up Provider Now + Set Up Provider Now diff --git a/src/Core/Models/Api/Request/Providers/ProviderOrganizationCreateRequestModel.cs b/src/Core/Models/Api/Request/Providers/ProviderOrganizationCreateRequestModel.cs index 001f483407..c6d0109ec8 100644 --- a/src/Core/Models/Api/Request/Providers/ProviderOrganizationCreateRequestModel.cs +++ b/src/Core/Models/Api/Request/Providers/ProviderOrganizationCreateRequestModel.cs @@ -6,6 +6,7 @@ namespace Bit.Core.Models.Api.Request public class ProviderOrganizationCreateRequestModel { [Required] + [StringLength(256)] [StrictEmailAddress] public string ClientOwnerEmail { get; set; } [Required] diff --git a/src/Core/Services/Implementations/OrganizationService.cs b/src/Core/Services/Implementations/OrganizationService.cs index 4024c399e4..ad969c002f 100644 --- a/src/Core/Services/Implementations/OrganizationService.cs +++ b/src/Core/Services/Implementations/OrganizationService.cs @@ -1488,8 +1488,8 @@ namespace Bit.Core.Services var orgUsers = keyedOrganizationUsers.GetValueOrDefault(user.Id, new List()); try { - if (organization.PlanType == PlanType.Free && orgUser.Type == OrganizationUserType.Admin - || orgUser.Type == OrganizationUserType.Owner) + if (organization.PlanType == PlanType.Free && (orgUser.Type == OrganizationUserType.Admin + || orgUser.Type == OrganizationUserType.Owner)) { // Since free organizations only supports a few users there is not much point in avoiding N+1 queries for this. var adminCount = await _organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(user.Id); diff --git a/src/Core/Utilities/StrictEmailAddressAttribute.cs b/src/Core/Utilities/StrictEmailAddressAttribute.cs index 0145e8c5ad..99065d8fde 100644 --- a/src/Core/Utilities/StrictEmailAddressAttribute.cs +++ b/src/Core/Utilities/StrictEmailAddressAttribute.cs @@ -1,4 +1,5 @@ using System.ComponentModel.DataAnnotations; +using System.Text.RegularExpressions; using MimeKit; namespace Bit.Core.Utilities @@ -25,7 +26,12 @@ namespace Bit.Core.Utilities return false; } } - catch (ParseException e) + catch (ParseException) + { + return false; + } + + if (!Regex.IsMatch(emailAddress, @"@.+\.[A-Za-z0-9]+$")) { return false; } diff --git a/test/Core.Test/Services/OrganizationServiceTests.cs b/test/Core.Test/Services/OrganizationServiceTests.cs index d55d6adae9..385ff6b09f 100644 --- a/test/Core.Test/Services/OrganizationServiceTests.cs +++ b/test/Core.Test/Services/OrganizationServiceTests.cs @@ -584,10 +584,12 @@ namespace Bit.Core.Test.Services () => sutProvider.Sut.ConfirmUserAsync(confirmingUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, userService)); Assert.Contains("User not valid.", exception.Message); } - - [Theory, CustomAutoData(typeof(SutProviderCustomization))] - public async Task ConfirmUser_AlreadyAdmin(Organization org, OrganizationUser confirmingUser, - [OrganizationUser(OrganizationUserStatusType.Accepted, OrganizationUserType.Admin)]OrganizationUser orgUser, User user, + + [Theory] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, OrganizationUserType.Owner)] + public async Task ConfirmUserToFree_AlreadyFreeAdminOrOwner_Throws(OrganizationUserType userType, Organization org, OrganizationUser confirmingUser, + [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser, User user, string key, SutProvider sutProvider) { var organizationUserRepository = sutProvider.GetDependency(); @@ -598,6 +600,7 @@ namespace Bit.Core.Test.Services org.PlanType = PlanType.Free; orgUser.OrganizationId = confirmingUser.OrganizationId = org.Id; orgUser.UserId = user.Id; + orgUser.Type = userType; organizationUserRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] {orgUser}); organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(orgUser.UserId.Value).Returns(1); organizationRepository.GetByIdAsync(org.Id).Returns(org); @@ -608,6 +611,55 @@ namespace Bit.Core.Test.Services Assert.Contains("User can only be an admin of one free organization.", exception.Message); } + [Theory] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.Custom, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.Custom, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseAnnually, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseAnnually, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseAnnually2019, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseAnnually2019, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseMonthly, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseMonthly, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseMonthly2019, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.EnterpriseMonthly2019, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.FamiliesAnnually, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.FamiliesAnnually, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.FamiliesAnnually2019, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.FamiliesAnnually2019, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsAnnually, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsAnnually, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsAnnually2019, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsAnnually2019, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsMonthly, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsMonthly, OrganizationUserType.Owner)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsMonthly2019, OrganizationUserType.Admin)] + [InlineCustomAutoData(new[] { typeof(SutProviderCustomization) }, PlanType.TeamsMonthly2019, OrganizationUserType.Owner)] + public async Task ConfirmUserToNonFree_AlreadyFreeAdminOrOwner_DoesNotThrow(PlanType planType, OrganizationUserType orgUserType, Organization org, OrganizationUser confirmingUser, + [OrganizationUser(OrganizationUserStatusType.Accepted)] OrganizationUser orgUser, User user, + string key, SutProvider sutProvider) + { + var organizationUserRepository = sutProvider.GetDependency(); + var organizationRepository = sutProvider.GetDependency(); + var userService = Substitute.For(); + var userRepository = sutProvider.GetDependency(); + + org.PlanType = planType; + orgUser.OrganizationId = confirmingUser.OrganizationId = org.Id; + orgUser.UserId = user.Id; + orgUser.Type = orgUserType; + organizationUserRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { orgUser }); + organizationUserRepository.GetCountByFreeOrganizationAdminUserAsync(orgUser.UserId.Value).Returns(1); + organizationRepository.GetByIdAsync(org.Id).Returns(org); + userRepository.GetManyAsync(default).ReturnsForAnyArgs(new[] { user }); + + await sutProvider.Sut.ConfirmUserAsync(orgUser.OrganizationId, orgUser.Id, key, confirmingUser.Id, userService); + + await sutProvider.GetDependency().Received(1).LogOrganizationUserEventAsync(orgUser, EventType.OrganizationUser_Confirmed); + await sutProvider.GetDependency().Received(1).SendOrganizationConfirmedEmailAsync(org.Name, user.Email); + await organizationUserRepository.Received(1).ReplaceManyAsync(Arg.Is>(users => users.Contains(orgUser) && users.Count == 1)); + } + + [Theory, CustomAutoData(typeof(SutProviderCustomization))] public async Task ConfirmUser_SingleOrgPolicy(Organization org, OrganizationUser confirmingUser, [OrganizationUser(OrganizationUserStatusType.Accepted)]OrganizationUser orgUser, User user, diff --git a/test/Core.Test/Utilities/StrictEmailAddressAttributeTests.cs b/test/Core.Test/Utilities/StrictEmailAddressAttributeTests.cs index 7ad4e2c0bf..56bdfbae6c 100644 --- a/test/Core.Test/Utilities/StrictEmailAddressAttributeTests.cs +++ b/test/Core.Test/Utilities/StrictEmailAddressAttributeTests.cs @@ -20,25 +20,29 @@ namespace Bit.Core.Test.Utilities } [Theory] - [InlineData(null)] // null - [InlineData("hello@world.com\t")] // trailing tab char - [InlineData("\thello@world.com")] // leading tab char - [InlineData("hel\tlo@world.com")] // local-part tab char - [InlineData("hello@world.com\b")] // trailing backspace char - [InlineData("\" \"hello@world.com")] // leading spaces in quotes - [InlineData("hello@world.com\" \"")] // trailing spaces in quotes - [InlineData("hel\" \"lo@world.com")] // local-part spaces in quotes - [InlineData("hello there@world.com")] // unescaped unquoted spaces - [InlineData("Hello ")] // friendly from - [InlineData("")] // wrapped angle brackets - [InlineData("hello(com)there@world.com")] // comment - [InlineData("hello@world.com.")] // trailing period - [InlineData(".hello@world.com")] // leading period - [InlineData("hello@world.com;")] // trailing semicolon - [InlineData(";hello@world.com")] // leading semicolon + [InlineData(null)] // null + [InlineData("hello@world.com\t")] // trailing tab char + [InlineData("\thello@world.com")] // leading tab char + [InlineData("hel\tlo@world.com")] // local-part tab char + [InlineData("hello@world.com\b")] // trailing backspace char + [InlineData("\" \"hello@world.com")] // leading spaces in quotes + [InlineData("hello@world.com\" \"")] // trailing spaces in quotes + [InlineData("hel\" \"lo@world.com")] // local-part spaces in quotes + [InlineData("hello there@world.com")] // unescaped unquoted spaces + [InlineData("Hello ")] // friendly from + [InlineData("")] // wrapped angle brackets + [InlineData("hello(com)there@world.com")] // comment + [InlineData("hello@world.com.")] // trailing period + [InlineData(".hello@world.com")] // leading period + [InlineData("hello@world.com;")] // trailing semicolon + [InlineData(";hello@world.com")] // leading semicolon [InlineData("hello@world.com; hello@world.com")] // semicolon separated list [InlineData("hello@world.com, hello@world.com")] // comma separated list - + [InlineData("hellothere@worldcom")] // dotless domain + [InlineData("hello.there@worldcom")] // dotless domain + [InlineData("hellothere@.worldcom")] // domain beginning with dot + [InlineData("hellothere@worldcom.")] // domain ending in dot + [InlineData("hellothere@world.com-")] // domain ending in hyphen public void IsValid_ReturnsFalseWhenInvalid(string email) { var sut = new StrictEmailAddressAttribute();