[SM-574] Wire up read/write for secret list and secret response (#2767)

* Wire up read/write for secret list and secret response

* Fix trash

* Remove UserHasReadPermission

* Fix list by project

* Implement admin and service accounts for AccessToSecretAsync

* Resolve feedback

* Fix tests

* Rename function

* Change create to return true, true

* Remove duplicated access check

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

@@ -1,6 +1,7 @@
 using System.Linq.Expressions;
 using AutoMapper;
 using Bit.Core.Enums;
+using Bit.Core.SecretsManager.Models.Data;
 using Bit.Core.SecretsManager.Repositories;
 using Bit.Infrastructure.EntityFramework;
 using Bit.Infrastructure.EntityFramework.Repositories;
@@ -35,40 +36,25 @@ public class SecretRepository : Repository<Core.SecretsManager.Entities.Secret,
         {
             var dbContext = GetDatabaseContext(scope);
             var secrets = await dbContext.Secret
-                                    .Where(c => ids.Contains(c.Id) && c.DeletedDate == null)
+                .Where(c => ids.Contains(c.Id) && c.DeletedDate == null)
-                                    .Include(c => c.Projects)
+                .Include(c => c.Projects)
-                                    .ToListAsync();
+                .ToListAsync();
             return Mapper.Map<List<Core.SecretsManager.Entities.Secret>>(secrets);
         }
     }
 
-    private static Expression<Func<Secret, bool>> ServiceAccountHasReadAccessToSecret(Guid serviceAccountId) => s =>
+    public async Task<IEnumerable<SecretPermissionDetails>> GetManyByOrganizationIdAsync(Guid organizationId, Guid userId, AccessClientType accessType)
-    s.Projects.Any(p =>
-        p.ServiceAccountAccessPolicies.Any(ap => ap.ServiceAccount.Id == serviceAccountId && ap.Read));
-
-    private static Expression<Func<Secret, bool>> UserHasReadAccessToSecret(Guid userId) => s =>
-        s.Projects.Any(p =>
-            p.UserAccessPolicies.Any(ap => ap.OrganizationUser.UserId == userId && ap.Read) ||
-            p.GroupAccessPolicies.Any(ap =>
-                ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.UserId == userId && ap.Read)));
-
-
-    public async Task<IEnumerable<Core.SecretsManager.Entities.Secret>> GetManyByOrganizationIdAsync(Guid organizationId, Guid userId, AccessClientType accessType)
     {
         using var scope = ServiceScopeFactory.CreateScope();
         var dbContext = GetDatabaseContext(scope);
-        var query = dbContext.Secret.Include(c => c.Projects).Where(c => c.OrganizationId == organizationId && c.DeletedDate == null);
+        var query = dbContext.Secret
+            .Include(c => c.Projects)
+            .Where(c => c.OrganizationId == organizationId && c.DeletedDate == null)
+            .OrderBy(s => s.RevisionDate);
 
-        query = accessType switch
+        var secrets = SecretToPermissionDetails(query, userId, accessType);
-        {
-            AccessClientType.NoAccessCheck => query,
-            AccessClientType.User => query.Where(UserHasReadAccessToSecret(userId)),
-            AccessClientType.ServiceAccount => query.Where(ServiceAccountHasReadAccessToSecret(userId)),
-            _ => throw new ArgumentOutOfRangeException(nameof(accessType), accessType, null),
-        };
 
-        var secrets = await query.OrderBy(c => c.RevisionDate).ToListAsync();
+        return await secrets.ToListAsync();
-        return Mapper.Map<List<Core.SecretsManager.Entities.Secret>>(secrets);
     }
 
     public async Task<IEnumerable<Core.SecretsManager.Entities.Secret>> GetManyByOrganizationIdInTrashByIdsAsync(Guid organizationId, IEnumerable<Guid> ids)
@@ -86,7 +72,7 @@ public class SecretRepository : Repository<Core.SecretsManager.Entities.Secret,
         }
     }
 
