doc fixes, sync right group

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala

@@ -165,21 +165,24 @@ class GoogleExtensions(
 
   /*
     - managed groups and access policies are both "groups"
-    - You can have a bunch of workspaces inside an auth domain (a type of managed group).
-    - A user must be a member of the auth domain in order to access any of the workspaces in that auth domain.
-    - If a workspace is in multiple auth domains, the user must be a member of all of them in order to access that workspace
-    - An access policy is specific to a single workspace (or other resource) - so you must be an Owner/Writer/... whatever
-      in order to be able to do the relevant action on that workspace
-    - To access a workspace, a user must BOTH be a member in all the auth domains that workspace is in AND also be a member
-      in one of the access policy groups.
-    - When someone gets added to an managed group or access policy, we have to figure out which google buckets they suddenly have access to.
+    - You can have a bunch of resources constrained an auth domain (a collection of managed groups).
+    - A user must be a member of the auth domain in order to access some actions on the resources in that auth domain.
+    - The user must be a member of all groups in an auth domain in order to access a resource
+    - An access policy is specific to a single resource
+    - To access an action on a resource, a user must BOTH be a member of the auth domain of the resource AND also be a member
+      of one of the access policies that has the action.
+    - When someone gets added to an managed group or access policy, we have to figure out which google groups they suddenly have access to.
     - To do this, when someone gets added to a group, we call onGroupUpdate, which retrieves a list of all of the groups that are affected
       by the groupUpdate, and for each publishes a message to a pub/sub queue telling a Sam background process
-      to sync the user's access for all of those access policy groups.
+      to sync the user's access for all of those groups.
     - To figure out which groups are affected - we first separate access policies and managed groups.
       For each access policy passed to onGroupUpdate, we publish a message to sync just that group
-      For each managed group passed to onGroupUpdate, we first find all of the ancestor groups of that group and then all of the access policies
-      for each of those groups and then publish a messsage to sync each of those access policies.
+      For each managed group passed to onGroupUpdate (these might be used in auth domains)
+        - find all ancestor groups because the updated group may be a sub group
+        - find resources that have any of these groups in their auth domain
+        - publish a message for each access policy of all those resources
+
+     see GoogleGroupSynchronizer for the background process that does the group synchronization
    */
   override def onGroupUpdate(groupIdentities: Seq[WorkbenchGroupIdentity]): Future[Unit] = {
     for {
@@ -190,13 +193,15 @@ class GoogleExtensions(
 
       // make all the publish messages for the previously synced groups
       messages <- previouslySyncedIds.traverse {
-          // if it's a group that isn't an access policy
-          case groupName: WorkbenchGroupName => makeGroupPublishMessages(groupName)
-          // if it's the access policy of a managed group
-          case FullyQualifiedPolicyId(FullyQualifiedResourceId(ManagedGroupService.managedGroupTypeName, id: ResourceId),
-                                                 ManagedGroupService.adminPolicyName | ManagedGroupService.memberPolicyName) =>
-            makeGroupPublishMessages(WorkbenchGroupName(id.value))
-          // if it's an access policy on a resource that's not a managed group
+          // it is a group that isn't an access policy, could be a managed group
+          case groupName: WorkbenchGroupName =>
+            makeConstrainedResourceAccessPolicyMessages(groupName).map(_  :+ groupName.toJson.compactPrint)
+
+          // it is the admin or member access policy of a managed group
+          case accessPolicyId@FullyQualifiedPolicyId(FullyQualifiedResourceId(ManagedGroupService.managedGroupTypeName, id: ResourceId), ManagedGroupService.adminPolicyName | ManagedGroupService.memberPolicyName) =>
+            makeConstrainedResourceAccessPolicyMessages(WorkbenchGroupName(id.value)).map(_  :+ accessPolicyId.toJson.compactPrint)
+
+          // it is an access policy on a resource that's not a managed group
           case accessPolicyId: FullyQualifiedPolicyId => IO.pure(List(accessPolicyId.toJson.compactPrint))
       }
 
@@ -206,22 +211,22 @@ class GoogleExtensions(
     } yield ()
   }.unsafeToFuture()
 
-  private def makeGroupPublishMessages(groupName: WorkbenchGroupName): IO[List[String]] = {
+  private def makeConstrainedResourceAccessPolicyMessages(groupIdentity: WorkbenchGroupIdentity): IO[List[String]] = {
    // start with a group
     for {
       // get all the ancestors of that group
-      ancestorGroupsOfManagedGroups <- directoryDAO.listAncestorGroups(groupName)
+      ancestorGroupsOfManagedGroups <- directoryDAO.listAncestorGroups(groupIdentity)
 
       // get all the ids of the group and its ancestors
-      managedGroupIds = (ancestorGroupsOfManagedGroups + groupName).collect {
+      managedGroupIds = (ancestorGroupsOfManagedGroups + groupIdentity).collect {
         case FullyQualifiedPolicyId(FullyQualifiedResourceId(ManagedGroupService.managedGroupTypeName, id), ManagedGroupService.adminPolicyName | ManagedGroupService.memberPolicyName) => id
       }
 
       // get all access policies on any resource that is constrained by the groups
       constrainedResourceAccessPolicies <- managedGroupIds.toList.traverse(id => getAccessPoliciesOnResourcesConstrainedByGroup(id))
 
       // return messages for all the affected access policies and the original group we started with
-    } yield constrainedResourceAccessPolicies.flatten.map(accessPolicy => accessPolicy.id.toJson.compactPrint) :+ groupName.toJson.compactPrint
+    } yield constrainedResourceAccessPolicies.flatten.map(accessPolicy => accessPolicy.id.toJson.compactPrint)
   }
 
   private def publishMessages(messages: Seq[String]): Future[Unit] = {

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleGroupSynchronizer.scala

@@ -14,6 +14,25 @@ import org.broadinstitute.dsde.workbench.util.FutureSupport
 import scala.concurrent.{ExecutionContext, Future}
 import scala.util.{Failure, Success, Try}
 
+/**
+  * This class makes sure that our google groups have the right members.
+  *
+  * For the simple case it merely compares
+  * group membership given by directoryDAO against group membership given by googleDirectoryDAO and does the
+  * appropriate adds and removes to google so that they look the same.
+  *
+  * The more complicated case involves resources contsrained by an auth domain. If a resource is constrained by an
+  * auth domain AND the policy being synchronized has actions or roles configured as contstrainable then we need to
+  * synchronize the *intersection* of the members of all the groups in the auth domain and the access policy. These
+  * are called intersection groups. In order to do this accurately all the groups must be unrolled (flattened).
+  *
+  * @param directoryDAO
+  * @param accessPolicyDAO
+  * @param googleDirectoryDAO
+  * @param googleExtensions
+  * @param resourceTypes
+  * @param executionContext
+  */
 class GoogleGroupSynchronizer(directoryDAO: DirectoryDAO,
                               accessPolicyDAO: AccessPolicyDAO,
                               googleDirectoryDAO: GoogleDirectoryDAO,
@@ -108,6 +127,14 @@ class GoogleGroupSynchronizer(directoryDAO: DirectoryDAO,
     }
   }
 
+  /**
+    * An access policy is constrainable if it contains an action or a role that contains an action that is
+    * configured as constrainable in the resource type definition.
+    *
+    * @param resource
+    * @param accessPolicy
+    * @return
+    */
   private[google] def isConstrainable(resource: FullyQualifiedResourceId, accessPolicy: AccessPolicy): Boolean =
     resourceTypes.get(resource.resourceTypeName) match {
       case Some(resourceType) =>
@@ -126,17 +153,19 @@ class GoogleGroupSynchronizer(directoryDAO: DirectoryDAO,
     }
 
   private[google] def calculateIntersectionGroup(resource: FullyQualifiedResourceId, policy: AccessPolicy): IO[Set[WorkbenchSubject]] = {
-    // if the policy has no members, there will be no intersections
-    if (policy.members.isEmpty)
+    // if the policy has no members, the intersection will be empty so short circuit here
+    if (policy.members.isEmpty) {
       IO.pure(Set())
-    else {
+    } else {
       for {
         result <- accessPolicyDAO.loadResourceAuthDomain(resource)
         members <- result match {
           case LoadResourceAuthDomainResult.Constrained(groups) =>
+            // auth domain exists, need to calculate intersection
             val groupsIdentity: Set[WorkbenchGroupIdentity] = groups.toList.toSet
             directoryDAO.listIntersectionGroupUsers(groupsIdentity + policy.id).map(_.map(_.asInstanceOf[WorkbenchSubject])) //Doesn't seem like I can avoid the asInstanceOf, would be interested to know if there's a way
           case LoadResourceAuthDomainResult.NotConstrained | LoadResourceAuthDomainResult.ResourceNotFound =>
+            // auth domain does not exist, return policy members as is
             IO.pure(policy.members)
         }
       } yield members