From 8997cefc124dfbcff06c43c0c5fa9ca3b792d0f2 Mon Sep 17 00:00:00 2001
From: Ralf Haferkamp <rhaferkamp@owncloud.com>
Date: Wed, 5 Jun 2024 14:52:00 +0200
Subject: [PATCH] graph/users: use ListRoleAssignmentsFiltered when filtering
 by role

When filtering users by role-assignment we now use the new ListRoleAssignmentsFiltered
request to the settings service instead calling ListRoleAssignments for every single user.
This reduces the number of network roundtrips to the settings service to one (was one per
user before).

Related Issue: #8938
---
 services/graph/pkg/service/v0/users_filter.go | 47 ++++++++++---------
 services/graph/pkg/service/v0/users_test.go   | 10 ++++
 2 files changed, 36 insertions(+), 21 deletions(-)

diff --git a/services/graph/pkg/service/v0/users_filter.go b/services/graph/pkg/service/v0/users_filter.go
index bf0d025b02f..d08e8243db5 100644
--- a/services/graph/pkg/service/v0/users_filter.go
+++ b/services/graph/pkg/service/v0/users_filter.go
@@ -6,6 +6,8 @@ import (
 
 	"github.com/CiscoM31/godata"
 	libregraph "github.com/owncloud/libre-graph-api-go"
+	settingsmsg "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0"
+	settingssvc "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0"
 )
 
 const (
@@ -339,6 +341,10 @@ func (g Graph) filterUsersByAppRoleID(ctx context.Context, req *godata.GoDataReq
 	// a user twice. The settings API, still has an issue that causes it to
 	// duplicate some assignments on restart:
 	// https://github.com/owncloud/ocis/issues/3432
+	usersByIdMap := make(map[string]*libregraph.User, len(users))
+	for _, user := range users {
+		usersByIdMap[user.GetId()] = user
+	}
 
 	var expand bool
 	exp := req.Query.GetExpand()
@@ -351,30 +357,29 @@ func (g Graph) filterUsersByAppRoleID(ctx context.Context, req *godata.GoDataReq
 		}
 	}
 
-	resultUsersMap := make(map[string]*libregraph.User, len(users))
-	for _, user := range users {
-		assignments, err := g.fetchAppRoleAssignments(ctx, user.GetId())
-		if err != nil {
-			return users, err
-		}
-		// To avoid re-fetching all assignments in the upper layer handler when
-		// the $expand query parameter is set, we're adding the assignments to the
-		// resulting users here already.
-		if expand {
-			user.AppRoleAssignments = assignments
-		}
-		for _, assignment := range assignments {
-			if assignment.GetAppRoleId() == id {
-				if _, ok := resultUsersMap[user.GetId()]; !ok {
-					resultUsersMap[user.GetId()] = user
-				}
+	assignmentsForRole, err := g.roleService.ListRoleAssignmentsFiltered(
+		ctx,
+		&settingssvc.ListRoleAssignmentsFilteredRequest{
+			Filters: []*settingsmsg.UserRoleAssignmentFilter{
+				{
+					Type: settingsmsg.UserRoleAssignmentFilter_TYPE_ROLE,
+					Term: &settingsmsg.UserRoleAssignmentFilter_RoleId{RoleId: id},
+				},
+			},
+		},
+	)
+	if err != nil {
+		return users, err
+	}
+	resultUsers := make([]*libregraph.User, 0, len(assignmentsForRole.GetAssignments()))
+	for _, assignment := range assignmentsForRole.GetAssignments() {
+		if user, ok := usersByIdMap[assignment.GetAccountUuid()]; ok {
+			if expand {
+				user.AppRoleAssignments = []libregraph.AppRoleAssignment{g.assignmentToAppRoleAssignment(assignment)}
 			}
+			resultUsers = append(resultUsers, user)
 		}
 	}
-	resultUsers := make([]*libregraph.User, 0, len(resultUsersMap))
-	for _, user := range resultUsersMap {
-		resultUsers = append(resultUsers, user)
-	}
 	return resultUsers, nil
 }
 
diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go
index 80698251f37..8e51f6c3bd3 100644
--- a/services/graph/pkg/service/v0/users_test.go
+++ b/services/graph/pkg/service/v0/users_test.go
@@ -517,6 +517,16 @@ var _ = Describe("Users", func() {
 			identityBackend.On("GetGroupMembers", mock.Anything, "25cb7bc0-3168-4a0c-adbe-396f478ad494", mock.Anything).Return(users, nil)
 			identityBackend.On("GetGroupMembers", mock.Anything, "2713f1d5-6822-42bd-ad56-9f6c55a3a8fa", mock.Anything).Return([]*libregraph.User{}, nil)
 			identityBackend.On("GetUsers", mock.Anything, mock.Anything).Return([]*libregraph.User{user}, nil)
+			roleService.On("ListRoleAssignmentsFiltered", mock.Anything, mock.Anything, mock.Anything).
+				Return(func(ctx context.Context, in *settings.ListRoleAssignmentsFilteredRequest, opts ...client.CallOption) *settings.ListRoleAssignmentsResponse {
+					return &settings.ListRoleAssignmentsResponse{Assignments: []*settingsmsg.UserRoleAssignment{
+						{
+							Id:          "some-appRoleAssignment-ID",
+							AccountUuid: user.GetId(),
+							RoleId:      "some-appRole-ID",
+						},
+					}}
+				}, nil)
 			roleService.On("ListRoleAssignments", mock.Anything, mock.Anything, mock.Anything).
 				Return(func(ctx context.Context, in *settings.ListRoleAssignmentsRequest, opts ...client.CallOption) *settings.ListRoleAssignmentsResponse {
 					return &settings.ListRoleAssignmentsResponse{Assignments: []*settingsmsg.UserRoleAssignment{