From 23ba6e4755f5b645e090faa7978e8ac7c2cddd00 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 12 Aug 2024 16:54:43 +0200 Subject: [PATCH] feat(graph): add support for filtering users by type This add support for equality filters on the `userType` property of users for the /users endpoint. It also changes the behavior of the /users endpoint to only return federated users when explicitly request via a `userType` filter. E.g. to search for federated users with matching "albert" you can use: `$filter=userType eq 'Federated'&$search="albert"` Related Issue: #9702 --- changelog/unreleased/ocm_users_graph.md | 14 ++++ services/graph/pkg/service/v0/users.go | 28 +++----- services/graph/pkg/service/v0/users_filter.go | 25 +++++++ services/graph/pkg/service/v0/users_test.go | 66 +++++++++++++++++-- 4 files changed, 109 insertions(+), 24 deletions(-) create mode 100644 changelog/unreleased/ocm_users_graph.md diff --git a/changelog/unreleased/ocm_users_graph.md b/changelog/unreleased/ocm_users_graph.md new file mode 100644 index 00000000000..0ec3fd1bd6c --- /dev/null +++ b/changelog/unreleased/ocm_users_graph.md @@ -0,0 +1,14 @@ +Enhancement: OCM related adjustments in graph + +The /users enpdoint of the graph service was changed with respect to how +it handles OCM federeated users: +- The 'userType' property is now alway returned. As new usertype 'Federated' + was introduced. To indicate that the user is a federated user. +- Supported for filtering users by 'userType' as added. Queries like + "$filter=userType eq 'Federated'" are now possible. +- Federated users are only returned when explicitly requested via filter. + When no filter is provider only 'Member' users are returned. + +https://github.com/owncloud/ocis/pull/9788 +https://github.com/owncloud/ocis/pull/9757 +https://github.com/owncloud/ocis/issues/9702 diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 783d211be23..1881b2548c0 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -269,25 +269,6 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) { return } - if g.config.IncludeOCMSharees { - ocmUsers, err := g.searchOCMAcceptedUsers(r.Context(), odataReq) - var errcode errorcode.Error - var godataerr *godata.GoDataError - switch { - case err == nil: - users = append(users, ocmUsers...) - case errors.As(err, &errcode): - errcode.Render(w, r) - return - case errors.As(err, &godataerr): - errorcode.GeneralException.Render(w, r, godataerr.ResponseCode, err.Error()) - return - default: - errorcode.GeneralException.Render(w, r, http.StatusInternalServerError, err.Error()) - return - } - } - // If the user isn't admin, we'll show just the minimum user attibutes if !ctxHasFullPerms { finalUsers := make([]*libregraph.User, len(users)) @@ -1008,6 +989,13 @@ func isValidUserType(userType string) bool { func (g Graph) searchOCMAcceptedUsers(ctx context.Context, odataReq *godata.GoDataRequest) ([]*libregraph.User, error) { logger := g.logger.SubloggerWithRequestID(ctx) + users := []*libregraph.User{} + + if !g.config.IncludeOCMSharees { + logger.Debug().Msg("OCM sharing is disabled") + return users, nil + } + gwc, err := g.gatewaySelector.Next() if err != nil { return nil, errorcode.New(errorcode.GeneralException, err.Error()) @@ -1027,7 +1015,7 @@ func (g Graph) searchOCMAcceptedUsers(ctx context.Context, odataReq *godata.GoDa } cs3Users := remoteUsersRes.GetAcceptedUsers() - users := make([]*libregraph.User, 0, len(cs3Users)) + users = make([]*libregraph.User, 0, len(cs3Users)) for _, user := range cs3Users { users = append(users, identity.CreateUserModelFromCS3(user)) } diff --git a/services/graph/pkg/service/v0/users_filter.go b/services/graph/pkg/service/v0/users_filter.go index 833c330f750..40b90e9ca69 100644 --- a/services/graph/pkg/service/v0/users_filter.go +++ b/services/graph/pkg/service/v0/users_filter.go @@ -135,6 +135,8 @@ func (g Graph) applyFilterLogical(ctx context.Context, req *godata.GoDataRequest return g.applyFilterLogicalAnd(ctx, req, root.Children[0], root.Children[1]) case "or": return g.applyFilterLogicalOr(ctx, req, root.Children[0], root.Children[1]) + case "eq": + return g.applyFilterEq(ctx, req, root.Children[0], root.Children[1]) } logger.Debug().Str("Token", root.Token.Value).Msg("unsupported logical filter") return users, unsupportedFilterError() @@ -220,6 +222,29 @@ func (g Graph) applyFilterLogicalOr(ctx context.Context, req *godata.GoDataReque } return filteredUsers, nil } + +func (g Graph) applyFilterEq(ctx context.Context, req *godata.GoDataRequest, operand1 *godata.ParseNode, operand2 *godata.ParseNode) (users []*libregraph.User, err error) { + // We only support the 'eq' on 'userType' for now + switch { + case operand1.Token.Type != godata.ExpressionTokenLiteral: + fallthrough + case operand1.Token.Value != "userType": + fallthrough + case operand2.Token.Type != godata.ExpressionTokenString: + return users, unsupportedFilterError() + } + + // unquote + value := strings.Trim(operand2.Token.Value, "'") + switch value { + case "Member", "Guest": + return g.identityBackend.GetUsers(ctx, req) + case "Federated": + return g.searchOCMAcceptedUsers(ctx, req) + } + return users, unsupportedFilterError() +} + func (g Graph) applyFilterLambda(ctx context.Context, req *godata.GoDataRequest, nodes []*godata.ParseNode) (users []*libregraph.User, err error) { logger := g.logger.SubloggerWithRequestID(ctx) if len(nodes) != 2 { diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index c8f7b18c9ec..26e8d28cccf 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -503,8 +503,8 @@ var _ = Describe("Users", func() { "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), + Entry("with unsupported userType in filter", "userType eq 'unsupported'", http.StatusNotImplemented), + Entry("with unsupported property in eq filter", "unsupported eq 'unsupported'", http.StatusNotImplemented), ) DescribeTable("With a valid filter", @@ -1125,7 +1125,33 @@ var _ = Describe("Users", func() { ) }) Describe("GetUsers", func() { - It("lists the users", func() { + It("does not list the federated users without a filter", func() { + gatewayClient.AssertNotCalled(GinkgoT(), "FindAcceptedUsers, mock.Anything, mock.Anything") + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ + Permission: &settingsmsg.Permission{ + Operation: settingsmsg.Permission_OPERATION_UNKNOWN, + Constraint: settingsmsg.Permission_CONSTRAINT_ALL, + }, + }, nil) + + identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return( + []*libregraph.User{}, nil, + ) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) + svc.GetUsers(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + data, err := io.ReadAll(rr.Body) + Expect(err).ToNot(HaveOccurred()) + + res := userList{} + err = json.Unmarshal(data, &res) + Expect(err).ToNot(HaveOccurred()) + + Expect(len(res.Value)).To(Equal(0)) + }) + It("does list federated users when the right filter is used", func() { gatewayClient.On("FindAcceptedUsers", mock.Anything, mock.Anything).Return(&invitepb.FindAcceptedUsersResponse{ Status: status.NewOK(ctx), AcceptedUsers: []*userv1beta1.User{ @@ -1149,7 +1175,7 @@ var _ = Describe("Users", func() { []*libregraph.User{}, nil, ) - r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$filter="+url.QueryEscape("userType eq 'Federated'"), nil) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) @@ -1164,6 +1190,38 @@ var _ = Describe("Users", func() { Expect(res.Value[0].GetId()).To(Equal("federated")) Expect(res.Value[0].GetUserType()).To(Equal("Federated")) }) + It("does not list federated users when filtering for 'Member' users", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ + Permission: &settingsmsg.Permission{ + Operation: settingsmsg.Permission_OPERATION_UNKNOWN, + Constraint: settingsmsg.Permission_CONSTRAINT_ALL, + }, + }, nil) + + user := &libregraph.User{} + user.SetId("user1") + user.SetUserType("Member") + users := []*libregraph.User{user} + + identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return( + users, nil, + ) + + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$filter="+url.QueryEscape("userType eq 'Member'"), nil) + svc.GetUsers(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + data, err := io.ReadAll(rr.Body) + Expect(err).ToNot(HaveOccurred()) + + res := userList{} + err = json.Unmarshal(data, &res) + Expect(err).ToNot(HaveOccurred()) + + Expect(len(res.Value)).To(Equal(1)) + Expect(res.Value[0].GetId()).To(Equal("user1")) + Expect(res.Value[0].GetUserType()).To(Equal("Member")) + }) }) }) })