Skip to content

Commit

Permalink
graph: Improve appRoleAssignemts filters
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@rhafer
rhafer committed Feb 21, 2023
1 parent 1552f6d commit 1dab0f7
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 29 deletions.
141 changes: 112 additions & 29 deletions services/graph/pkg/service/v0/users_filter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ import (
libregraph "github.com/owncloud/libre-graph-api-go"
)

const (
appRoleID = "appRoleId"
)

func invalidFilterError() error {
return godata.BadRequestError("invalid filter")
}
Expand Down Expand Up @@ -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)
// 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()
}
res2, err = g.applyUserFilter(ctx, req, operand2)
if err != nil {
return users, err
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)
}

// 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().Interface("subfilter", res).Msg("result part")
results = append(results, res)
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
}

// 'results' contains two slices of libregraph.Users now turn one of them
// 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)
}
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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()
}

Expand All @@ -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:
Expand All @@ -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
}
Expand All @@ -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 {
Expand Down
11 changes: 11 additions & 0 deletions services/graph/pkg/service/v0/users_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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() {
Expand Down

0 comments on commit 1dab0f7

Please sign in to comment.