Skip to content

Commit

Permalink
graph/users: use ListRoleAssignmentsFiltered when filtering by role
Browse files Browse the repository at this point in the history
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: owncloud#8938
  • Loading branch information
rhafer committed Jun 5, 2024
1 parent 53dc2ed commit f7cb716
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 21 deletions.
47 changes: 26 additions & 21 deletions services/graph/pkg/service/v0/users_filter.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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()
Expand All @@ -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
}

Expand Down
10 changes: 10 additions & 0 deletions services/graph/pkg/service/v0/users_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down

0 comments on commit f7cb716

Please sign in to comment.