Disable OneDrive permissions backup by default (#2374)

## Description

Since the backup permissions increases the backup time by a lot(mostly from higher number of requests and increased throttling), it was decided to disable it by default. It is still enabled in tests so as to make sure the code does not have regressions.

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

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

## Type of change

<!--- Please check the type of change your PR introduces: --->
- [x] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Test
- [ ] 💻 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. -->
* #<issue>

## Test Plan

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

src/cli/backup/onedrive.go

@@ -79,7 +79,7 @@ func addOneDriveCommands(cmd *cobra.Command) *cobra.Command {
 	switch cmd.Use {
 	case createCommand:
 		c, fs = utils.AddCommand(cmd, oneDriveCreateCmd())
-		options.AddFeatureToggle(cmd, options.DisablePermissionsBackup())
+		options.AddFeatureToggle(cmd, options.EnablePermissionsBackup())
 
 		c.Use = c.Use + " " + oneDriveServiceCommandCreateUseSuffix
 		c.Example = oneDriveServiceCommandCreateExamples

src/cli/backup/onedrive_integration_test.go

@@ -72,7 +72,13 @@ func (suite *NoBackupOneDriveIntegrationSuite) SetupSuite() {
 	suite.m365UserID = tester.M365UserID(t)
 
 	// init the repo first
-	suite.repo, err = repository.Initialize(ctx, suite.acct, suite.st, control.Options{})
+	suite.repo, err = repository.Initialize(
+		ctx,
+		suite.acct,
+		suite.st,
+		control.Options{
+			ToggleFeatures: control.Toggles{EnablePermissionsBackup: true},
+		})
 	require.NoError(t, err)
 }
 
@@ -152,7 +158,13 @@ func (suite *BackupDeleteOneDriveIntegrationSuite) SetupSuite() {
 	defer flush()
 
 	// init the repo first
-	suite.repo, err = repository.Initialize(ctx, suite.acct, suite.st, control.Options{})
+	suite.repo, err = repository.Initialize(
+		ctx,
+		suite.acct,
+		suite.st,
+		control.Options{
+			ToggleFeatures: control.Toggles{EnablePermissionsBackup: true},
+		})
 	require.NoError(t, err)
 
 	m365UserID := tester.M365UserID(t)

src/cli/options/options.go

@@ -15,7 +15,7 @@ func Control() control.Options {
 	opt.DisableMetrics = noStats
 	opt.RestorePermissions = restorePermissions
 	opt.ToggleFeatures.DisableIncrementals = disableIncrementals
-	opt.ToggleFeatures.DisablePermissionsBackup = disablePermissionsBackup
+	opt.ToggleFeatures.EnablePermissionsBackup = enablePermissionsBackup
 
 	return opt
 }
@@ -48,6 +48,8 @@ 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"))
 }
 
 // ---------------------------------------------------------------------------
@@ -55,8 +57,8 @@ func AddRestorePermissionsFlag(cmd *cobra.Command) {
 // ---------------------------------------------------------------------------
 
 var (
-	disableIncrementals      bool
+	disableIncrementals     bool
-	disablePermissionsBackup bool
+	enablePermissionsBackup bool
 )
 
 type exposeFeatureFlag func(*pflag.FlagSet)
@@ -83,15 +85,15 @@ func DisableIncrementals() func(*pflag.FlagSet) {
 	}
 }
 
-// Adds the hidden '--disable-permissions-backup' cli flag which, when
+// Adds the hidden '--enable-permissions-backup' cli flag which, when
-// set, disables backing up permissions.
+// set, enables backing up permissions.
-func DisablePermissionsBackup() func(*pflag.FlagSet) {
+func EnablePermissionsBackup() func(*pflag.FlagSet) {
 	return func(fs *pflag.FlagSet) {
 		fs.BoolVar(
-			&disablePermissionsBackup,
+			&enablePermissionsBackup,
-			"disable-permissions-backup",
+			"enable-permissions-backup",
 			false,
-			"Disable backing up item permissions for OneDrive")
+			"Enable backing up item permissions for OneDrive")
-		cobra.CheckErr(fs.MarkHidden("disable-permissions-backup"))
+		cobra.CheckErr(fs.MarkHidden("enable-permissions-backup"))
 	}
 }

src/internal/connector/graph_connector_test.go

@@ -238,7 +238,10 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreFailsBadService() {
 		acct,
 		sel,
 		dest,
-		control.Options{},
+		control.Options{
+			RestorePermissions: true,
+			ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+		},
 		nil,
 	)
 	assert.Error(t, err)
@@ -312,7 +315,10 @@ func (suite *GraphConnectorIntegrationSuite) TestEmptyCollections() {
 				suite.acct,
 				test.sel,
 				dest,
-				control.Options{RestorePermissions: true},
+				control.Options{
+					RestorePermissions: true,
+					ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+				},
 				test.col,
 			)
 			require.NoError(t, err)
@@ -448,7 +454,15 @@ func runRestoreBackupTest(
 	t.Logf("Selective backup of %s\n", backupSel)
 
 	start = time.Now()
-	dcs, excludes, err := backupGC.DataCollections(ctx, backupSel, nil, control.Options{RestorePermissions: true})
+	dcs, excludes, err := backupGC.DataCollections(
+		ctx,
+		backupSel,
+		nil,
+		control.Options{
+			RestorePermissions: true,
+			ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+		},
+	)
 	require.NoError(t, err)
 	// No excludes yet because this isn't an incremental backup.
 	assert.Empty(t, excludes)
@@ -568,7 +582,15 @@ func runRestoreBackupTestVersion0(
 	backupSel := backupSelectorForExpected(t, test.service, expectedDests)
 
 	start = time.Now()
-	dcs, excludes, err := backupGC.DataCollections(ctx, backupSel, nil, control.Options{RestorePermissions: true})
+	dcs, excludes, err := backupGC.DataCollections(
+		ctx,
+		backupSel,
+		nil,
+		control.Options{
+			RestorePermissions: true,
+			ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+		},
+	)
 	require.NoError(t, err)
 	// No excludes yet because this isn't an incremental backup.
 	assert.Empty(t, excludes)
@@ -1026,7 +1048,10 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackup() {
 				test,
 				suite.connector.tenant,
 				[]string{suite.user},
-				control.Options{RestorePermissions: true},
+				control.Options{
+					RestorePermissions: true,
+					ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+				},
 			)
 		})
 	}
@@ -1275,7 +1300,10 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackupVersion0() {
 				test,
 				suite.connector.tenant,
 				[]string{suite.user},
-				control.Options{RestorePermissions: true},
+				control.Options{
+					RestorePermissions: true,
+					ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+				},
 			)
 		})
 	}
