SM-695: Block Create & Update for Admins on Secrets Outside of the Org (#2844)

* SM-695: Block create or update for admins on secrets outside of the org

* SM-695: Update test, org is required on project

* SM-695: Update tests to set matching org id in project

* SM-695: Ensure there is no more than 1 project connected to a secret, plus remove org admin check in the CreateSecretCommand.

* SM-695: Add integration tests for create and update secrets security fixes

* SM-695: Update Create and Update secret tests, a secret can only be in one project at a time

bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/CreateSecretCommand.cs

@@ -31,6 +31,11 @@ public class CreateSecretCommand : ICreateSecretCommand
             throw new NotFoundException();
         }
 
+        if (secret.Projects != null && secret.Projects.Any() && !(await _projectRepository.ProjectsAreInOrganization(secret.Projects.Select(p => p.Id).ToList(), secret.OrganizationId)))
+        {
+            throw new NotFoundException();
+        }
+
         var hasAccess = accessClient switch
         {
             AccessClientType.NoAccessCheck => true,

bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs

@@ -28,6 +28,11 @@ public class UpdateSecretCommand : IUpdateSecretCommand
             throw new NotFoundException();
         }
 
+        if (updatedSecret.Projects != null && updatedSecret.Projects.Any() && !(await _projectRepository.ProjectsAreInOrganization(updatedSecret.Projects.Select(p => p.Id).ToList(), secret.OrganizationId)))
+        {
+            throw new NotFoundException();
+        }
+
         var orgAdmin = await _currentContext.OrganizationAdmin(secret.OrganizationId);
         var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin);
 

bitwarden_license/src/Commercial.Infrastructure.EntityFramework/SecretsManager/Repositories/ProjectRepository.cs

@@ -190,4 +190,13 @@ public class ProjectRepository : Repository<Core.SecretsManager.Entities.Project
 
         return (policy.Read, policy.Write);
     }
+
+    public async Task<bool> ProjectsAreInOrganization(List<Guid> projectIds, Guid organizationId)
+    {
+        using var scope = ServiceScopeFactory.CreateScope();
+        var dbContext = GetDatabaseContext(scope);
+        var results = await dbContext.Project.Where(p => p.OrganizationId == organizationId && projectIds.Contains(p.Id)).ToListAsync();
+
+        return projectIds.Count == results.Count;
+    }
 }

bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/CreateSecretCommandTests.cs

