From 640cb68d517fdea2ff542b5486c9b829d03723c9 Mon Sep 17 00:00:00 2001 From: Thomas Avery <43214426+Thomas-Avery@users.noreply.github.com> Date: Mon, 28 Aug 2023 10:16:50 -0500 Subject: [PATCH] [SM-863] Add endpoint for fetching multiple secrets by IDs (#3134) * Add support CanReadSecret authorization * Extract base response model for secret * Add support for SA bulk fetching event logging * secret repository bug fix * Add endpoint and request for bulk fetching secrets * Swap to original reference event * Add unit tests * Add integration tests * Add unit tests for authz handler * update authz handler tests --------- --- .../Secrets/SecretAuthorizationHandler.cs | 15 +++ .../Repositories/SecretRepository.cs | 3 +- .../SecretAuthorizationHandlerTests.cs | 63 +++++++++++ .../Controllers/SecretsController.cs | 41 +++++++ .../Models/Request/GetSecretsRequestModel.cs | 9 ++ .../Response/BaseSecretResponseModel.cs | 66 ++++++++++++ .../Models/Response/SecretResponseModel.cs | 53 +-------- .../SecretOperationRequirement.cs | 1 + src/Core/Services/IEventService.cs | 1 + .../Services/Implementations/EventService.cs | 34 ++++-- .../NoopImplementations/NoopEventService.cs | 6 ++ .../Controllers/SecretsControllerTests.cs | 63 +++++++++++ .../Controllers/SecretsControllerTests.cs | 101 ++++++++++++++++++ 13 files changed, 394 insertions(+), 62 deletions(-) create mode 100644 src/Api/SecretsManager/Models/Request/GetSecretsRequestModel.cs create mode 100644 src/Api/SecretsManager/Models/Response/BaseSecretResponseModel.cs diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandler.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandler.cs index 262d431fb3..92461e61a9 100644 --- a/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandler.cs +++ b/bitwarden_license/src/Commercial.Core/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandler.cs @@ -38,6 +38,9 @@ public class SecretAuthorizationHandler : AuthorizationHandler(s), Read = true, - Write = false, + Write = s.Projects.Any(p => + p.ServiceAccountAccessPolicies.Any(ap => ap.ServiceAccountId == userId && ap.Write)), }), _ => throw new ArgumentOutOfRangeException(nameof(accessType), accessType, null), }; diff --git a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandlerTests.cs b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandlerTests.cs index d7e49fb46d..f1737e0ad4 100644 --- a/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandlerTests.cs +++ b/bitwarden_license/test/Commercial.Core.Test/SecretsManager/AuthorizationHandlers/Secrets/SecretAuthorizationHandlerTests.cs @@ -232,6 +232,69 @@ public class SecretAuthorizationHandlerTests Assert.True(authzContext.HasSucceeded); } + [Theory] + [BitAutoData] + public async Task CanReadSecret_AccessToSecretsManagerFalse_DoesNotSucceed( + SutProvider sutProvider, Secret secret, + ClaimsPrincipal claimsPrincipal) + { + var requirement = SecretOperations.Read; + sutProvider.GetDependency().AccessSecretsManager(secret.OrganizationId) + .Returns(false); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, secret); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.False(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData] + public async Task CanReadSecret_NullResource_DoesNotSucceed( + SutProvider sutProvider, Secret secret, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = SecretOperations.Read; + SetupPermission(sutProvider, PermissionType.RunAsAdmin, secret.OrganizationId, userId); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, null); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.False(authzContext.HasSucceeded); + } + + [Theory] + [BitAutoData(PermissionType.RunAsAdmin, true, true, true)] + [BitAutoData(PermissionType.RunAsUserWithPermission, false, false, false)] + [BitAutoData(PermissionType.RunAsUserWithPermission, false, true, false)] + [BitAutoData(PermissionType.RunAsUserWithPermission, true, false, true)] + [BitAutoData(PermissionType.RunAsUserWithPermission, true, true, true)] + [BitAutoData(PermissionType.RunAsServiceAccountWithPermission, false, false, false)] + [BitAutoData(PermissionType.RunAsServiceAccountWithPermission, false, true, false)] + [BitAutoData(PermissionType.RunAsServiceAccountWithPermission, true, false, true)] + [BitAutoData(PermissionType.RunAsServiceAccountWithPermission, true, true, true)] + public async Task CanReadSecret_AccessCheck(PermissionType permissionType, bool read, bool write, + bool expected, + SutProvider sutProvider, Secret secret, + ClaimsPrincipal claimsPrincipal, + Guid userId) + { + var requirement = SecretOperations.Read; + SetupPermission(sutProvider, permissionType, secret.OrganizationId, userId); + sutProvider.GetDependency() + .AccessToSecretAsync(secret.Id, userId, Arg.Any()) + .Returns((read, write)); + var authzContext = new AuthorizationHandlerContext(new List { requirement }, + claimsPrincipal, secret); + + await sutProvider.Sut.HandleAsync(authzContext); + + Assert.Equal(expected, authzContext.HasSucceeded); + } + [Theory] [BitAutoData] public async Task CanUpdateSecret_AccessToSecretsManagerFalse_DoesNotSucceed( diff --git a/src/Api/SecretsManager/Controllers/SecretsController.cs b/src/Api/SecretsManager/Controllers/SecretsController.cs index 99f641a172..a88a784bed 100644 --- a/src/Api/SecretsManager/Controllers/SecretsController.cs +++ b/src/Api/SecretsManager/Controllers/SecretsController.cs @@ -207,4 +207,45 @@ public class SecretsController : Controller var responses = results.Select(r => new BulkDeleteResponseModel(r.Secret.Id, r.Error)); return new ListResponseModel(responses); } + + [HttpPost("secrets/get-by-ids")] + public async Task> GetSecretsByIdsAsync( + [FromBody] GetSecretsRequestModel request) + { + var secrets = (await _secretRepository.GetManyByIds(request.Ids)).ToList(); + if (!secrets.Any() || secrets.Count != request.Ids.Count()) + { + throw new NotFoundException(); + } + + // Ensure all secrets belong to the same organization. + var organizationId = secrets.First().OrganizationId; + if (secrets.Any(secret => secret.OrganizationId != organizationId) || + !_currentContext.AccessSecretsManager(organizationId)) + { + throw new NotFoundException(); + } + + + foreach (var secret in secrets) + { + var authorizationResult = await _authorizationService.AuthorizeAsync(User, secret, SecretOperations.Read); + if (!authorizationResult.Succeeded) + { + throw new NotFoundException(); + } + } + + if (_currentContext.ClientType == ClientType.ServiceAccount) + { + var userId = _userService.GetProperUserId(User).Value; + var org = await _organizationRepository.GetByIdAsync(organizationId); + await _eventService.LogServiceAccountSecretsEventAsync(userId, secrets, EventType.Secret_Retrieved); + await _referenceEventService.RaiseEventAsync( + new ReferenceEvent(ReferenceEventType.SmServiceAccountAccessedSecret, org, _currentContext)); + } + + var responses = secrets.Select(s => new BaseSecretResponseModel(s)); + return new ListResponseModel(responses); + } } diff --git a/src/Api/SecretsManager/Models/Request/GetSecretsRequestModel.cs b/src/Api/SecretsManager/Models/Request/GetSecretsRequestModel.cs new file mode 100644 index 0000000000..42dbce5232 --- /dev/null +++ b/src/Api/SecretsManager/Models/Request/GetSecretsRequestModel.cs @@ -0,0 +1,9 @@ +using System.ComponentModel.DataAnnotations; + +namespace Bit.Api.SecretsManager.Models.Request; + +public class GetSecretsRequestModel +{ + [Required] + public IEnumerable Ids { get; set; } +} diff --git a/src/Api/SecretsManager/Models/Response/BaseSecretResponseModel.cs b/src/Api/SecretsManager/Models/Response/BaseSecretResponseModel.cs new file mode 100644 index 0000000000..0579baec07 --- /dev/null +++ b/src/Api/SecretsManager/Models/Response/BaseSecretResponseModel.cs @@ -0,0 +1,66 @@ +using Bit.Core.Models.Api; +using Bit.Core.SecretsManager.Entities; + +namespace Bit.Api.SecretsManager.Models.Response; + +public class BaseSecretResponseModel : ResponseModel +{ + private const string _objectName = "baseSecret"; + + public BaseSecretResponseModel(Secret secret, string objectName = _objectName) : base(objectName) + { + if (secret == null) + { + throw new ArgumentNullException(nameof(secret)); + } + + Id = secret.Id; + OrganizationId = secret.OrganizationId; + Key = secret.Key; + Value = secret.Value; + Note = secret.Note; + CreationDate = secret.CreationDate; + RevisionDate = secret.RevisionDate; + Projects = secret.Projects?.Select(p => new SecretResponseInnerProject(p)); + } + + public BaseSecretResponseModel(string objectName = _objectName) : base(objectName) + { + } + + public BaseSecretResponseModel() : base(_objectName) + { + } + + public Guid Id { get; set; } + + public Guid OrganizationId { get; set; } + + public string Key { get; set; } + + public string Value { get; set; } + + public string Note { get; set; } + + public DateTime CreationDate { get; set; } + + public DateTime RevisionDate { get; set; } + + public IEnumerable Projects { get; set; } + + public class SecretResponseInnerProject + { + public SecretResponseInnerProject(Project project) + { + Id = project.Id; + Name = project.Name; + } + + public SecretResponseInnerProject() + { + } + + public Guid Id { get; set; } + public string Name { get; set; } + } +} diff --git a/src/Api/SecretsManager/Models/Response/SecretResponseModel.cs b/src/Api/SecretsManager/Models/Response/SecretResponseModel.cs index 9f940a8782..c4633f78f0 100644 --- a/src/Api/SecretsManager/Models/Response/SecretResponseModel.cs +++ b/src/Api/SecretsManager/Models/Response/SecretResponseModel.cs @@ -1,28 +1,13 @@ -using Bit.Core.Models.Api; -using Bit.Core.SecretsManager.Entities; +using Bit.Core.SecretsManager.Entities; namespace Bit.Api.SecretsManager.Models.Response; -public class SecretResponseModel : ResponseModel +public class SecretResponseModel : BaseSecretResponseModel { private const string _objectName = "secret"; - public SecretResponseModel(Secret secret, bool read, bool write) : base(_objectName) + public SecretResponseModel(Secret secret, bool read, bool write) : base(secret, _objectName) { - if (secret == null) - { - throw new ArgumentNullException(nameof(secret)); - } - - Id = secret.Id; - OrganizationId = secret.OrganizationId; - Key = secret.Key; - Value = secret.Value; - Note = secret.Note; - CreationDate = secret.CreationDate; - RevisionDate = secret.RevisionDate; - Projects = secret.Projects?.Select(p => new SecretResponseInnerProject(p)); - Read = read; Write = write; } @@ -31,39 +16,7 @@ public class SecretResponseModel : ResponseModel { } - public Guid Id { get; set; } - - public Guid OrganizationId { get; set; } - - public string Key { get; set; } - - public string Value { get; set; } - - public string Note { get; set; } - - public DateTime CreationDate { get; set; } - - public DateTime RevisionDate { get; set; } - - public IEnumerable Projects { get; set; } - public bool Read { get; set; } public bool Write { get; set; } - - public class SecretResponseInnerProject - { - public SecretResponseInnerProject(Project project) - { - Id = project.Id; - Name = project.Name; - } - - public SecretResponseInnerProject() - { - } - - public Guid Id { get; set; } - public string Name { get; set; } - } } diff --git a/src/Core/SecretsManager/AuthorizationRequirements/SecretOperationRequirement.cs b/src/Core/SecretsManager/AuthorizationRequirements/SecretOperationRequirement.cs index c6956ed306..e737960015 100644 --- a/src/Core/SecretsManager/AuthorizationRequirements/SecretOperationRequirement.cs +++ b/src/Core/SecretsManager/AuthorizationRequirements/SecretOperationRequirement.cs @@ -9,6 +9,7 @@ public class SecretOperationRequirement : OperationAuthorizationRequirement public static class SecretOperations { public static readonly SecretOperationRequirement Create = new() { Name = nameof(Create) }; + public static readonly SecretOperationRequirement Read = new() { Name = nameof(Read) }; public static readonly SecretOperationRequirement Update = new() { Name = nameof(Update) }; public static readonly SecretOperationRequirement Delete = new() { Name = nameof(Delete) }; } diff --git a/src/Core/Services/IEventService.cs b/src/Core/Services/IEventService.cs index 2288d1f926..10d8b6dbd0 100644 --- a/src/Core/Services/IEventService.cs +++ b/src/Core/Services/IEventService.cs @@ -29,4 +29,5 @@ public interface IEventService Task LogOrganizationDomainEventAsync(OrganizationDomain organizationDomain, EventType type, DateTime? date = null); Task LogOrganizationDomainEventAsync(OrganizationDomain organizationDomain, EventType type, EventSystemUser systemUser, DateTime? date = null); Task LogServiceAccountSecretEventAsync(Guid serviceAccountId, Secret secret, EventType type, DateTime? date = null); + Task LogServiceAccountSecretsEventAsync(Guid serviceAccountId, IEnumerable secrets, EventType type, DateTime? date = null); } diff --git a/src/Core/Services/Implementations/EventService.cs b/src/Core/Services/Implementations/EventService.cs index 96bdfe4500..d5dec0a106 100644 --- a/src/Core/Services/Implementations/EventService.cs +++ b/src/Core/Services/Implementations/EventService.cs @@ -406,22 +406,34 @@ public class EventService : IEventService } public async Task LogServiceAccountSecretEventAsync(Guid serviceAccountId, Secret secret, EventType type, DateTime? date = null) + { + await LogServiceAccountSecretsEventAsync(serviceAccountId, new[] { secret }, type, date); + } + + public async Task LogServiceAccountSecretsEventAsync(Guid serviceAccountId, IEnumerable secrets, EventType type, DateTime? date = null) { var orgAbilities = await _applicationCacheService.GetOrganizationAbilitiesAsync(); - if (!CanUseEvents(orgAbilities, secret.OrganizationId)) + var eventMessages = new List(); + + foreach (var secret in secrets) { - return; + if (!CanUseEvents(orgAbilities, secret.OrganizationId)) + { + continue; + } + + var e = new EventMessage(_currentContext) + { + OrganizationId = secret.OrganizationId, + Type = type, + SecretId = secret.Id, + ServiceAccountId = serviceAccountId, + Date = date.GetValueOrDefault(DateTime.UtcNow) + }; + eventMessages.Add(e); } - var e = new EventMessage(_currentContext) - { - OrganizationId = secret.OrganizationId, - Type = type, - SecretId = secret.Id, - ServiceAccountId = serviceAccountId, - Date = date.GetValueOrDefault(DateTime.UtcNow) - }; - await _eventWriteService.CreateAsync(e); + await _eventWriteService.CreateManyAsync(eventMessages); } private async Task GetProviderIdAsync(Guid? orgId) diff --git a/src/Core/Services/NoopImplementations/NoopEventService.cs b/src/Core/Services/NoopImplementations/NoopEventService.cs index 9eaefdab3a..068e03e35c 100644 --- a/src/Core/Services/NoopImplementations/NoopEventService.cs +++ b/src/Core/Services/NoopImplementations/NoopEventService.cs @@ -119,4 +119,10 @@ public class NoopEventService : IEventService { return Task.FromResult(0); } + + public Task LogServiceAccountSecretsEventAsync(Guid serviceAccountId, IEnumerable secrets, EventType type, + DateTime? date = null) + { + return Task.FromResult(0); + } } diff --git a/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs b/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs index 3f847d5f2c..0d937d3433 100644 --- a/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs +++ b/test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs @@ -709,6 +709,69 @@ public class SecretsControllerTests : IClassFixture, IAsy Assert.Empty(secrets); } + [Theory] + [InlineData(false, false)] + [InlineData(true, false)] + [InlineData(false, true)] + public async Task GetSecretsByIds_SmNotEnabled_NotFound(bool useSecrets, bool accessSecrets) + { + var (org, _) = await _organizationHelper.Initialize(useSecrets, accessSecrets); + await LoginAsync(_email); + + var secret = await _secretRepository.CreateAsync(new Secret + { + OrganizationId = org.Id, + Key = _mockEncryptedString, + Value = _mockEncryptedString, + Note = _mockEncryptedString, + }); + + var request = new GetSecretsRequestModel { Ids = new[] { secret.Id } }; + + var response = await _client.PostAsJsonAsync("/secrets/get-by-ids", request); + Assert.Equal(HttpStatusCode.NotFound, response.StatusCode); + } + + [Theory] + [InlineData(PermissionType.RunAsAdmin)] + [InlineData(PermissionType.RunAsUserWithPermission)] + public async Task GetSecretsByIds_Success(PermissionType permissionType) + { + var (org, _) = await _organizationHelper.Initialize(true, true); + await LoginAsync(_email); + + var (project, secretIds) = await CreateSecretsAsync(org.Id); + + if (permissionType == PermissionType.RunAsUserWithPermission) + { + var (email, orgUser) = await _organizationHelper.CreateNewUser(OrganizationUserType.User, true); + await LoginAsync(email); + + var accessPolicies = new List + { + new UserProjectAccessPolicy + { + GrantedProjectId = project.Id, OrganizationUserId = orgUser.Id, Read = true, Write = true, + }, + }; + await _accessPolicyRepository.CreateManyAsync(accessPolicies); + } + else + { + var (email, _) = await _organizationHelper.CreateNewUser(OrganizationUserType.Admin, true); + await LoginAsync(email); + } + + var request = new GetSecretsRequestModel { Ids = secretIds }; + + var response = await _client.PostAsJsonAsync("/secrets/get-by-ids", request); + response.EnsureSuccessStatusCode(); + var result = await response.Content.ReadFromJsonAsync>(); + Assert.NotNull(result); + Assert.NotEmpty(result!.Data); + Assert.Equal(secretIds.Count, result!.Data.Count()); + } + private async Task<(Project Project, List secretIds)> CreateSecretsAsync(Guid orgId, int numberToCreate = 3) { var project = await _projectRepository.CreateAsync(new Project diff --git a/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs b/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs index 8afa2000a4..373bf8b183 100644 --- a/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs +++ b/test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs @@ -346,4 +346,105 @@ public class SecretsControllerTests Assert.Null(result.Error); } } + + [Theory] + [BitAutoData] + public async void GetSecretsByIds_NoSecretsFound_ThrowsNotFound(SutProvider sutProvider, + List data) + { + var (ids, request) = BuildGetSecretsRequestModel(data); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(new List()); + await Assert.ThrowsAsync(() => sutProvider.Sut.GetSecretsByIdsAsync(request)); + } + + [Theory] + [BitAutoData] + public async void GetSecretsByIds_SecretsFoundMisMatch_ThrowsNotFound(SutProvider sutProvider, + List data, Secret mockSecret) + { + var (ids, request) = BuildGetSecretsRequestModel(data); + ids.Add(mockSecret.Id); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)) + .ReturnsForAnyArgs(new List { mockSecret }); + await Assert.ThrowsAsync(() => sutProvider.Sut.GetSecretsByIdsAsync(request)); + } + + [Theory] + [BitAutoData] + public async void GetSecretsByIds_OrganizationMisMatch_ThrowsNotFound(SutProvider sutProvider, + List data) + { + var (ids, request) = BuildGetSecretsRequestModel(data); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data); + await Assert.ThrowsAsync(() => sutProvider.Sut.GetSecretsByIdsAsync(request)); + } + + [Theory] + [BitAutoData] + public async void GetSecretsByIds_NoAccessToSecretsManager_ThrowsNotFound( + SutProvider sutProvider, List data) + { + var (ids, request) = BuildGetSecretsRequestModel(data); + var organizationId = SetOrganizations(ref data); + + sutProvider.GetDependency().AccessSecretsManager(Arg.Is(organizationId)) + .ReturnsForAnyArgs(false); + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data); + await Assert.ThrowsAsync(() => sutProvider.Sut.GetSecretsByIdsAsync(request)); + } + + [Theory] + [BitAutoData] + public async void GetSecretsByIds_AccessDenied_ThrowsNotFound(SutProvider sutProvider, + List data) + { + var (ids, request) = BuildGetSecretsRequestModel(data); + var organizationId = SetOrganizations(ref data); + + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data); + sutProvider.GetDependency().AccessSecretsManager(Arg.Is(organizationId)) + .ReturnsForAnyArgs(true); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), data.First(), + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Failed()); + + await Assert.ThrowsAsync(() => sutProvider.Sut.GetSecretsByIdsAsync(request)); + } + + [Theory] + [BitAutoData] + public async void GetSecretsByIds_Success(SutProvider sutProvider, List data) + { + var (ids, request) = BuildGetSecretsRequestModel(data); + var organizationId = SetOrganizations(ref data); + + sutProvider.GetDependency().GetManyByIds(Arg.Is(ids)).ReturnsForAnyArgs(data); + sutProvider.GetDependency().AccessSecretsManager(Arg.Is(organizationId)) + .ReturnsForAnyArgs(true); + sutProvider.GetDependency() + .AuthorizeAsync(Arg.Any(), data.First(), + Arg.Any>()).ReturnsForAnyArgs(AuthorizationResult.Success()); + + var results = await sutProvider.Sut.GetSecretsByIdsAsync(request); + Assert.Equal(data.Count, results.Data.Count()); + } + + private static (List Ids, GetSecretsRequestModel request) BuildGetSecretsRequestModel( + IEnumerable data) + { + var ids = data.Select(s => s.Id).ToList(); + var request = new GetSecretsRequestModel { Ids = ids }; + return (ids, request); + } + + private static Guid SetOrganizations(ref List data) + { + var organizationId = data.First().OrganizationId; + foreach (var s in data) + { + s.OrganizationId = organizationId; + } + + return organizationId; + } }