@@ -1391,7 +1419,10 @@ func (suite *GraphConnectorIntegrationSuite) TestMultiFolderBackupDifferentNames
 					suite.acct,
 					restoreSel,
 					dest,
-					control.Options{RestorePermissions: true},
+					control.Options{
+						RestorePermissions: true,
+						ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+					},
 					collections,
 				)
 				require.NoError(t, err)
@@ -1414,7 +1445,15 @@ func (suite *GraphConnectorIntegrationSuite) TestMultiFolderBackupDifferentNames
 			backupSel := backupSelectorForExpected(t, test.service, expectedDests)
 			t.Log("Selective backup of", backupSel)
 
-			dcs, excludes, err := backupGC.DataCollections(ctx, backupSel, nil, control.Options{RestorePermissions: true})
+			dcs, excludes, err := backupGC.DataCollections(
+				ctx,
+				backupSel,
+				nil,
+				control.Options{
+					RestorePermissions: true,
+					ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+				},
+			)
 			require.NoError(t, err)
 			// No excludes yet because this isn't an incremental backup.
 			assert.Empty(t, excludes)
@@ -1446,7 +1485,7 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsRestoreAndBackup() {
 
 	table := []restoreBackupInfo{
 		{
-			name:     "FilePermissionsResote",
+			name:     "FilePermissionsRestore",
 			service:  path.OneDriveService,
 			resource: Users,
 			collections: []colInfo{
@@ -1677,7 +1716,10 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsRestoreAndBackup() {
 				test,
 				suite.connector.tenant,
 				[]string{suite.user},
-				control.Options{RestorePermissions: true},
+				control.Options{
+					RestorePermissions: true,
+					ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+				},
 			)
 		})
 	}
@@ -1697,7 +1739,7 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsBackupAndNoRestore()
 
 	table := []restoreBackupInfo{
 		{
-			name:     "FilePermissionsResote",
+			name:     "FilePermissionsRestore",
 			service:  path.OneDriveService,
 			resource: Users,
 			collections: []colInfo{
@@ -1733,7 +1775,10 @@ func (suite *GraphConnectorIntegrationSuite) TestPermissionsBackupAndNoRestore()
 				test,
 				suite.connector.tenant,
 				[]string{suite.user},
-				control.Options{RestorePermissions: false},
+				control.Options{
+					RestorePermissions: true,
+					ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+				},
 			)
 		})
 	}
@@ -1769,6 +1814,9 @@ func (suite *GraphConnectorIntegrationSuite) TestRestoreAndBackup_largeMailAttac
 		test,
 		suite.connector.tenant,
 		[]string{suite.user},
