From 3f9dbbcf889d331464b824171958eb46f447a35b Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 21 Dec 2023 12:20:20 +0100 Subject: [PATCH 1/2] graph/users: More test coverage for GetUsers search --- services/graph/pkg/identity/ldap_test.go | 26 +++++++++++++++++++++ services/graph/pkg/service/v0/users.go | 2 +- services/graph/pkg/service/v0/users_test.go | 7 ++++++ 3 files changed, 34 insertions(+), 1 deletion(-) 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/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 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{} From 9a8e2a29aec5b68d53c241f1afd73094f54a6c75 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Thu, 21 Dec 2023 12:22:14 +0100 Subject: [PATCH 2/2] graph/groups: Handle quoted search terms in GetGroups Fixes: #7990 --- .../unreleased/fix-search-by-exact-mail.md | 10 +++-- services/graph/pkg/identity/backend.go | 2 +- services/graph/pkg/identity/cs3.go | 8 ++-- services/graph/pkg/identity/ldap_group.go | 21 ++++++--- .../graph/pkg/identity/ldap_group_test.go | 43 +++++++++++++++++-- services/graph/pkg/identity/mocks/backend.go | 18 ++++---- services/graph/pkg/identity/odata.go | 17 ++++++++ services/graph/pkg/service/v0/groups.go | 14 +++++- services/graph/pkg/service/v0/groups_test.go | 7 +++ 9 files changed, 109 insertions(+), 31 deletions(-) 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/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/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/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{}