[WOR-432][CORE-49] Add additional validation for updateAcl (take 2) #3072

cahrens · 2024-10-07T16:45:05Z

This reverts Revert "[WOR-432] Add additional validation for updateAcl" #3065 and modifies the code to list the actions that the user has on the workspace. Rawls checks that the user has the necessary share_policy::_ permissions to make the requested ACL change.
The change to list the user's workspace actions broke a lot of older unit tests that relied on the database and a test implementation of Sam MockSamDAO. We are trying to reduce unit test reliance on the database in Rawls as these tests are slow and modifying the test implementation of Sam can be difficult. Given this, I converted existing updateAcl tests into unit tests and moved them into a contained test file called WorkspaceServiceUpdateAclSpec.

)" (#…" This reverts commit 0cf2b94.

cahrens · 2024-10-07T20:14:48Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

@@ -976,7 +975,11 @@ class WorkspaceService(

          // figure out which of the incoming aclUpdates are actually changes by removing all the existingAcls
          aclChanges = normalize(aclUpdates) -- existingAcls
-          _ = validateAclChanges(aclChanges, existingAcls, workspace)
+          callingUserRoles <- samDAO.listUserRolesForResource(SamResourceTypeNames.workspace,


I wanted to just rely on what is in existingAcls, but if the calling user has the projectOwner role, they don't appear directly in existingAcls with their email address… instead there is a group for projectOwner, and presumably the calling user is a member of that group, but I don't know how to get that information.

This solution isn't ideal though because it does introduce another Sam call (though this method isn't called often, so that's a minor issue) and it breaks the pattern of going through the workspaceAclManager (though the Sam call could be hidden by adding a pass-through method to that interface, if folks felt like it was worth it).

The additional sam call isn't ideal, but this does have the benefit of working in cases where the user has access to the workspace via group membership more generally than just with the project-owner case (e.g. a group was added as a writer on the workspace).

We could add a new Sam API that returned a flattened policy membership for a resource if we wanted to, but we would still need the direct policy membership to calculate the ACL changes

marctalbott · 2024-10-16T19:20:59Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

@@ -976,7 +975,11 @@ class WorkspaceService(

          // figure out which of the incoming aclUpdates are actually changes by removing all the existingAcls
          aclChanges = normalize(aclUpdates) -- existingAcls
-          _ = validateAclChanges(aclChanges, existingAcls, workspace)
+          callingUserRoles <- samDAO.listUserRolesForResource(SamResourceTypeNames.workspace,


The additional sam call isn't ideal, but this does have the benefit of working in cases where the user has access to the workspace via group membership more generally than just with the project-owner case (e.g. a group was added as a writer on the workspace).

marctalbott · 2024-10-16T19:24:29Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

@@ -976,7 +975,11 @@ class WorkspaceService(

          // figure out which of the incoming aclUpdates are actually changes by removing all the existingAcls
          aclChanges = normalize(aclUpdates) -- existingAcls
-          _ = validateAclChanges(aclChanges, existingAcls, workspace)
+          callingUserRoles <- samDAO.listUserRolesForResource(SamResourceTypeNames.workspace,


We could add a new Sam API that returned a flattened policy membership for a resource if we wanted to, but we would still need the direct policy membership to calculate the ACL changes

marctalbott · 2024-10-16T19:41:30Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+    // be removing as well as what we are adding.
+    val allRolePermissionChanges =
+      aclChanges ++ existingAcls.filter(existingAcl => emailsBeingChanged.contains(existingAcl.email.toLowerCase))
+    if (callingUserMaxRole < WorkspaceAccessLevels.Owner) {


This makes assumptions about the actions contained in the workspace roles defined in Sam's resource type configuration, which isn't ideal as the name of a role in Sam shouldn't be used for authz control. Given that, this could be modified to check actions directly to make sure the user has the necessary share_policy::_ actions for the requested ACL changes which would be the right thing.

Still, I don't know if that's strictly necessary here, but happy to discuss. This check isn't intended to be true authorization as that is still handled by Sam -- the goal here is just to provide some guard rails and prevent ACL changes we believe are unlikely to succeed.

marctalbott · 2024-10-23T20:47:07Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

@@ -1075,53 +1143,6 @@ class WorkspaceService(
      .void
  }

-  private def validateAclChanges(aclChanges: Set[WorkspaceACLUpdate],


Moved inside of updateACL since it's only called from there

marctalbott · 2024-10-23T20:48:26Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+      if (callingUserActions.isEmpty) {
+        throw new InvalidWorkspaceAclUpdateException(
+          ErrorReport(StatusCodes.BadRequest, "you do not have access to change permissions for this workspace")
+        )
+      }
+      // Add the existingAcl entries that are being modified so we can check what we will
+      // be removing as well as what we are adding.
+      val changingPolicies =
+        (aclChanges ++ existingAcls.filter(existingAcl => emailsBeingChanged.contains(existingAcl.email.toLowerCase)))
+          .flatMap(aclUpdateToPolicies)
+
+      if (changingPolicies.exists(policy => !callingUserActions.contains(SamWorkspaceActions.sharePolicy(policy.value)))) {
+        throw new InvalidWorkspaceAclUpdateException(
+          ErrorReport(StatusCodes.BadRequest, "you do not have sufficient permissions to make these changes")
+        )
+      }


These changes and the new call to Sam to list user actions on the workspace are the functional changes this PR is making

marctalbott · 2024-10-23T20:51:30Z

core/src/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceServiceSpec.scala

-    }
-  }
-
-  it should "not clobber catalog permission" in withTestDataServicesCustomSam { services =>


This wasn't moved because WorkspaceService doesn't actually do anything to prevent the clobbering of catalog permissions. The RawlsWorkspaceAclManager filters out the catalog policy when listing policies, so a new unit test was added to RawlsWorkspaceAclManagerUnitTests.

marctalbott · 2024-10-23T21:01:04Z

core/src/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceServiceSpec.scala

@@ -497,244 +497,6 @@ class WorkspaceServiceSpec
    Await.result(populateAcl, Duration.Inf)
  }



In general, these tests were moved to WorkspaceServiceUpdateAclSpec and adjusted to remove their reliance on database and CustomizableSamDao. The one exception is called out below

marctalbott · 2024-10-23T21:04:08Z

core/src/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceServiceUnitTests.scala

-  it should "not allow readers to have compute access but should allow writers for Rawls workspaces" in {
-    val projectOwnerEmail = "[email protected]"
-    val ownerEmail = "[email protected]"
-    val writerEmail = "[email protected]"
-    val readerEmail = "[email protected]"
-
-    val samDAO = mockSamForAclTests()
-    when(samDAO.listPoliciesForResource(ArgumentMatchers.eq(SamResourceTypeNames.workspace), any(), any()))
-      .thenReturn(Future(samWorkspacePoliciesForAclTests(projectOwnerEmail, ownerEmail, writerEmail, readerEmail)))
-    when(samDAO.addUserToPolicy(any(), any(), any(), any(), any())).thenReturn(Future())
-
-    val workspaceRepository = mockWorkspaceRepositoryForAclTests(WorkspaceType.RawlsWorkspace)
-    val mockFastPassService = mock[FastPassService]
-    when(mockFastPassService.syncFastPassesForUserInWorkspace(any[Workspace], any[String])).thenReturn(Future())
-
-    val service = workspaceServiceConstructor(workspaceRepository = workspaceRepository,
-                                              samDAO = samDAO,
-                                              fastPassServiceConstructor = _ => mockFastPassService
-    )(ctx)
-
-    val writerAclUpdate = Set(WorkspaceACLUpdate(writerEmail, WorkspaceAccessLevels.Write, Option(false), Option(true)))
-    Await.result(service.updateACL(WorkspaceName("fake_namespace", "fake_name"), writerAclUpdate, true), Duration.Inf)
-
-    val readerAclUpdate = Set(WorkspaceACLUpdate(readerEmail, WorkspaceAccessLevels.Read, Option(false), Option(true)))
-
-    val thrown = intercept[RawlsExceptionWithErrorReport] {
-      Await.result(service.updateACL(WorkspaceName("fake_namespace", "fake_name"), readerAclUpdate, true), Duration.Inf)
-    }
-    thrown.errorReport.statusCode shouldBe Option(StatusCodes.BadRequest)
-  }
-
-  it should "allow readers with and without share access for Rawls workspaces" in {
-    val projectOwnerEmail = "[email protected]"
-    val ownerEmail = "[email protected]"
-    val writerEmail = "[email protected]"
-    val readerEmail = "[email protected]"
-
-    val samDAO = mockSamForAclTests()
-    when(samDAO.listPoliciesForResource(ArgumentMatchers.eq(SamResourceTypeNames.workspace), any(), any())).thenReturn(
-      Future(samWorkspacePoliciesForAclTests(projectOwnerEmail, ownerEmail, writerEmail, readerEmail))
-    )
-    when(samDAO.addUserToPolicy(any(), any(), any(), any(), any())).thenReturn(Future())
-
-    val workspaceRepository = mockWorkspaceRepositoryForAclTests(WorkspaceType.RawlsWorkspace)
-    val mockFastPassService = mock[FastPassService]
-    when(mockFastPassService.syncFastPassesForUserInWorkspace(any[Workspace], any[String])).thenReturn(Future())
-
-    val service = workspaceServiceConstructor(workspaceRepository = workspaceRepository,
-                                              samDAO = samDAO,
-                                              fastPassServiceConstructor = _ => mockFastPassService
-    )(ctx)
-
-    val aclUpdate = Set(
-      WorkspaceACLUpdate(readerEmail, WorkspaceAccessLevels.Read, Option(true), Option(false)),
-      WorkspaceACLUpdate("[email protected]", WorkspaceAccessLevels.Read, Option(false), Option(false))
-    )
-
-    Await.result(service.updateACL(WorkspaceName("fake_namespace", "fake_name"), aclUpdate, true), Duration.Inf)
-  }


These tests are covered by the various permutation tests, so they weren't moved over to WorkspaceServiceUpdateAclSpec

marctalbott · 2024-10-23T21:06:21Z

core/src/test/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceServiceUnitTests.scala

@@ -1570,247 +1572,6 @@ class WorkspaceServiceUnitTests
    verify(wsmDAO).getRoles(any(), any())
  }

-  behavior of "updateAcl"


Other than the exceptions at the bottom, these tests were moved over to WorkspaceServiceUpdateAclSpec and modified to mock the AclManagers instead of Sam, WSM, and the database.

dvoet · 2024-10-28T15:29:40Z

core/src/main/scala/org/broadinstitute/dsde/rawls/workspace/WorkspaceService.scala

+        (aclChanges ++ existingAcls.filter(existingAcl => emailsBeingChanged.contains(existingAcl.email.toLowerCase)))
+          .flatMap(aclUpdateToPolicies)
+
+      if (changingPolicies.exists(policy => !callingUserActions.contains(SamWorkspaceActions.sharePolicy(policy.value)))) {


We don't grant alter_policies to workspace users so it should not be necessary to check that here. Just calling it out here in case anyone disagrees.

➕ I also don't foresee us changing that model anytime soon since it'd let users modify the project owners policy which would not be great.

cahrens added 2 commits October 7, 2024 12:44

Revert "Revert "[WOR-432] Add additional validation for updateAcl (#3064

383ce61

)" (#…" This reverts commit 0cf2b94.

Change solution to account for projectOwner.

eaa13f2

cahrens commented Oct 7, 2024

View reviewed changes

cahrens changed the title ~~Revert "Revert "[WOR-432] Add additional validation for updateAcl""~~ [WOR-432] Add additional validation for updateAcl (take 2) Oct 7, 2024

cahrens requested a review from marctalbott October 8, 2024 15:14

marctalbott approved these changes Oct 16, 2024

View reviewed changes

marctalbott marked this pull request as ready for review October 16, 2024 19:41

marctalbott requested a review from dvoet October 16, 2024 19:41

marctalbott changed the title ~~[WOR-432] Add additional validation for updateAcl (take 2)~~ [WOR-432][CORE-49] Add additional validation for updateAcl (take 2) Oct 16, 2024

marctalbott added 8 commits October 21, 2024 16:58

list user actions instead of relying on roles

f527c3b

wip tests

94e233d

more test conversions

172ea43

scalafmt

b3abe8b

add test that can-catalog gets filtered out

e01a7dc

remove leftovers

c4fd82f

intellij import optimization has failed me

b46332a

cleanup

46f9554

marctalbott reviewed Oct 24, 2024

View reviewed changes

fix fastpass tests

55562f1

dvoet approved these changes Oct 28, 2024

View reviewed changes

marctalbott merged commit 5e862ec into develop Oct 29, 2024
29 checks passed

marctalbott deleted the revert-3065-revert-3064-WOR-432-rawls branch October 29, 2024 13:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WOR-432][CORE-49] Add additional validation for updateAcl (take 2) #3072

[WOR-432][CORE-49] Add additional validation for updateAcl (take 2) #3072

cahrens commented Oct 7, 2024 •

edited by jira bot

Loading

cahrens Oct 7, 2024

marctalbott Oct 16, 2024

marctalbott Oct 16, 2024

marctalbott Oct 16, 2024

marctalbott Oct 16, 2024

marctalbott Oct 16, 2024

marctalbott Oct 23, 2024

marctalbott Oct 23, 2024

marctalbott Oct 23, 2024

marctalbott Oct 23, 2024

marctalbott Oct 23, 2024

marctalbott Oct 23, 2024

dvoet Oct 28, 2024

marctalbott Oct 29, 2024

		@@ -497,244 +497,6 @@ class WorkspaceServiceSpec
		Await.result(populateAcl, Duration.Inf)
		}

[WOR-432][CORE-49] Add additional validation for updateAcl (take 2) #3072

[WOR-432][CORE-49] Add additional validation for updateAcl (take 2) #3072

Conversation

cahrens commented Oct 7, 2024 • edited by jira bot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Oct 7, 2024 •

edited by jira bot

Loading