-    public async Task<IEnumerable<Core.SecretsManager.Entities.Secret>> GetManyByOrganizationIdInTrashAsync(Guid organizationId)
+    public async Task<IEnumerable<SecretPermissionDetails>> GetManyByOrganizationIdInTrashAsync(Guid organizationId)
     {
         using (var scope = ServiceScopeFactory.CreateScope())
         {
@@ -97,29 +83,26 @@ public class SecretRepository : Repository<Core.SecretsManager.Entities.Secret,
                                     .OrderBy(c => c.RevisionDate)
                                     .ToListAsync();
 
-            return Mapper.Map<List<Core.SecretsManager.Entities.Secret>>(secrets);
+            // This should be changed if/when we allow non admins to access trashed items
+            return Mapper.Map<List<Core.SecretsManager.Entities.Secret>>(secrets).Select(s => new SecretPermissionDetails
+            {
+                Secret = s,
+                Read = true,
+                Write = true,
+            });
         }
     }
 
-    public async Task<IEnumerable<Core.SecretsManager.Entities.Secret>> GetManyByProjectIdAsync(Guid projectId, Guid userId, AccessClientType accessType)
+    public async Task<IEnumerable<SecretPermissionDetails>> GetManyByProjectIdAsync(Guid projectId, Guid userId, AccessClientType accessType)
     {
-        using (var scope = ServiceScopeFactory.CreateScope())
+        using var scope = ServiceScopeFactory.CreateScope();
-        {
+        var dbContext = GetDatabaseContext(scope);
-            var dbContext = GetDatabaseContext(scope);
+        var query = dbContext.Secret.Include(s => s.Projects)
-            var query = dbContext.Secret.Include(s => s.Projects)
+            .Where(s => s.Projects.Any(p => p.Id == projectId) && s.DeletedDate == null);
-                .Where(s => s.Projects.Any(p => p.Id == projectId) && s.DeletedDate == null);
 
-            query = accessType switch
+        var secrets = SecretToPermissionDetails(query, userId, accessType);
-            {
-                AccessClientType.NoAccessCheck => query,
-                AccessClientType.User => query.Where(UserHasReadAccessToSecret(userId)),
-                AccessClientType.ServiceAccount => query.Where(ServiceAccountHasReadAccessToSecret(userId)),
-                _ => throw new ArgumentOutOfRangeException(nameof(accessType), accessType, null),
-            };
 
-            var secrets = await query.OrderBy(s => s.RevisionDate).ToListAsync();
+        return await secrets.ToListAsync();
-            return Mapper.Map<List<Core.SecretsManager.Entities.Secret>>(secrets);
-        }
     }
 
     public override async Task<Core.SecretsManager.Entities.Secret> CreateAsync(Core.SecretsManager.Entities.Secret secret)
@@ -272,4 +255,85 @@ public class SecretRepository : Repository<Core.SecretsManager.Entities.Secret,
             await dbContext.SaveChangesAsync();
         }
     }