@@ -22,6 +22,9 @@ public class CreateSecretCommandTests
     public async Task CreateAsync_Success(PermissionType permissionType, Secret data,
       SutProvider<CreateSecretCommand> sutProvider, Guid userId, Project mockProject)
     {
+        sutProvider.GetDependency<IProjectRepository>().ProjectsAreInOrganization(default, default).ReturnsForAnyArgs(true);
+
+        mockProject.OrganizationId = data.OrganizationId;
         data.Projects = new List<Project>() { mockProject };
 
         if (permissionType == PermissionType.RunAsAdmin)

bitwarden_license/test/Commercial.Core.Test/SecretsManager/Secrets/UpdateSecretCommandTests.cs

@@ -34,6 +34,9 @@ public class UpdateSecretCommandTests
     public async Task UpdateAsync_Success(PermissionType permissionType, Secret data, SutProvider<UpdateSecretCommand> sutProvider, Guid userId, Project mockProject)
     {
         sutProvider.GetDependency<ICurrentContext>().AccessSecretsManager(data.OrganizationId).Returns(true);
+        sutProvider.GetDependency<IProjectRepository>().ProjectsAreInOrganization(default, default).ReturnsForAnyArgs(true);
+
+        mockProject.OrganizationId = data.OrganizationId;
         data.Projects = new List<Project>() { mockProject };
 
         if (permissionType == PermissionType.RunAsAdmin)

src/Api/SecretsManager/Controllers/SecretsController.cs

@@ -80,6 +80,11 @@ public class SecretsController : Controller
             throw new NotFoundException();
         }
 
+        if (createRequest.ProjectIds != null && createRequest.ProjectIds.Length > 1)
+        {
+            throw new BadRequestException();
+        }
+
         var userId = _userService.GetProperUserId(User).Value;
         var result = await _createSecretCommand.CreateAsync(createRequest.ToSecret(organizationId), userId);
 
@@ -140,6 +145,11 @@ public class SecretsController : Controller
     [HttpPut("secrets/{id}")]
     public async Task<SecretResponseModel> UpdateSecretAsync([FromRoute] Guid id, [FromBody] SecretUpdateRequestModel updateRequest)
     {
+        if (updateRequest.ProjectIds != null && updateRequest.ProjectIds.Length > 1)
+        {
+            throw new BadRequestException();
+        }
+
         var userId = _userService.GetProperUserId(User).Value;
         var secret = updateRequest.ToSecret(id);
         var result = await _updateSecretCommand.UpdateAsync(secret, userId);

src/Core/SecretsManager/Repositories/IProjectRepository.cs

@@ -18,4 +18,5 @@ public interface IProjectRepository
     Task<bool> ServiceAccountHasWriteAccessToProject(Guid id, Guid userId);
     Task<bool> ServiceAccountHasReadAccessToProject(Guid id, Guid userId);
     Task<(bool Read, bool Write)> AccessToProjectAsync(Guid id, Guid userId, AccessClientType accessType);
+    Task<bool> ProjectsAreInOrganization(List<Guid> projectIds, Guid organizationId);
 }

test/Api.IntegrationTest/SecretsManager/Controllers/SecretsControllerTests.cs

@@ -148,7 +148,7 @@ public class SecretsControllerTests : IClassFixture<ApiApplicationFactory>, IAsy
         var (org, _) = await _organizationHelper.Initialize(true, true);
         await LoginAsync(_email);
 
-        var project = await _projectRepository.CreateAsync(new Project { Name = "123" });
+        var project = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123" });
 
         var request = new SecretCreateRequestModel
         {
@@ -179,6 +179,47 @@ public class SecretsControllerTests : IClassFixture<ApiApplicationFactory>, IAsy
         Assert.Null(createdSecret.DeletedDate);
     }
 
+    [Fact]
+    public async Task CreateWithDifferentProjectOrgId_RunAsAdmin_NotFound()
+    {
+        var (org, _) = await _organizationHelper.Initialize(true, true);
+        await LoginAsync(_email);
+
+        var project = await _projectRepository.CreateAsync(new Project { Name = "123" });
+
+        var request = new SecretCreateRequestModel
+        {
+            ProjectIds = new Guid[] { project.Id },
+            Key = _mockEncryptedString,
+            Value = _mockEncryptedString,
+            Note = _mockEncryptedString,
+        };
+
+        var response = await _client.PostAsJsonAsync($"/organizations/{org.Id}/secrets", request);
+        Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task CreateWithMultipleProjects_RunAsAdmin_BadRequest()
+    {
+        var (org, _) = await _organizationHelper.Initialize(true, true);
+        await LoginAsync(_email);
+
+        var projectA = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123A" });
+        var projectB = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123B" });
+
+        var request = new SecretCreateRequestModel
+        {
+            ProjectIds = new Guid[] { projectA.Id, projectB.Id },
+            Key = _mockEncryptedString,
+            Value = _mockEncryptedString,
+            Note = _mockEncryptedString,
+        };
+
+        var response = await _client.PostAsJsonAsync($"/organizations/{org.Id}/secrets", request);
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
     [Fact]
     public async Task CreateWithoutProject_RunAsUser_NotFound()
     {
@@ -531,6 +572,63 @@ public class SecretsControllerTests : IClassFixture<ApiApplicationFactory>, IAsy
         Assert.NotEqual(secret.RevisionDate, updatedSecret.RevisionDate);
     }
 
+    [Fact]
+    public async Task UpdateWithDifferentProjectOrgId_RunAsAdmin_NotFound()
+    {
+        var (org, _) = await _organizationHelper.Initialize(true, true);
+        await LoginAsync(_email);
+
+        var project = await _projectRepository.CreateAsync(new Project { Name = "123" });
+
+        var secret = await _secretRepository.CreateAsync(new Secret
+        {
+            OrganizationId = org.Id,
+            Key = _mockEncryptedString,
+            Value = _mockEncryptedString,
+            Note = _mockEncryptedString
+        });
+
+        var request = new SecretUpdateRequestModel
+        {
+            Key = _mockEncryptedString,
+            Value = "2.3Uk+WNBIoU5xzmVFNcoWzz==|1MsPIYuRfdOHfu/0uY6H2Q==|/98xy4wb6pHP1VTZ9JcNCYgQjEUMFPlqJgCwRk1YXKg=",
+            Note = _mockEncryptedString,
+            ProjectIds = new Guid[] { project.Id },
+        };
+
+        var response = await _client.PutAsJsonAsync($"/secrets/{secret.Id}", request);
+        Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
+    }
+
+    [Fact]
+    public async Task UpdateWithMultipleProjects_BadRequest()
+    {
+        var (org, _) = await _organizationHelper.Initialize(true, true);
+        await LoginAsync(_email);
+
+        var projectA = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123A" });
+        var projectB = await _projectRepository.CreateAsync(new Project { OrganizationId = org.Id, Name = "123B" });
+
+        var secret = await _secretRepository.CreateAsync(new Secret
+        {
+            OrganizationId = org.Id,
+            Key = _mockEncryptedString,
+            Value = _mockEncryptedString,
+            Note = _mockEncryptedString
+        });
+
+        var request = new SecretUpdateRequestModel
+        {
+            Key = _mockEncryptedString,
+            Value = "2.3Uk+WNBIoU5xzmVFNcoWzz==|1MsPIYuRfdOHfu/0uY6H2Q==|/98xy4wb6pHP1VTZ9JcNCYgQjEUMFPlqJgCwRk1YXKg=",
+            Note = _mockEncryptedString,
+            ProjectIds = new Guid[] { projectA.Id, projectB.Id },
+        };
+
+        var response = await _client.PutAsJsonAsync($"/secrets/{secret.Id}", request);
+        Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode);
+    }
+
     [Theory]
     [InlineData(false, false)]
     [InlineData(true, false)]

test/Api.Test/SecretsManager/Controllers/SecretsControllerTests.cs

@@ -125,6 +125,12 @@ public class SecretsControllerTests
     [BitAutoData(PermissionType.RunAsUserWithPermission)]
     public async void CreateSecret_Success(PermissionType permissionType, SutProvider<SecretsController> sutProvider, SecretCreateRequestModel data, Guid organizationId, Project mockProject, Guid userId)
     {
+        // We currently only allow a secret to be in one project at a time
+        if (data.ProjectIds != null && data.ProjectIds.Length > 1)
+        {
+            data.ProjectIds = new Guid[] { data.ProjectIds.ElementAt(0) };
+        }
+
         var resultSecret = data.ToSecret(organizationId);
         sutProvider.GetDependency<IUserService>().GetProperUserId(default).ReturnsForAnyArgs(userId);
 
@@ -152,6 +158,12 @@ public class SecretsControllerTests
     [BitAutoData(PermissionType.RunAsUserWithPermission)]
     public async void UpdateSecret_Success(PermissionType permissionType, SutProvider<SecretsController> sutProvider, SecretUpdateRequestModel data, Guid secretId, Guid organizationId, Guid userId, Project mockProject)
     {
+        // We currently only allow a secret to be in one project at a time
+        if (data.ProjectIds != null && data.ProjectIds.Length > 1)
+        {
+            data.ProjectIds = new Guid[] { data.ProjectIds.ElementAt(0) };
+        }
+
         sutProvider.GetDependency<IUserService>().GetProperUserId(default).ReturnsForAnyArgs(userId);
 
         if (permissionType == PermissionType.RunAsAdmin)