From d2f4f7e737caa41aff270ca55a386485a5e90a1a Mon Sep 17 00:00:00 2001 From: Douglas Voet Date: Fri, 20 Dec 2024 16:27:20 -0500 Subject: [PATCH 1/2] bulk membership update implemented but not tested --- src/main/resources/swagger/api-docs.yaml | 88 +++++++++++++ .../workbench/sam/api/ResourceRoutes.scala | 16 ++- .../sam/api/SecurityDirectives.scala | 48 +++++++- .../sam/dataAccess/AccessPolicyDAO.scala | 7 ++ .../dataAccess/PostgresAccessPolicyDAO.scala | 32 ++++- .../sam/model/api/BulkMembershipUpdate.scala | 27 ++++ .../sam/service/ResourceService.scala | 116 ++++++++++-------- .../sam/dataAccess/MockAccessPolicyDAO.scala | 16 +++ .../sam/service/ResourceServiceSpec.scala | 44 ++----- 9 files changed, 302 insertions(+), 92 deletions(-) create mode 100644 src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index 7d7d82aff..d0746788b 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -1975,6 +1975,49 @@ paths: oneOf: - $ref: '#/components/schemas/FilteredResourcesFlatResponse' - $ref: '#/components/schemas/FilteredResourcesHierarchicalResponse' + /api/resources/v2/bulkMembershipUpdate: + post: + tags: + - Resources + summary: Bulk update the membership for resources + description: | + This endpoint is used to add and remove members of multiple policies across multiple resource in one api call. + Each policy is updated in order in its own transaction. If a policy does not exist on a resource, + processing stops with an error but all updates up to that point are committed. Missing member emails or policies fail fast. Additions are processed + before removals so if a subject is in both the add and remove lists for a policy, the ultimate result is removal. + This call is idempotent, if members being added already exist or members being removed do not exist, nothing will happen. + operationId: bulkMembershipUpdateV2 + requestBody: + description: The details of the membership updates + content: + 'application/json': + schema: + type: array + items: + $ref: '#/components/schemas/BulkMembershipUpdateRequestV2' + required: true + responses: + 200: + description: Successfully updated membership + content: { } + 400: + description: members or policies are not found + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' + 403: + description: Caller does not have permission to update the membership of one or more of the policies or one or more resources do not exist + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' + 500: + description: Internal Server Error + content: + application/json: + schema: + $ref: '#/components/schemas/ErrorReport' /api/resources/v2/{resourceTypeName}: get: tags: @@ -5062,6 +5105,51 @@ components: description: Actions on the role items: type: string + BulkMembershipUpdateRequestV2: + type: object + required: + - resourceTypeName + - resourceId + - policyUpdates + properties: + resourceTypeName: + type: string + description: The type of the resource + resourceId: + type: string + description: The id of the resource + policyUpdates: + type: array + items: + $ref: '#/components/schemas/PolicyMembershipUpdate' + PolicyMembershipUpdate: + type: object + required: + - policyName + properties: + policyName: + type: string + description: The name of the policy + addEmails: + type: array + description: Emails to add to the policy + items: + type: string + addPolicies: + type: array + description: Other policies to add to the specified policy + items: + $ref: '#/components/schemas/PolicyIdentifiers' + removeEmails: + type: array + description: Emails to remove from the policy + items: + type: string + removePolicies: + type: array + description: Other policies to remove from the specified policy + items: + $ref: '#/components/schemas/PolicyIdentifiers' securitySchemes: oidc: type: oauth2 diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala index 5f17cf02e..be4f0062c 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala @@ -14,7 +14,7 @@ import org.broadinstitute.dsde.workbench.sam.config.LiquibaseConfig import org.broadinstitute.dsde.workbench.sam.model.RootPrimitiveJsonSupport._ import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._ import org.broadinstitute.dsde.workbench.sam.model._ -import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, SamUser} +import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, BulkMembershipUpdate, SamUser} import org.broadinstitute.dsde.workbench.sam.model.api.FilteredResourcesHierarchical._ import org.broadinstitute.dsde.workbench.sam.model.api.FilteredResourcesFlat._ import org.broadinstitute.dsde.workbench.sam.service.ResourceService @@ -131,6 +131,20 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM listUserResources(samUser, samRequestContext) } } ~ + path("bulkMembershipUpdate") { + postWithTelemetry(samRequestContext) { + import BulkMembershipUpdate.bulkMembershipUpdateFormat + entity(as[Seq[BulkMembershipUpdate]]) { membershipUpdates => + verifyBulkMembershipUpdateAccess(membershipUpdates, samUser.id, samRequestContext) { + complete( + resourceService + .bulkMembershipUpdate(membershipUpdates, samRequestContext) + .map(_ => StatusCodes.OK) + ) + } + } + } + } ~ pathPrefix(Segment) { resourceTypeName => withNonAdminResourceType(ResourceTypeName(resourceTypeName)) { resourceType => pathEndOrSingleSlash { 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 index 662bf31c9..dca386efc 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SecurityDirectives.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SecurityDirectives.scala @@ -2,12 +2,14 @@ package org.broadinstitute.dsde.workbench.sam.api import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.server -import akka.http.scaladsl.server.Directives.onSuccess -import akka.http.scaladsl.server.{Directive0, Directives} +import akka.http.scaladsl.server.Directives.{complete, handleExceptions, onSuccess} +import akka.http.scaladsl.server.{Directive0, Directives, ExceptionHandler} import cats.effect.IO import org.broadinstitute.dsde.workbench.model.{ErrorReport, WorkbenchExceptionWithErrorReport, WorkbenchUserId} import org.broadinstitute.dsde.workbench.sam.ImplicitConversions.ioOnSuccessMagnet import org.broadinstitute.dsde.workbench.sam._ +import org.broadinstitute.dsde.workbench.sam.model.SamResourceTypes.resourceTypeAdminName +import org.broadinstitute.dsde.workbench.sam.model.api.BulkMembershipUpdate import org.broadinstitute.dsde.workbench.sam.model.{FullyQualifiedResourceId, ResourceAction, ResourceType, SamResourceActions, SamResourceTypes} import org.broadinstitute.dsde.workbench.sam.service.{PolicyEvaluatorService, ResourceService} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext @@ -211,4 +213,46 @@ trait SecurityDirectives { IO.pure(true) } } + + def verifyBulkMembershipUpdateAccess( + membershipUpdates: Seq[BulkMembershipUpdate], + userId: WorkbenchUserId, + samRequestContext: SamRequestContext + ): Directive0 = { + val requireDirectives = membershipUpdates.flatMap { membershipUpdate => + if (membershipUpdate.resourceTypeName == resourceTypeAdminName) { + Seq( + Directives.mapInnerRoute { innerRoute => + Directives.failWith( + new WorkbenchExceptionWithErrorReport( + ErrorReport(StatusCodes.BadRequest, "Please use the admin resourceTypes routes to view and make changes to admin resource policies.") + ) + ) + } + ) + } else { + val resource = FullyQualifiedResourceId(membershipUpdate.resourceTypeName, membershipUpdate.resourceId) + membershipUpdate.policyUpdates.map { policyUpdate => + requireOneOfAction( + resource, + Set(SamResourceActions.sharePolicy(policyUpdate.policyName), SamResourceActions.alterPolicies), + userId, + samRequestContext + ) + } + } + } + // requireOneOfAction may return Not Found but we want to return Forbidden instead in this case + changeNotFoundToForbidden & requireDirectives.foldLeft(Directives.pass)(_ & _) + } + + private val changeNotFoundToForbidden: Directive0 = { + import org.broadinstitute.dsde.workbench.model.ErrorReportJsonSupport._ + import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ + + handleExceptions(ExceptionHandler { + case withErrorReport: WorkbenchExceptionWithErrorReport if withErrorReport.errorReport.statusCode.contains(StatusCodes.NotFound) => + complete((StatusCodes.Forbidden, withErrorReport.errorReport.copy(statusCode = Option(StatusCodes.Forbidden)))) + }) + } } 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 755e027cd..6d16ff5f8 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 @@ -135,6 +135,13 @@ trait AccessPolicyDAO { samRequestContext: SamRequestContext ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] + def addAndRemovePolicyMembers( + policyId: FullyQualifiedPolicyId, + addSubjects: Set[WorkbenchSubject], + removeSubjects: Set[WorkbenchSubject], + samRequestContext: SamRequestContext + ): IO[Int] + } sealed abstract class LoadResourceAuthDomainResult 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 1bc3a0d9d..8037e90dc 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 @@ -1028,6 +1028,7 @@ from ${GroupMemberTable as groupMemberTable} override def overwritePolicyMembers(id: FullyQualifiedPolicyId, memberList: Set[WorkbenchSubject], samRequestContext: SamRequestContext): IO[Unit] = serializableWriteTransaction("overwritePolicyMembers", samRequestContext) { implicit session => overwritePolicyMembersInternal(id, memberList) + updateGroupUpdatedDateAndVersion(id) } // Steps: Delete every member from the underlying group and then add all of the new members. Do this in a *single* @@ -1038,7 +1039,6 @@ from ${GroupMemberTable as groupMemberTable} } removeAllGroupMembers(groupId) insertGroupMembers(groupId, memberList) - updateGroupUpdatedDateAndVersion(id) } override def overwritePolicy(newPolicy: AccessPolicy, samRequestContext: SamRequestContext): IO[AccessPolicy] = @@ -1056,7 +1056,7 @@ from ${GroupMemberTable as groupMemberTable} newPolicy ) setPolicyIsPublicInternal(policyPK, newPolicy.public) - + updateGroupUpdatedDateAndVersion(newPolicy.id) newPolicy } @@ -1661,7 +1661,11 @@ from ${GroupMemberTable as groupMemberTable} override def setPolicyIsPublic(policyId: FullyQualifiedPolicyId, isPublic: Boolean, samRequestContext: SamRequestContext): IO[Boolean] = serializableWriteTransaction("setPolicyIsPublic", samRequestContext) { implicit session => val policyPK = loadPolicyPK(policyId) - setPolicyIsPublicInternal(policyPK, isPublic) > 0 + val changed = setPolicyIsPublicInternal(policyPK, isPublic) > 0 + if (changed) { + updateGroupUpdatedDateAndVersion(policyId) + } + changed } override def getResourceParent(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Option[FullyQualifiedResourceId]] = { @@ -1975,6 +1979,28 @@ from ${GroupMemberTable as groupMemberTable} .filter(r => roles.isEmpty || r.role.exists(role => roles.contains(role))) .filter(r => actions.isEmpty || r.action.exists(action => actions.contains(action))) ++ privateResources + override def addAndRemovePolicyMembers( + policyId: FullyQualifiedPolicyId, + addSubjects: Set[WorkbenchSubject], + removeSubjects: Set[WorkbenchSubject], + samRequestContext: SamRequestContext + ): IO[Int] = + serializableWriteTransaction("addAndRemovePolicyMembers", samRequestContext) { implicit session => + val groupId = samsql"${workbenchGroupIdentityToGroupPK(policyId)}".map(rs => rs.get[GroupPK](1)).single().apply().getOrElse { + throw new WorkbenchException(s"Group for policy [$policyId] not found") + } + val insertedCount = insertGroupMembers(groupId, addSubjects) + // there is no bulk removeGroupMembers because figuring out which records to remove from the flat structure + // can't be done with an in clause because it needs to use array functions + val removedCount = removeSubjects + .map { subject => + removeGroupMember(policyId, subject) + } + .count(identity) // counts all that were true + updateGroupUpdatedDateAndVersion(policyId) + insertedCount + removedCount + } + private def recreateEffectivePolicyRolesTableEntry(resourceTypeNames: Set[ResourceTypeName])(implicit session: DBSession): Int = { val resource = ResourceTable.syntax("resource") val policyResource = ResourceTable.syntax("policyResource") diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala new file mode 100644 index 000000000..e928d8869 --- /dev/null +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala @@ -0,0 +1,27 @@ +package org.broadinstitute.dsde.workbench.sam.model.api + +import org.broadinstitute.dsde.workbench.model.WorkbenchEmail +import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicyName, PolicyIdentifiers, ResourceId, ResourceTypeName} +import spray.json.RootJsonFormat + +case class PolicyMembershipUpdate( + policyName: AccessPolicyName, + addEmails: Set[WorkbenchEmail], + addPolicies: Set[PolicyIdentifiers], + removeEmails: Set[WorkbenchEmail], + removePolicies: Set[PolicyIdentifiers] +) +object PolicyMembershipUpdate { + import spray.json.DefaultJsonProtocol._ + import SamJsonSupport._ + import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport._ + implicit val policyMembershipUpdateFormat: RootJsonFormat[PolicyMembershipUpdate] = jsonFormat5(PolicyMembershipUpdate.apply) +} + +case class BulkMembershipUpdate(resourceTypeName: ResourceTypeName, resourceId: ResourceId, policyUpdates: Seq[PolicyMembershipUpdate]) +object BulkMembershipUpdate { + import spray.json.DefaultJsonProtocol._ + import SamJsonSupport._ + import PolicyMembershipUpdate.policyMembershipUpdateFormat + implicit val bulkMembershipUpdateFormat: RootJsonFormat[BulkMembershipUpdate] = jsonFormat3(BulkMembershipUpdate.apply) +} 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 7447406be..123d24704 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 @@ -142,6 +142,46 @@ class ResourceService( createResource(resourceType, resourceId, defaultPolicies, Set.empty, None, samUser.id, samRequestContext) } + /** Adds and removes members from policies across multiple resources. Each policy is updated in order in its own transaction. If a resource or policy does not + * exist, processing stops with an error but all updates up to that point are committed. Missing member emails or policies fail fast. Additions are processed + * before removals so if a subject is in both the add and remove lists for a policy, the ultimate result is removal. + */ + def bulkMembershipUpdate(membershipUpdates: Seq[BulkMembershipUpdate], samRequestContext: SamRequestContext): IO[Unit] = { + val emails = membershipUpdates.flatMap(_.policyUpdates.flatMap(up => up.addEmails ++ up.removeEmails)).toSet + val memberPolicies = membershipUpdates.flatMap(_.policyUpdates.flatMap(up => up.addPolicies ++ up.removePolicies)).toSet + for { + emailsToSubjects <- mapEmailsToSubjects(emails, samRequestContext) + maybeEmailsError = validateMemberEmails(emailsToSubjects) + _ <- maybeRaiseBadRequest(maybeEmailsError) + + maybeMemberPoliciesError <- validateMemberPolicies(memberPolicies, samRequestContext) + _ <- maybeRaiseBadRequest(maybeMemberPoliciesError) + + _ <- membershipUpdates.toList.traverse { bulkMembershipUpdate => + val resource = FullyQualifiedResourceId(bulkMembershipUpdate.resourceTypeName, bulkMembershipUpdate.resourceId) + bulkMembershipUpdate.policyUpdates.toList.traverse { policyMembershipUpdate => + val policyId = FullyQualifiedPolicyId(resource, policyMembershipUpdate.policyName) + val addSubjects = policyMembershipUpdate.addEmails.flatMap(emailsToSubjects) ++ policyMembershipUpdate.addPolicies.map(_.toFullyQualifiedPolicyId) + val removeSubjects = + policyMembershipUpdate.removeEmails.flatMap(emailsToSubjects) ++ policyMembershipUpdate.removePolicies.map(_.toFullyQualifiedPolicyId) + for { + originalPolicies <- accessPolicyDAO.listAccessPolicies(policyId.resource, samRequestContext) + _ <- IO.raiseWhen(!originalPolicies.exists(_.id == policyId))( + new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, s"Policy $policyId not found")) + ) + count <- accessPolicyDAO.addAndRemovePolicyMembers(policyId, addSubjects, removeSubjects, samRequestContext) + _ <- onPolicyUpdateIfChanged(policyId, originalPolicies, samRequestContext)(count > 0) + } yield () + } + } + } yield () + } + + private def maybeRaiseBadRequest(maybeError: Option[ErrorReport]) = + IO.raiseWhen(maybeError.isDefined)( + new WorkbenchExceptionWithErrorReport(maybeError.get.copy(statusCode = Option(StatusCodes.BadRequest))) + ) + /** Validates the resource first and if any validations fail, an exception is thrown with an error report that describes what failed. If validations pass, * then the Resource should be persisted. * @@ -215,7 +255,9 @@ class ResourceService( private def constructAccessPolicy(resourceType: ResourceType, resourceId: ResourceId, validatableAccessPolicy: ValidatableAccessPolicy, public: Boolean) = AccessPolicy( FullyQualifiedPolicyId(FullyQualifiedResourceId(resourceType.name, resourceId), validatableAccessPolicy.policyName), - validatableAccessPolicy.emailsToSubjects.values.flatten.toSet, + validatableAccessPolicy.emailsToSubjects.values.flatten.toSet ++ validatableAccessPolicy.memberPolicies + .getOrElse(Set.empty) + .map(_.toFullyQualifiedPolicyId), generateGroupEmail(), validatableAccessPolicy.roles, validatableAccessPolicy.actions, @@ -365,7 +407,7 @@ class ResourceService( for { resourceType <- getResourceType(resource.resourceTypeName) error <- userId.map(validateAuthDomain(resourceType.get, authDomains, _, samRequestContext)).getOrElse(IO.none) - _ <- IO.raiseWhen(error.isDefined)(new WorkbenchExceptionWithErrorReport(error.get.copy(statusCode = Option(StatusCodes.BadRequest)))) + _ <- maybeRaiseBadRequest(error) resourceAndDescendants <- listResourceAndDescendants(resource, samRequestContext) _ <- resourceAndDescendants.traverse(validateNoPublicPolicies(_, samRequestContext)) _ <- resourceAndDescendants.traverse(accessPolicyDAO.addResourceAuthDomain(_, authDomains, samRequestContext)) @@ -586,7 +628,7 @@ class ResourceService( samRequestContext: SamRequestContext ): IO[AccessPolicy] = { val workbenchSubjects = policy.emailsToSubjects.values.flatten.toSet ++ - policy.memberPolicies.getOrElse(Set.empty).map(p => FullyQualifiedPolicyId(FullyQualifiedResourceId(p.resourceTypeName, p.resourceId), p.policyName)) + policy.memberPolicies.getOrElse(Set.empty).map(p => p.toFullyQualifiedPolicyId) accessPolicyDAO.listAccessPolicies(policyIdentity.resource, samRequestContext).flatMap { originalPolicies => originalPolicies.find(_.id == policyIdentity) match { case None => @@ -680,12 +722,17 @@ class ResourceService( private[service] def validatePolicy(resourceType: ResourceType, resourceId: ResourceId, policy: ValidatableAccessPolicy) = for { descendantPermissionsErrors <- validateDescendantPermissions(policy.descendantPermissions) + memberPolicyErrors <- policy.memberPolicies match { + case Some(memberPolicies) => validateMemberPolicies(memberPolicies, SamRequestContext()) + case None => IO.pure(None) + } } yield { val validationErrors = validateMemberEmails(policy.emailsToSubjects) ++ validateActions(resourceType, policy.actions) ++ validateRoles(resourceType, policy.roles) ++ validateUrlSafe(policy.policyName.value) ++ + memberPolicyErrors ++ descendantPermissionsErrors ++ validateResourceTypeAdminDescendantPermissions(resourceType, resourceId, policy.descendantPermissions) @@ -724,6 +771,18 @@ class ResourceService( } else None } + private def validateMemberPolicies(memberPolicies: Set[PolicyIdentifiers], samRequestContext: SamRequestContext): IO[Option[ErrorReport]] = + memberPolicies.toList.traverse { policy => + accessPolicyDAO.loadPolicy(policy.toFullyQualifiedPolicyId, samRequestContext).map { + case None => Some(ErrorReport(s"Invalid member policy: ${policy}")) + case _ => None + } + } map { errors => + if (errors.flatten.nonEmpty) { + Some(ErrorReport(s"You have specified at least one invalid member policy", errors.flatten)) + } else None + } + private[service] def validateRoles(resourceType: ResourceType, roles: Set[ResourceRoleName]) = { val invalidRoles = roles -- resourceType.roles.map(_.roleName) if (invalidRoles.nonEmpty) { @@ -778,10 +837,6 @@ class ResourceService( changeEvents = createAccessChangeEvents(policyId.resource, originalPolicies, updatedPolicies) _ <- AuditLogger.logAuditEventIO(samRequestContext, changeEvents.toSeq: _*) - _ <- directoryDAO.updateGroupUpdatedDateAndVersionWithSession( - FullyQualifiedPolicyId(policyId.resource, policyId.accessPolicyName), - samRequestContext - ) _ <- cloudExtensions.onGroupUpdate(Seq(policyId), removedMembers ++ addedMembers, samRequestContext).attempt.flatMap { case Left(regrets) => IO(logger.error(s"error calling cloudExtensions.onGroupUpdate for $policyId", regrets)) case Right(_) => IO.unit @@ -851,51 +906,14 @@ class ResourceService( ): IO[Set[ValidatableAccessPolicy]] = policies.toList .traverse { case (accessPolicyName, accessPolicyMembershipRequest) => - for { - // grouping member emails and policy emails together - allEmails <- getPolicyEmailsFromAccessPolicyMembership(accessPolicyMembershipRequest, samRequestContext) - .map { ioPolicyEmails => - for { - policyEmails <- ioPolicyEmails - } yield accessPolicyMembershipRequest.memberEmails ++ policyEmails - } - .getOrElse(IO(accessPolicyMembershipRequest.memberEmails)) - validatablePolicy <- makeValidatablePolicy( - accessPolicyName, - accessPolicyMembershipRequest.copy(memberEmails = allEmails), - samRequestContext - ) - } yield validatablePolicy + makeValidatablePolicy( + accessPolicyName, + accessPolicyMembershipRequest, + samRequestContext + ) } .map(_.toSet) - private def getPolicyEmailsFromAccessPolicyMembership( - accessPolicyMembership: AccessPolicyMembershipRequest, - samRequestContext: SamRequestContext - ): Option[IO[Set[WorkbenchEmail]]] = { - val maybePolicyEmails = accessPolicyMembership.memberPolicies.map { memberPolicy => - memberPolicy.map { memberPolicy => - val ownerPolicy = loadPolicyByPolicyIdentifiers(memberPolicy, samRequestContext) - ownerPolicy.map(p => p.map(_.email)) - } - } - // this mainly serves to simplify the complex structure of policy emails - maybePolicyEmails.map { policyEmails => - val listIOPolicyEmails = policyEmails.toList - val ioListPolicyEmails = listIOPolicyEmails.sequence - val ioSetPolicyEmails = for { - listPolicyEmails <- ioListPolicyEmails - } yield listPolicyEmails.flatten.toSet - ioSetPolicyEmails - } - } - - private def loadPolicyByPolicyIdentifiers(policyIdentifiers: PolicyIdentifiers, samRequestContext: SamRequestContext): IO[Option[AccessPolicy]] = - accessPolicyDAO.loadPolicy( - FullyQualifiedPolicyId(FullyQualifiedResourceId(policyIdentifiers.resourceTypeName, policyIdentifiers.resourceId), policyIdentifiers.policyName), - samRequestContext - ) - private def makeValidatablePolicy( accessPolicyName: AccessPolicyName, accessPolicyMembership: AccessPolicyMembershipRequest, 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 b9a00c3e4..944e22568 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 @@ -410,4 +410,20 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam override def listResourcesUsingAuthDomain(authDomainGroupName: WorkbenchGroupName, samRequestContext: SamRequestContext): IO[Set[FullyQualifiedResourceId]] = IO.pure(Set.empty) + + override def addAndRemovePolicyMembers( + policyId: FullyQualifiedPolicyId, + addSubjects: Set[WorkbenchSubject], + removeSubjects: Set[WorkbenchSubject], + samRequestContext: SamRequestContext + ): IO[Int] = + loadPolicy(policyId, samRequestContext).flatMap { + case None => throw new Exception("not found") + case Some(policy) => + val withAddedMembers = policy.members ++ addSubjects + val lessRemovedMembers = withAddedMembers -- removeSubjects + overwritePolicy(policy.copy(members = lessRemovedMembers), samRequestContext).map(_ => + (withAddedMembers.size - policy.members.size) + (withAddedMembers.size - lessRemovedMembers.size) + ) + } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala index 6a19133e9..81f2a04f9 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala @@ -1602,7 +1602,7 @@ class ResourceServiceSpec val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("policy-randomuuid@example.com"))) - assert(policies.contains(newPolicy.copy(version = 2))) + assert(policies.contains(newPolicy)) } it should "should add a memberPolicy as a member when specified through policy identifiers" in { @@ -1714,6 +1714,7 @@ class ResourceServiceSpec IO.pure(LazyList(accessPolicy)), // first call with empty membership IO.pure(LazyList(updatedPolicy)) // second call with updated membership ) + when(mockAccessPolicyDAO.loadPolicy(any[FullyQualifiedPolicyId], any[SamRequestContext])).thenReturn(IO.pure(Some(accessPolicy))) when(mockAccessPolicyDAO.overwritePolicy(any[AccessPolicy], any[SamRequestContext])).thenReturn(IO.pure(updatedPolicy)) when( mockCloudExtensions.onGroupUpdate( @@ -1723,13 +1724,6 @@ class ResourceServiceSpec ) ).thenReturn(IO.unit) - when( - mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession( - any[WorkbenchGroupIdentity], - any[SamRequestContext] - ) - ).thenReturn(IO.unit) - runAndWait( resourceService.overwritePolicy( defaultResourceType, @@ -1770,12 +1764,6 @@ class ResourceServiceSpec // function calls that should pass but what they return does not matter when(mockAccessPolicyDAO.overwritePolicy(ArgumentMatchers.eq(accessPolicy), any[SamRequestContext])).thenReturn(IO.pure(accessPolicy)) when(mockCloudExtensions.onGroupUpdate(ArgumentMatchers.eq(Seq(policyId)), ArgumentMatchers.eq(Set(member)), any[SamRequestContext])).thenReturn(IO.unit) - when( - mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession( - any[WorkbenchGroupIdentity], - any[SamRequestContext] - ) - ).thenReturn(IO.unit) // overwrite policy with no members runAndWait( @@ -1864,7 +1852,7 @@ class ResourceServiceSpec val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("policy-randomuuid@example.com"))) - assert(policies.contains(newPolicy.copy(version = 2))) + assert(policies.contains(newPolicy)) } it should "fail if any members are not test.firecloud.org accounts" in { @@ -1933,12 +1921,12 @@ class ResourceServiceSpec ) ) - runAndWait(service.overwritePolicyMembers(newPolicy.id, Set.empty, samRequestContext)) + runAndWait(service.overwritePolicyMembers(newPolicy.id, Set(dummyUser.email), samRequestContext)) val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("policy-randomuuid@example.com"))) - assert(policies.contains(newPolicy.copy(version = 2))) + assert(policies.contains(newPolicy.copy(version = 2, members = Set(dummyUser.id)))) } it should "call CloudExtensions.onGroupUpdate when members change" in { @@ -1968,12 +1956,6 @@ class ResourceServiceSpec // function calls that should pass but what they return does not matter when(mockAccessPolicyDAO.overwritePolicyMembers(ArgumentMatchers.eq(policyId), ArgumentMatchers.eq(Set.empty), any[SamRequestContext])).thenReturn(IO.unit) when(mockCloudExtensions.onGroupUpdate(ArgumentMatchers.eq(Seq(policyId)), ArgumentMatchers.eq(Set(member)), any[SamRequestContext])).thenReturn(IO.unit) - when( - mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession( - any[WorkbenchGroupIdentity], - any[SamRequestContext] - ) - ).thenReturn(IO.unit) // overwrite policy members with empty set runAndWait(resourceService.overwritePolicyMembers(policyId, Set.empty, samRequestContext)) @@ -2034,7 +2016,7 @@ class ResourceServiceSpec val policies = policyDAO.listAccessPolicies(resource, samRequestContext).unsafeRunSync() - assert(policies.contains(newPolicy.copy(version = 2))) + assert(policies.contains(newPolicy)) } it should "fail when given an invalid action" in { @@ -2623,12 +2605,6 @@ class ResourceServiceSpec IO.pure(LazyList(AccessPolicy(policyId, Set.empty, WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false))), IO.pure(LazyList(AccessPolicy(policyId, Set(member), WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false))) ) - when( - mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession( - any[WorkbenchGroupIdentity], - any[SamRequestContext] - ) - ).thenReturn(IO.unit) runAndWait(resourceService.addSubjectToPolicy(policyId, member, samRequestContext)) @@ -2687,12 +2663,6 @@ class ResourceServiceSpec IO.pure(LazyList(AccessPolicy(policyId, Set.empty, WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false))), IO.pure(LazyList(AccessPolicy(policyId, Set(member), WorkbenchEmail(""), Set.empty, Set.empty, Set.empty, false))) ) - when( - mockDirectoryDAO.updateGroupUpdatedDateAndVersionWithSession( - any[WorkbenchGroupIdentity], - any[SamRequestContext] - ) - ).thenReturn(IO.unit) runAndWait(resourceService.removeSubjectFromPolicy(policyId, member, samRequestContext)) @@ -3719,7 +3689,7 @@ class ResourceServiceSpec returnedPolicies should contain theSameElementsAs Set(expectedPolicy) policyDAO.loadPolicy(testPolicyId, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail(""))) shouldBe Some( - expectedPolicy.copy(version = 2) + expectedPolicy ) } From 1e042e7f07091c44c2acb218153e6095e89df6ad Mon Sep 17 00:00:00 2001 From: Douglas Voet Date: Wed, 8 Jan 2025 10:05:01 -0500 Subject: [PATCH 2/2] add id support --- src/main/resources/swagger/api-docs.yaml | 12 ++++++- .../sam/model/api/BulkMembershipUpdate.scala | 6 ++-- .../sam/service/ResourceService.scala | 32 ++++++++++++++----- 3 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index d0746788b..ceac66ea4 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -1983,7 +1983,7 @@ paths: description: | This endpoint is used to add and remove members of multiple policies across multiple resource in one api call. Each policy is updated in order in its own transaction. If a policy does not exist on a resource, - processing stops with an error but all updates up to that point are committed. Missing member emails or policies fail fast. Additions are processed + processing stops with an error but all updates up to that point are committed. Missing member ids, emails or policies fail fast. Additions are processed before removals so if a subject is in both the add and remove lists for a policy, the ultimate result is removal. This call is idempotent, if members being added already exist or members being removed do not exist, nothing will happen. operationId: bulkMembershipUpdateV2 @@ -5130,6 +5130,11 @@ components: policyName: type: string description: The name of the policy + addUserIds: + type: array + description: Sam user ids to add to the policy + items: + type: string addEmails: type: array description: Emails to add to the policy @@ -5140,6 +5145,11 @@ components: description: Other policies to add to the specified policy items: $ref: '#/components/schemas/PolicyIdentifiers' + removeUserIds: + type: array + description: Sam user ids to remove from the policy + items: + type: string removeEmails: type: array description: Emails to remove from the policy diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala index e928d8869..cc93b9296 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/BulkMembershipUpdate.scala @@ -1,13 +1,15 @@ package org.broadinstitute.dsde.workbench.sam.model.api -import org.broadinstitute.dsde.workbench.model.WorkbenchEmail +import org.broadinstitute.dsde.workbench.model.{WorkbenchEmail, WorkbenchUserId} import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicyName, PolicyIdentifiers, ResourceId, ResourceTypeName} import spray.json.RootJsonFormat case class PolicyMembershipUpdate( policyName: AccessPolicyName, + addUserIds: Set[WorkbenchUserId], addEmails: Set[WorkbenchEmail], addPolicies: Set[PolicyIdentifiers], + removeUserIds: Set[WorkbenchUserId], removeEmails: Set[WorkbenchEmail], removePolicies: Set[PolicyIdentifiers] ) @@ -15,7 +17,7 @@ object PolicyMembershipUpdate { import spray.json.DefaultJsonProtocol._ import SamJsonSupport._ import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport._ - implicit val policyMembershipUpdateFormat: RootJsonFormat[PolicyMembershipUpdate] = jsonFormat5(PolicyMembershipUpdate.apply) + implicit val policyMembershipUpdateFormat: RootJsonFormat[PolicyMembershipUpdate] = jsonFormat7(PolicyMembershipUpdate.apply) } case class BulkMembershipUpdate(resourceTypeName: ResourceTypeName, resourceId: ResourceId, policyUpdates: Seq[PolicyMembershipUpdate]) 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 123d24704..4829baece 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 @@ -152,18 +152,22 @@ class ResourceService( for { emailsToSubjects <- mapEmailsToSubjects(emails, samRequestContext) maybeEmailsError = validateMemberEmails(emailsToSubjects) - _ <- maybeRaiseBadRequest(maybeEmailsError) - maybeMemberPoliciesError <- validateMemberPolicies(memberPolicies, samRequestContext) - _ <- maybeRaiseBadRequest(maybeMemberPoliciesError) + maybeUserIdError <- validateUserIds(membershipUpdates.flatMap(_.policyUpdates.flatMap(up => up.addUserIds ++ up.removeUserIds)).toSet, samRequestContext) + + _ <- maybeRaiseBadRequest(maybeUserIdError ++ maybeEmailsError ++ maybeMemberPoliciesError) _ <- membershipUpdates.toList.traverse { bulkMembershipUpdate => val resource = FullyQualifiedResourceId(bulkMembershipUpdate.resourceTypeName, bulkMembershipUpdate.resourceId) bulkMembershipUpdate.policyUpdates.toList.traverse { policyMembershipUpdate => val policyId = FullyQualifiedPolicyId(resource, policyMembershipUpdate.policyName) - val addSubjects = policyMembershipUpdate.addEmails.flatMap(emailsToSubjects) ++ policyMembershipUpdate.addPolicies.map(_.toFullyQualifiedPolicyId) + val addSubjects = policyMembershipUpdate.addEmails.flatMap(emailsToSubjects) ++ policyMembershipUpdate.addPolicies.map( + _.toFullyQualifiedPolicyId + ) ++ policyMembershipUpdate.addUserIds val removeSubjects = - policyMembershipUpdate.removeEmails.flatMap(emailsToSubjects) ++ policyMembershipUpdate.removePolicies.map(_.toFullyQualifiedPolicyId) + policyMembershipUpdate.removeEmails.flatMap(emailsToSubjects) ++ policyMembershipUpdate.removePolicies.map( + _.toFullyQualifiedPolicyId + ) ++ policyMembershipUpdate.removeUserIds for { originalPolicies <- accessPolicyDAO.listAccessPolicies(policyId.resource, samRequestContext) _ <- IO.raiseWhen(!originalPolicies.exists(_.id == policyId))( @@ -177,9 +181,21 @@ class ResourceService( } yield () } - private def maybeRaiseBadRequest(maybeError: Option[ErrorReport]) = - IO.raiseWhen(maybeError.isDefined)( - new WorkbenchExceptionWithErrorReport(maybeError.get.copy(statusCode = Option(StatusCodes.BadRequest))) + private def validateUserIds(userIds: Set[WorkbenchUserId], samRequestContext: SamRequestContext): IO[Option[ErrorReport]] = + userIds.toList.traverse { userId => + directoryDAO.loadUser(userId, samRequestContext).map { + case None => Option(ErrorReport(s"User $userId not found")) + case _ => None + } + } map { errors => + if (errors.nonEmpty) { + Option(ErrorReport("Invalid user ids specified", errors.flatten)) + } else None + } + + private def maybeRaiseBadRequest(maybeErrors: Iterable[ErrorReport]) = + IO.raiseWhen(maybeErrors.nonEmpty)( + new WorkbenchExceptionWithErrorReport(ErrorReport("Bad Request", Option(StatusCodes.BadRequest), maybeErrors.toSeq, Seq.empty, None, None)) ) /** Validates the resource first and if any validations fail, an exception is thrown with an error report that describes what failed. If validations pass,