+
+    public async Task<(bool Read, bool Write)> AccessToSecretAsync(Guid id, Guid userId, AccessClientType accessType)
+    {
+        using var scope = ServiceScopeFactory.CreateScope();
+        var dbContext = GetDatabaseContext(scope);
+
+        var secret = dbContext.Secret
+            .Where(s => s.Id == id);
+
+        var query = accessType switch
+        {
+            AccessClientType.NoAccessCheck => secret.Select(_ => new { Read = true, Write = true }),
+            AccessClientType.User => secret.Select(s => new
+            {
+                Read = s.Projects.Any(p =>
+                    p.UserAccessPolicies.Any(ap => ap.OrganizationUser.User.Id == userId && ap.Read) ||
+                    p.GroupAccessPolicies.Any(ap =>
+                        ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.User.Id == userId && ap.Read))),
+                Write = s.Projects.Any(p =>
+                    p.UserAccessPolicies.Any(ap => ap.OrganizationUser.User.Id == userId && ap.Write) ||
+                    p.GroupAccessPolicies.Any(ap =>
+                        ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.User.Id == userId && ap.Write))),
+            }),
+            AccessClientType.ServiceAccount => secret.Select(s => new
+            {
+                Read = s.Projects.Any(p =>
+                    p.ServiceAccountAccessPolicies.Any(ap => ap.ServiceAccountId == userId && ap.Read)),
+                Write = s.Projects.Any(p =>
+                    p.ServiceAccountAccessPolicies.Any(ap => ap.ServiceAccountId == userId && ap.Write)),
+            }),
+            _ => secret.Select(_ => new { Read = false, Write = false }),
+        };
+
+        var policy = await query.FirstOrDefaultAsync();
+
+        return (policy.Read, policy.Write);
+    }
+
+    private IQueryable<SecretPermissionDetails> SecretToPermissionDetails(IQueryable<Secret> query, Guid userId, AccessClientType accessType)
+    {
+        var secrets = accessType switch
+        {
+            AccessClientType.NoAccessCheck => query.Select(s => new SecretPermissionDetails
+            {
+                Secret = Mapper.Map<Bit.Core.SecretsManager.Entities.Secret>(s),
+                Read = true,
+                Write = true,
+            }),
+            AccessClientType.User => query.Where(UserHasReadAccessToSecret(userId)).Select(SecretToPermissionsUser(userId, true)),
+            AccessClientType.ServiceAccount => query.Where(ServiceAccountHasReadAccessToSecret(userId)).Select(s =>
+                new SecretPermissionDetails
+                {
+                    Secret = Mapper.Map<Bit.Core.SecretsManager.Entities.Secret>(s),
+                    Read = true,
+                    Write = false,
+                }),
+            _ => throw new ArgumentOutOfRangeException(nameof(accessType), accessType, null),
+        };
+        return secrets;
+    }
+
+    private Expression<Func<Secret, SecretPermissionDetails>> SecretToPermissionsUser(Guid userId, bool read) =>
+        s => new SecretPermissionDetails
+        {
+            Secret = Mapper.Map<Bit.Core.SecretsManager.Entities.Secret>(s),
+            Read = read,
+            Write = s.Projects.Any(p =>
+                p.UserAccessPolicies.Any(ap => ap.OrganizationUser.User.Id == userId && ap.Write) ||
+                p.GroupAccessPolicies.Any(ap =>
+                    ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.User.Id == userId && ap.Write))),
+        };
+
+    private static Expression<Func<Secret, bool>> ServiceAccountHasReadAccessToSecret(Guid serviceAccountId) => s =>
+        s.Projects.Any(p =>
+            p.ServiceAccountAccessPolicies.Any(ap => ap.ServiceAccount.Id == serviceAccountId && ap.Read));
+
+    private static Expression<Func<Secret, bool>> UserHasReadAccessToSecret(Guid userId) => s =>
+        s.Projects.Any(p =>
+            p.UserAccessPolicies.Any(ap => ap.OrganizationUser.UserId == userId && ap.Read) ||
+            p.GroupAccessPolicies.Any(ap =>
+                ap.Group.GroupUsers.Any(gu => gu.OrganizationUser.UserId == userId && ap.Read)));
 }

src/Api/SecretsManager/Controllers/SecretsController.cs

@@ -8,7 +8,6 @@ using Bit.Core.Identity;
 using Bit.Core.Models.Business;
 using Bit.Core.Repositories;
 using Bit.Core.SecretsManager.Commands.Secrets.Interfaces;
-using Bit.Core.SecretsManager.Entities;
 using Bit.Core.SecretsManager.Repositories;
 using Bit.Core.Services;
 using Microsoft.AspNetCore.Authorization;
@@ -83,7 +82,9 @@ public class SecretsController : Controller
 
         var userId = _userService.GetProperUserId(User).Value;
         var result = await _createSecretCommand.CreateAsync(createRequest.ToSecret(organizationId), userId);
-        return new SecretResponseModel(result);
+
+        // Creating a secret means you have read & write permission.
+        return new SecretResponseModel(result, true, true);
     }
 
     [HttpGet("secrets/{id}")]
@@ -96,21 +97,26 @@ public class SecretsController : Controller
             throw new NotFoundException();
         }
 
