[SM-577] - ACCESS POLICY fixing issue with user being able to update …

…a secret if they are assi… (#2763) * fixing issue with user being able to update a secret if they are assigning it to a project that has read/write permissions. Even though the customer is only allowed to read. * Add additional check for newly assigned project access and original project access. * fixing Lint issue * Fixing tests * uneeded param removed * Updating to extract logic into function * renaming function * lint fixes * renaming function
bitwarden · Mar 7, 2023 · 7334de6 · 7334de6
1 parent 48ae4a2
commit 7334de6
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 16 deletions.
diff --git a/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs b/bitwarden_license/src/Commercial.Core/SecretsManager/Commands/Secrets/UpdateSecretCommand.cs
@@ -31,21 +31,7 @@ public async Task<Secret> UpdateAsync(Secret updatedSecret, Guid userId)
         var orgAdmin = await _currentContext.OrganizationAdmin(secret.OrganizationId);
         var accessClient = AccessClientHelper.ToAccessClient(_currentContext.ClientType, orgAdmin);
 
-        var project = updatedSecret.Projects?.FirstOrDefault();
-
-        if (secret.Projects != null && secret.Projects.Any() && project == null)
-        {
-            throw new NotFoundException();
-        }
-
-        var hasAccess = accessClient switch
-        {
-            AccessClientType.NoAccessCheck => true,
-            AccessClientType.User => project != null && await _projectRepository.UserHasWriteAccessToProject(project.Id, userId),
-            _ => false,
-        };
-
-        if (!hasAccess)
+        if (!await HasAccessToOriginalAndUpdatedProject(accessClient, secret, updatedSecret, userId))
         {
             throw new NotFoundException();
         }
@@ -59,4 +45,21 @@ public async Task<Secret> UpdateAsync(Secret updatedSecret, Guid userId)
         await _secretRepository.UpdateAsync(secret);
         return secret;
     }
+
+    public async Task<bool> HasAccessToOriginalAndUpdatedProject(AccessClientType accessClient, Secret secret, Secret updatedSecret, Guid userId)
+    {
+        switch (accessClient)
+        {
+            case AccessClientType.NoAccessCheck:
+                return true;
+            case AccessClientType.User:
+                var oldProject = secret.Projects?.FirstOrDefault();
+                var newProject = updatedSecret.Projects?.FirstOrDefault();
+                var accessToOld = oldProject != null && await _projectRepository.UserHasWriteAccessToProject(oldProject.Id, userId);
+                var accessToNew = newProject != null && await _projectRepository.UserHasWriteAccessToProject(newProject.Id, userId);
+                return accessToOld && accessToNew;
+            default:
+                return false;
+        }
+    }
 }
diff --git a/...rden_license/test/Commercial.Core.Test/SecretsManager/Secrets/UpdateSecretCommandTests.cs b/...rden_license/test/Commercial.Core.Test/SecretsManager/Secrets/UpdateSecretCommandTests.cs
@@ -34,14 +34,14 @@ public async Task UpdateAsync_SecretDoesNotExist_ThrowsNotFound(Secret data, Sut
     public async Task UpdateAsync_Success(PermissionType permissionType, Secret data, SutProvider<UpdateSecretCommand> sutProvider, Guid userId, Project mockProject)
     {
         sutProvider.GetDependency<ICurrentContext>().AccessSecretsManager(data.OrganizationId).Returns(true);
+        data.Projects = new List<Project>() { mockProject };
 
         if (permissionType == PermissionType.RunAsAdmin)
         {
             sutProvider.GetDependency<ICurrentContext>().OrganizationAdmin(data.OrganizationId).Returns(true);
         }
         else
         {
-            data.Projects = new List<Project>() { mockProject };
             sutProvider.GetDependency<ICurrentContext>().OrganizationAdmin(data.OrganizationId).Returns(false);
             sutProvider.GetDependency<IProjectRepository>().UserHasWriteAccessToProject((Guid)(data.Projects?.First().Id), userId).Returns(true);
         }