From cb319bb2ae72149476610b058e5a76d80c68dab5 Mon Sep 17 00:00:00 2001 From: Keepers Date: Thu, 14 Sep 2023 11:03:14 -0600 Subject: [PATCH] use display name to look up groups (#4242) Allows lookup of groups using their display name in addition to their ID. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :sunflower: Feature #### Issue(s) * #3988 #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/m365/graph/errors.go | 7 +++ src/pkg/services/m365/api/contacts.go | 2 +- src/pkg/services/m365/api/groups.go | 61 +++++++++++++++++++----- src/pkg/services/m365/api/groups_test.go | 48 ++++++++++++------- src/pkg/services/m365/api/mail.go | 2 +- src/pkg/services/m365/api/sites.go | 6 ++- 6 files changed, 94 insertions(+), 32 deletions(-) diff --git a/src/internal/m365/graph/errors.go b/src/internal/m365/graph/errors.go index 2587d5215..7c51f74b7 100644 --- a/src/internal/m365/graph/errors.go +++ b/src/internal/m365/graph/errors.go @@ -96,6 +96,13 @@ var ( // when filenames collide in a @microsoft.graph.conflictBehavior=fail request. ErrItemAlreadyExistsConflict = clues.New("item already exists") + // ErrMultipleResultsMatchIdentifier describes a situation where we're doing a lookup + // in some way other than by canonical url ID (ex: filtering, searching, etc). + // This error should only be returned if a unique result is an expected constraint + // of the call results. If it's possible to opportunistically select one of the many + // replies, no error should get returned. + ErrMultipleResultsMatchIdentifier = clues.New("multiple results match the identifier") + // ErrServiceNotEnabled identifies that a resource owner does not have // access to a given service. ErrServiceNotEnabled = clues.New("service is not enabled for that resource owner") diff --git a/src/pkg/services/m365/api/contacts.go b/src/pkg/services/m365/api/contacts.go index 4f89c88ec..c06f06ace 100644 --- a/src/pkg/services/m365/api/contacts.go +++ b/src/pkg/services/m365/api/contacts.go @@ -141,7 +141,7 @@ func (c Contacts) GetContainerByName( // Return an error if multiple container exist (unlikely) or if no container // is found. if len(gv) != 1 { - return nil, clues.New("unexpected number of folders returned"). + return nil, clues.Stack(graph.ErrMultipleResultsMatchIdentifier). With("returned_container_count", len(gv)). WithClues(ctx) } diff --git a/src/pkg/services/m365/api/groups.go b/src/pkg/services/m365/api/groups.go index abb601bdf..8c4bea922 100644 --- a/src/pkg/services/m365/api/groups.go +++ b/src/pkg/services/m365/api/groups.go @@ -2,9 +2,11 @@ package api import ( "context" + "fmt" "github.com/alcionai/clues" msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" + "github.com/microsoftgraph/msgraph-sdk-go/groups" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/internal/common/ptr" @@ -70,8 +72,8 @@ func getGroups( } var ( - groups = make([]models.Groupable, 0) - el = errs.Local() + results = make([]models.Groupable, 0) + el = errs.Local() ) iterator := func(item models.Groupable) bool { @@ -83,7 +85,7 @@ func getGroups( if err != nil { el.AddRecoverable(ctx, graph.Wrap(ctx, err, "validating groups")) } else { - groups = append(groups, item) + results = append(results, item) } return true @@ -93,9 +95,11 @@ func getGroups( return nil, graph.Wrap(ctx, err, "iterating all groups") } - return groups, el.Failure() + return results, el.Failure() } +const filterGroupByDisplayNameQueryTmpl = "displayName eq '%s'" + // GetID retrieves group by groupID. func (c Groups) GetByID( ctx context.Context, @@ -106,14 +110,49 @@ func (c Groups) GetByID( return nil, err } - resp, err := service.Client().Groups().ByGroupIdString(identifier).Get(ctx, nil) - if err != nil { - err := graph.Wrap(ctx, err, "getting group by id") + ctx = clues.Add(ctx, "resource_identifier", identifier) - return nil, err + var group models.Groupable + + // prefer lookup by id, but fallback to lookup by display name, + // even in the case of a uuid, just in case the display name itself + // is a uuid. + if uuidRE.MatchString(identifier) { + group, err = service. + Client(). + Groups(). + ByGroupIdString(identifier). + Get(ctx, nil) + if err == nil { + return group, nil + } + + logger.CtxErr(ctx, err).Info("finding group by id, falling back to display name") } - return resp, graph.Stack(ctx, err).OrNil() + opts := &groups.GroupsRequestBuilderGetRequestConfiguration{ + Headers: newEventualConsistencyHeaders(), + QueryParameters: &groups.GroupsRequestBuilderGetQueryParameters{ + Filter: ptr.To(fmt.Sprintf(filterGroupByDisplayNameQueryTmpl, identifier)), + }, + } + + resp, err := service.Client().Groups().Get(ctx, opts) + if err != nil { + return nil, graph.Wrap(ctx, err, "finding group by display name") + } + + vs := resp.GetValue() + + if len(vs) == 0 { + return nil, clues.Stack(graph.ErrResourceOwnerNotFound).WithClues(ctx) + } else if len(vs) > 1 { + return nil, clues.Stack(graph.ErrMultipleResultsMatchIdentifier).WithClues(ctx) + } + + group = vs[0] + + return group, nil } // GetRootSite retrieves the root site for the group. @@ -158,10 +197,10 @@ func ValidateGroup(item models.Groupable) error { return nil } -func OnlyTeams(ctx context.Context, groups []models.Groupable) []models.Groupable { +func OnlyTeams(ctx context.Context, gs []models.Groupable) []models.Groupable { var teams []models.Groupable - for _, g := range groups { + for _, g := range gs { if IsTeam(ctx, g) { teams = append(teams, g) } diff --git a/src/pkg/services/m365/api/groups_test.go b/src/pkg/services/m365/api/groups_test.go index c57640070..c00b64a13 100644 --- a/src/pkg/services/m365/api/groups_test.go +++ b/src/pkg/services/m365/api/groups_test.go @@ -33,7 +33,7 @@ func (suite *GroupUnitSuite) TestValidateGroup() { tests := []struct { name string args models.Groupable - errCheck assert.ErrorAssertionFunc + expectErr assert.ErrorAssertionFunc errIsSkippable bool }{ { @@ -44,7 +44,7 @@ func (suite *GroupUnitSuite) TestValidateGroup() { s.SetDisplayName(ptr.To("testgroup")) return s }(), - errCheck: assert.NoError, + expectErr: assert.NoError, }, { name: "No name", @@ -53,7 +53,7 @@ func (suite *GroupUnitSuite) TestValidateGroup() { s.SetId(ptr.To("id")) return s }(), - errCheck: assert.Error, + expectErr: assert.Error, }, { name: "No ID", @@ -62,7 +62,7 @@ func (suite *GroupUnitSuite) TestValidateGroup() { s.SetDisplayName(ptr.To("testgroup")) return s }(), - errCheck: assert.Error, + expectErr: assert.Error, }, } @@ -71,7 +71,7 @@ func (suite *GroupUnitSuite) TestValidateGroup() { t := suite.T() err := api.ValidateGroup(test.args) - test.errCheck(t, err, clues.ToCore(err)) + test.expectErr(t, err, clues.ToCore(err)) if test.errIsSkippable { assert.ErrorIs(t, err, api.ErrKnownSkippableCase) @@ -111,29 +111,43 @@ func (suite *GroupsIntgSuite) TestGetAll() { } func (suite *GroupsIntgSuite) TestGroups_GetByID() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + var ( groupID = suite.its.group.id groupsAPI = suite.its.ac.Groups() ) + grp, err := groupsAPI.GetByID(ctx, groupID) + require.NoError(t, err, clues.ToCore(err)) + table := []struct { name string id string - expectErr func(*testing.T, error) + expectErr assert.ErrorAssertionFunc }{ { - name: "valid id", - id: groupID, - expectErr: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, + name: "valid id", + id: groupID, + expectErr: assert.NoError, }, { - name: "invalid id", - id: uuid.NewString(), - expectErr: func(t *testing.T, err error) { - assert.Error(t, err, clues.ToCore(err)) - }, + name: "invalid id", + id: uuid.NewString(), + expectErr: assert.Error, + }, + { + name: "valid display name", + id: ptr.Val(grp.GetDisplayName()), + expectErr: assert.NoError, + }, + { + name: "invalid displayName", + id: "jabberwocky", + expectErr: assert.Error, }, } for _, test := range table { @@ -144,7 +158,7 @@ func (suite *GroupsIntgSuite) TestGroups_GetByID() { defer flush() _, err := groupsAPI.GetByID(ctx, test.id) - test.expectErr(t, err) + test.expectErr(t, err, clues.ToCore(err)) }) } } diff --git a/src/pkg/services/m365/api/mail.go b/src/pkg/services/m365/api/mail.go index 7d6b309ab..4320a6668 100644 --- a/src/pkg/services/m365/api/mail.go +++ b/src/pkg/services/m365/api/mail.go @@ -169,7 +169,7 @@ func (c Mail) GetContainerByName( // Return an error if multiple container exist (unlikely) or if no container // is found. if len(gv) != 1 { - return nil, clues.New("unexpected number of folders returned"). + return nil, clues.Stack(graph.ErrMultipleResultsMatchIdentifier). With("returned_container_count", len(gv)). WithClues(ctx) } diff --git a/src/pkg/services/m365/api/sites.go b/src/pkg/services/m365/api/sites.go index 101629d43..36719470f 100644 --- a/src/pkg/services/m365/api/sites.go +++ b/src/pkg/services/m365/api/sites.go @@ -102,12 +102,14 @@ func (c Sites) GetAll(ctx context.Context, errs *fault.Bus) ([]models.Siteable, return us, el.Failure() } -const uuidRE = "[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}" +const uuidRETmpl = "[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}" + +var uuidRE = regexp.MustCompile(uuidRETmpl) // matches a site ID, with or without a doman name. Ex, either one of: // 10rqc2.sharepoint.com,deadbeef-0000-0000-0000-000000000000,beefdead-0000-0000-0000-000000000000 // deadbeef-0000-0000-0000-000000000000,beefdead-0000-0000-0000-000000000000 -var siteIDRE = regexp.MustCompile(`(.+,)?` + uuidRE + "," + uuidRE) +var siteIDRE = regexp.MustCompile(`(.+,)?` + uuidRETmpl + "," + uuidRETmpl) const sitesWebURLGetTemplate = "https://graph.microsoft.com/v1.0/sites/%s:/%s?$expand=drive"