-        if (!await UserHasReadAccessToSecret(secret))
+        var userId = _userService.GetProperUserId(User).Value;
+        var orgAdmin = await _currentContext.OrganizationAdmin(secret.OrganizationId);
+        var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin);
+
+        var access = await _secretRepository.AccessToSecretAsync(id, userId, accessClient);
+
+        if (!access.Read)
         {
             throw new NotFoundException();
         }
 
         if (_currentContext.ClientType == ClientType.ServiceAccount)
         {
-            var userId = _userService.GetProperUserId(User).Value;
             await _eventService.LogServiceAccountSecretEventAsync(userId, secret, EventType.Secret_Retrieved);
 
             var org = await _organizationRepository.GetByIdAsync(secret.OrganizationId);
             await _referenceEventService.RaiseEventAsync(new ReferenceEvent(ReferenceEventType.SmServiceAccountAccessedSecret, org));
         }
 
-        return new SecretResponseModel(secret);
+        return new SecretResponseModel(secret, access.Read, access.Write);
     }
 
     [HttpGet("projects/{projectId}/secrets")]
@@ -137,7 +143,9 @@ public class SecretsController : Controller
         var userId = _userService.GetProperUserId(User).Value;
         var secret = updateRequest.ToSecret(id);
         var result = await _updateSecretCommand.UpdateAsync(secret, userId);
-        return new SecretResponseModel(result);
+
+        // Updating a secret means you have read & write permission.
+        return new SecretResponseModel(result, true, true);
     }
 
     [HttpPost("secrets/delete")]
@@ -148,26 +156,4 @@ public class SecretsController : Controller
         var responses = results.Select(r => new BulkDeleteResponseModel(r.Item1.Id, r.Item2));
         return new ListResponseModel<BulkDeleteResponseModel>(responses);
     }
-
-    public async Task<bool> UserHasReadAccessToSecret(Secret secret)
-    {
-        var userId = _userService.GetProperUserId(User).Value;
-        var orgAdmin = await _currentContext.OrganizationAdmin(secret.OrganizationId);
-        var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin);
-        var hasAccess = orgAdmin;
-
-        if (secret.Projects?.Count > 0)
-        {
-            Guid projectId = secret.Projects.FirstOrDefault().Id;
-            hasAccess = accessClient switch
-            {
-                AccessClientType.NoAccessCheck => true,
-                AccessClientType.User => await _projectRepository.UserHasReadAccessToProject(projectId, userId),
-                AccessClientType.ServiceAccount => await _projectRepository.ServiceAccountHasReadAccessToProject(projectId, userId),
-                _ => false,
-            };
-        }
-
-        return hasAccess;
-    }
 }

src/Api/SecretsManager/Controllers/SecretsManagerPortingController.cs

@@ -47,7 +47,7 @@ public class SecretsManagerPortingController : Controller
             throw new NotFoundException();
         }
 
-        return new SMExportResponseModel(projects, secrets);
+        return new SMExportResponseModel(projects, secrets.Select(s => s.Secret));
     }
 
     [HttpPost("sm/{organizationId}/import")]

src/Api/SecretsManager/Models/Response/SecretResponseModel.cs

