From 896c05f62322bb2e4d9c31cb313f95a96236e4ed Mon Sep 17 00:00:00 2001 From: Keepers Date: Fri, 7 Apr 2023 16:29:09 -0600 Subject: [PATCH] look up users and sites by id or name (#2973) Adds a lookup step to graph connector to find an owner's id and name given some identifier. The identifier, for either sites or users, can be a well formed id or name. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :sunflower: Feature #### Issue(s) * #2825 #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/cli/backup/backup.go | 4 +- src/cli/backup/exchange_e2e_test.go | 2 +- src/cli/backup/onedrive_e2e_test.go | 2 +- src/internal/connector/data_collections.go | 4 +- .../connector/data_collections_test.go | 42 ++- src/internal/connector/discovery/api/sites.go | 80 +++++- .../connector/discovery/api/sites_test.go | 47 ++++ src/internal/connector/discovery/api/users.go | 18 +- src/internal/connector/discovery/discovery.go | 2 +- src/internal/connector/graph/errors.go | 19 +- src/internal/connector/graph_connector.go | 241 ++++++++++++------ .../graph_connector_onedrive_test.go | 12 +- .../connector/graph_connector_test.go | 157 +++++++++--- .../operations/backup_integration_test.go | 2 +- src/internal/operations/restore_test.go | 4 +- src/internal/tester/config.go | 8 + src/internal/tester/resource_owners.go | 12 +- src/pkg/repository/repository.go | 2 +- src/pkg/repository/repository_test.go | 4 +- 19 files changed, 493 insertions(+), 169 deletions(-) diff --git a/src/cli/backup/backup.go b/src/cli/backup/backup.go index c55e49792..01832fe29 100644 --- a/src/cli/backup/backup.go +++ b/src/cli/backup/backup.go @@ -253,12 +253,12 @@ func runBackups( // genericDeleteCommand is a helper function that all services can use // for the removal of an entry from the repository func genericDeleteCommand(cmd *cobra.Command, bID, designation string, args []string) error { - ctx := clues.Add(cmd.Context(), "delete_backup_id", bID) - if utils.HasNoFlagsAndShownHelp(cmd) { return nil } + ctx := clues.Add(cmd.Context(), "delete_backup_id", bID) + r, _, err := getAccountAndConnect(ctx) if err != nil { return Only(ctx, err) diff --git a/src/cli/backup/exchange_e2e_test.go b/src/cli/backup/exchange_e2e_test.go index 9dba497e7..b2f93439e 100644 --- a/src/cli/backup/exchange_e2e_test.go +++ b/src/cli/backup/exchange_e2e_test.go @@ -235,7 +235,7 @@ func (suite *BackupExchangeE2ESuite) TestExchangeBackupCmd_UserNotInTenant() { assert.Contains( t, err.Error(), - "not found within tenant", "error missing user not found") + "not found in tenant", "error missing user not found") assert.NotContains(t, err.Error(), "runtime error", "panic happened") t.Logf("backup error message: %s", err.Error()) diff --git a/src/cli/backup/onedrive_e2e_test.go b/src/cli/backup/onedrive_e2e_test.go index 104603d20..b1c47808e 100644 --- a/src/cli/backup/onedrive_e2e_test.go +++ b/src/cli/backup/onedrive_e2e_test.go @@ -140,7 +140,7 @@ func (suite *NoBackupOneDriveE2ESuite) TestOneDriveBackupCmd_UserNotInTenant() { assert.Contains( t, err.Error(), - "not found within tenant", "error missing user not found") + "not found in tenant", "error missing user not found") assert.NotContains(t, err.Error(), "runtime error", "panic happened") t.Logf("backup error message: %s", err.Error()) diff --git a/src/internal/connector/data_collections.go b/src/internal/connector/data_collections.go index a76ad41d7..7c3f14f52 100644 --- a/src/internal/connector/data_collections.go +++ b/src/internal/connector/data_collections.go @@ -53,7 +53,7 @@ func (gc *GraphConnector) ProduceBackupCollections( serviceEnabled, err := checkServiceEnabled( ctx, - gc.Owners.Users(), + gc.Discovery.Users(), path.ServiceType(sels.Service), sels.DiscreteOwner) if err != nil { @@ -162,7 +162,7 @@ func verifyBackupInputs(sels selectors.Selector, siteIDs []string) error { } if !found { - return clues.New("resource owner not found within tenant").With("missing_resource_owner", sels.DiscreteOwner) + return clues.Stack(graph.ErrResourceOwnerNotFound).With("missing_resource_owner", sels.DiscreteOwner) } return nil diff --git a/src/internal/connector/data_collections_test.go b/src/internal/connector/data_collections_test.go index a50ea0106..479347a0d 100644 --- a/src/internal/connector/data_collections_test.go +++ b/src/internal/connector/data_collections_test.go @@ -25,27 +25,21 @@ import ( // DataCollection tests // --------------------------------------------------------------------------- -type ConnectorDataCollectionIntegrationSuite struct { +type DataCollectionIntgSuite struct { tester.Suite - connector *GraphConnector - user string - site string + user string + site string } -func TestConnectorDataCollectionIntegrationSuite(t *testing.T) { - suite.Run(t, &ConnectorDataCollectionIntegrationSuite{ +func TestDataCollectionIntgSuite(t *testing.T) { + suite.Run(t, &DataCollectionIntgSuite{ Suite: tester.NewIntegrationSuite( t, - [][]string{tester.M365AcctCredEnvs}, - ), + [][]string{tester.M365AcctCredEnvs}), }) } -func (suite *ConnectorDataCollectionIntegrationSuite) SetupSuite() { - ctx, flush := tester.NewContext() - defer flush() - - suite.connector = loadConnector(ctx, suite.T(), graph.HTTPClient(graph.NoTimeout()), AllResources) +func (suite *DataCollectionIntgSuite) SetupSuite() { suite.user = tester.M365UserID(suite.T()) suite.site = tester.M365SiteID(suite.T()) @@ -58,7 +52,7 @@ func (suite *ConnectorDataCollectionIntegrationSuite) SetupSuite() { // - mail // - contacts // - events -func (suite *ConnectorDataCollectionIntegrationSuite) TestExchangeDataCollection() { +func (suite *DataCollectionIntgSuite) TestExchangeDataCollection() { ctx, flush := tester.NewContext() defer flush() @@ -138,7 +132,7 @@ func (suite *ConnectorDataCollectionIntegrationSuite) TestExchangeDataCollection } // TestInvalidUserForDataCollections ensures verification process for users -func (suite *ConnectorDataCollectionIntegrationSuite) TestDataCollections_invalidResourceOwner() { +func (suite *DataCollectionIntgSuite) TestDataCollections_invalidResourceOwner() { ctx, flush := tester.NewContext() defer flush() @@ -223,7 +217,7 @@ func (suite *ConnectorDataCollectionIntegrationSuite) TestDataCollections_invali // TestSharePointDataCollection verifies interface between operation and // GraphConnector remains stable to receive a non-zero amount of Collections // for the SharePoint Package. -func (suite *ConnectorDataCollectionIntegrationSuite) TestSharePointDataCollection() { +func (suite *DataCollectionIntgSuite) TestSharePointDataCollection() { ctx, flush := tester.NewContext() defer flush() @@ -299,14 +293,14 @@ func (suite *ConnectorDataCollectionIntegrationSuite) TestSharePointDataCollecti // CreateSharePointCollection tests // --------------------------------------------------------------------------- -type ConnectorCreateSharePointCollectionIntegrationSuite struct { +type SPCollectionIntgSuite struct { tester.Suite connector *GraphConnector user string } -func TestConnectorCreateSharePointCollectionIntegrationSuite(t *testing.T) { - suite.Run(t, &ConnectorCreateSharePointCollectionIntegrationSuite{ +func TestSPCollectionIntgSuite(t *testing.T) { + suite.Run(t, &SPCollectionIntgSuite{ Suite: tester.NewIntegrationSuite( t, [][]string{tester.M365AcctCredEnvs}, @@ -314,7 +308,7 @@ func TestConnectorCreateSharePointCollectionIntegrationSuite(t *testing.T) { }) } -func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) SetupSuite() { +func (suite *SPCollectionIntgSuite) SetupSuite() { ctx, flush := tester.NewContext() defer flush() @@ -324,7 +318,7 @@ func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) SetupSuite() { tester.LogTimeOfTest(suite.T()) } -func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) TestCreateSharePointCollection_Libraries() { +func (suite *SPCollectionIntgSuite) TestCreateSharePointCollection_Libraries() { ctx, flush := tester.NewContext() defer flush() @@ -335,7 +329,7 @@ func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) TestCreateShar siteIDs = []string{siteID} ) - id, name, err := gc.PopulateOwnerIDAndNamesFrom(siteID, nil) + id, name, err := gc.PopulateOwnerIDAndNamesFrom(ctx, siteID, nil) require.NoError(t, err, clues.ToCore(err)) sel := selectors.NewSharePointBackup(siteIDs) @@ -368,7 +362,7 @@ func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) TestCreateShar cols[1].FullPath().Service().String()) } -func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) TestCreateSharePointCollection_Lists() { +func (suite *SPCollectionIntgSuite) TestCreateSharePointCollection_Lists() { ctx, flush := tester.NewContext() defer flush() @@ -379,7 +373,7 @@ func (suite *ConnectorCreateSharePointCollectionIntegrationSuite) TestCreateShar siteIDs = []string{siteID} ) - id, name, err := gc.PopulateOwnerIDAndNamesFrom(siteID, nil) + id, name, err := gc.PopulateOwnerIDAndNamesFrom(ctx, siteID, nil) require.NoError(t, err, clues.ToCore(err)) sel := selectors.NewSharePointBackup(siteIDs) diff --git a/src/internal/connector/discovery/api/sites.go b/src/internal/connector/discovery/api/sites.go index 5d64bb778..2bb47cc18 100644 --- a/src/internal/connector/discovery/api/sites.go +++ b/src/internal/connector/discovery/api/sites.go @@ -3,6 +3,8 @@ package api import ( "context" "fmt" + "net/url" + "regexp" "strings" "github.com/alcionai/clues" @@ -12,6 +14,7 @@ import ( "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/connector/graph" + "github.com/alcionai/corso/src/internal/connector/graph/betasdk/sites" "github.com/alcionai/corso/src/pkg/fault" ) @@ -85,15 +88,74 @@ func (c Sites) GetAll(ctx context.Context, errs *fault.Bus) ([]models.Siteable, return us, el.Failure() } -func (c Sites) GetByID(ctx context.Context, id string) (models.Siteable, error) { - resp, err := c.stable.Client().SitesById(id).Get(ctx, nil) +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}" + +// 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) + +const webURLGetTemplate = "https://graph.microsoft.com/v1.0/sites/%s:/%s" + +// GetByID looks up the site matching the given identifier. The identifier can be either a +// canonical site id or a webURL. Assumes the webURL is complete and well formed; +// eg: https://10rqc2.sharepoint.com/sites/Example +func (c Sites) GetByID(ctx context.Context, identifier string) (models.Siteable, error) { + var ( + resp models.Siteable + err error + ) + + ctx = clues.Add(ctx, "given_site_id", identifier) + + if siteIDRE.MatchString(identifier) { + resp, err = c.stable.Client().SitesById(identifier).Get(ctx, nil) + if err != nil { + return nil, graph.Wrap(ctx, err, "getting site by id") + } + + return resp, err + } + + // if the id is not a standard sharepoint ID, assume it's a url. + // if it has a leading slash, assume it's only a path. If it doesn't, + // ensure it has a prefix https:// + if !strings.HasPrefix(identifier, "/") { + identifier = strings.TrimPrefix(identifier, "https://") + identifier = "https://" + identifier + } + + u, err := url.Parse(identifier) if err != nil { - return nil, graph.Wrap(ctx, err, "getting site") + return nil, clues.Wrap(err, "site is not parseable as a url") + } + + // don't construct a path with double leading slashes + path := strings.TrimPrefix(u.Path, "/") + rawURL := fmt.Sprintf(webURLGetTemplate, u.Host, path) + + resp, err = sites. + NewItemSitesSiteItemRequestBuilder(rawURL, c.stable.Adapter()). + Get(ctx, nil) + if err != nil { + return nil, graph.Wrap(ctx, err, "getting site by weburl") } return resp, err } +// GetIDAndName looks up the site matching the given ID, and returns +// its canonical ID and the webURL as the name. Accepts an ID or a +// WebURL as an ID. +func (c Sites) GetIDAndName(ctx context.Context, siteID string) (string, string, error) { + s, err := c.GetByID(ctx, siteID) + if err != nil { + return "", "", err + } + + return ptr.Val(s.GetId()), ptr.Val(s.GetWebUrl()), nil +} + // --------------------------------------------------------------------------- // helpers // --------------------------------------------------------------------------- @@ -116,23 +178,23 @@ func validateSite(item any) (models.Siteable, error) { return nil, clues.New("missing ID") } - url := ptr.Val(m.GetWebUrl()) - if len(url) == 0 { + wURL := ptr.Val(m.GetWebUrl()) + if len(wURL) == 0 { return nil, clues.New("missing webURL").With("site_id", id) // TODO: pii } // personal (ie: oneDrive) sites have to be filtered out server-side. - if strings.Contains(url, personalSitePath) { + if strings.Contains(wURL, personalSitePath) { return nil, clues.Stack(errKnownSkippableCase). - With("site_id", id, "site_url", url) // TODO: pii + With("site_id", id, "site_web_url", wURL) // TODO: pii } name := ptr.Val(m.GetDisplayName()) if len(name) == 0 { // the built-in site at "https://{tenant-domain}/search" never has a name. - if strings.HasSuffix(url, "/search") { + if strings.HasSuffix(wURL, "/search") { return nil, clues.Stack(errKnownSkippableCase). - With("site_id", id, "site_url", url) // TODO: pii + With("site_id", id, "site_web_url", wURL) // TODO: pii } return nil, clues.New("missing site display name").With("site_id", id) diff --git a/src/internal/connector/discovery/api/sites_test.go b/src/internal/connector/discovery/api/sites_test.go index b03b464b9..b0c713a3d 100644 --- a/src/internal/connector/discovery/api/sites_test.go +++ b/src/internal/connector/discovery/api/sites_test.go @@ -1,9 +1,11 @@ package api import ( + "strings" "testing" "github.com/alcionai/clues" + "github.com/google/uuid" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -153,3 +155,48 @@ func (suite *SitesIntgSuite) TestGetAll() { assert.NotContains(t, ptr.Val(site.GetWebUrl()), personalSitePath, "must not return onedrive sites") } } + +func (suite *SitesIntgSuite) TestSites_GetByID() { + var ( + t = suite.T() + siteID = tester.M365SiteID(t) + host = strings.Split(siteID, ",")[0] + shortID = strings.TrimPrefix(siteID, host+",") + siteURL = tester.M365SiteURL(t) + acct = tester.NewM365Account(t) + ) + + creds, err := acct.M365Config() + require.NoError(t, err, clues.ToCore(err)) + + client, err := NewClient(creds) + require.NoError(t, err, clues.ToCore(err)) + + sitesAPI := client.Sites() + + table := []struct { + name string + id string + expectErr assert.ErrorAssertionFunc + }{ + {"3 part id", siteID, assert.NoError}, + {"2 part id", shortID, assert.NoError}, + {"malformed id", uuid.NewString(), assert.Error}, + {"random id", uuid.NewString() + "," + uuid.NewString(), assert.Error}, + {"url", siteURL, assert.NoError}, + {"host only", host, assert.NoError}, + {"malformed url", "barunihlda", assert.Error}, + {"non-matching url", "https://test/sites/testing", assert.Error}, + } + for _, test := range table { + suite.Run(test.name, func() { + ctx, flush := tester.NewContext() + defer flush() + + t := suite.T() + + _, err := sitesAPI.GetByID(ctx, test.id) + test.expectErr(t, err, clues.ToCore(err)) + }) + } +} diff --git a/src/internal/connector/discovery/api/users.go b/src/internal/connector/discovery/api/users.go index 186a44e90..931b689fe 100644 --- a/src/internal/connector/discovery/api/users.go +++ b/src/internal/connector/discovery/api/users.go @@ -10,6 +10,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/microsoftgraph/msgraph-sdk-go/users" + "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" @@ -147,13 +148,15 @@ func (c Users) GetAll(ctx context.Context, errs *fault.Bus) ([]models.Userable, return us, el.Failure() } -func (c Users) GetByID(ctx context.Context, userID string) (models.Userable, error) { +// GetByID looks up the user matching the given identifier. The identifier can be either a +// canonical user id or a princpalName. +func (c Users) GetByID(ctx context.Context, identifier string) (models.Userable, error) { var ( resp models.Userable err error ) - resp, err = c.stable.Client().UsersById(userID).Get(ctx, nil) + resp, err = c.stable.Client().UsersById(identifier).Get(ctx, nil) if err != nil { return nil, graph.Wrap(ctx, err, "getting user") @@ -162,6 +165,17 @@ func (c Users) GetByID(ctx context.Context, userID string) (models.Userable, err return resp, err } +// GetIDAndName looks up the user matching the given ID, and returns +// its canonical ID and the PrincipalName as the name. +func (c Users) GetIDAndName(ctx context.Context, userID string) (string, string, error) { + u, err := c.GetByID(ctx, userID) + if err != nil { + return "", "", err + } + + return ptr.Val(u.GetId()), ptr.Val(u.GetUserPrincipalName()), nil +} + func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { // Assume all services are enabled // then filter down to only services the user has enabled diff --git a/src/internal/connector/discovery/discovery.go b/src/internal/connector/discovery/discovery.go index 62e30cecc..f4ac9efaa 100644 --- a/src/internal/connector/discovery/discovery.go +++ b/src/internal/connector/discovery/discovery.go @@ -77,7 +77,7 @@ func User( u, err := gwi.GetByID(ctx, userID) if err != nil { if graph.IsErrUserNotFound(err) { - return nil, nil, clues.New("resource owner not found within tenant").With("user_id", userID) + return nil, nil, clues.Stack(graph.ErrResourceOwnerNotFound).With("user_id", userID) } return nil, nil, clues.Wrap(err, "getting user") diff --git a/src/internal/connector/graph/errors.go b/src/internal/connector/graph/errors.go index 4d84a94de..0df4c26f0 100644 --- a/src/internal/connector/graph/errors.go +++ b/src/internal/connector/graph/errors.go @@ -70,6 +70,8 @@ var ( // graph client's built-in retries. // https://github.com/microsoftgraph/msgraph-sdk-go/issues/302 ErrTimeout = clues.New("communication timeout") + + ErrResourceOwnerNotFound = clues.New("resource owner not found in tenant") ) func IsErrDeletedInFlight(err error) bool { @@ -191,7 +193,11 @@ func Wrap(ctx context.Context, e error, msg string) *clues.Err { return clues.Wrap(e, msg).WithClues(ctx) } - data, innerMsg := errData(odErr) + mainMsg, data, innerMsg := errData(odErr) + + if len(mainMsg) > 0 { + e = clues.Stack(e, clues.New(mainMsg)) + } return setLabels(clues.Wrap(e, msg).WithClues(ctx).With(data...), innerMsg) } @@ -208,7 +214,11 @@ func Stack(ctx context.Context, e error) *clues.Err { return clues.Stack(e).WithClues(ctx) } - data, innerMsg := errData(odErr) + mainMsg, data, innerMsg := errData(odErr) + + if len(mainMsg) > 0 { + e = clues.Stack(e, clues.New(mainMsg)) + } return setLabels(clues.Stack(e).WithClues(ctx).With(data...), innerMsg) } @@ -226,11 +236,12 @@ func setLabels(err *clues.Err, msg string) *clues.Err { return err } -func errData(err odataerrors.ODataErrorable) ([]any, string) { +func errData(err odataerrors.ODataErrorable) (string, []any, string) { data := make([]any, 0) // Get MainError mainErr := err.GetError() + mainMsg := ptr.Val(mainErr.GetMessage()) data = appendIf(data, "odataerror_code", mainErr.GetCode()) data = appendIf(data, "odataerror_message", mainErr.GetMessage()) @@ -251,7 +262,7 @@ func errData(err odataerrors.ODataErrorable) ([]any, string) { data = appendIf(data, "odataerror_inner_req_id", inner.GetRequestId()) } - return data, strings.ToLower(msgConcat) + return mainMsg, data, strings.ToLower(msgConcat) } func appendIf(a []any, k string, v *string) []any { diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index 4e5a411cc..b9003ef8c 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -9,7 +9,6 @@ import ( "sync" "github.com/alcionai/clues" - "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/common" "github.com/alcionai/corso/src/internal/connector/discovery/api" @@ -36,12 +35,13 @@ var ( // bookkeeping and interfacing with other component. type GraphConnector struct { Service graph.Servicer - Owners api.Client + Discovery api.Client itemClient *http.Client // configured to handle large item downloads tenant string credentials account.M365Config + ownerLookup getOwnerIDAndNamer // maps of resource owner ids to names, and names to ids. // not guaranteed to be populated, only here as a post-population // reference for processes that choose to populate the values. @@ -56,15 +56,6 @@ type GraphConnector struct { status support.ConnectorOperationStatus // contains the status of the last run status } -type resource int - -const ( - UnknownResource resource = iota - AllResources - Users - Sites -) - func NewGraphConnector( ctx context.Context, itemClient *http.Client, @@ -72,95 +63,51 @@ func NewGraphConnector( r resource, errs *fault.Bus, ) (*GraphConnector, error) { - m365, err := acct.M365Config() + creds, err := acct.M365Config() if err != nil { return nil, clues.Wrap(err, "retrieving m365 account configuration").WithClues(ctx) } - gc := GraphConnector{ - itemClient: itemClient, - tenant: m365.AzureTenantID, - wg: &sync.WaitGroup{}, - credentials: m365, - IDNameLookup: common.IDsNames{}, - } - - gc.Service, err = gc.createService() + service, err := createService(creds) if err != nil { return nil, clues.Wrap(err, "creating service connection").WithClues(ctx) } - gc.Owners, err = api.NewClient(m365) + discovery, err := api.NewClient(creds) if err != nil { return nil, clues.Wrap(err, "creating api client").WithClues(ctx) } + rc, err := r.resourceClient(discovery) + if err != nil { + return nil, clues.Wrap(err, "creating resource client").WithClues(ctx) + } + + gc := GraphConnector{ + Discovery: discovery, + IDNameLookup: common.IDsNames{}, + Service: service, + + credentials: creds, + itemClient: itemClient, + ownerLookup: rc, + tenant: acct.ID(), + wg: &sync.WaitGroup{}, + } + return &gc, nil } -// PopulateOwnerIDAndNamesFrom takes the provided owner identifier and produces -// the owner's name and ID from that value. Returns an error if the owner is -// not recognized by the current tenant. -// -// The id-name swapper is optional. Some processes will look up all owners in -// the tenant before reaching this step. In that case, the data gets handed -// down for this func to consume instead of performing further queries. The -// maps get stored inside the gc instance for later re-use. -// -// TODO: If the maps are nil or empty, this func will perform a lookup on the given -// owner, and populate each map with that owner's id and name for downstream -// guarantees about that data being present. Optional performance enhancement -// idea: downstream from here, we should _only_ need the given user's id and name, -// and could store minimal map copies with that info instead of the whole tenant. -func (gc *GraphConnector) PopulateOwnerIDAndNamesFrom( - owner string, // input value, can be either id or name - ins common.IDNameSwapper, -) (string, string, error) { - // move this to GC method - id, name, err := getOwnerIDAndNameFrom(owner, ins) - if err != nil { - return "", "", errors.Wrap(err, "resolving resource owner details") - } - - gc.IDNameLookup = ins - - if ins == nil || (len(ins.IDs()) == 0 && len(ins.Names()) == 0) { - gc.IDNameLookup = common.IDsNames{ - IDToName: map[string]string{id: name}, - NameToID: map[string]string{name: id}, - } - } - - return id, name, nil -} - -func getOwnerIDAndNameFrom( - owner string, - ins common.IDNameSwapper, -) (string, string, error) { - if ins == nil { - return owner, owner, nil - } - - if n, ok := ins.NameOf(owner); ok { - return owner, n, nil - } else if i, ok := ins.IDOf(owner); ok { - return i, owner, nil - } - - // TODO: look-up user by owner, either id or name, - // and populate with maps as a result. Only - // return owner, owner as a very last resort. - - return "", "", clues.New("not found within tenant") -} +// --------------------------------------------------------------------------- +// Service Client +// --------------------------------------------------------------------------- // createService constructor for graphService component -func (gc *GraphConnector) createService() (*graph.Service, error) { +func createService(creds account.M365Config) (*graph.Service, error) { adapter, err := graph.CreateAdapter( - gc.credentials.AzureTenantID, - gc.credentials.AzureClientID, - gc.credentials.AzureClientSecret) + creds.AzureTenantID, + creds.AzureClientID, + creds.AzureClientSecret) if err != nil { return &graph.Service{}, err } @@ -168,6 +115,10 @@ func (gc *GraphConnector) createService() (*graph.Service, error) { return graph.NewService(adapter), nil } +// --------------------------------------------------------------------------- +// Processing Status +// --------------------------------------------------------------------------- + // AwaitStatus waits for all gc tasks to complete and then returns status func (gc *GraphConnector) Wait() *data.CollectionStats { defer func() { @@ -223,3 +174,129 @@ func (gc *GraphConnector) incrementAwaitingMessages() { func (gc *GraphConnector) incrementMessagesBy(num int) { gc.wg.Add(num) } + +// --------------------------------------------------------------------------- +// Resource Lookup Handling +// --------------------------------------------------------------------------- + +type resource int + +const ( + UnknownResource resource = iota + AllResources // unused + Users + Sites +) + +func (r resource) resourceClient(discovery api.Client) (*resourceClient, error) { + switch r { + case Users: + return &resourceClient{enum: r, getter: discovery.Users()}, nil + case Sites: + return &resourceClient{enum: r, getter: discovery.Sites()}, nil + default: + return nil, clues.New("unrecognized owner resource enum").With("resource_enum", r) + } +} + +type resourceClient struct { + enum resource + getter getIDAndNamer +} + +type getIDAndNamer interface { + GetIDAndName(ctx context.Context, owner string) ( + ownerID string, + ownerName string, + err error, + ) +} + +var _ getOwnerIDAndNamer = &resourceClient{} + +type getOwnerIDAndNamer interface { + getOwnerIDAndNameFrom( + ctx context.Context, + discovery api.Client, + owner string, + ins common.IDNameSwapper, + ) ( + ownerID string, + ownerName string, + err error, + ) +} + +// getOwnerIDAndNameFrom looks up the owner's canonical id and display name. +// If the owner is present in the idNameSwapper, then that interface's id and +// name values are returned. As a fallback, the resource calls the discovery +// api to fetch the user or site using the owner value. This fallback assumes +// that the owner is a well formed ID or display name of appropriate design +// (PrincipalName for users, WebURL for sites). +func (r resourceClient) getOwnerIDAndNameFrom( + ctx context.Context, + discovery api.Client, + owner string, + ins common.IDNameSwapper, +) (string, string, error) { + if ins != nil { + if n, ok := ins.NameOf(owner); ok { + return owner, n, nil + } else if i, ok := ins.IDOf(owner); ok { + return i, owner, nil + } + } + + ctx = clues.Add(ctx, "owner_identifier", owner) + + var ( + id, name string + err error + ) + + // if r.enum == Sites { + // TODO: check all suffixes in nameToID + // } + + id, name, err = r.getter.GetIDAndName(ctx, owner) + if err != nil { + if graph.IsErrUserNotFound(err) { + return "", "", clues.Stack(graph.ErrResourceOwnerNotFound, err) + } + + return "", "", err + } + + if len(id) == 0 || len(name) == 0 { + return "", "", clues.Stack(graph.ErrResourceOwnerNotFound) + } + + return id, name, nil +} + +// PopulateOwnerIDAndNamesFrom takes the provided owner identifier and produces +// the owner's name and ID from that value. Returns an error if the owner is +// not recognized by the current tenant. +// +// The id-name swapper is optional. Some processes will look up all owners in +// the tenant before reaching this step. In that case, the data gets handed +// down for this func to consume instead of performing further queries. The +// data gets stored inside the gc instance for later re-use. +func (gc *GraphConnector) PopulateOwnerIDAndNamesFrom( + ctx context.Context, + owner string, // input value, can be either id or name + ins common.IDNameSwapper, +) (string, string, error) { + // move this to GC method + id, name, err := gc.ownerLookup.getOwnerIDAndNameFrom(ctx, gc.Discovery, owner, ins) + if err != nil { + return "", "", clues.Wrap(err, "identifying resource owner") + } + + gc.IDNameLookup = common.IDsNames{ + IDToName: map[string]string{id: name}, + NameToID: map[string]string{name: id}, + } + + return id, name, nil +} diff --git a/src/internal/connector/graph_connector_onedrive_test.go b/src/internal/connector/graph_connector_onedrive_test.go index 02b6f454e..242681b84 100644 --- a/src/internal/connector/graph_connector_onedrive_test.go +++ b/src/internal/connector/graph_connector_onedrive_test.go @@ -452,11 +452,11 @@ func (suite *GraphConnectorSharePointIntegrationSuite) SetupSuite() { si.resourceOwner = tester.M365SiteID(suite.T()) - user, err := si.connector.Owners.Users().GetByID(ctx, si.user) + user, err := si.connector.Discovery.Users().GetByID(ctx, si.user) require.NoError(suite.T(), err, "fetching user", si.user, clues.ToCore(err)) si.userID = ptr.Val(user.GetId()) - secondaryUser, err := si.connector.Owners.Users().GetByID(ctx, si.secondaryUser) + secondaryUser, err := si.connector.Discovery.Users().GetByID(ctx, si.secondaryUser) require.NoError(suite.T(), err, "fetching user", si.secondaryUser, clues.ToCore(err)) si.secondaryUserID = ptr.Val(secondaryUser.GetId()) @@ -499,11 +499,11 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) SetupSuite() { si.resourceOwner = si.user - user, err := si.connector.Owners.Users().GetByID(ctx, si.user) + user, err := si.connector.Discovery.Users().GetByID(ctx, si.user) require.NoError(suite.T(), err, "fetching user", si.user, clues.ToCore(err)) si.userID = ptr.Val(user.GetId()) - secondaryUser, err := si.connector.Owners.Users().GetByID(ctx, si.secondaryUser) + secondaryUser, err := si.connector.Discovery.Users().GetByID(ctx, si.secondaryUser) require.NoError(suite.T(), err, "fetching user", si.secondaryUser, clues.ToCore(err)) si.secondaryUserID = ptr.Val(secondaryUser.GetId()) @@ -558,11 +558,11 @@ func (suite *GraphConnectorOneDriveNightlySuite) SetupSuite() { si.resourceOwner = si.user - user, err := si.connector.Owners.Users().GetByID(ctx, si.user) + user, err := si.connector.Discovery.Users().GetByID(ctx, si.user) require.NoError(suite.T(), err, "fetching user", si.user, clues.ToCore(err)) si.userID = ptr.Val(user.GetId()) - secondaryUser, err := si.connector.Owners.Users().GetByID(ctx, si.secondaryUser) + secondaryUser, err := si.connector.Discovery.Users().GetByID(ctx, si.secondaryUser) require.NoError(suite.T(), err, "fetching user", si.secondaryUser, clues.ToCore(err)) si.secondaryUserID = ptr.Val(secondaryUser.GetId()) diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 3f12446b3..746cf1fbd 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -39,132 +39,231 @@ func TestGraphConnectorUnitSuite(t *testing.T) { suite.Run(t, &GraphConnectorUnitSuite{Suite: tester.NewUnitSuite(t)}) } +var _ getIDAndNamer = &mockNameIDGetter{} + +type mockNameIDGetter struct { + id, name string +} + +func (mnig mockNameIDGetter) GetIDAndName( + _ context.Context, + _ string, +) (string, string, error) { + return mnig.id, mnig.name, nil +} + func (suite *GraphConnectorUnitSuite) TestPopulateOwnerIDAndNamesFrom() { const ( - ownerID = "owner-id" - ownerName = "owner-name" + id = "owner-id" + name = "owner-name" ) var ( - itn = map[string]string{ownerID: ownerName} - nti = map[string]string{ownerName: ownerID} + itn = map[string]string{id: name} + nti = map[string]string{name: id} + lookup = &resourceClient{ + enum: Users, + getter: &mockNameIDGetter{id: id, name: name}, + } + noLookup = &resourceClient{enum: Users, getter: &mockNameIDGetter{}} ) table := []struct { name string owner string ins common.IDsNames + rc *resourceClient expectID string expectName string expectErr require.ErrorAssertionFunc }{ { name: "nil ins", - owner: ownerID, + owner: id, + rc: lookup, + expectID: id, + expectName: name, + expectErr: require.NoError, + }, + { + name: "nil ins no lookup", + owner: id, + rc: noLookup, expectID: "", expectName: "", expectErr: require.Error, }, { name: "only id map with owner id", - owner: ownerID, + owner: id, ins: common.IDsNames{ IDToName: itn, NameToID: nil, }, - expectID: ownerID, - expectName: ownerName, + rc: noLookup, + expectID: id, + expectName: name, expectErr: require.NoError, }, { name: "only name map with owner id", - owner: ownerID, + owner: id, ins: common.IDsNames{ IDToName: nil, NameToID: nti, }, + rc: noLookup, expectID: "", expectName: "", expectErr: require.Error, }, + { + name: "only name map with owner id and lookup", + owner: id, + ins: common.IDsNames{ + IDToName: nil, + NameToID: nti, + }, + rc: lookup, + expectID: id, + expectName: name, + expectErr: require.NoError, + }, { name: "only id map with owner name", - owner: ownerName, + owner: name, ins: common.IDsNames{ IDToName: itn, NameToID: nil, }, + rc: lookup, + expectID: id, + expectName: name, + expectErr: require.NoError, + }, + { + name: "only name map with owner name", + owner: name, + ins: common.IDsNames{ + IDToName: nil, + NameToID: nti, + }, + rc: noLookup, + expectID: id, + expectName: name, + expectErr: require.NoError, + }, + { + name: "only id map with owner name", + owner: name, + ins: common.IDsNames{ + IDToName: itn, + NameToID: nil, + }, + rc: noLookup, expectID: "", expectName: "", expectErr: require.Error, }, { - name: "only name map with owner name", - owner: ownerName, + name: "only id map with owner name and lookup", + owner: name, ins: common.IDsNames{ - IDToName: nil, - NameToID: nti, + IDToName: itn, + NameToID: nil, }, - expectID: ownerID, - expectName: ownerName, + rc: lookup, + expectID: id, + expectName: name, expectErr: require.NoError, }, { name: "both maps with owner id", - owner: ownerID, + owner: id, ins: common.IDsNames{ IDToName: itn, NameToID: nti, }, - expectID: ownerID, - expectName: ownerName, + rc: noLookup, + expectID: id, + expectName: name, expectErr: require.NoError, }, { name: "both maps with owner name", - owner: ownerName, + owner: name, ins: common.IDsNames{ IDToName: itn, NameToID: nti, }, - expectID: ownerID, - expectName: ownerName, + rc: noLookup, + expectID: id, + expectName: name, expectErr: require.NoError, }, { name: "non-matching maps with owner id", - owner: ownerID, + owner: id, ins: common.IDsNames{ IDToName: map[string]string{"foo": "bar"}, NameToID: map[string]string{"fnords": "smarf"}, }, + rc: noLookup, expectID: "", expectName: "", expectErr: require.Error, }, { name: "non-matching with owner name", - owner: ownerName, + owner: name, ins: common.IDsNames{ IDToName: map[string]string{"foo": "bar"}, NameToID: map[string]string{"fnords": "smarf"}, }, + rc: noLookup, expectID: "", expectName: "", expectErr: require.Error, }, + { + name: "non-matching maps with owner id and lookup", + owner: id, + ins: common.IDsNames{ + IDToName: map[string]string{"foo": "bar"}, + NameToID: map[string]string{"fnords": "smarf"}, + }, + rc: lookup, + expectID: id, + expectName: name, + expectErr: require.NoError, + }, + { + name: "non-matching with owner name and lookup", + owner: name, + ins: common.IDsNames{ + IDToName: map[string]string{"foo": "bar"}, + NameToID: map[string]string{"fnords": "smarf"}, + }, + rc: lookup, + expectID: id, + expectName: name, + expectErr: require.NoError, + }, } for _, test := range table { suite.Run(test.name, func() { + ctx, flush := tester.NewContext() + defer flush() + var ( t = suite.T() - gc = &GraphConnector{} + gc = &GraphConnector{ownerLookup: test.rc} ) - id, name, err := gc.PopulateOwnerIDAndNamesFrom(test.owner, test.ins) + rID, rName, err := gc.PopulateOwnerIDAndNamesFrom(ctx, test.owner, test.ins) test.expectErr(t, err, clues.ToCore(err)) - assert.Equal(t, test.expectID, id) - assert.Equal(t, test.expectName, name) + assert.Equal(t, test.expectID, rID, "id") + assert.Equal(t, test.expectName, rName, "name") }) } } @@ -1165,7 +1264,7 @@ func (suite *GraphConnectorIntegrationSuite) TestBackup_CreatesPrefixCollections start = time.Now() ) - id, name, err := backupGC.PopulateOwnerIDAndNamesFrom(backupSel.DiscreteOwner, nil) + id, name, err := backupGC.PopulateOwnerIDAndNamesFrom(ctx, backupSel.DiscreteOwner, nil) require.NoError(t, err, clues.ToCore(err)) backupSel.SetDiscreteOwnerIDName(id, name) diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index a88641ae1..202a83929 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -123,7 +123,7 @@ func prepNewTestBackupOp( t.FailNow() } - id, name, err := gc.PopulateOwnerIDAndNamesFrom(sel.DiscreteOwner, nil) + id, name, err := gc.PopulateOwnerIDAndNamesFrom(ctx, sel.DiscreteOwner, nil) require.NoError(t, err, clues.ToCore(err)) sel.SetDiscreteOwnerIDName(id, name) diff --git a/src/internal/operations/restore_test.go b/src/internal/operations/restore_test.go index 649bbe140..35ee6d8c6 100644 --- a/src/internal/operations/restore_test.go +++ b/src/internal/operations/restore_test.go @@ -278,7 +278,7 @@ func setupExchangeBackup( fault.New(true)) require.NoError(t, err, clues.ToCore(err)) - id, name, err := gc.PopulateOwnerIDAndNamesFrom(owner, nil) + id, name, err := gc.PopulateOwnerIDAndNamesFrom(ctx, owner, nil) require.NoError(t, err, clues.ToCore(err)) bsel.DiscreteOwner = owner @@ -340,7 +340,7 @@ func setupSharePointBackup( fault.New(true)) require.NoError(t, err, clues.ToCore(err)) - id, name, err := gc.PopulateOwnerIDAndNamesFrom(owner, nil) + id, name, err := gc.PopulateOwnerIDAndNamesFrom(ctx, owner, nil) require.NoError(t, err, clues.ToCore(err)) spsel.DiscreteOwner = owner diff --git a/src/internal/tester/config.go b/src/internal/tester/config.go index 615f4018a..ec08c239a 100644 --- a/src/internal/tester/config.go +++ b/src/internal/tester/config.go @@ -24,6 +24,7 @@ const ( // M365 config TestCfgAzureTenantID = "azure_tenantid" TestCfgSiteID = "m365siteid" + TestCfgSiteURL = "m365siteurl" TestCfgUserID = "m365userid" TestCfgSecondaryUserID = "secondarym365userid" TestCfgLoadTestUserID = "loadtestm365userid" @@ -34,6 +35,7 @@ const ( // test specific env vars const ( EnvCorsoM365TestSiteID = "CORSO_M365_TEST_SITE_ID" + EnvCorsoM365TestSiteURL = "CORSO_M365_TEST_SITE_URL" EnvCorsoM365TestUserID = "CORSO_M365_TEST_USER_ID" EnvCorsoSecondaryM365TestUserID = "CORSO_SECONDARY_M365_TEST_USER_ID" EnvCorsoM365LoadTestUserID = "CORSO_M365_LOAD_TEST_USER_ID" @@ -136,6 +138,12 @@ func readTestConfig() (map[string]string, error) { os.Getenv(EnvCorsoM365TestSiteID), vpr.GetString(TestCfgSiteID), "10rqc2.sharepoint.com,4892edf5-2ebf-46be-a6e5-a40b2cbf1c1a,38ab6d06-fc82-4417-af93-22d8733c22be") + fallbackTo( + testEnv, + TestCfgSiteURL, + os.Getenv(EnvCorsoM365TestSiteURL), + vpr.GetString(TestCfgSiteURL), + "https://10rqc2.sharepoint.com/sites/CorsoCI") testEnv[EnvCorsoTestConfigFilePath] = os.Getenv(EnvCorsoTestConfigFilePath) testConfig = testEnv diff --git a/src/internal/tester/resource_owners.go b/src/internal/tester/resource_owners.go index b5a1625a0..85c8ae5b7 100644 --- a/src/internal/tester/resource_owners.go +++ b/src/internal/tester/resource_owners.go @@ -81,7 +81,6 @@ func LoadTestM365SiteID(t *testing.T) string { cfg, err := readTestConfig() require.NoError(t, err, "retrieving load test m365 site id from test configuration", clues.ToCore(err)) - // TODO: load test site id, not standard test site id return cfg[TestCfgSiteID] } @@ -162,3 +161,14 @@ func M365SiteID(t *testing.T) string { return cfg[TestCfgSiteID] } + +// M365SiteURL returns a site webURL string representing the m365SiteURL described +// by either the env var CORSO_M365_TEST_SITE_URL, the corso_test.toml config +// file or the default value (in that order of priority). The default is a +// last-attempt fallback that will only work on alcion's testing org. +func M365SiteURL(t *testing.T) string { + cfg, err := readTestConfig() + require.NoError(t, err, "retrieving m365 site url from test configuration", clues.ToCore(err)) + + return cfg[TestCfgSiteURL] +} diff --git a/src/pkg/repository/repository.go b/src/pkg/repository/repository.go index a2bb2443f..2e787a0b2 100644 --- a/src/pkg/repository/repository.go +++ b/src/pkg/repository/repository.go @@ -314,7 +314,7 @@ func (r repository) NewBackupWithLookup( return operations.BackupOperation{}, errors.Wrap(err, "connecting to m365") } - ownerID, ownerName, err := gc.PopulateOwnerIDAndNamesFrom(sel.DiscreteOwner, ins) + ownerID, ownerName, err := gc.PopulateOwnerIDAndNamesFrom(ctx, sel.DiscreteOwner, ins) if err != nil { return operations.BackupOperation{}, errors.Wrap(err, "resolving resource owner details") } diff --git a/src/pkg/repository/repository_test.go b/src/pkg/repository/repository_test.go index cdea26d27..3d6c9979f 100644 --- a/src/pkg/repository/repository_test.go +++ b/src/pkg/repository/repository_test.go @@ -198,7 +198,9 @@ func (suite *RepositoryIntegrationSuite) TestNewBackup() { r, err := repository.Initialize(ctx, acct, st, control.Options{}) require.NoError(t, err, clues.ToCore(err)) - bo, err := r.NewBackup(ctx, selectors.Selector{DiscreteOwner: "test"}) + userID := tester.M365UserID(t) + + bo, err := r.NewBackup(ctx, selectors.Selector{DiscreteOwner: userID}) require.NoError(t, err, clues.ToCore(err)) require.NotNil(t, bo) }