From d5393618c70c584c6ed6c304d3c187e3f16637a7 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Wed, 6 Dec 2023 14:06:02 +0100 Subject: [PATCH] graph/groups: Allow unprivileged users to search for groups --- ...enhancement-allow-listing-regular-users.md | 8 +- services/graph/pkg/identity/ldap_group.go | 2 +- services/graph/pkg/service/v0/groups.go | 26 +++++ services/graph/pkg/service/v0/groups_test.go | 94 +++++++++++++++++-- services/graph/pkg/service/v0/service.go | 2 +- ...ected-failures-localAPI-on-OCIS-storage.md | 6 -- 6 files changed, 120 insertions(+), 18 deletions(-) diff --git a/changelog/unreleased/enhancement-allow-listing-regular-users.md b/changelog/unreleased/enhancement-allow-listing-regular-users.md index bc9e8e108a0..b8a2d3b3572 100644 --- a/changelog/unreleased/enhancement-allow-listing-regular-users.md +++ b/changelog/unreleased/enhancement-allow-listing-regular-users.md @@ -1,12 +1,14 @@ Enhancement: Allow regular users to list other users -Regular users can search for other users. The following limitations +Regular users can search for other users and groups. The following limitations apply: * Only search queries are allowed (using the `$search=term` query parameter) * The search term needs to have at least 3 characters -* The result set only contains the attribute `displayName`, `userType`, `mail` - and `id` +* for user searches the result set only contains the attributes `displayName`, + `userType`, `mail` and `id` +* for group searches the result set only contains the attributes `displayName`, + `groupTypes` and `id` https://github.com/owncloud/ocis/pull/7887 https://github.com/owncloud/ocis/issues/7782 diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go index ab02f5327ef..c7c66f94c4a 100644 --- a/services/graph/pkg/identity/ldap_group.go +++ b/services/graph/pkg/identity/ldap_group.go @@ -75,7 +75,7 @@ func (i *LDAP) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregr if search != "" { search = ldap.EscapeFilter(search) groupFilter = fmt.Sprintf( - "(|(%s=%s*)(%s=%s*))", + "(|(%s=*%s*)(%s=*%s*))", i.groupAttributeMap.name, search, i.groupAttributeMap.id, search, ) diff --git a/services/graph/pkg/service/v0/groups.go b/services/graph/pkg/service/v0/groups.go index 434ebef4b1b..f80924025ac 100644 --- a/services/graph/pkg/service/v0/groups.go +++ b/services/graph/pkg/service/v0/groups.go @@ -31,6 +31,20 @@ func (g Graph) GetGroups(w http.ResponseWriter, r *http.Request) { errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, err.Error()) return } + ctxHasFullPerms := g.contextUserHasFullAccountPerms(r.Context()) + if !ctxHasFullPerms && (odataReq.Query == nil || odataReq.Query.Search == nil || len(odataReq.Query.Search.RawValue) < g.config.API.IdentitySearchMinLength) { + // for regular user the search term must have a minimum length + logger.Debug().Interface("query", r.URL.Query()).Msgf("search with less than %d chars for a regular user", g.config.API.IdentitySearchMinLength) + errorcode.AccessDenied.Render(w, r, http.StatusForbidden, "search term too short") + return + } + + if !ctxHasFullPerms && (odataReq.Query.Filter != nil || 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") + return + } groups, err := g.identityBackend.GetGroups(r.Context(), r.URL.Query()) if err != nil { @@ -38,6 +52,18 @@ func (g Graph) GetGroups(w http.ResponseWriter, r *http.Request) { errorcode.RenderError(w, r, err) return } + // If the user isn't admin, we'll show just the minimum group attibutes + if !ctxHasFullPerms { + finalGroups := make([]*libregraph.Group, len(groups)) + for i, grp := range groups { + finalGroups[i] = &libregraph.Group{ + Id: grp.Id, + DisplayName: grp.DisplayName, + GroupTypes: grp.GroupTypes, + } + } + groups = finalGroups + } groups, err = sortGroups(odataReq, groups) if err != nil { diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go index d492798e23e..d8aab638466 100644 --- a/services/graph/pkg/service/v0/groups_test.go +++ b/services/graph/pkg/service/v0/groups_test.go @@ -19,6 +19,8 @@ import ( . "github.com/onsi/gomega" libregraph "github.com/owncloud/libre-graph-api-go" "github.com/owncloud/ocis/v2/ocis-pkg/shared" + settingsmsg "github.com/owncloud/ocis/v2/protogen/gen/ocis/messages/settings/v0" + settings "github.com/owncloud/ocis/v2/protogen/gen/ocis/services/settings/v0" "github.com/owncloud/ocis/v2/services/graph/mocks" "github.com/owncloud/ocis/v2/services/graph/pkg/config" "github.com/owncloud/ocis/v2/services/graph/pkg/config/defaults" @@ -35,13 +37,14 @@ type groupList struct { var _ = Describe("Groups", func() { var ( - svc service.Service - ctx context.Context - cfg *config.Config - gatewayClient *cs3mocks.GatewayAPIClient - gatewaySelector pool.Selectable[gateway.GatewayAPIClient] - eventsPublisher mocks.Publisher - identityBackend *identitymocks.Backend + svc service.Service + ctx context.Context + cfg *config.Config + gatewayClient *cs3mocks.GatewayAPIClient + gatewaySelector pool.Selectable[gateway.GatewayAPIClient] + eventsPublisher mocks.Publisher + identityBackend *identitymocks.Backend + permissionService *mocks.Permissions rr *httptest.ResponseRecorder @@ -65,6 +68,7 @@ var _ = Describe("Groups", func() { return gatewayClient }, ) + permissionService = &mocks.Permissions{} identityBackend = &identitymocks.Backend{} newGroup = libregraph.NewGroup() @@ -85,6 +89,7 @@ var _ = Describe("Groups", func() { service.WithGatewaySelector(gatewaySelector), service.EventsPublisher(&eventsPublisher), service.WithIdentityBackend(identityBackend), + service.PermissionService(permissionService), ) }) @@ -97,6 +102,12 @@ var _ = Describe("Groups", func() { }) It("handles invalid sorting queries", 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) identityBackend.On("GetGroups", ctx, mock.Anything).Return([]*libregraph.Group{newGroup}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups?$orderby=invalid", nil) @@ -113,6 +124,12 @@ var _ = Describe("Groups", func() { }) It("handles unknown backend errors", 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) identityBackend.On("GetGroups", ctx, mock.Anything).Return(nil, errors.New("failed")) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) @@ -128,6 +145,12 @@ var _ = Describe("Groups", func() { }) It("handles backend errors", 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) identityBackend.On("GetGroups", ctx, mock.Anything).Return(nil, errorcode.New(errorcode.AccessDenied, "access denied")) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) @@ -144,6 +167,12 @@ var _ = Describe("Groups", func() { }) It("renders an empty list of groups", 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) identityBackend.On("GetGroups", ctx, mock.Anything).Return([]*libregraph.Group{}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) @@ -160,6 +189,12 @@ var _ = Describe("Groups", func() { }) It("renders a list of groups", 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) identityBackend.On("GetGroups", ctx, mock.Anything).Return([]*libregraph.Group{newGroup}, nil) r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups", nil) @@ -176,6 +211,51 @@ var _ = Describe("Groups", func() { Expect(len(res.Value)).To(Equal(1)) Expect(res.Value[0].GetId()).To(Equal("group1")) }) + It("denies listing for unprivileged users", func() { + permissionService.On("GetPermissionByID", mock.Anything, mock.Anything).Return(&settings.GetPermissionByIDResponse{}, nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/users", nil) + svc.GetGroups(rr, r) + + Expect(rr.Code).To(Equal(http.StatusForbidden)) + }) + It("denies using to short search terms 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=a", nil) + svc.GetGroups(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) + group := &libregraph.Group{} + group.SetId("group1") + group.SetDisplayName("Group Name") + group.SetMembers( + []libregraph.User{ + libregraph.User{ + Id: libregraph.PtrString("userid"), + }, + }, + ) + groups := []*libregraph.Group{group} + + identityBackend.On("GetGroups", mock.Anything, mock.Anything, mock.Anything).Return(groups, nil) + r := httptest.NewRequest(http.MethodGet, "/graph/v1.0/groups?$search=abc", nil) + svc.GetGroups(rr, r) + + Expect(rr.Code).To(Equal(http.StatusOK)) + data, err := io.ReadAll(rr.Body) + Expect(err).ToNot(HaveOccurred()) + + res := groupList{} + err = json.Unmarshal(data, &res) + Expect(err).ToNot(HaveOccurred()) + groupMap, err := res.Value[0].ToMap() + Expect(err).ToNot(HaveOccurred()) + for k, _ := range groupMap { + Expect(k).Should(BeElementOf([]string{"displayName", "id", "groupTypes"})) + } + }) }) Describe("GetGroup", func() { diff --git a/services/graph/pkg/service/v0/service.go b/services/graph/pkg/service/v0/service.go index 66bdc6b55d4..5df40a0fc95 100644 --- a/services/graph/pkg/service/v0/service.go +++ b/services/graph/pkg/service/v0/service.go @@ -253,7 +253,7 @@ func NewService(opts ...Option) (Graph, error) { }) }) r.Route("/groups", func(r chi.Router) { - r.With(requireAdmin).Get("/", svc.GetGroups) + r.Get("/", svc.GetGroups) r.With(requireAdmin).Post("/", svc.PostGroup) r.Route("/{groupID}", func(r chi.Router) { r.Get("/", svc.GetGroup) diff --git a/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md b/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md index 0a1ebacb0f5..331c3953b0c 100644 --- a/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md +++ b/tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md @@ -115,15 +115,9 @@ The expected failures in this file are from features in the owncloud/ocis repo. - [apiGraph/editGroup.feature:35](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L35) - [apiGraph/editGroup.feature:34](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L34) - [apiGraph/editGroup.feature:36](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/editGroup.feature#L36) -- [apiGraph/getGroup.feature:54](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L54) -- [apiGraph/getGroup.feature:55](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L55) -- [apiGraph/getGroup.feature:56](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L56) - [apiGraph/getGroup.feature:103](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L103) - [apiGraph/getGroup.feature:104](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L104) - [apiGraph/getGroup.feature:105](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L105) -- [apiGraph/getGroup.feature:267](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L267) -- [apiGraph/getGroup.feature:268](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L268) -- [apiGraph/getGroup.feature:269](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/getGroup.feature#L269) - [apiGraph/removeUserFromGroup.feature:191](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/removeUserFromGroup.feature#L191) - [apiGraph/removeUserFromGroup.feature:192](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/removeUserFromGroup.feature#L192) - [apiGraph/removeUserFromGroup.feature:193](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiGraph/removeUserFromGroup.feature#L193)