graph/groups: Handle quoted search terms for in GetGroups

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

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.

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{

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
 	}
 

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"
 

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

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

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)

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{}

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

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{}