Repository: owncloud/ocis
Pull request #8050 (2023-12-21 11:27:18)

graph/groups: Handle quoted search terms for in GetGroups
Related Issue #7990 

sonarcloud[bot]: ## [![Quality Gate Passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/qg-passed-20px.png 'Quality Gate Passed')](https://sonarcloud.io/dashboard?id=owncloud_ocis&pullRequest=8050) **Quality Gate passed**  
The SonarCloud Quality Gate passed, but some issues were introduced.

[2 New issues](https://sonarcloud.io/project/issues?id=owncloud_ocis&pullRequest=8050&resolved=false&inNewCodePeriod=true)  
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=owncloud_ocis&pullRequest=8050&resolved=false&inNewCodePeriod=true)  
[78.1% Coverage on New Code](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=8050&metric=new_coverage&view=list)  
[11.7% Duplication on New Code](https://sonarcloud.io/component_measures?id=owncloud_ocis&pullRequest=8050&metric=new_duplicated_lines_density&view=list)  
  
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=owncloud_ocis&pullRequest=8050)



Patch:
diff --git a/changelog/unreleased/fix-search-by-exact-mail.md b/changelog/unreleased/fix-search-by-exact-mail.md
index 7e85130ba33..988f7faa4ee 100644
--- a/changelog/unreleased/fix-search-by-exact-mail.md
+++ b/changelog/unreleased/fix-search-by-exact-mail.md
@@ -1,7 +1,9 @@
-Bugfix: Fix search by exact email
+Bugfix: Fix search by containing special characters
 
-Users can be searched by exact email by using double quotes on the search
-parameter. Note that double quotes are required because the "@" char is
-being interpreted by the parser.
+As the OData query parser interprets characters like '@' or '-' in a special
+way. Search request for users or groups needs to be quoted. We fixed the libregraph
+users and groups endpoints to handle quoted search terms correctly.
 
+https://github.com/owncloud/ocis/pull/8050
 https://github.com/owncloud/ocis/pull/8035
+https://github.com/owncloud/ocis/issues/7990
diff --git a/services/graph/pkg/identity/backend.go b/services/graph/pkg/identity/backend.go
index cc94141d3ab..08afc0d4f7e 100644
--- a/services/graph/pkg/identity/backend.go
+++ b/services/graph/pkg/identity/backend.go
@@ -37,7 +37,7 @@ type Backend interface {
 	// UpdateGroupName updates the group name
 	UpdateGroupName(ctx context.Context, groupID string, groupName string) error
 	GetGroup(ctx context.Context, nameOrID string, queryParam url.Values) (*libregraph.Group, error)
-	GetGroups(ctx context.Context, queryParam url.Values) ([]*libregraph.Group, error)
+	GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*libregraph.Group, error)
 	// GetGroupMembers list all members of a group
 	GetGroupMembers(ctx context.Context, id string, oreq *godata.GoDataRequest) ([]*libregraph.User, error)
 	// AddMembersToGroup adds new members (reference by a slice of IDs) to supplied group in the identity backend.
diff --git a/services/graph/pkg/identity/cs3.go b/services/graph/pkg/identity/cs3.go
index 7a305c22fb6..b704ae312b0 100644
--- a/services/graph/pkg/identity/cs3.go
+++ b/services/graph/pkg/identity/cs3.go
@@ -112,7 +112,7 @@ func (i *CS3) GetUsers(ctx context.Context, oreq *godata.GoDataRequest) ([]*libr
 }
 
 // GetGroups implements the Backend Interface.
-func (i *CS3) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregraph.Group, error) {
+func (i *CS3) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*libregraph.Group, error) {
 	logger := i.Logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "cs3").Msg("GetGroups")
 	gatewayClient, err := i.GatewaySelector.Next()
@@ -121,9 +121,9 @@ func (i *CS3) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregra
 		return nil, errorcode.New(errorcode.ServiceNotAvailable, err.Error())
 	}
 
