From 99df9bd3bb3fd8da44fecefcda8988fe6235209f Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 13 Aug 2024 14:06:45 +0200 Subject: [PATCH] graph: allow regular users to filter users by userType --- services/graph/pkg/service/v0/users.go | 16 +++++++++- services/graph/pkg/service/v0/users_test.go | 35 +++++++++------------ 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go index 1881b2548c..06e08d4a6c 100644 --- a/services/graph/pkg/service/v0/users.go +++ b/services/graph/pkg/service/v0/users.go @@ -237,7 +237,21 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) { return } - if !ctxHasFullPerms && (odataReq.Query.Filter != nil || odataReq.Query.Apply != nil || odataReq.Query.Expand != nil || odataReq.Query.Compute != nil) { + if !ctxHasFullPerms && odataReq.Query.Filter != nil { + // regular users are allowed to filter only by userType + filter := odataReq.Query.Filter + switch { + case filter.Tree.Token.Type != godata.ExpressionTokenLogical: + fallthrough + case filter.Tree.Token.Value != "eq": + fallthrough + case filter.Tree.Children[0].Token.Value != "userType": + logger.Debug().Interface("query", r.URL.Query()).Msg("forbidden filter for a regular user") + errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "filter has forbidden elements for regular users") + return + } + } + if !ctxHasFullPerms && (odataReq.Query.Apply != nil || odataReq.Query.Expand != nil || odataReq.Query.Compute != nil) { // regular users can't use filter, apply, expand and compute logger.Debug().Interface("query", r.URL.Query()).Msg("forbidden query elements for a regular user") errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "query has forbidden elements for regular users") diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go index 26e8d28ccc..f18d101d3d 100644 --- a/services/graph/pkg/service/v0/users_test.go +++ b/services/graph/pkg/service/v0/users_test.go @@ -291,6 +291,14 @@ var _ = Describe("Users", func() { Expect(rr.Code).To(Equal(http.StatusForbidden)) }) + It("denies filters other that 'userType eq ...' for unprivileged users", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=%22abc%22&$filter="+url.QueryEscape("memberOf/any(n:n/id eq 25cb7bc0-3168-4a0c-adbe-396f478ad494)"), nil) + svc.GetUsers(rr, r) + + Expect(rr.Code).To(Equal(http.StatusForbidden)) + }) + It("only returns a restricted set of attributes for unprivileged users", func() { permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) user := &libregraph.User{} @@ -1127,18 +1135,13 @@ var _ = Describe("Users", func() { Describe("GetUsers", 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) + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return( []*libregraph.User{}, nil, ) - r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=user", nil) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) @@ -1164,18 +1167,13 @@ var _ = Describe("Users", func() { }, }, }, nil) - permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{ - Permission: &settingsmsg.Permission{ - Operation: settingsmsg.Permission_OPERATION_UNKNOWN, - Constraint: settingsmsg.Permission_CONSTRAINT_ALL, - }, - }, nil) + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) identityBackend.On("GetUsers", mock.Anything, mock.Anything, mock.Anything).Return( []*libregraph.User{}, nil, ) - r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$filter="+url.QueryEscape("userType eq 'Federated'"), nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=federated&$filter="+url.QueryEscape("userType eq 'Federated'"), nil) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(http.StatusOK)) @@ -1191,12 +1189,7 @@ var _ = Describe("Users", func() { 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) + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) user := &libregraph.User{} user.SetId("user1") @@ -1207,7 +1200,7 @@ var _ = Describe("Users", func() { users, nil, ) - r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$filter="+url.QueryEscape("userType eq 'Member'"), nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users?$search=user&$filter="+url.QueryEscape("userType eq 'Member'"), nil) svc.GetUsers(rr, r) Expect(rr.Code).To(Equal(http.StatusOK))