graph/groups: Allow unprivileged users to search for groups

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

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,
 		)

services/graph/pkg/service/v0/groups.go

@@ -31,13 +31,39 @@ 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 {
 		logger.Debug().Err(err).Msg("could not get groups: backend error")
 		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 {

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() {

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)

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)