diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ac5b3dc6..8608857df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] (beta) +### Added +- Permissions backup for OneDrive is now out of experimental (By default, only newly backed up items will have their permissions backed up. You will have to run a full backup to ensure all items have their permissions backed up.) + ### Fixed - Fixed permissions restore in latest backup version. - Incremental OneDrive backups could panic if the delta token expired and a folder was seen and deleted in the course of item enumeration for the backup. diff --git a/src/cli/backup/onedrive.go b/src/cli/backup/onedrive.go index 9429e4ea5..96a9bff0a 100644 --- a/src/cli/backup/onedrive.go +++ b/src/cli/backup/onedrive.go @@ -68,7 +68,7 @@ func addOneDriveCommands(cmd *cobra.Command) *cobra.Command { c, fs = utils.AddCommand(cmd, oneDriveCreateCmd()) fs.SortFlags = false - options.AddFeatureToggle(cmd, options.EnablePermissionsBackup()) + options.AddFeatureToggle(cmd) c.Use = c.Use + " " + oneDriveServiceCommandCreateUseSuffix c.Example = oneDriveServiceCommandCreateExamples diff --git a/src/cli/backup/onedrive_e2e_test.go b/src/cli/backup/onedrive_e2e_test.go index 515002f31..fe0aed7eb 100644 --- a/src/cli/backup/onedrive_e2e_test.go +++ b/src/cli/backup/onedrive_e2e_test.go @@ -80,7 +80,7 @@ func (suite *NoBackupOneDriveE2ESuite) SetupSuite() { suite.acct, suite.st, control.Options{ - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }) require.NoError(t, err, clues.ToCore(err)) } @@ -201,7 +201,7 @@ func (suite *BackupDeleteOneDriveE2ESuite) SetupSuite() { suite.acct, suite.st, control.Options{ - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }) require.NoError(t, err, clues.ToCore(err)) diff --git a/src/cli/options/options.go b/src/cli/options/options.go index 30cbdbf8a..56a43dc49 100644 --- a/src/cli/options/options.go +++ b/src/cli/options/options.go @@ -19,7 +19,6 @@ func Control() control.Options { opt.RestorePermissions = restorePermissions opt.SkipReduce = skipReduce opt.ToggleFeatures.DisableIncrementals = disableIncrementals - opt.ToggleFeatures.EnablePermissionsBackup = enablePermissionsBackup opt.ItemFetchParallelism = fetchParallelism return opt @@ -55,8 +54,6 @@ func AddGlobalOperationFlags(cmd *cobra.Command) { func AddRestorePermissionsFlag(cmd *cobra.Command) { fs := cmd.Flags() fs.BoolVar(&restorePermissions, "restore-permissions", false, "Restore permissions for files and folders") - // TODO: reveal this flag once backing up permissions becomes default - cobra.CheckErr(fs.MarkHidden("restore-permissions")) } // AddSkipReduceFlag adds a hidden flag that allows callers to skip the selector @@ -81,10 +78,7 @@ func AddFetchParallelismFlag(cmd *cobra.Command) { // Feature Flags // --------------------------------------------------------------------------- -var ( - disableIncrementals bool - enablePermissionsBackup bool -) +var disableIncrementals bool type exposeFeatureFlag func(*pflag.FlagSet) @@ -109,16 +103,3 @@ func DisableIncrementals() func(*pflag.FlagSet) { cobra.CheckErr(fs.MarkHidden("disable-incrementals")) } } - -// Adds the hidden '--enable-permissions-backup' cli flag which, when -// set, enables backing up permissions. -func EnablePermissionsBackup() func(*pflag.FlagSet) { - return func(fs *pflag.FlagSet) { - fs.BoolVar( - &enablePermissionsBackup, - "enable-permissions-backup", - false, - "Enable backing up item permissions for OneDrive") - cobra.CheckErr(fs.MarkHidden("enable-permissions-backup")) - } -} diff --git a/src/internal/connector/graph_connector_onedrive_test.go b/src/internal/connector/graph_connector_onedrive_test.go index 385055860..02b6f454e 100644 --- a/src/internal/connector/graph_connector_onedrive_test.go +++ b/src/internal/connector/graph_connector_onedrive_test.go @@ -526,143 +526,6 @@ func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsInheritanceR testPermissionsInheritanceRestoreAndBackup(suite, version.Backup) } -// TestPermissionsRestoreAndNoBackup checks that even if permissions exist -// not setting EnablePermissionsBackup results in empty permissions. This test -// only needs to run on the current version.Backup because it's about backup -// behavior not restore behavior (restore behavior is checked in other tests). -func (suite *GraphConnectorOneDriveIntegrationSuite) TestPermissionsRestoreAndNoBackup() { - ctx, flush := tester.NewContext() - defer flush() - - t := suite.T() - - secondaryUserName, secondaryUserID := suite.SecondaryUser() - - driveID := mustGetDefaultDriveID( - t, - ctx, - suite.BackupService(), - suite.Service(), - suite.BackupResourceOwner(), - ) - - secondaryUserRead := permData{ - user: secondaryUserName, - entityID: secondaryUserID, - roles: readPerm, - } - - secondaryUserWrite := permData{ - user: secondaryUserName, - entityID: secondaryUserID, - roles: writePerm, - } - - test := restoreBackupInfoMultiVersion{ - service: suite.BackupService(), - resource: suite.Resource(), - backupVersion: version.Backup, - collectionsPrevious: []colInfo{ - newOneDriveCollection( - suite.T(), - suite.BackupService(), - []string{ - "drives", - driveID, - "root:", - }, - version.Backup, - ). - withFile( - fileName, - fileAData, - secondaryUserWrite, - ). - withFolder( - folderBName, - secondaryUserRead, - ). - collection(), - newOneDriveCollection( - suite.T(), - suite.BackupService(), - []string{ - "drives", - driveID, - "root:", - folderBName, - }, - version.Backup, - ). - withFile( - fileName, - fileEData, - secondaryUserRead, - ). - withPermissions( - secondaryUserRead, - ). - collection(), - }, - collectionsLatest: []colInfo{ - newOneDriveCollection( - suite.T(), - suite.BackupService(), - []string{ - "drives", - driveID, - "root:", - }, - version.Backup, - ). - withFile( - fileName, - fileAData, - permData{}, - ). - withFolder( - folderBName, - permData{}, - ). - collection(), - newOneDriveCollection( - suite.T(), - suite.BackupService(), - []string{ - "drives", - driveID, - "root:", - folderBName, - }, - version.Backup, - ). - withFile( - fileName, - fileEData, - permData{}, - ). - // Call this to generate a meta file with the folder name that we can - // check. - withPermissions( - permData{}, - ). - collection(), - }, - } - - runRestoreBackupTestVersions( - t, - suite.Account(), - test, - suite.Tenant(), - []string{suite.BackupResourceOwner()}, - control.Options{ - RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: false}, - }, - ) -} - // --------------------------------------------------------------------------- // OneDrive regression // --------------------------------------------------------------------------- @@ -862,7 +725,7 @@ func testRestoreAndBackupMultipleFilesAndFoldersNoPermissions( []string{suite.BackupResourceOwner()}, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, ) }) @@ -1073,7 +936,7 @@ func testPermissionsRestoreAndBackup(suite oneDriveSuite, startVersion int) { []string{suite.BackupResourceOwner()}, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, ) }) @@ -1156,7 +1019,7 @@ func testPermissionsBackupAndNoRestore(suite oneDriveSuite, startVersion int) { []string{suite.BackupResourceOwner()}, control.Options{ RestorePermissions: false, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, ) }) @@ -1308,7 +1171,7 @@ func testPermissionsInheritanceRestoreAndBackup(suite oneDriveSuite, startVersio []string{suite.BackupResourceOwner()}, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, ) }) diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index 5c45b3fa9..5b93a2118 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -249,7 +249,7 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreFailsBadService() { dest, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, nil, fault.New(true)) @@ -328,7 +328,7 @@ func (suite *GraphConnectorIntegrationSuite) TestEmptyCollections() { dest, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, test.col, fault.New(true)) @@ -851,7 +851,7 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackup() { []string{suite.user}, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, ) }) @@ -973,7 +973,7 @@ func (suite *GraphConnectorIntegrationSuite) TestMultiFolderBackupDifferentNames dest, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, collections, fault.New(true)) @@ -1004,7 +1004,7 @@ func (suite *GraphConnectorIntegrationSuite) TestMultiFolderBackupDifferentNames nil, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, fault.New(true)) require.NoError(t, err, clues.ToCore(err)) @@ -1064,7 +1064,7 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackup_largeMailAttac []string{suite.user}, control.Options{ RestorePermissions: true, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }, ) } @@ -1157,7 +1157,7 @@ func (suite *GraphConnectorIntegrationSuite) TestBackup_CreatesPrefixCollections nil, control.Options{ RestorePermissions: false, - ToggleFeatures: control.Toggles{EnablePermissionsBackup: false}, + ToggleFeatures: control.Toggles{}, }, fault.New(true)) require.NoError(t, err) diff --git a/src/internal/connector/onedrive/collection.go b/src/internal/connector/onedrive/collection.go index ba8b715fa..997c7c94b 100644 --- a/src/internal/connector/onedrive/collection.go +++ b/src/internal/connector/onedrive/collection.go @@ -132,7 +132,6 @@ type itemMetaReaderFunc func( service graph.Servicer, driveID string, item models.DriveItemable, - fetchPermissions bool, ) (io.ReadCloser, int, error) // NewCollection creates a Collection @@ -481,8 +480,7 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) { ctx, oc.service, oc.driveID, - item, - oc.ctrl.ToggleFeatures.EnablePermissionsBackup) + item) if err != nil { el.AddRecoverable(clues.Wrap(err, "getting item metadata").Label(fault.LabelForceNoBackupCreation)) @@ -544,8 +542,10 @@ func (oc *Collection) populateItems(ctx context.Context, errs *fault.Bus) { }) oc.data <- &MetadataItem{ - id: metaFileName + metaSuffix, - data: metaReader, + id: metaFileName + metaSuffix, + data: metaReader, + // Metadata file should always use the latest time as + // permissions change does not update mod time. modTime: time.Now(), } diff --git a/src/internal/connector/onedrive/collection_test.go b/src/internal/connector/onedrive/collection_test.go index e870f94e3..3afa31984 100644 --- a/src/internal/connector/onedrive/collection_test.go +++ b/src/internal/connector/onedrive/collection_test.go @@ -213,7 +213,7 @@ func (suite *CollectionUnitTestSuite) TestCollection() { suite, suite.testStatusUpdater(&wg, &collStatus), test.source, - control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}, + control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) require.NotNil(t, coll) @@ -237,7 +237,6 @@ func (suite *CollectionUnitTestSuite) TestCollection() { _ graph.Servicer, _ string, _ models.DriveItemable, - _ bool, ) (io.ReadCloser, int, error) { metaJSON, err := json.Marshal(testItemMeta) if err != nil { @@ -353,7 +352,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadError() { suite, suite.testStatusUpdater(&wg, &collStatus), test.source, - control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}, + control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) @@ -378,7 +377,6 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadError() { _ graph.Servicer, _ string, _ models.DriveItemable, - _ bool, ) (io.ReadCloser, int, error) { return io.NopCloser(strings.NewReader(`{}`)), 2, nil } @@ -443,7 +441,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadUnauthorizedErrorRetry() suite, suite.testStatusUpdater(&wg, &collStatus), test.source, - control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}, + control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) @@ -484,7 +482,6 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadUnauthorizedErrorRetry() _ graph.Servicer, _ string, _ models.DriveItemable, - _ bool, ) (io.ReadCloser, int, error) { return io.NopCloser(strings.NewReader(`{}`)), 2, nil } @@ -504,7 +501,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadUnauthorizedErrorRetry() } } -// TODO(meain): Remove this test once we start always backing up permissions +// Ensure metadata file always uses latest time for mod time func (suite *CollectionUnitTestSuite) TestCollectionPermissionBackupLatestModTime() { table := []struct { name string @@ -543,7 +540,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionPermissionBackupLatestModTim suite, suite.testStatusUpdater(&wg, &collStatus), test.source, - control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}, + control.Options{ToggleFeatures: control.Toggles{}}, CollectionScopeFolder, true) @@ -571,7 +568,6 @@ func (suite *CollectionUnitTestSuite) TestCollectionPermissionBackupLatestModTim _ graph.Servicer, _ string, _ models.DriveItemable, - _ bool, ) (io.ReadCloser, int, error) { return io.NopCloser(strings.NewReader(`{}`)), 16, nil } diff --git a/src/internal/connector/onedrive/collections_test.go b/src/internal/connector/onedrive/collections_test.go index ffe42f777..2f823ce60 100644 --- a/src/internal/connector/onedrive/collections_test.go +++ b/src/internal/connector/onedrive/collections_test.go @@ -786,7 +786,7 @@ func (suite *OneDriveCollectionsUnitSuite) TestUpdateCollections() { testFolderMatcher{tt.scope}, &MockGraphService{}, nil, - control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}) + control.Options{ToggleFeatures: control.Toggles{}}) c.CollectionMap[driveID] = map[string]*Collection{} @@ -2237,7 +2237,7 @@ func (suite *OneDriveCollectionsUnitSuite) TestGet() { testFolderMatcher{anyFolder}, &MockGraphService{}, func(*support.ConnectorOperationStatus) {}, - control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}}, + control.Options{ToggleFeatures: control.Toggles{}}, ) c.drivePagerFunc = drivePagerFunc c.itemPagerFunc = itemPagerFunc diff --git a/src/internal/connector/onedrive/drive_test.go b/src/internal/connector/onedrive/drive_test.go index 1bbc28779..930569a94 100644 --- a/src/internal/connector/onedrive/drive_test.go +++ b/src/internal/connector/onedrive/drive_test.go @@ -430,7 +430,7 @@ func (suite *OneDriveSuite) TestOneDriveNewCollections() { service, service.updateStatus, control.Options{ - ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}, + ToggleFeatures: control.Toggles{}, }) odcs, excludes, err := colls.Get(ctx, nil, fault.New(true)) diff --git a/src/internal/connector/onedrive/item.go b/src/internal/connector/onedrive/item.go index f508ce506..8eddfb6d6 100644 --- a/src/internal/connector/onedrive/item.go +++ b/src/internal/connector/onedrive/item.go @@ -66,9 +66,8 @@ func oneDriveItemMetaReader( service graph.Servicer, driveID string, item models.DriveItemable, - fetchPermissions bool, ) (io.ReadCloser, int, error) { - return baseItemMetaReader(ctx, service, driveID, item, fetchPermissions) + return baseItemMetaReader(ctx, service, driveID, item) } func sharePointItemMetaReader( @@ -76,10 +75,9 @@ func sharePointItemMetaReader( service graph.Servicer, driveID string, item models.DriveItemable, - fetchPermissions bool, ) (io.ReadCloser, int, error) { // TODO: include permissions - return baseItemMetaReader(ctx, service, driveID, item, false) + return baseItemMetaReader(ctx, service, driveID, item) } func baseItemMetaReader( @@ -87,7 +85,6 @@ func baseItemMetaReader( service graph.Servicer, driveID string, item models.DriveItemable, - fetchPermissions bool, ) (io.ReadCloser, int, error) { var ( perms []UserPermission @@ -101,7 +98,7 @@ func baseItemMetaReader( meta.SharingMode = SharingModeCustom } - if meta.SharingMode == SharingModeCustom && fetchPermissions { + if meta.SharingMode == SharingModeCustom { perms, err = driveItemPermissionInfo(ctx, service, driveID, ptr.Val(item.GetId())) if err != nil { return nil, 0, err diff --git a/src/internal/operations/backup.go b/src/internal/operations/backup.go index 9d5af2860..340d77fbd 100644 --- a/src/internal/operations/backup.go +++ b/src/internal/operations/backup.go @@ -302,18 +302,12 @@ func (op *BackupOperation) do( func useIncrementalBackup(sel selectors.Selector, opts control.Options) bool { enabled := !opts.ToggleFeatures.DisableIncrementals - switch sel.Service { - case selectors.ServiceExchange: + if sel.Service == selectors.ServiceExchange || + sel.Service == selectors.ServiceOneDrive { return enabled - - case selectors.ServiceOneDrive: - // TODO(ashmrtn): Remove the && part once we support permissions and - // incrementals. - return enabled && !opts.ToggleFeatures.EnablePermissionsBackup - - default: - return false } + + return false } // --------------------------------------------------------------------------- diff --git a/src/internal/operations/backup_integration_test.go b/src/internal/operations/backup_integration_test.go index c87a3ec71..3ec53b3c1 100644 --- a/src/internal/operations/backup_integration_test.go +++ b/src/internal/operations/backup_integration_test.go @@ -1155,7 +1155,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDrive() { sel.Include(sel.AllData()) - bo, _, _, _, _, closer := prepNewTestBackupOp(t, ctx, mb, sel.Selector, control.Toggles{EnablePermissionsBackup: true}) + bo, _, _, _, _, closer := prepNewTestBackupOp(t, ctx, mb, sel.Selector, control.Toggles{}) defer closer() runAndCheckBackup(t, ctx, &bo, mb, false) diff --git a/src/pkg/control/options.go b/src/pkg/control/options.go index 0bc119f52..5f194f5ca 100644 --- a/src/pkg/control/options.go +++ b/src/pkg/control/options.go @@ -88,9 +88,4 @@ type Toggles struct { // DisableIncrementals prevents backups from using incremental lookups, // forcing a new, complete backup of all data regardless of prior state. DisableIncrementals bool `json:"exchangeIncrementals,omitempty"` - - // EnablePermissionsBackup is used to enable backups of item - // permissions. Permission metadata increases graph api call count, - // so disabling their retrieval when not needed is advised. - EnablePermissionsBackup bool `json:"enablePermissionsBackup,omitempty"` }