[CA-572] remove shallow permissions checks (#417)

broadinstitute · Apr 21, 2020 · ca7b0cd · ca7b0cd
1 parent 9ffd8a8
commit ca7b0cd
Show file tree

Hide file tree

Showing 3 changed files with 3 additions and 183 deletions.
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SecurityDirectives.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SecurityDirectives.scala
@@ -45,11 +45,5 @@ trait SecurityDirectives {
     }
 
   private def hasPermissionOneOf(resource: FullyQualifiedResourceId, actions: Iterable[ResourceAction], userId: WorkbenchUserId): IO[Boolean] =
-    //  first quickly check if we have permission using the shallow check across all actions, then try the full check
-    for {
-      attempt1 <- actions.toList.existsM(policyEvaluatorService.hasPermissionShallowCheck(resource, _, userId))
-      attempt2 <- if (attempt1) IO.pure(attempt1) else actions.toList.existsM(policyEvaluatorService.hasPermissionFullCheck(resource, _, userId))
-    } yield {
-      attempt2
-    }
+    actions.toList.existsM(policyEvaluatorService.hasPermission(resource, _, userId))
 }
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorService.scala
@@ -38,17 +38,7 @@ class PolicyEvaluatorService(
       }
   }
 
-  def hasPermission(resource: FullyQualifiedResourceId, action: ResourceAction, userId: WorkbenchUserId, parentSpan: Span = null): IO[Boolean] = {
-    // first attempt the shallow check and fallback to the full check if it returns false
-    for {
-      attempt1 <- hasPermissionShallowCheck(resource, action, userId)
-      attempt2 <- if (attempt1) IO.pure(attempt1) else traceIOWithParent("fullCheck", parentSpan)(_ => hasPermissionFullCheck(resource, action, userId))
-    } yield {
-      attempt2
-    }
-  }
-
-  def hasPermissionFullCheck(resource: FullyQualifiedResourceId, action: ResourceAction, userId: WorkbenchUserId, parentSpan: Span = null): IO[Boolean] = {
+  def hasPermission(resource: FullyQualifiedResourceId, action: ResourceAction, userId: WorkbenchUserId, parentSpan: Span = null): IO[Boolean] = traceIOWithParent("hasPermission", parentSpan)(_ => {
     def checkPermission(force: Boolean) =
       listUserResourceActions(resource, userId, force).map { _.contains(action) }
 
@@ -60,56 +50,7 @@ class PolicyEvaluatorService(
     } yield {
       attempt2
     }
-  }
-
-  /**
-    * Check if the user has the action on the resource.  A true result means they do, and can be trusted.  A false result only means that
-    * the shallow check did not find that to be true, but a more exhaustive check might.
-    *
-    * In many cases users are direct members of policies, and it is very fast to query direct members of a policy (as opposed to
-    * calling isMemberOf for a user), this method leverages that to do a fast check for presence
-    */
-  def hasPermissionShallowCheck(resource: FullyQualifiedResourceId, action: ResourceAction, userId: WorkbenchUserId): IO[Boolean] = {
-
-    for {
-      res <- resourceTypes.get(resource.resourceTypeName).traverse {
-        rt =>
-
-          val roleNamesWithAction = rt.roles.filter(_.actions.contains(action)).map(_.roleName)
-          val hasAction = directMemberHasActionOnResource(resource, roleNamesWithAction, action, userId)
-
-          // if the resource type OR action are NOT auth domain constrainable... just return
-          if (!rt.isAuthDomainConstrainable || !rt.actionPatterns.exists( ap => ap.authDomainConstrainable && ap.matches(action)) ) {
-            hasAction
-          } else {
-            for {
-              // check if our result should be modulated by the auth domain constraint (ie we are not a member)
-              authDomainsResult <- accessPolicyDAO.loadResourceAuthDomain(resource)
-              authConstraintOk <- authDomainsResult match {
-                case LoadResourceAuthDomainResult.NotConstrained | LoadResourceAuthDomainResult.ResourceNotFound =>
-                  IO.pure(true)
-                case LoadResourceAuthDomainResult.Constrained(authDomains) =>
-                  // if there is more than one group, just fallback
-                  if (authDomains.size > 1) {
-                    IO.pure(false)
-                  } else {
-                    directoryDAO.isGroupMember(authDomains.head, userId)
-                  }
-              }
-              res2 <- if (authConstraintOk) hasAction else IO.pure(false)
-            } yield res2
-          }
-      }
-    } yield res.getOrElse(false)
-  }
-
-  private def directMemberHasActionOnResource(resource: FullyQualifiedResourceId, roleNamesWithAction: Set[ResourceRoleName], action: ResourceAction, userId: WorkbenchUserId ): IO[Boolean] = {
-    for {
-      // get the policies for this resource and filter to ones that contain the roles with the action, or the action directly
-      policies <- accessPolicyDAO.listAccessPolicies(resource).map(_.toList.filter(p => p.roles.intersect(roleNamesWithAction).nonEmpty || p.actions.contains(action)))
-      members = policies.flatMap(_.members)
-    } yield members.contains(userId)
-  }
+  })
 
   /**
     * Lists all the actions a user has on the specified resource

diff --git a/...test/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorServiceSpec.scala b/...test/scala/org/broadinstitute/dsde/workbench/sam/service/PolicyEvaluatorServiceSpec.scala
@@ -243,121 +243,6 @@ class PolicyEvaluatorServiceSpec extends FlatSpec with Matchers with TestSupport
     res.unsafeRunSync()
   }
 
-
-  "hasPermission" should "return false if given action is not allowed for a user using the shallow check" in {
-    val user = genUserInfo.sample.get
-    val samplePolicy = genPolicy.sample.get
-    val action = ResourceAction("weirdAction")
-    val resource = genResource.sample.get.copy(resourceTypeName = defaultResourceType.name)
-    val policyWithUser = AccessPolicy.members.set(samplePolicy.members + user.userId)(samplePolicy)
-    val policyExcludeAction = AccessPolicy.actions.set(samplePolicy.actions - action)(policyWithUser)
-    val policy = SamLenses.resourceIdentityAccessPolicy.set(resource.fullyQualifiedId)(policyExcludeAction)
-
-    val res = for{
-      _ <- setup()
-      _ <- policyDAO.createResourceType(managedGroupResourceType)
-      _ <- dirDAO.createUser(WorkbenchUser(user.userId, Some(TestSupport.genGoogleSubjectId()), user.userEmail, Some(TestSupport.genIdentityConcentratorId())))
-      _ <- resource.authDomain.toList.parTraverse(a => managedGroupService.createManagedGroup(ResourceId(a.value), dummyUserInfo))
-      _ <- savePolicyMembers(policy)
-
-      _ <- policyDAO.createResourceType(defaultResourceType)
-      _ <- policyDAO.createResource(resource)
-      _ <- policyDAO.createPolicy(policy)
-      r <- service.policyEvaluatorService.hasPermissionShallowCheck(policy.id.resource, action, user.userId)
-    } yield {
-      r shouldBe false
-    }
-
-    res.unsafeRunSync()
-  }
-
-  it should "return true if given action is on a policy directly for a direct member of the policy using the shallow check" in {
-    val user = genUserInfo.sample.get
-    val samplePolicy = genPolicy.sample.get
-    val action = defaultResourceType.roles.head.actions.head
-
-    val resource = genResource.sample.get.copy(authDomain = Set.empty, resourceTypeName = defaultResourceType.name)
-    val policyWithUser = AccessPolicy.members.modify(_ + user.userId)(samplePolicy)
-    val policyWithAction = AccessPolicy.actions.modify(_ + action)(policyWithUser)
-    val policy = SamLenses.resourceIdentityAccessPolicy.set(resource.fullyQualifiedId)(policyWithAction)
-
-    val res = for{
-      _ <- setup()
-      _ <- policyDAO.createResourceType(managedGroupResourceType)
-      _ <- dirDAO.createUser(WorkbenchUser(user.userId, Some(TestSupport.genGoogleSubjectId()), user.userEmail, Some(TestSupport.genIdentityConcentratorId())))
-      _ <- resource.authDomain.toList.parTraverse(a => managedGroupService.createManagedGroup(ResourceId(a.value), dummyUserInfo))
-      _ <- savePolicyMembers(policy)
-
-      _ <- policyDAO.createResourceType(defaultResourceType)
-      _ <- policyDAO.createResource(resource)
-      _ <- policyDAO.createPolicy(policy)
-      r <- service.policyEvaluatorService.hasPermissionShallowCheck(policy.id.resource, action, user.userId)
-    } yield {
-      r shouldBe(true)
-    }
-
-    res.unsafeRunSync()
-  }
-
-  it should "return true if given action is allowed via a role for a direct member of the policy using the shallow check" in {
-    val user = genUserInfo.sample.get
-    val samplePolicy = genPolicy.sample.get
-    val sampleRole = defaultResourceType.roles.head.roleName
-    val action = defaultResourceType.roles.head.actions.head
-
-    val resource = genResource.sample.get.copy(authDomain = Set.empty, resourceTypeName = defaultResourceType.name)
-    val policyWithUser = AccessPolicy.members.modify(_ + user.userId)(samplePolicy)
-    val policyWithRole = AccessPolicy.roles.modify(_ + sampleRole)(policyWithUser)
-    val policy = SamLenses.resourceIdentityAccessPolicy.set(resource.fullyQualifiedId)(policyWithRole)
-
-    val res = for{
-      _ <- setup()
-      _ <- policyDAO.createResourceType(managedGroupResourceType)
-      _ <- dirDAO.createUser(WorkbenchUser(user.userId, Some(TestSupport.genGoogleSubjectId()), user.userEmail, Some(TestSupport.genIdentityConcentratorId())))
-      _ <- resource.authDomain.toList.parTraverse(a => managedGroupService.createManagedGroup(ResourceId(a.value), dummyUserInfo))
-      _ <- savePolicyMembers(policy)
-
-      _ <- policyDAO.createResourceType(defaultResourceType)
-      _ <- policyDAO.createResource(resource)
-      _ <- policyDAO.createPolicy(policy)
-      r <- service.policyEvaluatorService.hasPermissionShallowCheck(policy.id.resource, action, user.userId)
-    } yield {
-      r shouldBe(true)
-    }
-
-    res.unsafeRunSync()
-  }
-
-  it should "return false if given action is not present for a user via a role or directly using the shallow check" in {
-    val user = genUserInfo.sample.get
-    val samplePolicy = genPolicy.sample.get
-    val sampleRole = ResourceRoleName("owner")
-
-    val action = ResourceAction("just_a_made_up_action") // an action not given to owner
-
-    val resource = genResource.sample.get.copy(resourceTypeName = defaultResourceType.name)
-    val policyWithUser = AccessPolicy.members.modify(_ + user.userId)(samplePolicy)
-    val policyWithRole = AccessPolicy.roles.modify(_ + sampleRole)(policyWithUser)
-    val policy = SamLenses.resourceIdentityAccessPolicy.set(resource.fullyQualifiedId)(policyWithRole)
-
-    val res = for{
-      _ <- setup()
-      _ <- policyDAO.createResourceType(managedGroupResourceType)
-      _ <- dirDAO.createUser(WorkbenchUser(user.userId, Some(TestSupport.genGoogleSubjectId()), user.userEmail, Some(TestSupport.genIdentityConcentratorId())))
-      _ <- resource.authDomain.toList.parTraverse(a => managedGroupService.createManagedGroup(ResourceId(a.value), dummyUserInfo))
-      _ <- savePolicyMembers(policy)
-
-      _ <- policyDAO.createResourceType(defaultResourceType)
-      _ <- policyDAO.createResource(resource)
-      _ <- policyDAO.createPolicy(policy)
-      r <- service.policyEvaluatorService.hasPermissionShallowCheck(policy.id.resource, action, user.userId)
-    } yield {
-      r shouldBe(false)
-    }
-
-    res.unsafeRunSync()
-  }
-
   it should "return true if given action is allowed for a user and resource is not constrained by auth domains" in {
     val user = genUserInfo.sample.get
     val samplePolicy = genPolicy.sample.get