From 59bf3506039f03e7ff214478c703df5141083819 Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Wed, 6 Sep 2023 22:45:40 +0530 Subject: [PATCH] Add service isolation (#4096) 1. Renamed `IsBackupRunnable` to `IsServiceEnabled` & extended to restore operation. Removed `checkServiceEnabled`. Reasons: - The above 2 functions were doing the same thing. Removed `checkServiceEnabled` in favor of `IsBackupRunnable` since we want this check to be as soon as possible during backup/restore op initialization - Renamed `IsBackupRunnable` to `IsServiceEnabled`, because a) we are only doing service enabled checks right now, b) common code that can be used for both restore & backup. 2. Wire corso code to use new helpers in internal/m365/common.go 3. Note: SDK wiring and related integ tests will be added in a follow up PR --- #### 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: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) * https://github.com/alcionai/corso/issues/3844 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/cli/backup/backup.go | 4 +- src/internal/m365/backup.go | 108 +++++-------------- src/internal/m365/controller_test.go | 40 ------- src/internal/m365/mock/connector.go | 2 +- src/internal/m365/restore.go | 14 +-- src/internal/m365/service/exchange/backup.go | 14 +++ src/internal/m365/service/groups/enabled.go | 21 ++++ src/internal/operations/backup.go | 26 ++--- src/internal/operations/backup_test.go | 2 +- src/internal/operations/inject/inject.go | 15 ++- src/internal/operations/restore.go | 16 +++ src/pkg/services/m365/api/user_info.go | 18 ---- src/pkg/services/m365/api/users.go | 1 + src/pkg/services/m365/users.go | 2 + 14 files changed, 113 insertions(+), 170 deletions(-) create mode 100644 src/internal/m365/service/groups/enabled.go diff --git a/src/cli/backup/backup.go b/src/cli/backup/backup.go index 432e5798e..6727abcd7 100644 --- a/src/cli/backup/backup.go +++ b/src/cli/backup/backup.go @@ -229,7 +229,9 @@ func runBackups( err = bo.Run(ictx) if err != nil { if errors.Is(err, graph.ErrServiceNotEnabled) { - logger.Ctx(ctx).Infow("service not enabled", "resource_owner_name", bo.ResourceOwner.Name()) + logger.Ctx(ctx).Infow("service not enabled", + "resource_owner_id", bo.ResourceOwner.ID(), + "service", serviceName) continue } diff --git a/src/internal/m365/backup.go b/src/internal/m365/backup.go index 55c4c7fdb..fab51f0da 100644 --- a/src/internal/m365/backup.go +++ b/src/internal/m365/backup.go @@ -19,7 +19,6 @@ import ( "github.com/alcionai/corso/src/pkg/logger" "github.com/alcionai/corso/src/pkg/path" "github.com/alcionai/corso/src/pkg/selectors" - "github.com/alcionai/corso/src/pkg/services/m365/api" ) // --------------------------------------------------------------------------- @@ -55,25 +54,26 @@ func (ctrl *Controller) ProduceBackupCollections( return nil, nil, false, clues.Stack(err).WithClues(ctx) } - serviceEnabled, canMakeDeltaQueries, err := checkServiceEnabled( - ctx, - ctrl.AC.Users(), - service, - bpc.ProtectedResource.ID()) - if err != nil { - return nil, nil, false, err - } - - if !serviceEnabled { - return []data.BackupCollection{}, nil, false, nil - } - var ( colls []data.BackupCollection ssmb *prefixmatcher.StringSetMatcher canUsePreviousBackup bool ) + // All services except Exchange can make delta queries by default. + // Exchange can only make delta queries if the mailbox is not over quota. + canMakeDeltaQueries := true + if service == path.ExchangeService { + canMakeDeltaQueries, err = exchange.CanMakeDeltaQueries( + ctx, + service, + ctrl.AC.Users(), + bpc.ProtectedResource.ID()) + if err != nil { + return nil, nil, false, clues.Stack(err) + } + } + if !canMakeDeltaQueries { logger.Ctx(ctx).Info("delta requests not available") @@ -148,48 +148,23 @@ func (ctrl *Controller) ProduceBackupCollections( return colls, ssmb, canUsePreviousBackup, nil } -// IsBackupRunnable verifies that the users provided has the services enabled and -// data can be backed up. The canMakeDeltaQueries provides info if the mailbox is -// full and delta queries can be made on it. -func (ctrl *Controller) IsBackupRunnable( +func (ctrl *Controller) IsServiceEnabled( ctx context.Context, service path.ServiceType, resourceOwner string, ) (bool, error) { - if service == path.GroupsService { - _, err := ctrl.AC.Groups().GetByID(ctx, resourceOwner) - if err != nil { - // TODO(meain): check for error message in case groups are - // not enabled at all similar to sharepoint - return false, err - } - - return true, nil + switch service { + case path.ExchangeService: + return exchange.IsServiceEnabled(ctx, ctrl.AC.Users(), resourceOwner) + case path.OneDriveService: + return onedrive.IsServiceEnabled(ctx, ctrl.AC.Users(), resourceOwner) + case path.SharePointService: + return sharepoint.IsServiceEnabled(ctx, ctrl.AC.Users().Sites(), resourceOwner) + case path.GroupsService: + return groups.IsServiceEnabled(ctx, ctrl.AC.Groups(), resourceOwner) } - if service == path.SharePointService { - _, err := ctrl.AC.Sites().GetRoot(ctx) - if err != nil { - if clues.HasLabel(err, graph.LabelsNoSharePointLicense) { - return false, clues.Stack(graph.ErrServiceNotEnabled, err) - } - - return false, err - } - - return true, nil - } - - info, err := ctrl.AC.Users().GetInfo(ctx, resourceOwner) - if err != nil { - return false, clues.Stack(err) - } - - if !info.ServiceEnabled(service) { - return false, clues.Wrap(graph.ErrServiceNotEnabled, "checking service access") - } - - return true, nil + return false, clues.Wrap(clues.New(service.String()), "service not supported").WithClues(ctx) } func verifyBackupInputs(sels selectors.Selector, cachedIDs []string) error { @@ -211,36 +186,3 @@ func verifyBackupInputs(sels selectors.Selector, cachedIDs []string) error { return nil } - -type getInfoer interface { - GetInfo(context.Context, string) (*api.UserInfo, error) -} - -func checkServiceEnabled( - ctx context.Context, - gi getInfoer, - service path.ServiceType, - resource string, -) (bool, bool, error) { - if service == path.SharePointService || service == path.GroupsService { - // No "enabled" check required for sharepoint or groups. - return true, true, nil - } - - info, err := gi.GetInfo(ctx, resource) - if err != nil { - return false, false, clues.Stack(err) - } - - if !info.ServiceEnabled(service) { - return false, false, clues.Wrap(graph.ErrServiceNotEnabled, "checking service access") - } - - canMakeDeltaQueries := true - if service == path.ExchangeService { - // we currently can only check quota exceeded for exchange - canMakeDeltaQueries = info.CanMakeDeltaQueries() - } - - return true, canMakeDeltaQueries, nil -} diff --git a/src/internal/m365/controller_test.go b/src/internal/m365/controller_test.go index ec2c8c72c..cf290e320 100644 --- a/src/internal/m365/controller_test.go +++ b/src/internal/m365/controller_test.go @@ -16,8 +16,6 @@ import ( "github.com/alcionai/corso/src/internal/common/idname" inMock "github.com/alcionai/corso/src/internal/common/idname/mock" "github.com/alcionai/corso/src/internal/data" - dataMock "github.com/alcionai/corso/src/internal/data/mock" - "github.com/alcionai/corso/src/internal/m365/graph" "github.com/alcionai/corso/src/internal/m365/mock" "github.com/alcionai/corso/src/internal/m365/resource" exchMock "github.com/alcionai/corso/src/internal/m365/service/exchange/mock" @@ -375,44 +373,6 @@ func (suite *ControllerIntegrationSuite) SetupSuite() { tester.LogTimeOfTest(t) } -func (suite *ControllerIntegrationSuite) TestRestoreFailsBadService() { - t := suite.T() - - ctx, flush := tester.NewContext(t) - defer flush() - - var ( - restoreCfg = testdata.DefaultRestoreConfig("") - sel = selectors.Selector{ - Service: selectors.ServiceUnknown, - } - ) - - restoreCfg.IncludePermissions = true - - rcc := inject.RestoreConsumerConfig{ - BackupVersion: version.Backup, - Options: control.DefaultOptions(), - ProtectedResource: sel, - RestoreConfig: restoreCfg, - Selector: sel, - } - - deets, err := suite.ctrl.ConsumeRestoreCollections( - ctx, - rcc, - []data.RestoreCollection{&dataMock.Collection{}}, - fault.New(true), - count.New()) - assert.Error(t, err, graph.ErrServiceNotEnabled, clues.ToCore(err)) - assert.Nil(t, deets) - - status := suite.ctrl.Wait() - assert.Equal(t, 0, status.Objects) - assert.Equal(t, 0, status.Folders) - assert.Equal(t, 0, status.Successes) -} - func (suite *ControllerIntegrationSuite) TestEmptyCollections() { restoreCfg := testdata.DefaultRestoreConfig("") restoreCfg.IncludePermissions = true diff --git a/src/internal/m365/mock/connector.go b/src/internal/m365/mock/connector.go index 0665053a4..8d373a223 100644 --- a/src/internal/m365/mock/connector.go +++ b/src/internal/m365/mock/connector.go @@ -46,7 +46,7 @@ func (ctrl Controller) ProduceBackupCollections( return ctrl.Collections, ctrl.Exclude, ctrl.Err == nil, ctrl.Err } -func (ctrl Controller) IsBackupRunnable( +func (ctrl Controller) IsServiceEnabled( _ context.Context, _ path.ServiceType, _ string, diff --git a/src/internal/m365/restore.go b/src/internal/m365/restore.go index 3455e650f..1abd4d9b7 100644 --- a/src/internal/m365/restore.go +++ b/src/internal/m365/restore.go @@ -40,23 +40,11 @@ func (ctrl *Controller) ConsumeRestoreCollections( return nil, clues.New("no data collections to restore") } - serviceEnabled, _, err := checkServiceEnabled( - ctx, - ctrl.AC.Users(), - rcc.Selector.PathService(), - rcc.ProtectedResource.ID()) - if err != nil { - return nil, err - } - - if !serviceEnabled { - return nil, clues.Stack(graph.ErrServiceNotEnabled).WithClues(ctx) - } - var ( service = rcc.Selector.PathService() status *support.ControllerOperationStatus deets = &details.Builder{} + err error ) switch service { diff --git a/src/internal/m365/service/exchange/backup.go b/src/internal/m365/service/exchange/backup.go index 39190c773..0d6c6cb6f 100644 --- a/src/internal/m365/service/exchange/backup.go +++ b/src/internal/m365/service/exchange/backup.go @@ -95,3 +95,17 @@ func ProduceBackupCollections( return collections, nil, canUsePreviousBackup, el.Failure() } + +func CanMakeDeltaQueries( + ctx context.Context, + service path.ServiceType, + gmi getMailboxer, + resourceOwner string, +) (bool, error) { + mi, err := GetMailboxInfo(ctx, gmi, resourceOwner) + if err != nil { + return false, clues.Stack(err) + } + + return !mi.QuotaExceeded, nil +} diff --git a/src/internal/m365/service/groups/enabled.go b/src/internal/m365/service/groups/enabled.go new file mode 100644 index 000000000..f0e8061af --- /dev/null +++ b/src/internal/m365/service/groups/enabled.go @@ -0,0 +1,21 @@ +package groups + +import ( + "context" + + "github.com/microsoftgraph/msgraph-sdk-go/models" +) + +type getByIDer interface { + GetByID(ctx context.Context, identifier string) (models.Groupable, error) +} + +func IsServiceEnabled( + ctx context.Context, + gbi getByIDer, + resource string, +) (bool, error) { + // TODO(meain): check for error message in case groups are + // not enabled at all similar to sharepoint + return true, nil +} diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index 049686bcf..54f321472 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -202,23 +202,25 @@ func (op *BackupOperation) Run(ctx context.Context) (err error) { ctx, flushMetrics := events.NewMetrics(ctx, logger.Writer{Ctx: ctx}) defer flushMetrics() - var runnable bool - - // IsBackupRunnable checks if the user has services enabled to run a backup. - // it also checks for conditions like mailbox full. - runnable, err = op.bp.IsBackupRunnable(ctx, op.Selectors.PathService(), op.ResourceOwner.ID()) + // Check if the protected resource has the service enabled in order for us + // to run a backup. + enabled, err := op.bp.IsServiceEnabled( + ctx, + op.Selectors.PathService(), + op.ResourceOwner.ID()) if err != nil { - logger.CtxErr(ctx, err).Error("verifying backup is runnable") - op.Errors.Fail(clues.Wrap(err, "verifying backup is runnable")) + logger.CtxErr(ctx, err).Error("verifying service backup is enabled") + op.Errors.Fail(clues.Wrap(err, "verifying service backup is enabled")) - return + return err } - if !runnable { - logger.CtxErr(ctx, graph.ErrServiceNotEnabled).Error("checking if backup is enabled") - op.Errors.Fail(clues.Wrap(err, "checking if backup is enabled")) + if !enabled { + // Return named error so that we can check for it in caller. + err = clues.Wrap(graph.ErrServiceNotEnabled, "service not enabled for backup") + op.Errors.Fail(err) - return + return err } // ----- diff --git a/src/internal/operations/backup_test.go b/src/internal/operations/backup_test.go index 0247ef15d..bf92f4bf5 100644 --- a/src/internal/operations/backup_test.go +++ b/src/internal/operations/backup_test.go @@ -1579,7 +1579,7 @@ func (mbp *mockBackupProducer) ProduceBackupCollections( return mbp.colls, nil, true, nil } -func (mbp *mockBackupProducer) IsBackupRunnable( +func (mbp *mockBackupProducer) IsServiceEnabled( context.Context, path.ServiceType, string, diff --git a/src/internal/operations/inject/inject.go b/src/internal/operations/inject/inject.go index 379d56de5..7b9ddd1c8 100644 --- a/src/internal/operations/inject/inject.go +++ b/src/internal/operations/inject/inject.go @@ -23,7 +23,8 @@ type ( bpc BackupProducerConfig, errs *fault.Bus, ) ([]data.BackupCollection, prefixmatcher.StringSetReader, bool, error) - IsBackupRunnable(ctx context.Context, service path.ServiceType, resourceOwner string) (bool, error) + + IsServiceEnableder Wait() *data.CollectionStats } @@ -37,12 +38,24 @@ type ( ctr *count.Bus, ) (*details.Details, error) + IsServiceEnableder + Wait() *data.CollectionStats CacheItemInfoer PopulateProtectedResourceIDAndNamer } + IsServiceEnableder interface { + // IsServiceEnabled checks if the service is enabled for backup/restore + // for the provided resource owner. + IsServiceEnabled( + ctx context.Context, + service path.ServiceType, + resourceOwner string, + ) (bool, error) + } + CacheItemInfoer interface { // CacheItemInfo is used by the consumer to cache metadata that is // sourced from per-item info, but may be valuable to the restore at diff --git a/src/internal/operations/restore.go b/src/internal/operations/restore.go index 6790d4e41..a692241e9 100644 --- a/src/internal/operations/restore.go +++ b/src/internal/operations/restore.go @@ -16,6 +16,7 @@ import ( "github.com/alcionai/corso/src/internal/diagnostics" "github.com/alcionai/corso/src/internal/events" "github.com/alcionai/corso/src/internal/kopia" + "github.com/alcionai/corso/src/internal/m365/graph" "github.com/alcionai/corso/src/internal/m365/service/onedrive" "github.com/alcionai/corso/src/internal/model" "github.com/alcionai/corso/src/internal/observe" @@ -228,6 +229,21 @@ func (op *RestoreOperation) do( "restore_protected_resource_id", restoreToProtectedResource.ID(), "restore_protected_resource_name", clues.Hide(restoreToProtectedResource.Name())) + // Check if the resource has the service enabled to be able to restore. + enabled, err := op.rc.IsServiceEnabled( + ctx, + op.Selectors.PathService(), + restoreToProtectedResource.ID()) + if err != nil { + return nil, clues.Wrap(err, "verifying service restore is enabled").WithClues(ctx) + } + + if !enabled { + return nil, clues.Wrap( + graph.ErrServiceNotEnabled, + "service not enabled for restore").WithClues(ctx) + } + observe.Message(ctx, "Restoring", observe.Bullet, clues.Hide(restoreToProtectedResource.Name())) paths, err := formatDetailsForRestoration( diff --git a/src/pkg/services/m365/api/user_info.go b/src/pkg/services/m365/api/user_info.go index 0b24d2f3d..90c1145db 100644 --- a/src/pkg/services/m365/api/user_info.go +++ b/src/pkg/services/m365/api/user_info.go @@ -68,24 +68,6 @@ func newUserInfo() *UserInfo { } } -// ServiceEnabled returns true if the UserInfo has an entry for the -// service. If no entry exists, the service is assumed to not be enabled. -func (ui *UserInfo) ServiceEnabled(service path.ServiceType) bool { - if ui == nil || len(ui.ServicesEnabled) == 0 { - return false - } - - _, ok := ui.ServicesEnabled[service] - - return ok -} - -// Returns if we can run delta queries on a mailbox. We cannot run -// them if the mailbox is full which is indicated by QuotaExceeded. -func (ui *UserInfo) CanMakeDeltaQueries() bool { - return !ui.Mailbox.QuotaExceeded -} - func ParseMailboxSettings( settings models.Userable, mi MailboxInfo, diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index 644272ad0..f20b9dfc1 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -172,6 +172,7 @@ func appendIfErr(errs []error, err error) []error { // Info // --------------------------------------------------------------------------- +// TODO(pandeyabs): Remove this once the SDK users have migrated to new APIs func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { var ( // Assume all services are enabled diff --git a/src/pkg/services/m365/users.go b/src/pkg/services/m365/users.go index 35b3a0630..ee3d99336 100644 --- a/src/pkg/services/m365/users.go +++ b/src/pkg/services/m365/users.go @@ -192,6 +192,8 @@ func parseUser(item models.Userable) (*User, error) { } // UserInfo returns the corso-specific set of user metadata. +// TODO(pandeyabs): Remove support for this API. SDK users would be using +// per service API calls - UserHasMailbox, UserGetMailboxInfo, UserHasDrive, etc. func GetUserInfo( ctx context.Context, acct account.Account,