diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ba6122fd..708cacb13 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Resolved an issue where progress bar displays could fail to exit, causing unbounded CPU consumption. - Fix Corso panic within Docker images - Debugging with the CORSO_URL_LOGGING env variable no longer causes accidental request failures. +- Don't discover all users when backing up each user in a multi-user backup ### Changed - When using Restore and Details on Exchange Calendars, the `--event-calendar` flag can now identify calendars by either a Display Name or a Microsoft 365 ID. diff --git a/src/cmd/factory/impl/common.go b/src/cmd/factory/impl/common.go index a347d94db..6c5f502b8 100644 --- a/src/cmd/factory/impl/common.go +++ b/src/cmd/factory/impl/common.go @@ -6,6 +6,7 @@ import ( "strings" "time" + "github.com/alcionai/clues" "github.com/google/uuid" "github.com/pkg/errors" @@ -23,6 +24,7 @@ import ( "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/selectors" + "github.com/alcionai/corso/src/pkg/services/m365" ) var ( @@ -118,15 +120,14 @@ func getGCAndVerifyUser(ctx context.Context, userID string) (*connector.GraphCon // build a graph connector // TODO: log/print recoverable errors errs := fault.New(false) - - gc, err := connector.NewGraphConnector(ctx, graph.HTTPClient(graph.NoTimeout()), acct, connector.Users, errs) - if err != nil { - return nil, account.Account{}, errors.Wrap(err, "connecting to graph api") - } - normUsers := map[string]struct{}{} - for k := range gc.Users { + users, err := m365.UserPNs(ctx, acct, errs) + if err != nil { + return nil, account.Account{}, clues.Wrap(err, "getting tenant users") + } + + for _, k := range users { normUsers[strings.ToLower(k)] = struct{}{} } @@ -134,6 +135,16 @@ func getGCAndVerifyUser(ctx context.Context, userID string) (*connector.GraphCon return nil, account.Account{}, errors.New("user not found within tenant") } + gc, err := connector.NewGraphConnector( + ctx, + graph.HTTPClient(graph.NoTimeout()), + acct, + connector.Users, + errs) + if err != nil { + return nil, account.Account{}, errors.Wrap(err, "connecting to graph api") + } + return gc, acct, nil } diff --git a/src/cmd/purge/purge.go b/src/cmd/purge/purge.go index a57bea188..3de643aa5 100644 --- a/src/cmd/purge/purge.go +++ b/src/cmd/purge/purge.go @@ -5,6 +5,7 @@ import ( "os" "time" + "github.com/alcionai/clues" "github.com/hashicorp/go-multierror" "github.com/pkg/errors" "github.com/spf13/cobra" @@ -19,6 +20,7 @@ import ( "github.com/alcionai/corso/src/pkg/credentials" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/logger" + "github.com/alcionai/corso/src/pkg/services/m365" ) var purgeCmd = &cobra.Command{ @@ -75,13 +77,16 @@ func handleAllFolderPurge(cmd *cobra.Command, args []string) error { return nil } - gc, t, err := getGCAndBoundaryTime(ctx) + acct, gc, t, err := getGCAndBoundaryTime(ctx) if err != nil { return err } err = runPurgeForEachUser( - ctx, gc, t, + ctx, + acct, + gc, + t, purgeOneDriveFolders, ) if err != nil { @@ -98,12 +103,12 @@ func handleOneDriveFolderPurge(cmd *cobra.Command, args []string) error { return nil } - gc, t, err := getGCAndBoundaryTime(ctx) + acct, gc, t, err := getGCAndBoundaryTime(ctx) if err != nil { return err } - if err := runPurgeForEachUser(ctx, gc, t, purgeOneDriveFolders); err != nil { + if err := runPurgeForEachUser(ctx, acct, gc, t, purgeOneDriveFolders); err != nil { logger.Ctx(ctx).Error(err) return Only(ctx, errors.Wrap(ErrPurging, "OneDrive folders")) } @@ -124,17 +129,30 @@ type purger func(context.Context, *connector.GraphConnector, time.Time, string) func runPurgeForEachUser( ctx context.Context, + acct account.Account, gc *connector.GraphConnector, boundary time.Time, ps ...purger, ) error { - var errs error + var ( + errs error + ferrs = fault.New(false) + ) - for pn, uid := range userOrUsers(user, gc.Users) { - Infof(ctx, "\nUser: %s - %s", pn, uid) + users, err := m365.Users(ctx, acct, ferrs) + if err != nil { + return clues.Wrap(err, "getting users") + } + + if len(ferrs.Errors().Recovered) > 0 { + errs = multierror.Append(errs, ferrs.Errors().Recovered...) + } + + for _, u := range userOrUsers(user, users) { + Infof(ctx, "\nUser: %s - %s", u.PrincipalName, u.ID) for _, p := range ps { - if err := p(ctx, gc, boundary, pn); err != nil { + if err := p(ctx, gc, boundary, u.PrincipalName); err != nil { errs = multierror.Append(errs, err) } } @@ -248,7 +266,7 @@ func purgeFolders( // Helpers // ------------------------------------------------------------------------------------------ -func getGC(ctx context.Context) (*connector.GraphConnector, error) { +func getGC(ctx context.Context) (account.Account, *connector.GraphConnector, error) { // get account info m365Cfg := account.M365Config{ M365: credentials.GetM365(), @@ -257,7 +275,7 @@ func getGC(ctx context.Context) (*connector.GraphConnector, error) { acct, err := account.NewAccount(account.ProviderM365, m365Cfg) if err != nil { - return nil, Only(ctx, errors.Wrap(err, "finding m365 account details")) + return account.Account{}, nil, Only(ctx, errors.Wrap(err, "finding m365 account details")) } // build a graph connector @@ -266,10 +284,10 @@ func getGC(ctx context.Context) (*connector.GraphConnector, error) { gc, err := connector.NewGraphConnector(ctx, graph.HTTPClient(graph.NoTimeout()), acct, connector.Users, errs) if err != nil { - return nil, Only(ctx, errors.Wrap(err, "connecting to graph api")) + return account.Account{}, nil, Only(ctx, errors.Wrap(err, "connecting to graph api")) } - return gc, nil + return acct, gc, nil } func getBoundaryTime(ctx context.Context) (time.Time, error) { @@ -289,21 +307,23 @@ func getBoundaryTime(ctx context.Context) (time.Time, error) { return boundaryTime, nil } -func getGCAndBoundaryTime(ctx context.Context) (*connector.GraphConnector, time.Time, error) { - gc, err := getGC(ctx) +func getGCAndBoundaryTime( + ctx context.Context, +) (account.Account, *connector.GraphConnector, time.Time, error) { + acct, gc, err := getGC(ctx) if err != nil { - return nil, time.Time{}, err + return account.Account{}, nil, time.Time{}, err } t, err := getBoundaryTime(ctx) if err != nil { - return nil, time.Time{}, err + return account.Account{}, nil, time.Time{}, err } - return gc, t, nil + return acct, gc, t, nil } -func userOrUsers(u string, us map[string]string) map[string]string { +func userOrUsers(u string, us []*m365.User) []*m365.User { if len(u) == 0 { return nil } @@ -312,5 +332,5 @@ func userOrUsers(u string, us map[string]string) map[string]string { return us } - return map[string]string{u: u} + return []*m365.User{{PrincipalName: u}} } diff --git a/src/internal/connector/discovery/discovery.go b/src/internal/connector/discovery/discovery.go index ea71eb2bb..1ecf85aa5 100644 --- a/src/internal/connector/discovery/discovery.go +++ b/src/internal/connector/discovery/discovery.go @@ -4,11 +4,13 @@ import ( "context" "fmt" + "github.com/alcionai/clues" "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/account" "github.com/alcionai/corso/src/pkg/fault" ) @@ -16,10 +18,6 @@ import ( // interfaces // --------------------------------------------------------------------------- -type getAller interface { - GetAll(context.Context, *fault.Bus) ([]models.Userable, error) -} - type getter interface { GetByID(context.Context, string) (models.Userable, error) } @@ -38,8 +36,22 @@ type getWithInfoer interface { // --------------------------------------------------------------------------- // Users fetches all users in the tenant. -func Users(ctx context.Context, ga getAller, errs *fault.Bus) ([]models.Userable, error) { - return ga.GetAll(ctx, errs) +func Users( + ctx context.Context, + acct account.Account, + errs *fault.Bus, +) ([]models.Userable, error) { + m365, err := acct.M365Config() + if err != nil { + return nil, clues.Wrap(err, "retrieving m365 account configuration").WithClues(ctx) + } + + client, err := api.NewClient(m365) + if err != nil { + return nil, clues.Wrap(err, "creating api client").WithClues(ctx) + } + + return client.Users().GetAll(ctx, errs) } func User(ctx context.Context, gwi getWithInfoer, userID string) (models.Userable, *api.UserInfo, error) { diff --git a/src/internal/connector/discovery/discovery_test.go b/src/internal/connector/discovery/discovery_test.go new file mode 100644 index 000000000..fc8f3832e --- /dev/null +++ b/src/internal/connector/discovery/discovery_test.go @@ -0,0 +1,98 @@ +package discovery_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" + + "github.com/alcionai/corso/src/internal/connector/discovery" + "github.com/alcionai/corso/src/internal/tester" + "github.com/alcionai/corso/src/pkg/account" + "github.com/alcionai/corso/src/pkg/credentials" + "github.com/alcionai/corso/src/pkg/fault" +) + +type DiscoveryIntegrationSuite struct { + tester.Suite +} + +func TestDiscoveryIntegrationSuite(t *testing.T) { + suite.Run(t, &DiscoveryIntegrationSuite{Suite: tester.NewIntegrationSuite( + t, + [][]string{tester.M365AcctCredEnvs}, + tester.CorsoGraphConnectorTests, + )}) +} + +func (suite *DiscoveryIntegrationSuite) TestUsers() { + ctx, flush := tester.NewContext() + defer flush() + + t := suite.T() + + acct := tester.NewM365Account(t) + errs := fault.New(true) + + users, err := discovery.Users(ctx, acct, errs) + assert.NoError(t, err) + + ferrs := errs.Errors() + assert.NoError(t, ferrs.Failure) + assert.Empty(t, ferrs.Recovered) + + assert.Less(t, 0, len(users)) +} + +func (suite *DiscoveryIntegrationSuite) TestUsers_InvalidCredentials() { + ctx, flush := tester.NewContext() + defer flush() + + table := []struct { + name string + acct func(t *testing.T) account.Account + }{ + { + name: "Invalid Credentials", + acct: func(t *testing.T) account.Account { + a, err := account.NewAccount( + account.ProviderM365, + account.M365Config{ + M365: credentials.M365{ + AzureClientID: "Test", + AzureClientSecret: "without", + }, + AzureTenantID: "data", + }, + ) + require.NoError(t, err) + + return a + }, + }, + { + name: "Empty Credentials", + acct: func(t *testing.T) account.Account { + // intentionally swallowing the error here + a, _ := account.NewAccount(account.ProviderM365) + return a + }, + }, + } + + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + a := test.acct(t) + errs := fault.New(true) + users, err := discovery.Users(ctx, a, errs) + + assert.Empty(t, users, "returned some users") + assert.NotNil(t, err) + // TODO(ashmrtn): Uncomment when fault package is used in discovery API. + // assert.NotNil(t, errs.Err()) + }) + } +} diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index b52813568..410e4b858 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -17,7 +17,6 @@ import ( "github.com/pkg/errors" "golang.org/x/exp/maps" - "github.com/alcionai/corso/src/internal/connector/discovery" "github.com/alcionai/corso/src/internal/connector/discovery/api" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/sharepoint" @@ -41,7 +40,6 @@ type GraphConnector struct { itemClient *http.Client // configured to handle large item downloads tenant string - Users map[string]string // key value Sites map[string]string // key value credentials account.M365Config @@ -78,7 +76,6 @@ func NewGraphConnector( gc := GraphConnector{ itemClient: itemClient, tenant: m365.AzureTenantID, - Users: make(map[string]string, 0), wg: &sync.WaitGroup{}, credentials: m365, } @@ -93,16 +90,6 @@ func NewGraphConnector( return nil, clues.Wrap(err, "creating api client").WithClues(ctx) } - // TODO(ashmrtn): When selectors only encapsulate a single resource owner that - // is not a wildcard don't populate users or sites when making the connector. - // For now this keeps things functioning if callers do pass in a selector like - // "*" instead of. - if r == AllResources || r == Users { - if err = gc.setTenantUsers(ctx, errs); err != nil { - return nil, errors.Wrap(err, "retrieving tenant user list") - } - } - if r == AllResources || r == Sites { if err = gc.setTenantSites(ctx, errs); err != nil { return nil, errors.Wrap(err, "retrieveing tenant site list") @@ -126,27 +113,6 @@ func (gc *GraphConnector) createService() (*graph.Service, error) { return graph.NewService(adapter), nil } -// setTenantUsers queries the M365 to identify the users in the -// workspace. The users field is updated during this method -// iff the returned error is nil -func (gc *GraphConnector) setTenantUsers(ctx context.Context, errs *fault.Bus) error { - ctx, end := D.Span(ctx, "gc:setTenantUsers") - defer end() - - users, err := discovery.Users(ctx, gc.Owners.Users(), errs) - if err != nil { - return err - } - - gc.Users = make(map[string]string, len(users)) - - for _, u := range users { - gc.Users[*u.GetUserPrincipalName()] = *u.GetId() - } - - return nil -} - // 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 c8d85f777..8ac17003b 100644 --- a/src/internal/connector/graph_connector_disconnected_test.go +++ b/src/internal/connector/graph_connector_disconnected_test.go @@ -75,7 +75,7 @@ func (suite *DisconnectedGraphConnectorSuite) TestBadConnection() { ctx, graph.HTTPClient(graph.NoTimeout()), test.acct(t), - Users, + Sites, fault.New(true)) assert.Nil(t, gc, test.name+" failed") assert.NotNil(t, err, test.name+" failed") diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 88eaf89de..3eddb9238 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -11,7 +11,6 @@ import ( "github.com/stretchr/testify/suite" "golang.org/x/exp/maps" - "github.com/alcionai/corso/src/internal/connector/discovery/api" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/mockconnector" "github.com/alcionai/corso/src/internal/connector/support" @@ -168,33 +167,7 @@ func (suite *GraphConnectorIntegrationSuite) SetupSuite() { tester.LogTimeOfTest(suite.T()) } -// TestSetTenantUsers verifies GraphConnector's ability to query -// the users associated with the credentials -func (suite *GraphConnectorIntegrationSuite) TestSetTenantUsers() { - t := suite.T() - newConnector := GraphConnector{ - tenant: "test_tenant", - Users: make(map[string]string, 0), - credentials: suite.connector.credentials, - } - - ctx, flush := tester.NewContext() - defer flush() - - owners, err := api.NewClient(suite.connector.credentials) - require.NoError(t, err) - - newConnector.Owners = owners - assert.Empty(t, len(newConnector.Users)) - - errs := fault.New(true) - - err = newConnector.setTenantUsers(ctx, errs) - assert.NoError(t, err) - assert.Less(t, 0, len(newConnector.Users)) -} - -// TestSetTenantUsers verifies GraphConnector's ability to query +// TestSetTenantSites verifies GraphConnector's ability to query // the sites associated with the credentials func (suite *GraphConnectorIntegrationSuite) TestSetTenantSites() { newConnector := GraphConnector{ diff --git a/src/pkg/services/m365/m365.go b/src/pkg/services/m365/m365.go index c05f2e4d7..948650ef6 100644 --- a/src/pkg/services/m365/m365.go +++ b/src/pkg/services/m365/m365.go @@ -37,12 +37,7 @@ func UsersCompat(ctx context.Context, acct account.Account) ([]*User, error) { // Users returns a list of users in the specified M365 tenant // TODO: Implement paging support func Users(ctx context.Context, acct account.Account, errs *fault.Bus) ([]*User, error) { - gc, err := connector.NewGraphConnector(ctx, graph.HTTPClient(graph.NoTimeout()), acct, connector.Users, errs) - if err != nil { - return nil, errors.Wrap(err, "initializing M365 graph connection") - } - - users, err := discovery.Users(ctx, gc.Owners.Users(), errs) + users, err := discovery.Users(ctx, acct, errs) if err != nil { return nil, err }