From 236171624eee13225df31b6a308ee42b9b9bb9b8 Mon Sep 17 00:00:00 2001 From: Saman Ehsan Date: Tue, 12 Nov 2024 12:03:43 -0500 Subject: [PATCH 1/2] Update auto-approve-broadbot-prs.yml (#1584) --- .github/workflows/auto-approve-broadbot-prs.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/auto-approve-broadbot-prs.yml b/.github/workflows/auto-approve-broadbot-prs.yml index c93fcc047..7a5f2eb54 100644 --- a/.github/workflows/auto-approve-broadbot-prs.yml +++ b/.github/workflows/auto-approve-broadbot-prs.yml @@ -5,9 +5,9 @@ permissions: pull-requests: write jobs: - dependabot: + broadbot: runs-on: ubuntu-latest - if: github.actor == 'broadbot[bot]' + if: github.actor == 'broadbot' steps: - name: Approve a PR run: gh pr review --approve "$PR_URL" From 57b278fdcd4b061bef696aefe6b0800f82416aab Mon Sep 17 00:00:00 2001 From: dvoet Date: Tue, 12 Nov 2024 13:41:48 -0500 Subject: [PATCH 2/2] Revert "CORE-137 Optimize filter resources (#1577)" (#1588) This reverts commit ab2829dba3c0a62f9f1f2b29426a9a8798fd874c. --- pact4s/README.md | 4 +- .../dsde/workbench/sam/MockTestSupport.scala | 2 +- project/Dependencies.scala | 5 +- .../sam/dataAccess/AccessPolicyDAO.scala | 1 + .../dataAccess/PostgresAccessPolicyDAO.scala | 145 ++++++++-------- .../sam/model/FilterResourcesResult.scala | 9 +- .../sam/service/ResourceService.scala | 160 +++--------------- .../sam/dataAccess/MockAccessPolicyDAO.scala | 53 +++--- .../PostgresAccessPolicyDAOSpec.scala | 129 ++++++++++---- .../StatefulMockAccessPolicyDaoBuilder.scala | 80 ++++++--- .../sam/service/ResourceServiceUnitSpec.scala | 104 +++++++----- 11 files changed, 364 insertions(+), 328 deletions(-) diff --git a/pact4s/README.md b/pact4s/README.md index 55daf9a24..eb8053df8 100644 --- a/pact4s/README.md +++ b/pact4s/README.md @@ -13,8 +13,8 @@ On the command line, you can try the following: source env/local.env source src/main/resources/rendered/secrets.env export PACT_BROKER_URL="https://pact-broker.dsp-eng-tools.broadinstitute.org/" -export PACT_BROKER_USERNAME="$(gcloud secrets versions access latest --project 'broad-dsp-eng-tools' --secret 'pact-broker-users-read-only' | jq -r '.basic_auth_read_only_username')" -export PACT_BROKER_PASSWORD="$(gcloud secrets versions access latest --project 'broad-dsp-eng-tools' --secret 'pact-broker-users-read-only' | jq -r '.basic_auth_read_only_password')" +export PACT_BROKER_USERNAME=$(vault read -field=basic_auth_read_only_username secret/dsp/pact-broker/users/read-only) +export PACT_BROKER_PASSWORD=$(vault read -field=basic_auth_read_only_password secret/dsp/pact-broker/users/read-only) ``` In IntelliJ, you can create a Run Configuration for `SamProviderSpec.scala` and save `Environment Variables` for: diff --git a/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala b/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala index e7ed9ae97..5e3d9e603 100644 --- a/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala +++ b/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala @@ -158,7 +158,7 @@ object MockTestSupport extends MockTestSupport { policyDAO, googleExt, FakeOpenIDConnectConfiguration, - azureService: Option[AzureService] + azureService:Option[AzureService] ) } diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 8051115a5..8dc277824 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -171,8 +171,6 @@ object Dependencies { // was included transitively before, now explicit val commonsCodec: ModuleID = "commons-codec" % "commons-codec" % "1.17.1" - val caffeine: ModuleID = "com.github.ben-manes.caffeine" % "caffeine" % "3.1.8" - val rootDependencies = Seq( // proactively pull in latest versions of Jackson libs, instead of relying on the versions // specified as transitive dependencies, due to OWASP DependencyCheck warnings for earlier versions. @@ -224,8 +222,7 @@ object Dependencies { sentry, sentryLogback, okio, - terraCommonLib, - caffeine + terraCommonLib ) // Needed because it looks like the dependency overrides of wb-libs doesn't propagate to the importing project... diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala index 191e4329a..755e027cd 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala @@ -125,6 +125,7 @@ trait AccessPolicyDAO { resourceTypeNames: Set[ResourceTypeName], policies: Set[AccessPolicyName], roles: Set[ResourceRoleName], + actions: Set[ResourceAction], includePublic: Boolean, samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index b53735eea..68d7e508d 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -22,8 +22,10 @@ import scalikejdbc._ import scala.collection.concurrent.TrieMap import scala.util.{Failure, Try} import cats.effect.Temporal -import com.github.benmanes.caffeine.cache.Caffeine -import java.util.concurrent.{ConcurrentMap, TimeUnit} +import org.apache.commons.collections4.map.PassiveExpiringMap + +import java.util.Collections +import java.util.concurrent.TimeUnit class PostgresAccessPolicyDAO( protected val writeDbRef: DbReference, @@ -1777,8 +1779,8 @@ from ${GroupMemberTable as groupMemberTable} } } - private val publicResourcesCache: ConcurrentMap[ResourceTypeName, Seq[FilterResourcesResult]] = - Caffeine.newBuilder().expireAfterWrite(1, TimeUnit.HOURS).build[ResourceTypeName, Seq[FilterResourcesResult]]().asMap() + private val publicResourcesCache: java.util.Map[ResourceTypeName, Seq[FilterResourcesResult]] = + Collections.synchronizedMap(new PassiveExpiringMap(1, TimeUnit.HOURS)) private def getPublicResourcesOfType(resourceTypeName: ResourceTypeName, samRequestContext: SamRequestContext): IO[Seq[FilterResourcesResult]] = { val resourcePolicy = PolicyTable.syntax("resourcePolicy") @@ -1792,63 +1794,61 @@ from ${GroupMemberTable as groupMemberTable} val resourceTypeConstraint = samsqls"and ${resource.resourceTypeId} = ${resourceTypePKsByName.get(resourceTypeName)}" + val notNullConstraintRoleAction = + samsqls"and not (${resourceRole.role} is null and ${resourceAction.action} is null)" + val notNullConstraintPolicyAction = samsqls"and not (${resourceAction.action} is null)" val publicRoleActionQuery = - samsql""" - select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, ${resourceRole.result.role}, ${resourcePolicy.result.public}, ${resourcePolicy.resourceId} != ${resource.id} as inherited + samsqls""" + select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, ${resourceRole.result.role}, ${resourceAction.result.action}, ${resourcePolicy.result.public}, ${resourcePolicy.resourceId} != ${resource.id} as inherited from ${PolicyTable as resourcePolicy} - join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} and ${resourcePolicy.public} + left join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} and ${resourcePolicy.public} left join ${EffectivePolicyRoleTable as effectivePolicyRole} on ${effectiveResourcePolicy.id} = ${effectivePolicyRole.effectiveResourcePolicyId} left join ${ResourceRoleTable as resourceRole} on ${effectivePolicyRole.resourceRoleId} = ${resourceRole.id} - join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} $resourceTypeConstraint + left join ${RoleActionTable as roleAction} on ${effectivePolicyRole.resourceRoleId} = ${roleAction.resourceRoleId} + left join ${ResourceActionTable as resourceAction} on ${roleAction.resourceActionId} = ${resourceAction.id} + left join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} $resourceTypeConstraint where ${resourcePolicy.public} $resourceTypeConstraint + $notNullConstraintRoleAction """ val publicPolicyActionQuery = - samsql""" - select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, ${resourceAction.result.action}, ${resourcePolicy.result.public}, ${resourcePolicy.resourceId} != ${resource.id} as inherited + samsqls""" + select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, null as ${resourceRole.resultName.role}, ${resourceAction.result.action}, ${resourcePolicy.result.public}, ${resourcePolicy.resourceId} != ${resource.id} as inherited from ${PolicyTable as resourcePolicy} - join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} and ${resourcePolicy.public} + left join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} and ${resourcePolicy.public} left join ${EffectivePolicyActionTable as effectivePolicyAction} on ${effectiveResourcePolicy.id} = ${effectivePolicyAction.effectiveResourcePolicyId} left join ${ResourceActionTable as resourceAction} on ${effectivePolicyAction.resourceActionId} = ${resourceAction.id} - join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} $resourceTypeConstraint + left join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} $resourceTypeConstraint where ${resourcePolicy.public} $resourceTypeConstraint + $notNullConstraintPolicyAction """ + val includePublicPolicyActionQuery = samsqls"union all $publicPolicyActionQuery" + val publicResourcesQuery = samsql"$publicRoleActionQuery $includePublicPolicyActionQuery" + readOnlyTransaction("filterResourcesPublic", samRequestContext) { implicit session => publicResourcesCache.computeIfAbsent( resourceTypeName, - resourceTypeName => { - val roles = publicRoleActionQuery + resourceTypeName => + publicResourcesQuery .map(rs => FilterResourcesResult( rs.get[ResourceId](resource.resultName.name), resourceTypeNamesByPK(rs.get[ResourceTypePK](resource.resultName.resourceTypeId)), - rs.get[AccessPolicyName](resourcePolicy.resultName.name), - rs.stringOpt(resourceRole.resultName.role).map(r => Left[ResourceRoleName, ResourceAction](ResourceRoleName(r))), + rs.stringOpt(resourcePolicy.resultName.name).map(AccessPolicyName(_)), + rs.stringOpt(resourceRole.resultName.role).map(ResourceRoleName(_)), + rs.stringOpt(resourceAction.resultName.action).map(ResourceAction(_)), rs.get[Boolean](resourcePolicy.resultName.public), + None, + false, rs.booleanOpt("inherited").getOrElse(false) ) ) .list() .apply() - val actions = publicPolicyActionQuery - .map(rs => - FilterResourcesResult( - rs.get[ResourceId](resource.resultName.name), - resourceTypeNamesByPK(rs.get[ResourceTypePK](resource.resultName.resourceTypeId)), - rs.get[AccessPolicyName](resourcePolicy.resultName.name), - rs.stringOpt(resourceAction.resultName.action).map(a => Right[ResourceRoleName, ResourceAction](ResourceAction(a))), - rs.get[Boolean](resourcePolicy.resultName.public), - rs.booleanOpt("inherited").getOrElse(false) - ) - ) - .list() - .apply() - roles ++ actions - } ) } } @@ -1857,6 +1857,7 @@ from ${GroupMemberTable as groupMemberTable} resourceTypeNames: Set[ResourceTypeName], policies: Set[AccessPolicyName], roles: Set[ResourceRoleName], + actions: Set[ResourceAction], samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] = { val groupMemberFlat = GroupMemberFlatTable.syntax("groupMemberFlat") @@ -1868,77 +1869,83 @@ from ${GroupMemberTable as groupMemberTable} val roleAction = RoleActionTable.syntax("roleAction") val resourceAction = ResourceActionTable.syntax("resourceAction") val resource = ResourceTable.syntax("resource") + val authDomain = AuthDomainTable.syntax("authDomain") + val authDomainGroup = GroupTable.syntax("authDomainGroup") + val authDomainGroupMemberFlat = GroupMemberFlatTable.syntax("authDomainGroupMemberFlat") val resourceTypeConstraint = if (resourceTypeNames.nonEmpty) samsqls"and ${resource.resourceTypeId} in (${resourceTypeNames.flatMap(resourceTypePKsByName.get)})" else samsqls"and ${resource.resourceTypeId} in (${resourceTypeNamesByPK.keys.map(_.value)})" val policyConstraint = if (policies.nonEmpty) samsqls"and ${resourcePolicy.name} in (${policies})" else samsqls"" val roleConstraint = if (roles.nonEmpty) samsqls"and ${resourceRole.role} in (${roles})" else samsqls"" + val actionConstraint = if (actions.nonEmpty) samsqls"and ${resourceAction.action} in (${actions})" else samsqls"" + val notNullConstraintRoleAction = + samsqls"and not (${resourceRole.role} is null and ${resourceAction.action} is null)" + val notNullConstraintPolicyAction = samsqls"and not (${resourceAction.action} is null)" val policyRoleActionQuery = - samsql""" - select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, ${resourceRole.result.role}, ${resourcePolicy.result.public}, ${resourcePolicy.resourceId} != ${resource.id} as inherited + samsqls""" + select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, ${resourceRole.result.role}, ${resourceAction.result.action}, ${resourcePolicy.result.public}, ${authDomainGroup.result.name}, ${authDomainGroupMemberFlat.memberUserId} is not null as in_auth_domain, ${resourcePolicy.resourceId} != ${resource.id} as inherited from ${GroupMemberFlatTable as groupMemberFlat} - join ${PolicyTable as resourcePolicy} on ${groupMemberFlat.groupId} = ${resourcePolicy.groupId} - join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} - join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} + left join ${PolicyTable as resourcePolicy} on ${groupMemberFlat.groupId} = ${resourcePolicy.groupId} + left join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} left join ${EffectivePolicyRoleTable as effectivePolicyRole} on ${effectiveResourcePolicy.id} = ${effectivePolicyRole.effectiveResourcePolicyId} left join ${ResourceRoleTable as resourceRole} on ${effectivePolicyRole.resourceRoleId} = ${resourceRole.id} + left join ${RoleActionTable as roleAction} on ${effectivePolicyRole.resourceRoleId} = ${roleAction.resourceRoleId} + left join ${ResourceActionTable as resourceAction} on ${roleAction.resourceActionId} = ${resourceAction.id} + left join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} + left join ${AuthDomainTable as authDomain} on ${authDomain.resourceId} = ${resource.id} + left join ${GroupTable as authDomainGroup} on ${authDomainGroup.id} = ${authDomain.groupId} + left join ${GroupMemberFlatTable as authDomainGroupMemberFlat} on ${authDomainGroup.id} = ${authDomainGroupMemberFlat.groupId} and ${authDomainGroupMemberFlat.memberUserId} = ${samUserId} where ${groupMemberFlat.memberUserId} = ${samUserId} $resourceTypeConstraint $policyConstraint $roleConstraint + $actionConstraint + $notNullConstraintRoleAction """ val policyActionQuery = - samsql""" - select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, ${resourceAction.result.action}, ${resourcePolicy.result.public}, ${resourcePolicy.resourceId} != ${resource.id} as inherited + samsqls""" + select ${resource.result.name}, ${resource.result.resourceTypeId}, ${resourcePolicy.result.name}, null as ${resourceRole.resultName.role}, ${resourceAction.result.action}, ${resourcePolicy.result.public}, ${authDomainGroup.result.name}, ${authDomainGroupMemberFlat.memberUserId} is not null as in_auth_domain, ${resourcePolicy.resourceId} != ${resource.id} as inherited from ${GroupMemberFlatTable as groupMemberFlat} - join ${PolicyTable as resourcePolicy} on ${groupMemberFlat.groupId} = ${resourcePolicy.groupId} - join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} - join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} + left join ${PolicyTable as resourcePolicy} on ${groupMemberFlat.groupId} = ${resourcePolicy.groupId} + left join ${EffectiveResourcePolicyTable as effectiveResourcePolicy} on ${resourcePolicy.id} = ${effectiveResourcePolicy.sourcePolicyId} left join ${EffectivePolicyActionTable as effectivePolicyAction} on ${effectiveResourcePolicy.id} = ${effectivePolicyAction.effectiveResourcePolicyId} left join ${ResourceActionTable as resourceAction} on ${effectivePolicyAction.resourceActionId} = ${resourceAction.id} + left join ${ResourceTable as resource} on ${effectiveResourcePolicy.resourceId} = ${resource.id} + left join ${AuthDomainTable as authDomain} on ${authDomain.resourceId} = ${resource.id} + left join ${GroupTable as authDomainGroup} on ${authDomainGroup.id} = ${authDomain.groupId} + left join ${GroupMemberFlatTable as authDomainGroupMemberFlat} on ${authDomainGroup.id} = ${authDomainGroupMemberFlat.groupId} and ${authDomainGroupMemberFlat.memberUserId} = ${samUserId} where ${groupMemberFlat.memberUserId} = ${samUserId} $resourceTypeConstraint $policyConstraint + $actionConstraint + $notNullConstraintPolicyAction """ + val includePolicyActionQuery = if (roles.isEmpty) samsqls"union all $policyActionQuery" else samsqls"" + val query = + samsqls"""$policyRoleActionQuery + $includePolicyActionQuery""" + readOnlyTransaction("filterResources", samRequestContext) { implicit session => - val rolesResult = policyRoleActionQuery + samsql"$query" .map(rs => FilterResourcesResult( rs.get[ResourceId](resource.resultName.name), resourceTypeNamesByPK(rs.get[ResourceTypePK](resource.resultName.resourceTypeId)), - rs.get[AccessPolicyName](resourcePolicy.resultName.name), - rs.stringOpt(resourceRole.resultName.role).map(r => Left[ResourceRoleName, ResourceAction](ResourceRoleName(r))), + rs.stringOpt(resourcePolicy.resultName.name).map(AccessPolicyName(_)), + rs.stringOpt(resourceRole.resultName.role).map(ResourceRoleName(_)), + rs.stringOpt(resourceAction.resultName.action).map(ResourceAction(_)), rs.get[Boolean](resourcePolicy.resultName.public), + rs.stringOpt(authDomainGroup.resultName.name).map(WorkbenchGroupName(_)), + rs.booleanOpt("in_auth_domain").getOrElse(false), rs.booleanOpt("inherited").getOrElse(false) ) ) .list() .apply() - - // if roles is not empty that means we are filtering by roles and we don't need to query for actions directly on a policy since they have no roles - val actionsResult = if (roles.nonEmpty) { - List.empty - } else { - policyActionQuery - .map(rs => - FilterResourcesResult( - rs.get[ResourceId](resource.resultName.name), - resourceTypeNamesByPK(rs.get[ResourceTypePK](resource.resultName.resourceTypeId)), - rs.get[AccessPolicyName](resourcePolicy.resultName.name), - rs.stringOpt(resourceAction.resultName.action).map(a => Right[ResourceRoleName, ResourceAction](ResourceAction(a))), - rs.get[Boolean](resourcePolicy.resultName.public), - rs.booleanOpt("inherited").getOrElse(false) - ) - ) - .list() - .apply() - } - rolesResult ++ actionsResult - } } @@ -1947,6 +1954,7 @@ from ${GroupMemberTable as groupMemberTable} resourceTypeNames: Set[ResourceTypeName], policies: Set[AccessPolicyName], roles: Set[ResourceRoleName], + actions: Set[ResourceAction], includePublic: Boolean, samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] = @@ -1958,10 +1966,11 @@ from ${GroupMemberTable as groupMemberTable} .sequence .map(_.flatten) } else IO.pure(List.empty) - privateResources <- filterPrivateResources(samUserId, resourceTypeNames, policies, roles, samRequestContext) + privateResources <- filterPrivateResources(samUserId, resourceTypeNames, policies, roles, actions, samRequestContext) } yield publicResources - .filter(r => policies.isEmpty || policies.contains(r.policy)) - .filter(r => roles.isEmpty || r.roleOrAction.exists(_.left.exists(role => roles.contains(role)))) ++ privateResources + .filter(r => policies.isEmpty || r.policy.exists(p => policies.contains(p))) + .filter(r => roles.isEmpty || r.role.exists(role => roles.contains(role))) + .filter(r => actions.isEmpty || r.action.exists(action => actions.contains(action))) ++ privateResources private def recreateEffectivePolicyRolesTableEntry(resourceTypeNames: Set[ResourceTypeName])(implicit session: DBSession): Int = { val resource = ResourceTable.syntax("resource") diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/FilterResourcesResult.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/FilterResourcesResult.scala index 04d729632..2a6cdcc5a 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/FilterResourcesResult.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/FilterResourcesResult.scala @@ -1,10 +1,15 @@ package org.broadinstitute.dsde.workbench.sam.model +import org.broadinstitute.dsde.workbench.model.WorkbenchGroupName + case class FilterResourcesResult( resourceId: ResourceId, resourceTypeName: ResourceTypeName, - policy: AccessPolicyName, - roleOrAction: Option[Either[ResourceRoleName, ResourceAction]], + policy: Option[AccessPolicyName], + role: Option[ResourceRoleName], + action: Option[ResourceAction], isPublic: Boolean, + authDomain: Option[WorkbenchGroupName], + inAuthDomain: Boolean, inherited: Boolean ) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala index ebf1c33f4..5ba6767af 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -116,10 +116,7 @@ class ResourceService( samRequestContext ) } - } yield { - logger.info(s"Upserted policy $fullyQualifiedPolicyId") - upsertedPolicy - } + } yield upsertedPolicy upsertIO.attempt.map(fullyQualifiedPolicyId -> _) } .map(_.toMap) @@ -1031,91 +1028,62 @@ class ResourceService( authDomainGroups: Map[WorkbenchGroupName, Boolean] = Map.empty ) - private def groupFlat( - dbResult: Seq[FilterResourcesResult], - filterActions: Set[ResourceAction], - resourceAuthDomains: Map[FullyQualifiedResourceId, Set[WorkbenchGroupName]], - userGroups: Set[WorkbenchGroupName] - ): FilteredResourcesFlat = { + private def groupFlat(dbResult: Seq[FilterResourcesResult]): FilteredResourcesFlat = { val groupedFilteredResource = dbResult .groupBy(_.resourceId) .map { tuple => val (k, v) = tuple - val grouped = v.foldLeft(GroupedDbRows()) { (acc: GroupedDbRows, r: FilterResourcesResult) => - val role = r.roleOrAction.flatMap(_.left.toOption) - val roleActions = role.flatMap(roleName => getRoleActions(r.resourceTypeName, roleName, filterActions)).getOrElse(Set.empty) - // if filterActions is not emtpy, we only want to include roles that still have actions - val filteredRole = if (filterActions.isEmpty) role else role.filter(_ => roleActions.nonEmpty) - val policyActions = r.roleOrAction.flatMap(_.toOption).toSet - // if filterActions is not emtpy, we only want to include actions that are in the filterActions set - val filteredPolicyActions = if (filterActions.isEmpty) policyActions else policyActions.intersect(filterActions) + val grouped = v.foldLeft(GroupedDbRows())((acc: GroupedDbRows, r: FilterResourcesResult) => acc.copy( - policies = acc.policies + FilteredResourceFlatPolicy(r.policy, r.isPublic, r.inherited), - roles = acc.roles ++ filteredRole, - actions = acc.actions ++ filteredPolicyActions ++ roleActions + policies = acc.policies ++ r.policy.map(p => FilteredResourceFlatPolicy(p, r.isPublic, r.inherited)), + roles = acc.roles ++ r.role, + actions = acc.actions ++ r.action, + authDomainGroups = acc.authDomainGroups ++ r.authDomain.map(_ -> r.inAuthDomain) ) - } + ) - val authDomainGroups = resourceAuthDomains.getOrElse(FullyQualifiedResourceId(v.head.resourceTypeName, k), Set.empty) FilteredResourceFlat( resourceId = k, resourceType = v.head.resourceTypeName, policies = grouped.policies, roles = grouped.roles, actions = grouped.actions, - authDomainGroups = authDomainGroups, - missingAuthDomainGroups = authDomainGroups -- userGroups + authDomainGroups = grouped.authDomainGroups.keySet, + missingAuthDomainGroups = grouped.authDomainGroups.filter(!_._2).keySet // Get only the auth domains where the user is not a member. ) } .toSet FilteredResourcesFlat(resources = groupedFilteredResource) } - private def getRoleActions(resourceTypeName: ResourceTypeName, roleName: ResourceRoleName, filterActions: Set[ResourceAction]) = - // if filterActions is not emtpy, we only want to include actions that are in the filterActions set - resourceTypes(resourceTypeName).roles - .find(_.roleName == roleName) - .map(actions => if (filterActions.isEmpty) actions.actions else actions.actions.intersect(filterActions)) - - private def groupHierarchical( - dbResult: Seq[FilterResourcesResult], - filterActions: Set[ResourceAction], - resourceAuthDomains: Map[FullyQualifiedResourceId, Set[WorkbenchGroupName]], - userGroups: Set[WorkbenchGroupName] - ): FilteredResourcesHierarchical = { + private def groupHierarchical(dbResult: Seq[FilterResourcesResult]): FilteredResourcesHierarchical = { val groupedFilteredResources = dbResult .groupBy(_.resourceId) .map { tuple => val (resourceId, resourceRows) = tuple val policies = resourceRows - .groupBy(_.policy) + .groupBy(_.policy.get) .map { policyTuple => val (policyName, policyRows) = policyTuple - // if filterActions is not emtpy, we only want to include actions that are in the filterActions set - val policyActions = policyRows.flatMap(_.roleOrAction.flatMap(_.toOption)).toSet - val filteredPolicyActions = if (filterActions.isEmpty) policyActions else policyActions.intersect(filterActions) - val roles = policyRows - .flatMap(_.roleOrAction.flatMap(_.left.toOption)) - .map { roleName => - FilteredResourceHierarchicalRole( - roleName, - getRoleActions(policyRows.head.resourceTypeName, roleName, filterActions).getOrElse(Set.empty) - ) + val actionsWithoutRoles = policyRows.filter(_.role.isEmpty).flatMap(_.action).toSet + val actionsWithRoles = policyRows.filter(_.role.nonEmpty) + val roles = actionsWithRoles + .groupBy(_.role.get) + .map { roleTuple => + val (roleName, roleRows) = roleTuple + FilteredResourceHierarchicalRole(roleName, roleRows.flatMap(_.action).toSet) } .toSet - // if filterActions is not emtpy, we only want to include roles that still have actions - val filteredRoles = if (filterActions.isEmpty) roles else roles.filter(_.actions.nonEmpty) - FilteredResourceHierarchicalPolicy(policyName, filteredRoles, filteredPolicyActions, policyRows.head.isPublic, policyRows.head.inherited) + FilteredResourceHierarchicalPolicy(policyName, roles, actionsWithoutRoles, policyRows.head.isPublic, policyRows.head.inherited) } .toSet - val fullyQualifiedResourceId = FullyQualifiedResourceId(resourceRows.head.resourceTypeName, resourceId) - val authDomainGroups = resourceAuthDomains.getOrElse(fullyQualifiedResourceId, Set.empty) + val authDomainGroupMemberships = resourceRows.flatMap(r => r.authDomain.map(_ -> r.inAuthDomain)).toMap FilteredResourceHierarchical( resourceId = resourceId, resourceType = resourceRows.head.resourceTypeName, policies = policies, - authDomainGroups = authDomainGroups, - missingAuthDomainGroups = authDomainGroups -- userGroups + authDomainGroups = authDomainGroupMemberships.keySet, + missingAuthDomainGroups = authDomainGroupMemberships.filter(!_._2).keySet // Get only the auth domains where the user is not a member. ) } .toSet @@ -1157,16 +1125,7 @@ class ResourceService( includePublic: Boolean, samRequestContext: SamRequestContext ): IO[FilteredResourcesFlat] = - listResourcesAndTransform(samUserId, resourceTypeNames, policies, roles, includePublic, samRequestContext) { - case (filterResults, resourceAuthDomains, userGroups) => - val groupedResults = groupFlat(filterResults, actions, resourceAuthDomains, userGroups) - // If we are filtering by actions, remove any resources that don't have the actions either directly or through a role - if (actions.nonEmpty) { - groupedResults.copy(resources = groupedResults.resources.filter(resource => resource.roles.nonEmpty || resource.actions.nonEmpty)) - } else { - groupedResults - } - } + accessPolicyDAO.filterResources(samUserId, resourceTypeNames, policies, roles, actions, includePublic, samRequestContext).map(groupFlat) def listResourcesHierarchical( samUserId: WorkbenchUserId, @@ -1177,72 +1136,7 @@ class ResourceService( includePublic: Boolean, samRequestContext: SamRequestContext ): IO[FilteredResourcesHierarchical] = - listResourcesAndTransform(samUserId, resourceTypeNames, policies, roles, includePublic, samRequestContext) { - case (filterResults, resourceAuthDomains, userGroups) => - val groupedResults = groupHierarchical(filterResults, actions, resourceAuthDomains, userGroups) - // If we are filtering by actions, remove any resources that don't have the actions either directly or through a role - if (actions.nonEmpty) { - groupedResults.copy(resources = - groupedResults.resources.filter(resource => resource.policies.exists(policy => policy.actions.nonEmpty || policy.roles.nonEmpty)) - ) - } else { - groupedResults - } - } - - private def listResourcesAndTransform[T]( - samUserId: WorkbenchUserId, - resourceTypeNames: Set[ResourceTypeName], - policies: Set[AccessPolicyName], - roles: Set[ResourceRoleName], - includePublic: Boolean, - samRequestContext: SamRequestContext - )(transform: (Seq[FilterResourcesResult], Map[FullyQualifiedResourceId, Set[WorkbenchGroupName]], Set[WorkbenchGroupName]) => T): IO[T] = - // note that filtering by actions is implemented by application logic, not by the database query - // adding actions to the query would make return many more rows than necessary because roles can have many actions - // and those actions are static and already in memory - for { - filterResults <- accessPolicyDAO.filterResources(samUserId, resourceTypeNames, policies, roles, includePublic, samRequestContext) - resourceAuthDomains <- listResourceAuthDomains(filterResults, samRequestContext) - // only load user groups is there are auth domains - userGroups <- - if (resourceAuthDomains.values.exists(_.nonEmpty)) { - listUserManagedGroups(samUserId, samRequestContext) - } else { - IO.pure(Set.empty[WorkbenchGroupName]) - } - } yield transform(filterResults, resourceAuthDomains, userGroups) - - private def listResourceAuthDomains( - filteredResources: Seq[FilterResourcesResult], - samRequestContext: SamRequestContext - ): IO[Map[FullyQualifiedResourceId, Set[WorkbenchGroupName]]] = - filteredResources - .groupBy(_.resourceTypeName) - .toList - .traverse { case (resourceTypeName, resources) => - for { - resourceTypeOpt <- getResourceType(resourceTypeName) - authDomainsByResourceId <- resourceTypeOpt match { - case Some(resourceType) if resourceType.isAuthDomainConstrainable => - accessPolicyDAO.listResourcesWithAuthdomains(resourceTypeName, resources.map(_.resourceId).toSet, samRequestContext).map { authDomainResults => - authDomainResults.map { authDomainResult => - authDomainResult.fullyQualifiedId -> authDomainResult.authDomain - }.toMap - } - case _ => - IO.pure(Map.empty[FullyQualifiedResourceId, Set[WorkbenchGroupName]]) - } - } yield authDomainsByResourceId - } - .map(_.flatten.toMap) - - private def listUserManagedGroups(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Set[WorkbenchGroupName]] = - for { - groupPolicies <- accessPolicyDAO.listAccessPolicies(ManagedGroupService.managedGroupTypeName, userId, samRequestContext) - } yield { - val membershipPolicies = - groupPolicies.filter(p => ManagedGroupService.userMembershipRoleNames.contains(ManagedGroupService.getRoleName(p.accessPolicyName.value))) - membershipPolicies.map(policy => WorkbenchGroupName(policy.resourceId.value)) - } + accessPolicyDAO + .filterResources(samUserId, resourceTypeNames, policies, roles, actions, includePublic, samRequestContext) + .map(groupHierarchical) } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index e7e76b737..b9a00c3e4 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -361,6 +361,7 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam resourceTypeNames: Set[ResourceTypeName], policies: Set[AccessPolicyName], roles: Set[ResourceRoleName], + actions: Set[ResourceAction], includePublic: Boolean, samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] = IO { @@ -369,28 +370,38 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam this.policies.collect { case (fqPolicyId @ FullyQualifiedPolicyId(FullyQualifiedResourceId(`resourceTypeName`, _), _), accessPolicy: AccessPolicy) if accessPolicy.members.contains(samUserId) || accessPolicy.public => - val roleResults = accessPolicy.roles.map { role => - FilterResourcesResult( - fqPolicyId.resource.resourceId, - fqPolicyId.resource.resourceTypeName, - fqPolicyId.accessPolicyName, - Option(Left(role)), - accessPolicy.public, - false - ) + val rolesAndActions = RolesAndActions.fromPolicy(accessPolicy) + rolesAndActions.roles.flatMap { role => + if (actions.isEmpty) { + Set( + FilterResourcesResult( + fqPolicyId.resource.resourceId, + fqPolicyId.resource.resourceTypeName, + Some(fqPolicyId.accessPolicyName), + Some(role), + None, + accessPolicy.public, + None, + false, + false + ) + ) + } else { + rolesAndActions.actions.map { action => + FilterResourcesResult( + fqPolicyId.resource.resourceId, + fqPolicyId.resource.resourceTypeName, + Some(fqPolicyId.accessPolicyName), + Some(role), + Some(action), + accessPolicy.public, + None, + false, + false + ) + } + } } - - val actionResults = accessPolicy.actions.map { action => - FilterResourcesResult( - fqPolicyId.resource.resourceId, - fqPolicyId.resource.resourceTypeName, - fqPolicyId.accessPolicyName, - Option(Right(action)), - accessPolicy.public, - false - ) - } - roleResults ++ actionResults } } .flatten diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala index 80d4dc530..6c85d9c52 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala @@ -3277,11 +3277,18 @@ class PostgresAccessPolicyDAOSpec extends AnyFreeSpec with Matchers with BeforeA "filterResources" - { - def verify(dbResultRows: Seq[FilterResourcesResult], roles: Set[ResourceRoleName], policyActions: Set[ResourceAction]): Any = { - val testRoles: Set[ResourceRoleName] = dbResultRows.flatMap(_.roleOrAction.flatMap(_.left.toOption)).toSet - val testPolicyActions: Set[ResourceAction] = dbResultRows.flatMap(_.roleOrAction.flatMap(_.toOption)).toSet + def verify( + dbResultRows: Seq[FilterResourcesResult], + roles: Set[ResourceRoleName], + roleActions: Set[ResourceAction], + policyActions: Set[ResourceAction] + ) = { + val testRoles: Set[ResourceRoleName] = dbResultRows.flatMap(_.role).toSet + val testRoleActions: Set[ResourceAction] = dbResultRows.filter(_.role.isDefined).flatMap(_.action).toSet + val testPolicyActions: Set[ResourceAction] = dbResultRows.filter(_.role.isEmpty).flatMap(_.action).toSet testRoles should be(roles) + testRoleActions should be(roleActions) testPolicyActions should be(policyActions) } @@ -3328,67 +3335,84 @@ class PostgresAccessPolicyDAOSpec extends AnyFreeSpec with Matchers with BeforeA false ) // No roles available - val publicProbeEmptyPolicy = AccessPolicy( + val publicProbePolicy = AccessPolicy( FullyQualifiedPolicyId(kitchenSink.fullyQualifiedId, AccessPolicyName(uuid)), Set.empty, WorkbenchEmail(s"${uuid}@policy.com"), - Set.empty, + Set(actionlessRole.roleName), Set.empty, Set.empty, true ) - val privateProbeEmptyPolicy = AccessPolicy( - FullyQualifiedPolicyId(kitchenSink.fullyQualifiedId, AccessPolicyName(uuid)), - Set(user.id), - WorkbenchEmail(s"${uuid}@policy.com"), - Set.empty, - Set.empty, - Set.empty, - false - ) dao.createPolicy(directProbePolicy, samRequestContext).unsafeRunSync() - dao.createPolicy(publicProbeEmptyPolicy, samRequestContext).unsafeRunSync() - dao.createPolicy(privateProbeEmptyPolicy, samRequestContext).unsafeRunSync() + dao.createPolicy(publicProbePolicy, samRequestContext).unsafeRunSync() + + val writeActions = + dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set.empty, Set(writeAction), false, samRequestContext).unsafeRunSync() + writeActions.length should be(5) + writeActions.map(_.action).forall(a => a.exists(_.equals(writeAction))) should be(true) + writeActions.map(_.isPublic).forall(ip => !ip) should be(true) + + val writerViaOwner = writeActions.filter(r => r.role.exists(_.equals(ownerRole.roleName))) + writerViaOwner.size should be(1) + + val readActions = dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set.empty, Set(readAction), false, samRequestContext).unsafeRunSync() + readActions.length should be(6) + + val readerViaOwner = readActions.filter(r => r.role.exists(_.equals(ownerRole.roleName))) + readerViaOwner.size should be(1) val readerRoles = - dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set(readerRole.roleName), false, samRequestContext).unsafeRunSync() + dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set(readerRole.roleName), Set.empty, false, samRequestContext).unsafeRunSync() readerRoles.size should be(5) val policies = dao - .filterResources(user.id, Set(resourceTypeName), Set(directProbePolicy.id.accessPolicyName), Set.empty, false, samRequestContext) + .filterResources(user.id, Set(resourceTypeName), Set(directProbePolicy.id.accessPolicyName), Set.empty, Set.empty, false, samRequestContext) .unsafeRunSync() - val foundPolicies = policies.map(_.policy).toSet + val foundPolicies = policies.flatMap(_.policy).toSet foundPolicies.size should be(1) foundPolicies.head should be(directProbePolicy.id.accessPolicyName) + val writeActionsIncludingPublic = + dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set.empty, Set(writeAction), true, samRequestContext).unsafeRunSync() + writeActionsIncludingPublic.length should be(7) + writeActionsIncludingPublic.filter(_.isPublic).map(_.resourceId).toSet should be(Set(publicResource.resourceId, publicChildResource.resourceId)) + val readerRolesIncludingPublic = - dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set(readerRole.roleName), true, samRequestContext).unsafeRunSync() + dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set(readerRole.roleName), Set.empty, true, samRequestContext).unsafeRunSync() readerRolesIncludingPublic.size should be(7) readerRolesIncludingPublic.filter(_.isPublic).map(_.resourceId).toSet should be(Set(publicResource.resourceId, publicChildResource.resourceId)) + println(dao.filterResources(user.id, Set(resourceTypeName), Set.empty, Set.empty, Set.empty, true, samRequestContext).unsafeRunSync()) + val inheritedReaderRoles = - dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set(readerRole.roleName), true, samRequestContext).unsafeRunSync() + dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set(readerRole.roleName), Set.empty, true, samRequestContext).unsafeRunSync() val inheritedPolicies = inheritedReaderRoles.filter(_.inherited) inheritedPolicies.map(_.resourceId).toSet should be( Set(childResource1.resourceId, childResource2.resourceId, publicChildResource.resourceId, kitchenSink.resourceId) ) - val filtered = dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set.empty, true, samRequestContext).unsafeRunSync() - - val emptyPublicResults = filtered.filter(_.policy == publicProbeEmptyPolicy.id.accessPolicyName) - emptyPublicResults.map(_.roleOrAction) should contain only None - val emptyPrivateResults = filtered.filter(_.policy == privateProbeEmptyPolicy.id.accessPolicyName) - emptyPrivateResults.map(_.roleOrAction) should contain only None + val filtered = dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set.empty, Set.empty, true, samRequestContext).unsafeRunSync() val readRoleWriteActionResources = Seq(userReadRoleWriteAction, groupReadRoleWriteAction, childResource1, childResource2, publicResource, publicChildResource) readRoleWriteActionResources .map(resource => - verify(filtered.filter(_.resourceId.equals(resource.resourceId)), roles = Set(readerRole.roleName), policyActions = Set(writeAction)) + verify( + filtered.filter(_.resourceId.equals(resource.resourceId)), + roles = Set(readerRole.roleName), + roleActions = Set(readAction), + policyActions = Set(writeAction) + ) ) - verify(filtered.filter(_.resourceId.equals(kitchenSink.resourceId)), roles = Set(readerRole.roleName, ownerRole.roleName), policyActions = Set.empty) + verify( + filtered.filter(_.resourceId.equals(kitchenSink.resourceId)), + roles = Set(readerRole.roleName, ownerRole.roleName, actionlessRole.roleName), + roleActions = Set(readAction, writeAction), + policyActions = Set.empty + ) } "filters on the user's policies, roles, and actions when using nested roles" in { @@ -3425,17 +3449,60 @@ class PostgresAccessPolicyDAOSpec extends AnyFreeSpec with Matchers with BeforeA createResourceHierarchy(Option(parentGroup.id), Set(writeAction), Set(includesRole.roleName, descendsRole.roleName), false, nestedResourceType.name) val nestedReaderRoles = - dao.filterResources(user.id, Set(nestedResourceType.name), Set.empty, Set(readerRole.roleName), false, samRequestContext).unsafeRunSync() + dao.filterResources(user.id, Set(nestedResourceType.name), Set.empty, Set(readerRole.roleName), Set.empty, false, samRequestContext).unsafeRunSync() nestedReaderRoles.size should be(4) nestedReaderRoles.map(_.resourceId).toSet should be( Set(directAccessResource.resourceId, groupAccessResource.resourceId, directAccessChildResource.resourceId, groupAccessChildResource.resourceId) ) val publicNestedReaderRoles = - dao.filterResources(user.id, Set(nestedResourceType.name), Set.empty, Set(readerRole.roleName), true, samRequestContext).unsafeRunSync() + dao.filterResources(user.id, Set(nestedResourceType.name), Set.empty, Set(readerRole.roleName), Set.empty, true, samRequestContext).unsafeRunSync() publicNestedReaderRoles.size should be(6) publicNestedReaderRoles.filter(_.isPublic).map(_.resourceId).toSet should be(Set(publicResource.resourceId, publicChildResource.resourceId)) } + + "includes Authorization Domain information in its queries" in { + assume(databaseEnabled, databaseEnabledClue) + + val user = Generator.genWorkbenchUserGoogle.sample.get + + val subGroup = BasicWorkbenchGroup(WorkbenchGroupName("subGroup"), Set(user.id), WorkbenchEmail("sub@groups.com")) + val parentGroup = BasicWorkbenchGroup(WorkbenchGroupName("parent"), Set(subGroup.id), WorkbenchEmail("parent@groups.com")) + val authDomainGroup1 = BasicWorkbenchGroup(WorkbenchGroupName("authDomainGroup1"), Set(parentGroup.id), WorkbenchEmail("authDomainGroup1@groups.com")) + val authDomainGroup2 = BasicWorkbenchGroup(WorkbenchGroupName("authDomainGroup2"), Set.empty, WorkbenchEmail("authDomainGroup2@groups.com")) + + dirDao.createUser(user, samRequestContext).unsafeRunSync() + dirDao.createGroup(subGroup, samRequestContext = samRequestContext).unsafeRunSync() + dirDao.createGroup(parentGroup, samRequestContext = samRequestContext).unsafeRunSync() + dirDao.createGroup(authDomainGroup1, samRequestContext = samRequestContext).unsafeRunSync() + dirDao.createGroup(authDomainGroup2, samRequestContext = samRequestContext).unsafeRunSync() + dirDao.addGroupMember(subGroup.id, user.id, samRequestContext).unsafeRunSync() + dirDao.addGroupMember(parentGroup.id, subGroup.id, samRequestContext).unsafeRunSync() + dao.createResourceType(resourceType, samRequestContext).unsafeRunSync() + dao.createResourceType(otherResourceType, samRequestContext).unsafeRunSync() + + // can access via auth domain + val resource1 = createResource(Option(user.id), Set(writeAction), Set(readerRole.roleName), public = false, authDomainGroups = Set(authDomainGroup1)) + // cannot access via auth domain + val resource2 = + createResource(Option(user.id), Set(writeAction), Set(readerRole.roleName), false, authDomainGroups = Set(authDomainGroup1, authDomainGroup2)) + + val writeActions = + dao.filterResources(user.id, Set(resourceType.name), Set.empty, Set.empty, Set(writeAction), false, samRequestContext).unsafeRunSync() + val byResource = writeActions.groupBy(_.resourceId) + + val resource1Results = byResource(resource1.resourceId) + resource1Results.length should be(1) + resource1Results.head.authDomain should be(Some(authDomainGroup1.id)) + resource1Results.head.inAuthDomain should be(true) + + val resource2Results = byResource(resource2.resourceId) + resource2Results.length should be(2) + resource2Results.flatMap(_.authDomain).toSet should be(Set(authDomainGroup1.id, authDomainGroup2.id)) + resource2Results.map(_.inAuthDomain).toSet should be(Set(true, false)) + resource2Results.filter(_.inAuthDomain).head.authDomain should be(Some(authDomainGroup1.id)) + resource2Results.filter(!_.inAuthDomain).head.authDomain should be(Some(authDomainGroup2.id)) + } } "listResourcesUsingAuthDomain" - { diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockAccessPolicyDaoBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockAccessPolicyDaoBuilder.scala index c9c3f1fa0..9acb40e3a 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockAccessPolicyDaoBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/StatefulMockAccessPolicyDaoBuilder.scala @@ -92,6 +92,7 @@ case class StatefulMockAccessPolicyDaoBuilder() extends MockitoSugar { ArgumentMatchers.eq(Set(policy.id.resource.resourceTypeName)), ArgumentMatchers.eq(Set.empty), ArgumentMatchers.eq(Set.empty), + ArgumentMatchers.eq(Set.empty), ArgumentMatchers.eq(true), any[SamRequestContext] ) @@ -114,42 +115,67 @@ case class StatefulMockAccessPolicyDaoBuilder() extends MockitoSugar { ) } - private def constructFilterResourcesResult(accessPolicy: AccessPolicy): Seq[FilterResourcesResult] = { - val results = accessPolicy.roles.map { role => - FilterResourcesResult( - accessPolicy.id.resource.resourceId, - accessPolicy.id.resource.resourceTypeName, - accessPolicy.id.accessPolicyName, - Option(Left(role)), - accessPolicy.public, - false - ) - }.toSeq ++ accessPolicy.actions.map { action => - FilterResourcesResult( - accessPolicy.id.resource.resourceId, - accessPolicy.id.resource.resourceTypeName, - accessPolicy.id.accessPolicyName, - Option(Right(action)), - accessPolicy.public, - false - ) - - }.toSeq - if (results.isEmpty) { + private def constructFilterResourcesResult(accessPolicy: AccessPolicy): Seq[FilterResourcesResult] = + if (accessPolicy.roles.isEmpty) { Seq( FilterResourcesResult( accessPolicy.id.resource.resourceId, accessPolicy.id.resource.resourceTypeName, - accessPolicy.id.accessPolicyName, + Some(accessPolicy.id.accessPolicyName), + None, None, accessPolicy.public, + None, + false, false ) ) - } else { - results - } - } + } else + { + accessPolicy.roles.map { role => + FilterResourcesResult( + accessPolicy.id.resource.resourceId, + accessPolicy.id.resource.resourceTypeName, + Some(accessPolicy.id.accessPolicyName), + Some(role), + None, + accessPolicy.public, + None, + false, + false + ) + }.toSeq + } ++ + (if (accessPolicy.actions.isEmpty) { + Seq( + FilterResourcesResult( + accessPolicy.id.resource.resourceId, + accessPolicy.id.resource.resourceTypeName, + Some(accessPolicy.id.accessPolicyName), + None, + None, + accessPolicy.public, + None, + false, + false + ) + ) + } else { + accessPolicy.actions.map { action => + FilterResourcesResult( + accessPolicy.id.resource.resourceId, + accessPolicy.id.resource.resourceTypeName, + Some(accessPolicy.id.accessPolicyName), + None, + Some(action), + accessPolicy.public, + None, + false, + false + ) + + }.toSeq + }) def withRandomAccessPolicy(resourceTypeName: ResourceTypeName, members: Set[WorkbenchSubject]): StatefulMockAccessPolicyDaoBuilder = { val policy = AccessPolicy( diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceUnitSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceUnitSpec.scala index 91938664b..2487bc3d6 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceUnitSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceUnitSpec.scala @@ -40,64 +40,107 @@ class ResourceServiceUnitSpec extends AnyFlatSpec with Matchers with ScalaFuture FilterResourcesResult( ResourceId(UUID.randomUUID().toString), resourceTypeName, - AccessPolicyName(UUID.randomUUID().toString), - Option(Left(readerRoleName)), + Some(AccessPolicyName(UUID.randomUUID().toString)), + Some(readerRoleName), + Some(readAction), true, + None, + false, + false + ), + FilterResourcesResult( + ResourceId(UUID.randomUUID().toString), + resourceTypeName, + Some(AccessPolicyName(UUID.randomUUID().toString)), + None, + None, + true, + None, + false, + false + ), + FilterResourcesResult( + ResourceId(UUID.randomUUID().toString), + resourceTypeName, + Some(AccessPolicyName(UUID.randomUUID().toString)), + Some(readerRoleName), + Some(readAction), + false, + None, + false, false ), FilterResourcesResult( ResourceId(UUID.randomUUID().toString), resourceTypeName, - AccessPolicyName(UUID.randomUUID().toString), - Option(Left(readerRoleName)), + Some(AccessPolicyName(UUID.randomUUID().toString)), + None, + None, + false, + None, false, false ), // Testable DB Results - FilterResourcesResult(testResourceId, resourceTypeName, testPolicy1, Option(Left(readerRoleName)), false, false), + FilterResourcesResult(testResourceId, resourceTypeName, Some(testPolicy1), Some(readerRoleName), Some(readAction), false, None, false, false), FilterResourcesResult( testResourceId, resourceTypeName, - testPolicy1, - Option(Left(readerRoleName)), + Some(testPolicy1), + Some(readerRoleName), + Some(readAction), + false, + None, false, false ), // testing duplicate row results FilterResourcesResult( testResourceId, resourceTypeName, - testPolicy1, - Option(Left(readerRoleName)), + Some(testPolicy1), + Some(readerRoleName), + Some(readAction), + false, + None, false, false ), // testing duplicate row results - FilterResourcesResult(testResourceId, resourceTypeName, testPolicy2, Option(Left(nothingRoleName)), true, false), - FilterResourcesResult(testResourceId, resourceTypeName, testPolicy3, None, false, false), - FilterResourcesResult(testResourceId, resourceTypeName, testPolicy4, Option(Left(ownerRoleName)), false, false), - FilterResourcesResult(testResourceId, resourceTypeName, testPolicy4, Option(Left(ownerRoleName)), false, false), - FilterResourcesResult(testResourceId, resourceTypeName, testPolicy5, Option(Right(readAction)), true, false), + FilterResourcesResult(testResourceId, resourceTypeName, Some(testPolicy2), Some(nothingRoleName), None, true, None, false, false), + FilterResourcesResult(testResourceId, resourceTypeName, Some(testPolicy3), None, None, false, None, false, false), + FilterResourcesResult(testResourceId, resourceTypeName, Some(testPolicy4), Some(ownerRoleName), Some(readAction), false, None, false, false), + FilterResourcesResult(testResourceId, resourceTypeName, Some(testPolicy4), Some(ownerRoleName), Some(writeAction), false, None, false, false), + FilterResourcesResult(testResourceId, resourceTypeName, Some(testPolicy5), None, Some(readAction), true, None, false, false), FilterResourcesResult( testResourceId, resourceTypeName, - testPolicy5, - Option(Right(readAction)), + Some(testPolicy5), + None, + Some(readAction), true, + None, + false, false ), // testing duplicate row results // Auth Domain Results FilterResourcesResult( testResourceId2, resourceTypeName, - testPolicy6, - Option(Left(readerRoleName)), + Some(testPolicy6), + Some(readerRoleName), + Some(readAction), false, + Some(authDomainGroup1), + true, false ), FilterResourcesResult( testResourceId2, resourceTypeName, - testPolicy6, - Option(Left(readerRoleName)), + Some(testPolicy6), + Some(readerRoleName), + Some(readAction), + false, + Some(authDomainGroup2), false, false ) @@ -110,29 +153,15 @@ class ResourceServiceUnitSpec extends AnyFlatSpec with Matchers with ScalaFuture any[Set[ResourceTypeName]], any[Set[AccessPolicyName]], any[Set[ResourceRoleName]], + any[Set[ResourceAction]], any[Boolean], any[SamRequestContext] ) ) .thenReturn(IO.pure(dbResult)) - when(mockAccessPolicyDAO.listResourcesWithAuthdomains(any[ResourceTypeName], any[Set[ResourceId]], any[SamRequestContext])) - .thenReturn(IO.pure(Set(Resource(resourceTypeName, testResourceId2, Set(authDomainGroup1, authDomainGroup2), Set.empty)))) - when(mockAccessPolicyDAO.listAccessPolicies(eqTo(ManagedGroupService.managedGroupTypeName), any[WorkbenchUserId], any[SamRequestContext])) - .thenReturn(IO.pure(Set(ResourceIdAndPolicyName(ResourceId(authDomainGroup1.value), ManagedGroupService.memberPolicyName)))) val resourceService = new ResourceService( - Map( - resourceTypeName -> ResourceType( - resourceTypeName, - Set(ResourceActionPattern(readAction.value, "", true)), - Set( - ResourceRole(readerRoleName, Set(readAction)), - ResourceRole(ownerRoleName, Set(readAction, writeAction)), - ResourceRole(nothingRoleName, Set.empty, Set.empty) - ), - ownerRoleName - ) - ), + Map.empty, mock[PolicyEvaluatorService], mockAccessPolicyDAO, mock[DirectoryDAO], @@ -186,9 +215,6 @@ class ResourceServiceUnitSpec extends AnyFlatSpec with Matchers with ScalaFuture role.role should be(ownerRoleName) role.actions should be(Set(readAction, writeAction)) - policies.filter(_.policy.equals(testPolicy3)).flatMap(_.roles) should be(empty) - policies.filter(_.policy.equals(testPolicy3)).flatMap(_.actions) should be(empty) - val authDomainResource = filteredResources.resources.filter(_.resourceId.equals(testResourceId2)).head authDomainResource.resourceType should be(resourceTypeName) authDomainResource.policies.map(_.policy) should be(Set(testPolicy6))