Merge pull request #211 from pixlise/feature/sub-group-sharing-bugs

Sub-Group Membership Inheritance Bug Fixes

api/endpoints/Image.go

@@ -75,10 +75,11 @@ func GetImage(params apiRouter.ApiHandlerStreamParams) (*s3.GetObjectOutput, str
 
 	// Check access to each associated scan. The user should already have a web socket open by this point, so we can
 	// look to see if there is a cached copy of their user group membership. If we don't find one, we stop
-	memberOfGroupIds, ok := wsHelpers.GetCachedUserGroupMembership(params.UserInfo.UserID)
-	if !ok {
+	memberOfGroupIds, isMemberOfNoGroups := wsHelpers.GetCachedUserGroupMembership(params.UserInfo.UserID)
+	viewerOfGroupIds, isViewerOfNoGroups := wsHelpers.GetCachedUserGroupViewership(params.UserInfo.UserID)
+	if !isMemberOfNoGroups && !isViewerOfNoGroups {
 		// User is probably not logged in
-		return nil, "", "", "", 0, errorwithstatus.MakeBadRequestError(errors.New("User group membership not found, can't determine permissions"))
+		return nil, "", "", "", 0, errorwithstatus.MakeBadRequestError(errors.New("User has no group membership, can't determine permissions"))
 	}
 
 	// Now read the DB record for the image, so we can determine what scans it's associated with
@@ -103,7 +104,7 @@ func GetImage(params apiRouter.ApiHandlerStreamParams) (*s3.GetObjectOutput, str
 	}
 
 	for _, scanId := range dbImage.AssociatedScanIds {
-		_, err := wsHelpers.CheckObjectAccessForUser(false, scanId, protos.ObjectType_OT_SCAN, params.UserInfo.UserID, memberOfGroupIds, params.Svcs.MongoDB)
+		_, err := wsHelpers.CheckObjectAccessForUser(false, scanId, protos.ObjectType_OT_SCAN, params.UserInfo.UserID, memberOfGroupIds, viewerOfGroupIds, params.Svcs.MongoDB)
 		if err != nil {
 			return nil, "", "", "", 0, err
 		}

api/ws/wsHelpers/ownership.go

@@ -37,10 +37,10 @@ func MakeOwnerForWrite(objectId string, objectType protos.ObjectType, creatorUse
 // otherwise just checks for view access. Returns an error if it failed to determine
 // or if access is not granted, returns error formed with MakeUnauthorisedError
 func CheckObjectAccess(requireEdit bool, objectId string, objectType protos.ObjectType, hctx HandlerContext) (*protos.OwnershipItem, error) {
-	return CheckObjectAccessForUser(requireEdit, objectId, objectType, hctx.SessUser.User.Id, hctx.SessUser.MemberOfGroupIds, hctx.Svcs.MongoDB)
+	return CheckObjectAccessForUser(requireEdit, objectId, objectType, hctx.SessUser.User.Id, hctx.SessUser.MemberOfGroupIds, hctx.SessUser.ViewerOfGroupIds, hctx.Svcs.MongoDB)
 }
 
-func CheckObjectAccessForUser(requireEdit bool, objectId string, objectType protos.ObjectType, userId string, memberOfGroupIds []string, db *mongo.Database) (*protos.OwnershipItem, error) {
+func CheckObjectAccessForUser(requireEdit bool, objectId string, objectType protos.ObjectType, userId string, memberOfGroupIds []string, viewerOfGroupIds []string, db *mongo.Database) (*protos.OwnershipItem, error) {
 	ownerCollectionId := objectId
 
 	result := db.Collection(dbCollections.OwnershipName).FindOne(context.TODO(), bson.M{"_id": ownerCollectionId})
@@ -88,6 +88,15 @@ func CheckObjectAccessForUser(requireEdit bool, objectId string, objectType prot
 						return ownership, nil // User has access via group it belongs to
 					}
 				}
+
+				if !requireEdit {
+					// If we don't require editing, check if the user is a viewer of any of the groups too
+					for _, groupId := range viewerOfGroupIds {
+						if utils.ItemInSlice(groupId, toCheckItem.GroupIds) {
+							return ownership, nil // User has access via group it belongs to
+						}
+					}
+				}
 			}
 		}
 	}
@@ -107,12 +116,19 @@ func ListAccessibleIDs(requireEdit bool, objectType protos.ObjectType, svcs *ser
 		idLookups = append(idLookups, bson.D{{Key: "viewers.userids", Value: requestorSession.User.Id}})
 	}
 
-	// Add the group IDs
-	for _, groupId := range requestorSession.MemberOfGroupIds {
-		idLookups = append(idLookups, bson.D{{Key: "editors.groupids", Value: groupId}})
-		if !requireEdit {
+	// If we don't require editing, then we just care if the uesr can see the ID
+	if !requireEdit {
+		allGroupsUserIsIn := append(requestorSession.MemberOfGroupIds, requestorSession.ViewerOfGroupIds...)
+
+		for _, groupId := range allGroupsUserIsIn {
+			idLookups = append(idLookups, bson.D{{Key: "editors.groupids", Value: groupId}})
 			idLookups = append(idLookups, bson.D{{Key: "viewers.groupids", Value: groupId}})
 		}
+	} else {
+		// If we require editing, then we only care if the user can edit the ID
+		for _, groupId := range requestorSession.MemberOfGroupIds {
+			idLookups = append(idLookups, bson.D{{Key: "editors.groupids", Value: groupId}})
+		}
 	}
 
 	filter := bson.D{
@@ -199,9 +215,6 @@ func FetchOwnershipSummary(ownership *protos.OwnershipItem, sessionUser SessionU
 		}
 	}
 
-	// Still have to be an editor even if you're the creator
-	result.CanEdit = false
-
 	if ownership.Viewers != nil {
 		result.ViewerUserCount = uint32(len(ownership.Viewers.UserIds))
 

api/ws/wsHelpers/sessionUser.go

@@ -19,6 +19,7 @@ type SessionUser struct {
 	User                   *protos.UserInfo
 	Permissions            map[string]bool
 	MemberOfGroupIds       []string
+	ViewerOfGroupIds       []string
 	NotificationSubscribed bool
 }
 
@@ -38,6 +39,7 @@ func GetSessionUser(s *melody.Session) (SessionUser, error) {
 }
 
 var cachedUserGroupMembership = map[string][]string{}
+var cachedUserGroupViewership = map[string][]string{}
 
 // JWT user has the user ID and permissions that we get from Auth0. The rest is handled
 // within PIXLISE, so lets read our DB to see if this user exists and get their
@@ -84,7 +86,7 @@ func CreateDBUser(sessionId string, jwtUser jwtparser.JWTUserInfo, db *mongo.Dat
 }
 
 func makeSessionUser(userId string, sessionId string, permissions map[string]bool, userDBItem *protos.UserDBItem, db *mongo.Database) (*SessionUser, error) {
-	ourGroups := map[string]bool{}
+	ourGroups := map[string]bool{} // Map of group IDs we are members of - true for members, false for viewers
 
 	// Now we read all the groups and find which ones we are members of
 	filter := bson.D{}
@@ -105,46 +107,77 @@ func makeSessionUser(userId string, sessionId string, permissions map[string]boo
 			if utils.ItemInSlice(userId, userGroup.Members.UserIds) {
 				ourGroups[userGroup.Id] = true
 			}
-		} else if userGroup.Viewers != nil {
+		}
+
+		if _, userInGroup := ourGroups[userGroup.Id]; userGroup.Viewers != nil && !userInGroup {
 			if utils.ItemInSlice(userId, userGroup.Viewers.UserIds) {
-				ourGroups[userGroup.Id] = true
+				ourGroups[userGroup.Id] = false
 			}
 		}
 	}
 
 	// Finally, if we are in a group which itself is also within a group, find again
 	// TODO: This may not detect outside of 2 levels deep grouping, we may want more...
 	for _, userGroup := range userGroups {
-		for groupToCheck, _ := range ourGroups {
+		if userGroup.Members == nil && userGroup.Viewers == nil {
+			continue
+		}
+
+		// If we are already a member of this group, we don't need to check for additional permissions
+		if _, isMemberOfGroup := ourGroups[userGroup.Id]; isMemberOfGroup {
+			continue
+		}
+
+		// Check if any group we're a member of is a member of this group
+		for groupToCheck, isMemberOfGroupToCheck := range ourGroups {
 			if userGroup.Id != groupToCheck {
 				if userGroup.Members != nil {
+					// If a group we're in is a member of this group, we have the same permissions (eg. viewer of a member group is still a viewer)
 					if utils.ItemInSlice(groupToCheck, userGroup.Members.GroupIds) {
-						ourGroups[userGroup.Id] = true
+						ourGroups[userGroup.Id] = isMemberOfGroupToCheck
 					}
-				} else if userGroup.Viewers != nil {
+				}
+
+				if _, userInGroup := ourGroups[userGroup.Id]; userGroup.Viewers != nil && !userInGroup {
 					if utils.ItemInSlice(groupToCheck, userGroup.Viewers.GroupIds) {
-						ourGroups[userGroup.Id] = true
+						ourGroups[userGroup.Id] = false
 					}
 				}
 			}
 		}
 	}
 
-	memberOfGroups := utils.GetMapKeys(ourGroups)
+	memberOfGroups := []string{}
+	viewerOfGroups := []string{}
+
+	for item, isMember := range ourGroups {
+		if isMember {
+			memberOfGroups = append(memberOfGroups, item)
+		} else {
+			viewerOfGroups = append(viewerOfGroups, item)
+		}
+	}
 
 	// Any time we create a session user, we cache the list of groups it's a member of
 	// so that HTTP endpoints can also access this and determine permissions properly
 	cachedUserGroupMembership[userId] = memberOfGroups
+	cachedUserGroupViewership[userId] = viewerOfGroups
 
 	return &SessionUser{
 		SessionId:        sessionId,
 		User:             userDBItem.Info,
 		Permissions:      permissions,
 		MemberOfGroupIds: memberOfGroups,
+		ViewerOfGroupIds: viewerOfGroups,
 	}, nil
 }
 
 func GetCachedUserGroupMembership(userId string) ([]string, bool) {
 	membership, ok := cachedUserGroupMembership[userId]
 	return membership, ok
 }
+
+func GetCachedUserGroupViewership(userId string) ([]string, bool) {
+	membership, ok := cachedUserGroupViewership[userId]
+	return membership, ok
+}

internal/cmd-line-tools/api-integration-test/testImage.go

@@ -148,7 +148,7 @@ func testImageGet_NoMembership(apiHost string, jwt string) {
 	status, body, err := doGet("http", apiHost, imagePath, "", jwt)
 
 	failIf(err != nil, err)
-	failIf(string(body) != "User group membership not found, can't determine permissions\n" || status != 400, fmt.Errorf("Unexpected response! Status %v, body: %v", status, string(body)))
+	failIf(string(body) != "User has no group membership, can't determine permissions\n" || status != 400, fmt.Errorf("Unexpected response! Status %v, body: %v", status, string(body)))
 }
 
 func testImageGet_OK(apiHost string, jwt string) {