@@ -7,7 +7,7 @@ public class SecretResponseModel : ResponseModel
 {
     private const string _objectName = "secret";
 
-    public SecretResponseModel(Secret secret) : base(_objectName)
+    public SecretResponseModel(Secret secret, bool read, bool write) : base(_objectName)
     {
         if (secret == null)
         {
@@ -22,6 +22,9 @@ public class SecretResponseModel : ResponseModel
         CreationDate = secret.CreationDate;
         RevisionDate = secret.RevisionDate;
         Projects = secret.Projects?.Select(p => new InnerProject(p));
+
+        Read = read;
+        Write = write;
     }
 
     public SecretResponseModel() : base(_objectName)
@@ -44,6 +47,10 @@ public class SecretResponseModel : ResponseModel
 
     public IEnumerable<InnerProject> Projects { get; set; }
 
+    public bool Read { get; set; }
+
+    public bool Write { get; set; }
+
     public class InnerProject
     {
         public InnerProject(Project project)

src/Api/SecretsManager/Models/Response/SecretWithProjectsListResponseModel.cs

@@ -1,5 +1,6 @@
 using Bit.Core.Models.Api;
 using Bit.Core.SecretsManager.Entities;
+using Bit.Core.SecretsManager.Models.Data;
 
 namespace Bit.Api.SecretsManager.Models.Response;
 
@@ -7,10 +8,10 @@ public class SecretWithProjectsListResponseModel : ResponseModel
 {
     private const string _objectName = "SecretsWithProjectsList";
 
-    public SecretWithProjectsListResponseModel(IEnumerable<Secret> secrets) : base(_objectName)
+    public SecretWithProjectsListResponseModel(IEnumerable<SecretPermissionDetails> secrets) : base(_objectName)
     {
         Secrets = secrets.Select(s => new InnerSecret(s));
-        Projects = secrets.SelectMany(s => s.Projects).DistinctBy(p => p.Id).Select(p => new InnerProject(p));
+        Projects = secrets.SelectMany(s => s.Secret.Projects).DistinctBy(p => p.Id).Select(p => new InnerProject(p));
     }
 
     public SecretWithProjectsListResponseModel() : base(_objectName)
@@ -38,14 +39,16 @@ public class SecretWithProjectsListResponseModel : ResponseModel
 
     public class InnerSecret
     {
-        public InnerSecret(Secret secret)
+        public InnerSecret(SecretPermissionDetails secret)
         {
-            Id = secret.Id.ToString();
+            Id = secret.Secret.Id.ToString();
-            OrganizationId = secret.OrganizationId.ToString();
+            OrganizationId = secret.Secret.OrganizationId.ToString();
-            Key = secret.Key;
+            Key = secret.Secret.Key;
-            CreationDate = secret.CreationDate;
+            CreationDate = secret.Secret.CreationDate;
-            RevisionDate = secret.RevisionDate;
+            RevisionDate = secret.Secret.RevisionDate;
-            Projects = secret.Projects?.Select(p => new InnerProject(p));
+            Projects = secret.Secret.Projects?.Select(p => new InnerProject(p));
+            Read = secret.Read;
+            Write = secret.Write;
         }
 
         public InnerSecret()
@@ -63,6 +66,8 @@ public class SecretWithProjectsListResponseModel : ResponseModel
         public DateTime RevisionDate { get; set; }
 
         public IEnumerable<InnerProject> Projects { get; set; }
+        public bool Read { get; set; }
+        public bool Write { get; set; }
     }
 }
 

src/Core/SecretsManager/Models/Data/SecretPermissionDetails.cs

@@ -0,0 +1,10 @@
+using Bit.Core.SecretsManager.Entities;
+
+namespace Bit.Core.SecretsManager.Models.Data;
+
+public class SecretPermissionDetails
+{
+    public Secret Secret;
+    public bool Read { get; set; }
+    public bool Write { get; set; }
+}

src/Core/SecretsManager/Repositories/ISecretRepository.cs

@@ -1,15 +1,16 @@
 using Bit.Core.Enums;
 using Bit.Core.SecretsManager.Entities;
+using Bit.Core.SecretsManager.Models.Data;
 
 namespace Bit.Core.SecretsManager.Repositories;
 
 public interface ISecretRepository
 {
-    Task<IEnumerable<Secret>> GetManyByOrganizationIdAsync(Guid organizationId, Guid userId, AccessClientType accessType);
+    Task<IEnumerable<SecretPermissionDetails>> GetManyByOrganizationIdAsync(Guid organizationId, Guid userId, AccessClientType accessType);
-    Task<IEnumerable<Secret>> GetManyByOrganizationIdInTrashAsync(Guid organizationId);
+    Task<IEnumerable<SecretPermissionDetails>> GetManyByOrganizationIdInTrashAsync(Guid organizationId);
     Task<IEnumerable<Secret>> GetManyByOrganizationIdInTrashByIdsAsync(Guid organizationId, IEnumerable<Guid> ids);
     Task<IEnumerable<Secret>> GetManyByIds(IEnumerable<Guid> ids);
-    Task<IEnumerable<Secret>> GetManyByProjectIdAsync(Guid projectId, Guid userId, AccessClientType accessType);
+    Task<IEnumerable<SecretPermissionDetails>> GetManyByProjectIdAsync(Guid projectId, Guid userId, AccessClientType accessType);
     Task<Secret> GetByIdAsync(Guid id);
     Task<Secret> CreateAsync(Secret secret);
     Task<Secret> UpdateAsync(Secret secret);
@@ -18,4 +19,5 @@ public interface ISecretRepository
     Task RestoreManyByIdAsync(IEnumerable<Guid> ids);
     Task<IEnumerable<Secret>> ImportAsync(IEnumerable<Secret> secrets);
     Task UpdateRevisionDates(IEnumerable<Guid> ids);
+    Task<(bool Read, bool Write)> AccessToSecretAsync(Guid id, Guid userId, AccessClientType accessType);
 }

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

@@ -244,7 +244,8 @@ public class SecretsControllerTests : IClassFixture<ApiApplicationFactory>, IAsy
         secretResponse.EnsureSuccessStatusCode();
         var secretResult = await secretResponse.Content.ReadFromJsonAsync<SecretResponseModel>();
 
-        var secret = (await _secretRepository.GetManyByProjectIdAsync(project.Id, orgUserId, accessType)).First();
+        var result = (await _secretRepository.GetManyByProjectIdAsync(project.Id, orgUserId, accessType)).First();
+        var secret = result.Secret;
 
         Assert.NotNull(secretResult);
         Assert.Equal(secret.Id.ToString(), secretResult!.Id);

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

@@ -6,6 +6,7 @@ using Bit.Core.Enums;
 using Bit.Core.Exceptions;
 using Bit.Core.SecretsManager.Commands.Secrets.Interfaces;
 using Bit.Core.SecretsManager.Entities;
+using Bit.Core.SecretsManager.Models.Data;
 using Bit.Core.SecretsManager.Repositories;
 using Bit.Core.Services;
 using Bit.Core.Test.SecretsManager.AutoFixture.SecretsFixture;
@@ -45,7 +46,11 @@ public class SecretsControllerTests
     public async void GetSecretsByOrganization_Success(PermissionType permissionType, SutProvider<SecretsController> sutProvider, Core.SecretsManager.Entities.Secret resultSecret, Guid organizationId, Guid userId, Core.SecretsManager.Entities.Project mockProject, AccessClientType accessType)
     {
         sutProvider.GetDependency<ICurrentContext>().AccessSecretsManager(default).ReturnsForAnyArgs(true);
-        sutProvider.GetDependency<ISecretRepository>().GetManyByOrganizationIdAsync(default, default, default).ReturnsForAnyArgs(new List<Core.SecretsManager.Entities.Secret> { resultSecret });
+        sutProvider.GetDependency<ISecretRepository>().GetManyByOrganizationIdAsync(default, default, default)
+            .ReturnsForAnyArgs(new List<SecretPermissionDetails>
+            {
+                new() { Secret = resultSecret, Read = true, Write = true },
+            });
         sutProvider.GetDependency<IUserService>().GetProperUserId(default).ReturnsForAnyArgs(userId);
 
         if (permissionType == PermissionType.RunAsAdmin)
@@ -95,6 +100,8 @@ public class SecretsControllerTests
         resultSecret.OrganizationId = organizationId;
 
         sutProvider.GetDependency<ISecretRepository>().GetByIdAsync(default).ReturnsForAnyArgs(resultSecret);
+        sutProvider.GetDependency<ISecretRepository>().AccessToSecretAsync(default, default, default)
+            .ReturnsForAnyArgs(Task.FromResult((true, true)));
 
         if (permissionType == PermissionType.RunAsAdmin)
         {
@@ -107,7 +114,7 @@ public class SecretsControllerTests
             sutProvider.GetDependency<IProjectRepository>().UserHasReadAccessToProject(mockProject.Id, userId).Returns(true);
         }
 
-        var result = await sutProvider.Sut.GetAsync(resultSecret.Id);
+        await sutProvider.Sut.GetAsync(resultSecret.Id);
 
         await sutProvider.GetDependency<ISecretRepository>().Received(1)
                      .GetByIdAsync(Arg.Is(AssertHelper.AssertPropertyEqual(resultSecret.Id)));