Enable permissions backup by default (#2981)

Previously we had a flag that needed to be flipped to backup permissions. Now we always backup permissions.

- Should we wait till https://github.com/alcionai/corso/issues/2976 is done to do this
- Users will have to do a non incremental backup first as of now to get permissions. We should probably have some way to force this.

---

#### Does this PR need a docs update or release note?

- [x] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [ ] ⛔ No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [x] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [ ] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* closes https://github.com/alcionai/corso/issues/2409

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x] ⚡ Unit test
- [x] 💚 E2E

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.

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

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))
 

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 (
+var disableIncrementals bool
-	disableIncrementals     bool
-	enablePermissionsBackup 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"))
-	}
-}

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{},
 				},
 			)
 		})

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)

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,
+				item)
-				oc.ctrl.ToggleFeatures.EnablePermissionsBackup)
 
 			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,
+				id:   metaFileName + metaSuffix,
-				data:    metaReader,
+				data: metaReader,
+				// Metadata file should always use the latest time as
+				// permissions change does not update mod time.
 				modTime: time.Now(),
 			}
 

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
 			}

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

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))

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

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 {
+	if sel.Service == selectors.ServiceExchange ||
-	case 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
 }
 
 // ---------------------------------------------------------------------------

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)

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"`
 }