From cf62c6283caefdf877435f11afd77a383a5c227d Mon Sep 17 00:00:00 2001 From: ashmrtn Date: Mon, 20 Feb 2023 09:50:47 -0800 Subject: [PATCH] Consolidate logic to see if Exchange/OneDrive user exists (#2508) ## Description Use the check to see if a user has access to OneDrive/Exchange to also see if the user exists. This does not stop the population of GraphConnector users every time an instance is created, but does move us one step closer to that Manually tested the following cases: * backup "*" * backup single existing user without error * backup multiple existing users without error * error when attempting to backup user that doesn't exist in the same domain * error when attempting to backup user that doesn't exist in a different domain Sample error output (progress disabled): ```text No backups available Error: 1 error occurred: * Failed to run Exchange backup for user foo@other.onmicrosoft.com: doing backup: producing backup data collections: resource owner [foo@other.onmicrosoft.com] not found within tenant ``` ## Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No ## Type of change - [ ] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup ## Issue(s) * #1547 ## Test Plan - [x] :muscle: Manual - [ ] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/connector/data_collections.go | 7 +- src/internal/connector/discovery/discovery.go | 6 ++ src/internal/connector/graph/errors.go | 5 ++ src/internal/connector/graph_connector.go | 10 --- .../graph_connector_disconnected_test.go | 68 ++----------------- 5 files changed, 19 insertions(+), 77 deletions(-) diff --git a/src/internal/connector/data_collections.go b/src/internal/connector/data_collections.go index 7429b9ea9..de9ae080f 100644 --- a/src/internal/connector/data_collections.go +++ b/src/internal/connector/data_collections.go @@ -44,7 +44,7 @@ func (gc *GraphConnector) DataCollections( ctx, end := D.Span(ctx, "gc:dataCollections", D.Index("service", sels.Service.String())) defer end() - err := verifyBackupInputs(sels, gc.GetUsers(), gc.GetSiteIDs()) + err := verifyBackupInputs(sels, gc.GetSiteIDs()) if err != nil { return nil, nil, clues.Stack(err).WithClues(ctx) } @@ -118,12 +118,13 @@ func (gc *GraphConnector) DataCollections( } } -func verifyBackupInputs(sels selectors.Selector, userPNs, siteIDs []string) error { +func verifyBackupInputs(sels selectors.Selector, siteIDs []string) error { var ids []string switch sels.Service { case selectors.ServiceExchange, selectors.ServiceOneDrive: - ids = userPNs + // Exchange and OneDrive user existence now checked in checkServiceEnabled. + return nil case selectors.ServiceSharePoint: ids = siteIDs diff --git a/src/internal/connector/discovery/discovery.go b/src/internal/connector/discovery/discovery.go index f8a6f27c7..a8da177e1 100644 --- a/src/internal/connector/discovery/discovery.go +++ b/src/internal/connector/discovery/discovery.go @@ -2,11 +2,13 @@ package discovery import ( "context" + "fmt" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/connector/discovery/api" + "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/pkg/fault" ) @@ -43,6 +45,10 @@ func Users(ctx context.Context, ga getAller, errs *fault.Errors) ([]models.Usera func User(ctx context.Context, gwi getWithInfoer, userID string) (models.Userable, *api.UserInfo, error) { u, err := gwi.GetByID(ctx, userID) if err != nil { + if graph.IsErrUserNotFound(err) { + return nil, nil, fmt.Errorf("resource owner [%s] not found within tenant", userID) + } + return nil, nil, errors.Wrap(err, "getting user") } diff --git a/src/internal/connector/graph/errors.go b/src/internal/connector/graph/errors.go index b8399da4c..56e6d98ed 100644 --- a/src/internal/connector/graph/errors.go +++ b/src/internal/connector/graph/errors.go @@ -26,6 +26,7 @@ const ( errCodeSyncFolderNotFound = "ErrorSyncFolderNotFound" errCodeSyncStateNotFound = "SyncStateNotFound" errCodeResourceNotFound = "ResourceNotFound" + errCodeRequestResourceNotFound = "Request_ResourceNotFound" errCodeMailboxNotEnabledForRESTAPI = "MailboxNotEnabledForRESTAPI" ) @@ -83,6 +84,10 @@ func IsErrExchangeMailFolderNotFound(err error) bool { return hasErrorCode(err, errCodeResourceNotFound, errCodeMailboxNotEnabledForRESTAPI) } +func IsErrUserNotFound(err error) bool { + return hasErrorCode(err, errCodeRequestResourceNotFound) +} + // Timeout errors are identified for tracking the need to retry calls. // Other delay errors, like throttling, are already handled by the // graph client's built-in retries. diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index 5b1679060..a61253c0f 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -147,16 +147,6 @@ func (gc *GraphConnector) setTenantUsers(ctx context.Context, errs *fault.Errors return nil } -// GetUsers returns the email address of users within the tenant. -func (gc *GraphConnector) GetUsers() []string { - return maps.Keys(gc.Users) -} - -// GetUsersIds returns the M365 id for the user -func (gc *GraphConnector) GetUsersIds() []string { - return maps.Values(gc.Users) -} - // setTenantSites queries the M365 to identify the sites in the // workspace. The sites field is updated during this method // iff the returned error is nil. diff --git a/src/internal/connector/graph_connector_disconnected_test.go b/src/internal/connector/graph_connector_disconnected_test.go index 226ac2904..651089295 100644 --- a/src/internal/connector/graph_connector_disconnected_test.go +++ b/src/internal/connector/graph_connector_disconnected_test.go @@ -130,67 +130,7 @@ func (suite *DisconnectedGraphConnectorSuite) TestGraphConnector_Status() { assert.Equal(t, 2, gc.Status().FolderCount) } -func (suite *DisconnectedGraphConnectorSuite) TestVerifyBackupInputs() { - users := []string{ - "elliotReid@someHospital.org", - "chrisTurk@someHospital.org", - "carlaEspinosa@someHospital.org", - "bobKelso@someHospital.org", - "johnDorian@someHospital.org", - } - - tests := []struct { - name string - getSelector func(t *testing.T) selectors.Selector - checkError assert.ErrorAssertionFunc - }{ - { - name: "No scopes", - checkError: assert.Error, - getSelector: func(t *testing.T) selectors.Selector { - return selectors.NewExchangeBackup(nil).Selector - }, - }, - { - name: "Valid Single User", - checkError: assert.NoError, - getSelector: func(t *testing.T) selectors.Selector { - sel := selectors.NewExchangeBackup([]string{"bobKelso@someHospital.org"}) - sel.Include(sel.MailFolders(selectors.Any())) - return sel.Selector - }, - }, - { - name: "Partial invalid user", - checkError: assert.Error, - getSelector: func(t *testing.T) selectors.Selector { - sel := selectors.NewExchangeBackup([]string{"bobkelso@someHospital.org", "janitor@someHospital.org"}) - sel.Include(sel.MailFolders(selectors.Any())) - sel.DiscreteOwner = "janitor@someHospital.org" - return sel.Selector - }, - }, - { - name: "Invalid discrete owner", - checkError: assert.Error, - getSelector: func(t *testing.T) selectors.Selector { - sel := selectors.NewOneDriveBackup([]string{"janitor@someHospital.org"}) - sel.Include(sel.AllData()) - return sel.Selector - }, - }, - } - - for _, test := range tests { - suite.T().Run(test.name, func(t *testing.T) { - err := verifyBackupInputs(test.getSelector(t), users, nil) - test.checkError(t, err) - }) - } -} - func (suite *DisconnectedGraphConnectorSuite) TestVerifyBackupInputs_allServices() { - users := []string{"elliotReid@someHospital.org"} sites := []string{"abc.site.foo", "bar.site.baz"} tests := []struct { @@ -224,7 +164,7 @@ func (suite *DisconnectedGraphConnectorSuite) TestVerifyBackupInputs_allServices }, { name: "Invalid User", - checkError: assert.Error, + checkError: assert.NoError, excludes: func(t *testing.T) selectors.Selector { sel := selectors.NewOneDriveBackup([]string{"foo@SomeCompany.org"}) sel.Exclude(sel.Folders(selectors.Any())) @@ -286,11 +226,11 @@ func (suite *DisconnectedGraphConnectorSuite) TestVerifyBackupInputs_allServices for _, test := range tests { suite.T().Run(test.name, func(t *testing.T) { - err := verifyBackupInputs(test.excludes(t), users, sites) + err := verifyBackupInputs(test.excludes(t), sites) test.checkError(t, err) - err = verifyBackupInputs(test.filters(t), users, sites) + err = verifyBackupInputs(test.filters(t), sites) test.checkError(t, err) - err = verifyBackupInputs(test.includes(t), users, sites) + err = verifyBackupInputs(test.includes(t), sites) test.checkError(t, err) }) }