From 1dab0f7a54fed67a27aacecb1d79f731059a3d38 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 16 Feb 2023 10:50:16 +0100 Subject: [PATCH] graph: Improve appRoleAssignemts filters This should improve the processing of filters for appRoleAssignments a bit when combining them with other filters. We try to avoid reading the full user list if possible. And delay the processing of an appRoleAssignments filter so we can apply it on a subset of user. E.g. a filter: `appRoleAssignments/any(m:m/appRoleId eq 71881883-1768-46bd-a24d-a356a2afdf7f) and memberOf/any(m:m/id eq 509a9dcd-bb37-4f4f-a01a-19dca27d9cfa)` Will be reordered to first process the memberOf filter (which can be executed without reading the full user list) and only apply the appRoleAssignments filter on the resultset of the memberOf filter. --- services/graph/pkg/service/v0/users_filter.go | 143 ++++++++++++++---- services/graph/pkg/service/v0/users_test.go | 11 ++ 2 files changed, 124 insertions(+), 30 deletions(-) diff --git a/services/graph/pkg/service/v0/users_filter.go b/services/graph/pkg/service/v0/users_filter.go index 8f46d4d12..a836f5378 100644 --- a/services/graph/pkg/service/v0/users_filter.go +++ b/services/graph/pkg/service/v0/users_filter.go @@ -8,6 +8,10 @@ import ( libregraph "github.com/owncloud/libre-graph-api-go" ) +const ( + appRoleID = "appRoleId" +) + func invalidFilterError() error { return godata.BadRequestError("invalid filter") } @@ -53,22 +57,51 @@ func (g Graph) applyFilterLogical(ctx context.Context, req *godata.GoDataRequest func (g Graph) applyFilterLogicalAnd(ctx context.Context, req *godata.GoDataRequest, operand1 *godata.ParseNode, operand2 *godata.ParseNode) (users []*libregraph.User, err error) { logger := g.logger.SubloggerWithRequestID(ctx) - results := make([][]*libregraph.User, 0, 2) + var res1, res2 []*libregraph.User - for _, node := range []*godata.ParseNode{operand1, operand2} { - res, err := g.applyUserFilter(ctx, req, node) - if err != nil { - return users, err + // The appRoleAssignmentFilter requires a full list of user, get try to avoid a full user listing and + // process the other part of the filter first, if that is an appRoleAssignmentFilter as well, we have + // to bite the bullet and get a full user listing anyway + if matched, property, value := g.isAppRoleAssignmentFilter(ctx, operand1); matched { + logger.Debug().Str("property", property).Str("value", value).Msg("delay processing of approleAssignments filter") + if property != appRoleID { + return users, unsupportedFilterError() } - logger.Debug().Interface("subfilter", res).Msg("result part") - results = append(results, res) + res2, err = g.applyUserFilter(ctx, req, operand2) + if err != nil { + return []*libregraph.User{}, err + } + logger.Debug().Str("property", property).Str("value", value).Msg("applying approleAssignments filter on result set") + return g.filterUsersByAppRoleID(ctx, value, res2) } - // 'results' contains two slices of libregraph.Users now turn one of them + // 1st part is no appRoleAssignmentFilter, run the filter query + res1, err = g.applyUserFilter(ctx, req, operand1) + if err != nil { + return []*libregraph.User{}, err + } + + // Now check 2nd part for appRoleAssignmentFilter and apply that using the result return from the first + // filter + if matched, property, value := g.isAppRoleAssignmentFilter(ctx, operand2); matched { + if property != appRoleID { + return users, unsupportedFilterError() + } + logger.Debug().Str("property", property).Str("value", value).Msg("applying approleAssignments filter on result set") + return g.filterUsersByAppRoleID(ctx, value, res1) + } + + // 2nd part is no appRoleAssignmentFilter either + res2, err = g.applyUserFilter(ctx, req, operand2) + if err != nil { + return []*libregraph.User{}, err + } + + // We now have two slice with results of the subfilters. Now turn one of them // into a map for efficiently getting the intersection of both slices - userSet := userSliceToMap(results[0]) + userSet := userSliceToMap(res1) var filteredUsers []*libregraph.User - for _, user := range results[1] { + for _, user := range res2 { if _, found := userSet[user.GetId()]; found { filteredUsers = append(filteredUsers, user) } @@ -132,16 +165,11 @@ func (g Graph) applyMemberOfEq(ctx context.Context, req *godata.GoDataRequest, n switch nodes[0].Children[1].Token.Value { case "id": - var filterValue string - switch nodes[1].Token.Type { - case godata.ExpressionTokenGuid: - filterValue = nodes[1].Token.Value - case godata.ExpressionTokenString: - // unquote - filterValue = strings.Trim(nodes[1].Token.Value, "'") - default: + filterValue, err := g.getUUIDTokenValue(ctx, nodes[1]) + if err != nil { return users, unsupportedFilterError() } + logger.Debug().Str("property", nodes[0].Children[1].Token.Value).Str("value", filterValue).Msg("Filtering memberOf by group id") return g.identityBackend.GetGroupMembers(ctx, filterValue, req) default: @@ -180,15 +208,9 @@ func (g Graph) applyAppRoleAssignmentEq(ctx context.Context, req *godata.GoDataR return users, invalidFilterError() } - if nodes[0].Children[1].Token.Value == "appRoleId" { - var filterValue string - switch nodes[1].Token.Type { - case godata.ExpressionTokenGuid: - filterValue = nodes[1].Token.Value - case godata.ExpressionTokenString: - // unquote - filterValue = strings.Trim(nodes[1].Token.Value, "'") - default: + if nodes[0].Children[1].Token.Value == appRoleID { + filterValue, err := g.getUUIDTokenValue(ctx, nodes[1]) + if err != nil { return users, unsupportedFilterError() } @@ -197,12 +219,12 @@ func (g Graph) applyAppRoleAssignmentEq(ctx context.Context, req *godata.GoDataR return users, err } - return g.filterUsersByAppRoleId(ctx, filterValue, users) + return g.filterUsersByAppRoleID(ctx, filterValue, users) } return users, unsupportedFilterError() } -func (g Graph) filterUsersByAppRoleId(ctx context.Context, appRoleId string, users []*libregraph.User) ([]*libregraph.User, error) { +func (g Graph) filterUsersByAppRoleID(ctx context.Context, id string, users []*libregraph.User) ([]*libregraph.User, error) { // We're using a map for the results here, in order to avoid returning // a user twice. The settings API, still has an issue that causes it to // duplicate some assignments on restart: @@ -214,7 +236,7 @@ func (g Graph) filterUsersByAppRoleId(ctx context.Context, appRoleId string, use return users, err } for _, assignment := range assignments { - if assignment.GetAppRoleId() == appRoleId { + if assignment.GetAppRoleId() == id { if _, ok := resultUsersMap[user.GetId()]; !ok { resultUsersMap[user.GetId()] = user } @@ -228,6 +250,67 @@ func (g Graph) filterUsersByAppRoleId(ctx context.Context, appRoleId string, use return resultUsers, nil } +func (g Graph) getUUIDTokenValue(ctx context.Context, node *godata.ParseNode) (string, error) { + var value string + switch node.Token.Type { + case godata.ExpressionTokenGuid: + value = node.Token.Value + case godata.ExpressionTokenString: + // unquote + value = strings.Trim(node.Token.Value, "'") + default: + return "", unsupportedFilterError() + } + return value, nil +} + +func (g Graph) isAppRoleAssignmentFilter(ctx context.Context, node *godata.ParseNode) (match bool, property string, filter string) { + if node.Token.Type != godata.ExpressionTokenLambdaNav { + return false, "", "" + } + + if len(node.Children) != 2 { + return false, "", "" + } + + if node.Children[0].Token.Type != godata.ExpressionTokenLiteral || node.Children[0].Token.Value != "appRoleAssignments" { + return false, "", "" + } + + if node.Children[1].Token.Type != godata.ExpressionTokenLambda || node.Children[1].Token.Value != "any" { + return false, "", "" + } + + if len(node.Children[1].Children) != 2 { + return false, "", "" + } + lambdaParam := node.Children[1].Children + // We only support the 'eq' expression for now + if lambdaParam[1].Token.Type != godata.ExpressionTokenLogical || lambdaParam[1].Token.Value != "eq" { + return false, "", "" + } + + if len(lambdaParam[1].Children) != 2 { + return false, "", "" + } + expression := lambdaParam[1].Children + if expression[0].Token.Type != godata.ExpressionTokenNav || expression[0].Token.Value != "/" { + return false, "", "" + } + + if len(expression[0].Children) != 2 { + return false, "", "" + } + if expression[0].Children[1].Token.Value != appRoleID { + return false, "", "" + } + filterValue, err := g.getUUIDTokenValue(ctx, expression[1]) + if err != nil { + return false, "", "" + } + return true, appRoleID, filterValue +} + func userSliceToMap(users []*libregraph.User) map[string]*libregraph.User { resMap := make(map[string]*libregraph.User, len(users)) for _, user := range users { diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index c3ebdd5d6..b020ef207 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -342,6 +342,14 @@ var _ = Describe("Users", func() { Entry("with unsupported filter operand type", "memberOf/any(n:n/id eq 1)", http.StatusNotImplemented), Entry("with unsupported memberOf lambda filter property", "memberOf/any(n:n/name eq 'name')", http.StatusNotImplemented), Entry("with unsupported appRoleAssignments lambda filter property", "appRoleAssignments/any(n:n/id eq 'id')", http.StatusNotImplemented), + Entry("with unsupported appRoleAssignments lambda filter property", + "appRoleAssignments/any(n:n/id eq 'id') and appRoleAssignments/any(n:n/id eq 'id')", http.StatusNotImplemented), + Entry("with unsupported appRoleAssignments lambda filter operation", + "appRoleAssignments/all(n:n/appRoleId eq 'id') and appRoleAssignments/any(n:n/appRoleId eq 'id')", http.StatusNotImplemented), + Entry("with unsupported appRoleAssignments lambda filter operation", + "appRoleAssignments/any(n:n/appRoleId ne 'id') and appRoleAssignments/any(n:n/appRoleId eq 'id')", http.StatusNotImplemented), + Entry("with unsupported appRoleAssignments lambda filter operation", + "appRoleAssignments/any(n:n/appRoleId eq 1) and appRoleAssignments/any(n:n/appRoleId eq 'id')", http.StatusNotImplemented), ) DescribeTable("With a valid filter", @@ -373,6 +381,9 @@ var _ = Describe("Users", func() { Entry("with two memberOf lambda filters", "memberOf/any(n:n/id eq 25cb7bc0-3168-4a0c-adbe-396f478ad494) and memberOf/any(n:n/id eq 2713f1d5-6822-42bd-ad56-9f6c55a3a8fa)", http.StatusOK), + Entry("with supported appRoleAssignments lambda filter property", + "appRoleAssignments/any(n:n/appRoleId eq 'some-appRoleAssignment-ID') and memberOf/any(n:n/id eq 2713f1d5-6822-42bd-ad56-9f6c55a3a8fa)", + http.StatusOK), ) Describe("GetUser", func() {