-	search := queryParam.Get("search")
-	if search == "" {
-		search = queryParam.Get("$search")
+	search, err := GetSearchValues(oreq.Query)
+	if err != nil {
+		return nil, err
 	}
 
 	res, err := gatewayClient.FindGroups(ctx, &cs3group.FindGroupsRequest{
diff --git a/services/graph/pkg/identity/ldap_group.go b/services/graph/pkg/identity/ldap_group.go
index c7c66f94c4a..ed55ea00a51 100644
--- a/services/graph/pkg/identity/ldap_group.go
+++ b/services/graph/pkg/identity/ldap_group.go
@@ -55,19 +55,26 @@ func (i *LDAP) GetGroup(ctx context.Context, nameOrID string, queryParam url.Val
 }
 
 // GetGroups implements the Backend Interface for the LDAP Backend
-func (i *LDAP) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregraph.Group, error) {
+func (i *LDAP) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*libregraph.Group, error) {
 	logger := i.logger.SubloggerWithRequestID(ctx)
 	logger.Debug().Str("backend", "ldap").Msg("GetGroups")
 
-	search := queryParam.Get("search")
-	if search == "" {
-		search = queryParam.Get("$search")
+	search, err := GetSearchValues(oreq.Query)
+	if err != nil {
+		return nil, err
 	}
 
 	var expandMembers bool
-	sel := strings.Split(queryParam.Get("$select"), ",")
-	exp := strings.Split(queryParam.Get("$expand"), ",")
-	if slices.Contains(sel, "members") || slices.Contains(exp, "members") {
+	exp, err := GetExpandValues(oreq.Query)
+	if err != nil {
+		return nil, err
+	}
+	sel, err := GetSelectValues(oreq.Query)
+	if err != nil {
+		return nil, err
+	}
+
+	if slices.Contains(exp, "members") || slices.Contains(sel, "members") {
 		expandMembers = true
 	}
 
diff --git a/services/graph/pkg/identity/mocks/backend.go b/services/graph/pkg/identity/mocks/backend.go
index 69e876002a8..8bb281f42e1 100644
--- a/services/graph/pkg/identity/mocks/backend.go
+++ b/services/graph/pkg/identity/mocks/backend.go
@@ -165,25 +165,25 @@ func (_m *Backend) GetGroupMembers(ctx context.Context, id string, oreq *godata.
 	return r0, r1
 }
 
-// GetGroups provides a mock function with given fields: ctx, queryParam
-func (_m *Backend) GetGroups(ctx context.Context, queryParam url.Values) ([]*libregraph.Group, error) {
-	ret := _m.Called(ctx, queryParam)
+// GetGroups provides a mock function with given fields: ctx, oreq
+func (_m *Backend) GetGroups(ctx context.Context, oreq *godata.GoDataRequest) ([]*libregraph.Group, error) {
+	ret := _m.Called(ctx, oreq)
 
 	var r0 []*libregraph.Group
 	var r1 error
-	if rf, ok := ret.Get(0).(func(context.Context, url.Values) ([]*libregraph.Group, error)); ok {
-		return rf(ctx, queryParam)
+	if rf, ok := ret.Get(0).(func(context.Context, *godata.GoDataRequest) ([]*libregraph.Group, error)); ok {
+		return rf(ctx, oreq)
 	}
-	if rf, ok := ret.Get(0).(func(context.Context, url.Values) []*libregraph.Group); ok {
-		r0 = rf(ctx, queryParam)
+	if rf, ok := ret.Get(0).(func(context.Context, *godata.GoDataRequest) []*libregraph.Group); ok {
+		r0 = rf(ctx, oreq)
 	} else {
 		if ret.Get(0) != nil {
 			r0 = ret.Get(0).([]*libregraph.Group)
 		}
 	}
 
-	if rf, ok := ret.Get(1).(func(context.Context, url.Values) error); ok {
-		r1 = rf(ctx, queryParam)
+	if rf, ok := ret.Get(1).(func(context.Context, *godata.GoDataRequest) error); ok {
+		r1 = rf(ctx, oreq)
 	} else {
 		r1 = ret.Error(1)
 	}
diff --git a/services/graph/pkg/identity/odata.go b/services/graph/pkg/identity/odata.go
index ef102cd7eb4..698b84e69e8 100644
--- a/services/graph/pkg/identity/odata.go
+++ b/services/graph/pkg/identity/odata.go
@@ -29,6 +29,23 @@ func GetExpandValues(req *godata.GoDataQuery) ([]string, error) {
 	return expand, nil
 }
 
+// GetSelectValues extracts the values of the $select query parameter and
+// returns them in a []string, rejects any $select value that consists of more
+// than just a single path segment
+func GetSelectValues(req *godata.GoDataQuery) ([]string, error) {
+	if req == nil || req.Select == nil {
+		return []string{}, nil
+	}
+	sel := make([]string, 0, len(req.Select.SelectItems))
+	for _, item := range req.Select.SelectItems {
+		if len(item.Segments) > 1 {
+			return []string{}, godata.NotImplementedError("multiple segments in $select not supported")
+		}
+		sel = append(sel, item.Segments[0].Value)
+	}
+	return sel, nil
+}
+
 // GetSearchValues extracts the value of the $search query parameter and returns
 // it as a string. Rejects any search query that is more than just a simple string
 func GetSearchValues(req *godata.GoDataQuery) (string, error) {
diff --git a/services/graph/pkg/service/v0/groups.go b/services/graph/pkg/service/v0/groups.go
index f80924025ac..8f6f707a55e 100644
--- a/services/graph/pkg/service/v0/groups.go
+++ b/services/graph/pkg/service/v0/groups.go
@@ -32,7 +32,17 @@ func (g Graph) GetGroups(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	ctxHasFullPerms := g.contextUserHasFullAccountPerms(r.Context())
-	if !ctxHasFullPerms && (odataReq.Query == nil || odataReq.Query.Search == nil || len(odataReq.Query.Search.RawValue) < g.config.API.IdentitySearchMinLength) {
+	searchHasAcceptableLength := false
+	if odataReq.Query != nil && odataReq.Query.Search != nil {
+		minSearchLength := g.config.API.IdentitySearchMinLength
+		if strings.HasPrefix(odataReq.Query.Search.RawValue, "\"") {
+			// if search starts with double quotes then it must finish with double quotes
+			// add +2 to the minimum search length in this case
+			minSearchLength += 2
+		}
+		searchHasAcceptableLength = len(odataReq.Query.Search.RawValue) >= minSearchLength
+	}
+	if !ctxHasFullPerms && !searchHasAcceptableLength {
 		// 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")
@@ -46,7 +56,7 @@ func (g Graph) GetGroups(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	groups, err := g.identityBackend.GetGroups(r.Context(), r.URL.Query())
+	groups, err := g.identityBackend.GetGroups(r.Context(), odataReq)
 	if err != nil {
 		logger.Debug().Err(err).Msg("could not get groups: backend error")
 		errorcode.RenderError(w, r, err)
diff --git a/services/graph/pkg/service/v0/users.go b/services/graph/pkg/service/v0/users.go
index eec46b13d75..6902af8dda1 100644
--- a/services/graph/pkg/service/v0/users.go
+++ b/services/graph/pkg/service/v0/users.go
@@ -223,9 +223,9 @@ func (g Graph) GetUsers(w http.ResponseWriter, r *http.Request) {
 	}
 
 	ctxHasFullPerms := g.contextUserHasFullAccountPerms(r.Context())
-	minSearchLength := g.config.API.IdentitySearchMinLength
 	searchHasAcceptableLength := false
 	if odataReq.Query != nil && odataReq.Query.Search != nil {
+		minSearchLength := g.config.API.IdentitySearchMinLength
 		if strings.HasPrefix(odataReq.Query.Search.RawValue, "\"") {
 			// if search starts with double quotes then it must finish with double quotes
 			// add +2 to the minimum search length in this case


Test patch:
diff --git a/services/graph/pkg/identity/ldap_group_test.go b/services/graph/pkg/identity/ldap_group_test.go
index 79431d4760f..bb7adfa9f35 100644
--- a/services/graph/pkg/identity/ldap_group_test.go
+++ b/services/graph/pkg/identity/ldap_group_test.go
@@ -6,6 +6,7 @@ import (
 	"net/url"
 	"testing"
 
+	"github.com/CiscoM31/godata"
 	"github.com/go-ldap/ldap/v3"
 	"github.com/owncloud/ocis/v2/services/graph/mocks"
 	"github.com/stretchr/testify/assert"
@@ -219,15 +220,19 @@ func TestGetGroupReadOnlySubtree(t *testing.T) {
 
 func TestGetGroups(t *testing.T) {
 	lm := &mocks.Client{}
+	oDataReq, err := godata.ParseRequest(context.Background(), "", url.Values{})
+	if err != nil {
+		t.Errorf("Expected success, got '%s'", err.Error())
+	}
 	lm.On("Search", mock.Anything).Return(nil, ldap.NewError(ldap.LDAPResultOperationsError, errors.New("mock")))
 	b, _ := getMockedBackend(lm, lconfig, &logger)
-	_, err := b.GetGroups(context.Background(), url.Values{})
+	_, err = b.GetGroups(context.Background(), oDataReq)
 	assert.ErrorContains(t, err, "itemNotFound:")
 
 	lm = &mocks.Client{}
 	lm.On("Search", mock.Anything).Return(&ldap.SearchResult{}, nil)
 	b, _ = getMockedBackend(lm, lconfig, &logger)
-	g, err := b.GetGroups(context.Background(), url.Values{})
+	g, err := b.GetGroups(context.Background(), oDataReq)
 	if err != nil {
 		t.Errorf("Expected success, got '%s'", err.Error())
 	} else if g == nil || len(g) != 0 {
@@ -239,7 +244,7 @@ func TestGetGroups(t *testing.T) {
 		Entries: []*ldap.Entry{groupEntry},
 	}, nil)
 	b, _ = getMockedBackend(lm, lconfig, &logger)
-	g, err = b.GetGroups(context.Background(), url.Values{})
+	g, err = b.GetGroups(context.Background(), oDataReq)
 	if err != nil {
 		t.Errorf("Expected GetGroup to succeed. Got %s", err.Error())
 	} else if *g[0].Id != groupEntry.GetEqualFoldAttributeValue(b.groupAttributeMap.id) {
@@ -264,11 +269,15 @@ func TestGetGroups(t *testing.T) {
 	}
 
 	for _, param := range []url.Values{queryParamSelect, queryParamExpand} {
+		oDataReq, err := godata.ParseRequest(context.Background(), "", param)
+		if err != nil {
+			t.Errorf("Expected success, got '%s'", err.Error())
+		}
 		lm.On("Search", groupListSeachRequest).Return(&ldap.SearchResult{Entries: []*ldap.Entry{groupEntry}}, nil)
 		lm.On("Search", sr2).Return(&ldap.SearchResult{Entries: []*ldap.Entry{userEntry}}, nil)
 		lm.On("Search", sr3).Return(&ldap.SearchResult{Entries: []*ldap.Entry{invalidUserEntry}}, nil)
 		b, _ = getMockedBackend(lm, lconfig, &logger)
-		g, err = b.GetGroups(context.Background(), param)
+		g, err = b.GetGroups(context.Background(), oDataReq)
 		switch {
 		case err != nil:
 			t.Errorf("Expected GetGroup to succeed. Got %s", err.Error())
@@ -281,6 +290,32 @@ func TestGetGroups(t *testing.T) {
 		}
 	}
 }
+
+func TestGetGroupsSearch(t *testing.T) {
+	lm := &mocks.Client{}
+	odataReqDefault, err := godata.ParseRequest(context.Background(), "",
+		url.Values{
+			"$search": []string{"\"term\""},
+		},
+	)
+	if err != nil {
+		t.Errorf("Expected success got '%s'", err.Error())
+	}
+
+	// only match if the filter contains the search term unquoted
+	lm.On("Search", mock.MatchedBy(
+		func(req *ldap.SearchRequest) bool {
+			return req.Filter == "(&(objectClass=groupOfNames)(|(cn=*term*)(entryUUID=*term*)))"
+		})).
+		Return(&ldap.SearchResult{}, nil)
+	b, _ := getMockedBackend(lm, lconfig, &logger)
+	g, err := b.GetGroups(context.Background(), odataReqDefault)
+	if err != nil {
+		t.Errorf("Expected success, got '%s'", err.Error())
+	} else if g == nil || len(g) != 0 {
+		t.Errorf("Expected zero length user slice")
+	}
+}
 func TestUpdateGroupName(t *testing.T) {
 	groupDn := "cn=TheGroup,ou=groups,dc=owncloud,dc=com"
 
diff --git a/services/graph/pkg/identity/ldap_test.go b/services/graph/pkg/identity/ldap_test.go
index 908862f8197..3d95795426c 100644
--- a/services/graph/pkg/identity/ldap_test.go
+++ b/services/graph/pkg/identity/ldap_test.go
@@ -286,6 +286,32 @@ func TestGetUsers(t *testing.T) {
 	}
 }
 
+func TestGetUsersSearch(t *testing.T) {
+	lm := &mocks.Client{}
+	odataReqDefault, err := godata.ParseRequest(context.Background(), "",
+		url.Values{
+			"$search": []string{"\"term\""},
+		},
+	)
+	if err != nil {
+		t.Errorf("Expected success got '%s'", err.Error())
+	}
+
+	// only match if the filter contains the search term unquoted
+	lm.On("Search", mock.MatchedBy(
+		func(req *ldap.SearchRequest) bool {
+			return req.Filter == "(&(objectClass=inetOrgPerson)(|(uid=*term*)(mail=*term*)(displayname=*term*)))"
+		})).
+		Return(&ldap.SearchResult{}, nil)
+	b, _ := getMockedBackend(lm, lconfig, &logger)
+	g, err := b.GetUsers(context.Background(), odataReqDefault)
+	if err != nil {
+		t.Errorf("Expected success, got '%s'", err.Error())
+	} else if g == nil || len(g) != 0 {
+		t.Errorf("Expected zero length user slice")
+	}
+}
+
 func TestUpdateUser(t *testing.T) {
 	falseBool := false
 	trueBool := true
diff --git a/services/graph/pkg/service/v0/groups_test.go b/services/graph/pkg/service/v0/groups_test.go
index d8aab638466..1ce23af4446 100644
--- a/services/graph/pkg/service/v0/groups_test.go
+++ b/services/graph/pkg/service/v0/groups_test.go
@@ -225,6 +225,13 @@ var _ = Describe("Groups", func() {
 
 			Expect(rr.Code).To(Equal(http.StatusForbidden))
 		})
+		It("denies using to short quoted 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/groups?$search=%22ab%22", 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{}
diff --git a/services/graph/pkg/service/v0/users_test.go b/services/graph/pkg/service/v0/users_test.go
index 4cc9561e268..20bc10aa5d8 100644
--- a/services/graph/pkg/service/v0/users_test.go
+++ b/services/graph/pkg/service/v0/users_test.go
@@ -278,6 +278,13 @@ var _ = Describe("Users", func() {
 
 			Expect(rr.Code).To(Equal(http.StatusForbidden))
 		})
+		It("denies using to short quoted 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=%22ab%22", 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{}