-		control.Options{RestorePermissions: true},
+		control.Options{
+			RestorePermissions: true,
+			ToggleFeatures:     control.Toggles{EnablePermissionsBackup: true},
+		},
 	)
 }

src/internal/connector/onedrive/collection.go

@@ -279,10 +279,11 @@ func (oc *Collection) populateItems(ctx context.Context) {
 			if oc.source == OneDriveSource {
 				// Fetch metadata for the file
 				for i := 1; i <= maxRetries; i++ {
-					if oc.ctrl.ToggleFeatures.DisablePermissionsBackup {
+					if !oc.ctrl.ToggleFeatures.EnablePermissionsBackup {
 						// We are still writing the metadata file but with
-						// empty permissions as we are not sure how the
+						// empty permissions as we don't have a way to
-						// restore will be called.
+						// signify that the permissions was explicitly
+						// not added.
 						itemMeta = io.NopCloser(strings.NewReader("{}"))
 						itemMetaSize = 2
 

src/internal/connector/onedrive/collection_test.go

@@ -168,7 +168,7 @@ func (suite *CollectionUnitTestSuite) TestCollection() {
 				suite,
 				suite.testStatusUpdater(&wg, &collStatus),
 				test.source,
-				control.Options{})
+				control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}})
 			require.NotNil(t, coll)
 			assert.Equal(t, folderPath, coll.FullPath())
 
@@ -301,7 +301,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionReadError() {
 				suite,
 				suite.testStatusUpdater(&wg, &collStatus),
 				test.source,
-				control.Options{})
+				control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}})
 
 			mockItem := models.NewDriveItem()
 			mockItem.SetId(&testItemID)
@@ -372,7 +372,7 @@ func (suite *CollectionUnitTestSuite) TestCollectionDisablePermissionsBackup() {
 				suite,
 				suite.testStatusUpdater(&wg, &collStatus),
 				test.source,
-				control.Options{ToggleFeatures: control.Toggles{DisablePermissionsBackup: true}})
+				control.Options{ToggleFeatures: control.Toggles{}})
 
 			now := time.Now()
 			mockItem := models.NewDriveItem()

src/internal/connector/onedrive/collections_test.go

@@ -635,7 +635,7 @@ func (suite *OneDriveCollectionsSuite) TestUpdateCollections() {
 				testFolderMatcher{tt.scope},
 				&MockGraphService{},
 				nil,
-				control.Options{})
+				control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}})
 
 			err := c.UpdateCollections(
 				ctx,
@@ -1380,7 +1380,7 @@ func (suite *OneDriveCollectionsSuite) TestGet() {
 				testFolderMatcher{anyFolder},
 				&MockGraphService{},
 				func(*support.ConnectorOperationStatus) {},
-				control.Options{},
+				control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}},
 			)
 			c.drivePagerFunc = drivePagerFunc
 			c.itemPagerFunc = itemPagerFunc

src/internal/connector/onedrive/drive_test.go

@@ -462,7 +462,7 @@ func (suite *OneDriveSuite) TestOneDriveNewCollections() {
 				testFolderMatcher{scope},
 				service,
 				service.updateStatus,
-				control.Options{},
+				control.Options{ToggleFeatures: control.Toggles{EnablePermissionsBackup: true}},
 			).Get(ctx, nil)
 			assert.NoError(t, err)
 			// Don't expect excludes as this isn't an incremental backup.

src/internal/operations/backup_integration_test.go

@@ -1081,7 +1081,7 @@ func (suite *BackupOpIntegrationSuite) TestBackup_Run_oneDrive() {
 
 	sel.Include(sel.AllData())
 
-	bo, _, _, _, closer := prepNewTestBackupOp(t, ctx, mb, sel.Selector, control.Toggles{})
+	bo, _, _, _, closer := prepNewTestBackupOp(t, ctx, mb, sel.Selector, control.Toggles{EnablePermissionsBackup: true})
 	defer closer()
 
 	runAndCheckBackup(t, ctx, &bo, mb)

src/pkg/control/options.go

@@ -76,8 +76,8 @@ type Toggles struct {
 	// forcing a new, complete backup of all data regardless of prior state.
 	DisableIncrementals bool `json:"exchangeIncrementals,omitempty"`
 
-	// DisablePermissionsBackup is used to disable backups of item
+	// 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.
-	DisablePermissionsBackup bool `json:"disablePermissionsBackup,omitempty"`
+	EnablePermissionsBackup bool `json:"enablePermissionsBackup,omitempty"`
 }