From 9bb97f0ebd6f9da384c763e8ce1163c3157ad70f Mon Sep 17 00:00:00 2001 From: ryanfkeepers Date: Tue, 28 Mar 2023 15:07:20 -0600 Subject: [PATCH] look up users and sites by id or name 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. --- src/internal/connector/data_collections.go | 2 +- src/internal/connector/discovery/api/sites.go | 60 +++++- .../connector/discovery/api/sites_test.go | 58 ++++++ src/internal/connector/discovery/api/users.go | 12 ++ src/internal/connector/graph_connector.go | 179 ++++++++++++++---- .../connector/graph_connector_test.go | 80 ++++++-- src/internal/tester/config.go | 8 + src/internal/tester/resource_owners.go | 12 +- src/pkg/repository/repository.go | 6 +- 9 files changed, 354 insertions(+), 63 deletions(-) diff --git a/src/internal/connector/data_collections.go b/src/internal/connector/data_collections.go index 0eeb428ba..59d53546b 100644 --- a/src/internal/connector/data_collections.go +++ b/src/internal/connector/data_collections.go @@ -55,7 +55,7 @@ func (gc *GraphConnector) ProduceBackupCollections( serviceEnabled, err := checkServiceEnabled( ctx, - gc.Owners.Users(), + gc.Discovery.Users(), path.ServiceType(sels.Service), sels.DiscreteOwner) if err != nil { diff --git a/src/internal/connector/discovery/api/sites.go b/src/internal/connector/discovery/api/sites.go index 2365b41cb..12b181651 100644 --- a/src/internal/connector/discovery/api/sites.go +++ b/src/internal/connector/discovery/api/sites.go @@ -3,6 +3,7 @@ package api import ( "context" "fmt" + "regexp" "strings" "github.com/alcionai/clues" @@ -12,6 +13,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" ) @@ -88,15 +90,67 @@ func (c Sites) GetAll(ctx context.Context, errs *fault.Bus) ([]models.Siteable, return us, el.Failure() } +const uuidRE = "[0-9a-fA-F]{8}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[0-9a-fA-F]{4}\b-[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 ID. The ID 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, id string) (models.Siteable, error) { - resp, err := c.stable.Client().SitesById(id).Get(ctx, nil) - if err != nil { - return nil, graph.Wrap(ctx, err, "getting site") + var ( + resp models.Siteable + err error + ) + + ctx = clues.Add(ctx, "given_site_id", id) + + if siteIDRE.MatchString(id) { + resp, err = c.stable.Client().SitesById(id).Get(ctx, nil) + if err != nil { + return nil, graph.Wrap(ctx, err, "getting site by id") + } + } else { + var ( + url = strings.TrimPrefix(id, "https://") + parts = strings.SplitN(url, "/", 1) + host = parts[0] + path string + ) + + if len(parts) > 1 { + path = parts[1] + } + + rawURL := fmt.Sprintf(webURLGetTemplate, 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 // --------------------------------------------------------------------------- diff --git a/src/internal/connector/discovery/api/sites_test.go b/src/internal/connector/discovery/api/sites_test.go index 757b85827..1e7a5a31d 100644 --- a/src/internal/connector/discovery/api/sites_test.go +++ b/src/internal/connector/discovery/api/sites_test.go @@ -1,11 +1,14 @@ 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" "github.com/stretchr/testify/suite" "github.com/alcionai/corso/src/internal/common/ptr" @@ -106,3 +109,58 @@ func (suite *SitesUnitSuite) TestValidateSite() { }) } } + +type SitesIntgSuite struct { + tester.Suite +} + +func TestSitesIntgSuite(t *testing.T) { + suite.Run(t, &SitesIntgSuite{ + Suite: tester.NewIntegrationSuite(t, [][]string{tester.M365AcctCredEnvs}), + }) +} + +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 f4e28aa96..fe35fbb68 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" @@ -150,6 +151,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/graph_connector.go b/src/internal/connector/graph_connector.go index 7eab83c6f..5277a3f7b 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -28,12 +28,13 @@ import ( // 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. @@ -49,15 +50,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, @@ -65,31 +57,43 @@ 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, - } - - 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{ + itemClient: itemClient, + Discovery: discovery, + tenant: acct.ID(), + wg: &sync.WaitGroup{}, + credentials: creds, + ownerLookup: rc, + Service: service, + } + return &gc, nil } +// --------------------------------------------------------------------------- +// Owner Lookup +// --------------------------------------------------------------------------- + // 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. @@ -105,6 +109,7 @@ func NewGraphConnector( // 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( + ctx context.Context, owner string, // input value, can be either id or name idToName, nameToID map[string]string, // optionally pre-populated lookups ) (string, string, error) { @@ -118,8 +123,7 @@ func (gc *GraphConnector) PopulateOwnerIDAndNamesFrom( nameToID = map[string]string{} } - // move this to GC method - id, name, err := getOwnerIDAndNameFrom(owner, idToName, nameToID) + id, name, err := gc.ownerLookup.getOwnerIDAndNameFrom(ctx, gc.Discovery, owner, idToName, nameToID) if err != nil { return "", "", errors.Wrap(err, "resolving resource owner details") } @@ -130,29 +134,16 @@ func (gc *GraphConnector) PopulateOwnerIDAndNamesFrom( return id, name, nil } -func getOwnerIDAndNameFrom( - owner string, - idToName, nameToID map[string]string, -) (string, string, error) { - if n, ok := idToName[owner]; ok { - return owner, n, nil - } else if i, ok := nameToID[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 owner, owner, nil -} +// --------------------------------------------------------------------------- +// 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 } @@ -160,6 +151,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() { @@ -215,3 +210,103 @@ func (gc *GraphConnector) incrementAwaitingMessages() { func (gc *GraphConnector) incrementMessagesBy(num int) { gc.wg.Add(num) } + +// --------------------------------------------------------------------------- +// Resource 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, + idToName, nameToID map[string]string, + ) ( + ownerID string, + ownerName string, + err error, + ) +} + +var ErrResourceOwnerNotFound = clues.New("resource owner not found in tenant") + +// getOwnerIDAndNameFrom looks up the owner's canonical id and display name. +// if idToName and nameToID are populated, and the owner is a key of one of +// those maps, then those 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). +// If the fallback lookup is used, the maps are populated to contain the +// id and name references. +func (r resourceClient) getOwnerIDAndNameFrom( + ctx context.Context, + discovery api.Client, + owner string, + idToName, nameToID map[string]string, +) (string, string, error) { + if n, ok := idToName[owner]; ok { + return owner, n, nil + } else if i, ok := nameToID[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 { + return "", "", err + } + + if len(id) == 0 || len(name) == 0 { + return "", "", clues.Stack(ErrResourceOwnerNotFound) + } + + idToName[id] = name + nameToID[name] = id + + return id, name, nil +} diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 1d9ef1098..be298d370 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -38,6 +38,19 @@ 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" @@ -45,8 +58,13 @@ func (suite *GraphConnectorUnitSuite) TestPopulateOwnerIDAndNamesFrom() { ) var ( - itn = map[string]string{ownerID: ownerName} - nti = map[string]string{ownerName: ownerID} + itn = map[string]string{ownerID: ownerName} + nti = map[string]string{ownerName: ownerID} + lookup = &resourceClient{ + enum: Users, + getter: &mockNameIDGetter{id: ownerID, name: ownerName}, + } + noLookup = &resourceClient{enum: Users, getter: &mockNameIDGetter{}} ) table := []struct { @@ -54,93 +72,125 @@ func (suite *GraphConnectorUnitSuite) TestPopulateOwnerIDAndNamesFrom() { owner string idToName map[string]string nameToID map[string]string + rc *resourceClient expectID string expectName string + expectErr assert.ErrorAssertionFunc }{ { - name: "nil maps", + name: "nil maps, getter lookup", owner: ownerID, + rc: lookup, idToName: nil, nameToID: nil, expectID: ownerID, - expectName: ownerID, + expectName: ownerName, + expectErr: assert.NoError, }, { name: "only id map with owner id", owner: ownerID, + rc: noLookup, idToName: itn, nameToID: nil, expectID: ownerID, expectName: ownerName, + expectErr: assert.NoError, }, { name: "only name map with owner id", owner: ownerID, + rc: lookup, idToName: nil, nameToID: nti, expectID: ownerID, - expectName: ownerID, + expectName: ownerName, + expectErr: assert.NoError, }, { name: "only id map with owner name", owner: ownerName, + rc: lookup, idToName: itn, nameToID: nil, - expectID: ownerName, + expectID: ownerID, expectName: ownerName, + expectErr: assert.NoError, }, { name: "only name map with owner name", owner: ownerName, + rc: lookup, idToName: nil, nameToID: nti, expectID: ownerID, expectName: ownerName, + expectErr: assert.NoError, }, { name: "both maps with owner id", owner: ownerID, + rc: noLookup, idToName: itn, nameToID: nti, expectID: ownerID, expectName: ownerName, + expectErr: assert.NoError, }, { name: "both maps with owner name", owner: ownerName, + rc: noLookup, idToName: itn, nameToID: nti, expectID: ownerID, expectName: ownerName, + expectErr: assert.NoError, }, { name: "non-matching maps with owner id", owner: ownerID, + rc: noLookup, idToName: map[string]string{"foo": "bar"}, nameToID: map[string]string{"fnords": "smarf"}, - expectID: ownerID, - expectName: ownerID, + expectID: "", + expectName: "", + expectErr: assert.Error, }, { name: "non-matching with owner name", owner: ownerName, + rc: noLookup, idToName: map[string]string{"foo": "bar"}, nameToID: map[string]string{"fnords": "smarf"}, - expectID: ownerName, - expectName: ownerName, + expectID: "", + expectName: "", + expectErr: assert.Error, }, } 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.idToName, test.nameToID) - require.NoError(t, err, clues.ToCore(err)) - assert.Equal(t, test.expectID, id) - assert.Equal(t, test.expectName, name) + id, name, err := gc.PopulateOwnerIDAndNamesFrom( + ctx, + test.owner, + test.idToName, + test.nameToID) + test.expectErr(t, err, clues.ToCore(err)) + + if err != nil { + return + } + + assert.Equal(t, test.expectID, id, "id") + assert.Equal(t, test.expectName, name, "name") }) } } 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 c36386b96..9e861c4b8 100644 --- a/src/internal/tester/resource_owners.go +++ b/src/internal/tester/resource_owners.go @@ -52,7 +52,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] } @@ -133,3 +132,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 fa0c21aa8..f5ef5d2a0 100644 --- a/src/pkg/repository/repository.go +++ b/src/pkg/repository/repository.go @@ -296,7 +296,11 @@ func (r repository) NewBackup( return operations.BackupOperation{}, errors.Wrap(err, "connecting to m365") } - ownerID, ownerName, err := gc.PopulateOwnerIDAndNamesFrom(sel.DiscreteOwner, ownerIDToName, ownerNameToID) + ownerID, ownerName, err := gc.PopulateOwnerIDAndNamesFrom( + ctx, + sel.DiscreteOwner, + ownerIDToName, + ownerNameToID) if err != nil { return operations.BackupOperation{}, errors.Wrap(err, "resolving resource owner details") }