From 5d90483bfafa5419af0580fd482c1db723e1c38f Mon Sep 17 00:00:00 2001 From: Abin Simon Date: Mon, 16 Oct 2023 17:35:55 +0530 Subject: [PATCH] Enforce site filter for Groups restore (#4496) This PR reworks the groups restore to and makes the site to restore to mandatory. This also updates some missing filtering capabilities in groups export. --- #### Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [ ] :no_entry: No #### Type of change - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * closes https://github.com/alcionai/corso/issues/4462 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- CHANGELOG.md | 4 ++++ src/cli/backup/sharepoint.go | 4 ++-- src/cli/export/groups.go | 5 ++++- src/cli/flags/sharepoint.go | 26 +++++++++++++----------- src/cli/flags/testdata/flags.go | 1 + src/cli/restore/groups.go | 7 ++++--- src/cli/restore/groups_test.go | 1 + src/cli/utils/groups.go | 34 +++++++++++++++++++++++++++++--- src/cli/utils/groups_test.go | 31 ++++++++++++++++++++--------- src/pkg/selectors/groups.go | 30 +++++++++++++++++++++++++++- src/pkg/selectors/groups_test.go | 4 ++++ 11 files changed, 117 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 14a11c924..12c60ecff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Catch and report cases where a protected resource is locked out of access. SDK consumers have a new errs sentinel that allows them to check for this case. - Fix a case where missing item LastModifiedTimes could cause incremental backups to fail. - Email size metadata was incorrectly set to the size of the last attachment. Emails will now correctly report the size of the mail content plus the size of all attachments. +- Improves the filtering capabilities for Groups restore and backup + +### Changed +- Groups restore now expects the site whose backup we should restore ## [v0.14.0] (beta) - 2023-10-09 diff --git a/src/cli/backup/sharepoint.go b/src/cli/backup/sharepoint.go index bfeefaa54..f4b4fdd5c 100644 --- a/src/cli/backup/sharepoint.go +++ b/src/cli/backup/sharepoint.go @@ -76,8 +76,8 @@ func addSharePointCommands(cmd *cobra.Command) *cobra.Command { c.Use = c.Use + " " + sharePointServiceCommandCreateUseSuffix c.Example = sharePointServiceCommandCreateExamples - flags.AddSiteFlag(c) - flags.AddSiteIDFlag(c) + flags.AddSiteFlag(c, true) + flags.AddSiteIDFlag(c, true) flags.AddDataFlag(c, []string{flags.DataLibraries}, true) flags.AddFailFastFlag(c) flags.AddDisableIncrementalsFlag(c) diff --git a/src/cli/export/groups.go b/src/cli/export/groups.go index 54add842c..cf370f760 100644 --- a/src/cli/export/groups.go +++ b/src/cli/export/groups.go @@ -27,6 +27,9 @@ func addGroupsCommands(cmd *cobra.Command) *cobra.Command { fs.SortFlags = false flags.AddBackupIDFlag(c, true) + flags.AddSiteFlag(c, false) + flags.AddSiteIDFlag(c, false) + flags.AddSharePointDetailsAndRestoreFlags(c) flags.AddGroupDetailsAndRestoreFlags(c) flags.AddExportConfigFlags(c) flags.AddFailFastFlag(c) @@ -89,7 +92,7 @@ func exportGroupsCmd(cmd *cobra.Command, args []string) error { return nil } - if err := utils.ValidateGroupsRestoreFlags(flags.BackupIDFV, opts); err != nil { + if err := utils.ValidateGroupsRestoreFlags(flags.BackupIDFV, opts, false); err != nil { return err } diff --git a/src/cli/flags/sharepoint.go b/src/cli/flags/sharepoint.go index d0cdbfe0b..9a4a67ada 100644 --- a/src/cli/flags/sharepoint.go +++ b/src/cli/flags/sharepoint.go @@ -95,24 +95,28 @@ func AddSharePointDetailsAndRestoreFlags(cmd *cobra.Command) { // AddSiteIDFlag adds the --site-id flag, which accepts site ID values. // This flag is hidden, since we expect users to prefer the --site url // and do not want to encourage confusion. -func AddSiteIDFlag(cmd *cobra.Command) { +func AddSiteIDFlag(cmd *cobra.Command, multiple bool) { fs := cmd.Flags() + message := "ID of the site to operate on" + if multiple { + //nolint:lll + message += "; accepts '" + Wildcard + "' to select all sites. Args cannot be comma-delimited and must use multiple flags." + } + // note string ARRAY var. IDs naturally contain commas, so we cannot accept // duplicate values within a flag declaration. ie: --site-id a,b,c does not // work. Users must call --site-id a --site-id b --site-id c. - fs.StringArrayVar( - &SiteIDFV, - SiteIDFN, nil, - //nolint:lll - "Backup data by site ID; accepts '"+Wildcard+"' to select all sites. Args cannot be comma-delimited and must use multiple flags.") + fs.StringArrayVar(&SiteIDFV, SiteIDFN, nil, message) cobra.CheckErr(fs.MarkHidden(SiteIDFN)) } // AddSiteFlag adds the --site flag, which accepts webURL values. -func AddSiteFlag(cmd *cobra.Command) { - cmd.Flags().StringSliceVar( - &WebURLFV, - SiteFN, nil, - "Backup data by site URL; accepts '"+Wildcard+"' to select all sites.") +func AddSiteFlag(cmd *cobra.Command, multiple bool) { + message := "Web URL of the site to operate on" + if multiple { + message += "; accepts '" + Wildcard + "' to select all sites." + } + + cmd.Flags().StringSliceVar(&WebURLFV, SiteFN, nil, message) } diff --git a/src/cli/flags/testdata/flags.go b/src/cli/flags/testdata/flags.go index 7dec134f4..d3bea0c93 100644 --- a/src/cli/flags/testdata/flags.go +++ b/src/cli/flags/testdata/flags.go @@ -10,6 +10,7 @@ func FlgInputs(in []string) string { return strings.Join(in, ",") } var ( BackupInput = "backup-id" + SiteInput = "site-id" GroupsInput = []string{"team1", "group2"} MailboxInput = []string{"mailbox1", "mailbox2"} diff --git a/src/cli/restore/groups.go b/src/cli/restore/groups.go index 3d1f3df6e..c9ba2d6b7 100644 --- a/src/cli/restore/groups.go +++ b/src/cli/restore/groups.go @@ -27,9 +27,10 @@ func addGroupsCommands(cmd *cobra.Command) *cobra.Command { fs.SortFlags = false flags.AddBackupIDFlag(c, true) + flags.AddSiteFlag(c, false) + flags.AddSiteIDFlag(c, false) flags.AddNoPermissionsFlag(c) - flags.AddSharePointDetailsAndRestoreFlags(c) // for sp restores - flags.AddSiteIDFlag(c) + flags.AddSharePointDetailsAndRestoreFlags(c) flags.AddRestoreConfigFlags(c, false) flags.AddFailFastFlag(c) } @@ -83,7 +84,7 @@ func restoreGroupsCmd(cmd *cobra.Command, args []string) error { return nil } - if err := utils.ValidateGroupsRestoreFlags(flags.BackupIDFV, opts); err != nil { + if err := utils.ValidateGroupsRestoreFlags(flags.BackupIDFV, opts, true); err != nil { return err } diff --git a/src/cli/restore/groups_test.go b/src/cli/restore/groups_test.go index 58af79e09..0a8e2f17f 100644 --- a/src/cli/restore/groups_test.go +++ b/src/cli/restore/groups_test.go @@ -52,6 +52,7 @@ func (suite *GroupsUnitSuite) TestAddGroupsCommands() { []string{ "--" + flags.RunModeFN, flags.RunModeFlagTest, "--" + flags.BackupFN, flagsTD.BackupInput, + "--" + flags.SiteFN, flagsTD.SiteInput, "--" + flags.LibraryFN, flagsTD.LibraryInput, "--" + flags.FileFN, flagsTD.FlgInputs(flagsTD.FileNameInput), "--" + flags.FolderFN, flagsTD.FlgInputs(flagsTD.FolderPathInput), diff --git a/src/cli/utils/groups.go b/src/cli/utils/groups.go index 3c7d378ec..9ded3c9ed 100644 --- a/src/cli/utils/groups.go +++ b/src/cli/utils/groups.go @@ -21,6 +21,7 @@ type GroupsOpts struct { MessageLastReplyBefore string SiteID []string + WebURL []string Library string FileName []string // for libraries, to duplicate onedrive interface FolderPath []string // for libraries, to duplicate onedrive interface @@ -70,6 +71,7 @@ func MakeGroupsOpts(cmd *cobra.Command) GroupsOpts { Groups: flags.GroupFV, Channels: flags.ChannelFV, Messages: flags.MessageFV, + WebURL: flags.WebURLFV, SiteID: flags.SiteIDFV, Library: flags.LibraryFV, @@ -101,11 +103,22 @@ func MakeGroupsOpts(cmd *cobra.Command) GroupsOpts { } // ValidateGroupsRestoreFlags checks common flags for correctness and interdependencies -func ValidateGroupsRestoreFlags(backupID string, opts GroupsOpts) error { +func ValidateGroupsRestoreFlags(backupID string, opts GroupsOpts, isRestore bool) error { if len(backupID) == 0 { return clues.New("a backup ID is required") } + // The user has to explicitly specify which resource to restore. In + // this case, since we can only restore sites, the user is supposed + // to specify which site to restore. + if isRestore { + if len(opts.WebURL)+len(opts.SiteID) == 0 { + return clues.New("web URL of the site to restore is required. Use --" + flags.SiteFN + " to provide one.") + } else if len(opts.WebURL)+len(opts.SiteID) > 1 { + return clues.New("only a single site can be selected for restore") + } + } + if _, ok := opts.Populated[flags.FileCreatedAfterFN]; ok && !IsValidTimeFormat(opts.FileCreatedAfter) { return clues.New("invalid time format for " + flags.FileCreatedAfterFN) } @@ -164,8 +177,6 @@ func IncludeGroupsRestoreDataSelectors(ctx context.Context, opts GroupsOpts) *se llf, lli = len(opts.ListFolder), len(opts.ListItem) lpf, lpi = len(opts.PageFolder), len(opts.Page) lg, lch, lm = len(opts.Groups), len(opts.Channels), len(opts.Messages) - // TODO(meain): handle sites once we add non-root site backup - // ls := len(opts.SiteID) ) if lg == 0 { @@ -259,6 +270,23 @@ func FilterGroupsRestoreInfoSelectors( sel *selectors.GroupsRestore, opts GroupsOpts, ) { + var site string + + if len(opts.SiteID) > 0 { + site = opts.SiteID[0] + } else { + // using else instead of else if so that it would have a hard + // fail in case we somehow miss checking this earlier + site = opts.WebURL[0] + } + + // sel.Site can accept both ID and URL and so irrespective of + // which flag the user uses, we can process both weburl and siteid + // Also since the url will start with `https://` in the data that + // we store and the id is a uuid, we can grantee that there will be + // no collisions. + AddGroupsFilter(sel, site, sel.Site) + AddGroupsFilter(sel, opts.Library, sel.Library) AddGroupsFilter(sel, opts.FileCreatedAfter, sel.CreatedAfter) AddGroupsFilter(sel, opts.FileCreatedBefore, sel.CreatedBefore) diff --git a/src/cli/utils/groups_test.go b/src/cli/utils/groups_test.go index 65585d97e..d6239b780 100644 --- a/src/cli/utils/groups_test.go +++ b/src/cli/utils/groups_test.go @@ -71,7 +71,6 @@ func (suite *GroupsUtilsSuite) TestIncludeGroupsRestoreDataSelectors() { opts: utils.GroupsOpts{ FileName: empty, FolderPath: containsOnly, - SiteID: empty, }, expectIncludeLen: 1, }, @@ -80,7 +79,6 @@ func (suite *GroupsUtilsSuite) TestIncludeGroupsRestoreDataSelectors() { opts: utils.GroupsOpts{ FileName: empty, FolderPath: prefixOnly, - SiteID: empty, }, expectIncludeLen: 1, }, @@ -89,7 +87,6 @@ func (suite *GroupsUtilsSuite) TestIncludeGroupsRestoreDataSelectors() { opts: utils.GroupsOpts{ FileName: empty, FolderPath: containsAndPrefix, - SiteID: empty, }, expectIncludeLen: 2, }, @@ -100,7 +97,6 @@ func (suite *GroupsUtilsSuite) TestIncludeGroupsRestoreDataSelectors() { FolderPath: empty, ListItem: empty, ListFolder: containsOnly, - SiteID: empty, }, expectIncludeLen: 1, }, @@ -123,7 +119,6 @@ func (suite *GroupsUtilsSuite) TestIncludeGroupsRestoreDataSelectors() { opts: utils.GroupsOpts{ FileName: empty, FolderPath: empty, - // SiteID: empty, // TODO(meain): Update once we support multiple sites }, expectIncludeLen: 2, }, @@ -132,7 +127,6 @@ func (suite *GroupsUtilsSuite) TestIncludeGroupsRestoreDataSelectors() { opts: utils.GroupsOpts{ FileName: empty, FolderPath: empty, - // SiteID: empty, // TODO(meain): update once we support multiple sites }, expectIncludeLen: 2, }, @@ -231,11 +225,29 @@ func (suite *GroupsUtilsSuite) TestValidateGroupsRestoreFlags() { expect assert.ErrorAssertionFunc }{ { - name: "no opts", + name: "just site", backupID: "id", - opts: utils.GroupsOpts{}, + opts: utils.GroupsOpts{WebURL: []string{"site"}}, // site is mandatory expect: assert.NoError, }, + { + name: "just siteid", + backupID: "id", + opts: utils.GroupsOpts{SiteID: []string{"site-id"}}, + expect: assert.NoError, + }, + { + name: "multiple sites", + backupID: "id", + opts: utils.GroupsOpts{SiteID: []string{"site-id1", "site-id2"}}, + expect: assert.Error, + }, + { + name: "site and siteid", + backupID: "id", + opts: utils.GroupsOpts{SiteID: []string{"site-id"}, WebURL: []string{"site"}}, + expect: assert.Error, + }, { name: "no backupID", backupID: "", @@ -246,6 +258,7 @@ func (suite *GroupsUtilsSuite) TestValidateGroupsRestoreFlags() { name: "all valid", backupID: "id", opts: utils.GroupsOpts{ + WebURL: []string{"site"}, FileCreatedAfter: dttm.Now(), FileCreatedBefore: dttm.Now(), FileModifiedAfter: dttm.Now(), @@ -362,7 +375,7 @@ func (suite *GroupsUtilsSuite) TestValidateGroupsRestoreFlags() { for _, test := range table { suite.Run(test.name, func() { t := suite.T() - test.expect(t, utils.ValidateGroupsRestoreFlags(test.backupID, test.opts)) + test.expect(t, utils.ValidateGroupsRestoreFlags(test.backupID, test.opts, true)) }) } } diff --git a/src/pkg/selectors/groups.go b/src/pkg/selectors/groups.go index e6399fbf1..6a2614149 100644 --- a/src/pkg/selectors/groups.go +++ b/src/pkg/selectors/groups.go @@ -261,6 +261,21 @@ func (s *groups) ChannelMessages(channels, messages []string, opts ...option) [] return scopes } +// Sites produces one or more Groups site scopes, where the site +// matches upon a given site by ID or URL. +// If any slice contains selectors.Any, that slice is reduced to [selectors.Any] +// If any slice contains selectors.None, that slice is reduced to [selectors.None] +// If any slice is empty, it defaults to [selectors.None] +func (s *groups) Site(site string) []GroupsScope { + return []GroupsScope{ + makeInfoScope[GroupsScope]( + GroupsLibraryItem, + GroupsInfoSite, + []string{site}, + filters.Equal), + } +} + // Library produces one or more Group library scopes, where the library // matches upon a given drive by ID or Name. In order to ensure library selection // this should always be embedded within the Filter() set; include(Library()) will @@ -519,6 +534,7 @@ const ( GroupsInfoLibraryItemModifiedBefore groupsCategory = "GroupsInfoLibraryItemModifiedBefore" // channel and drive selection + GroupsInfoSite groupsCategory = "GroupsInfoSite" GroupsInfoSiteLibraryDrive groupsCategory = "GroupsInfoSiteLibraryDrive" // data contained within details.ItemInfo @@ -562,7 +578,7 @@ func (c groupsCategory) leafCat() categorizer { GroupsInfoChannelMessageCreatedAfter, GroupsInfoChannelMessageCreatedBefore, GroupsInfoChannelMessageCreator, GroupsInfoChannelMessageLastReplyAfter, GroupsInfoChannelMessageLastReplyBefore: return GroupsChannelMessage - case GroupsLibraryFolder, GroupsLibraryItem, GroupsInfoSiteLibraryDrive, + case GroupsLibraryFolder, GroupsLibraryItem, GroupsInfoSite, GroupsInfoSiteLibraryDrive, GroupsInfoLibraryItemCreatedAfter, GroupsInfoLibraryItemCreatedBefore, GroupsInfoLibraryItemModifiedAfter, GroupsInfoLibraryItemModifiedBefore: return GroupsLibraryItem @@ -781,6 +797,18 @@ func (s GroupsScope) matchesInfo(dii details.ItemInfo) bool { } switch infoCat { + case GroupsInfoSite: + ds := []string{} + + if len(info.SiteID) > 0 { + ds = append(ds, info.SiteID) + } + + if len(info.WebURL) > 0 { + ds = append(ds, info.WebURL) + } + + return matchesAny(s, GroupsInfoSite, ds) case GroupsInfoSiteLibraryDrive: ds := []string{} diff --git a/src/pkg/selectors/groups_test.go b/src/pkg/selectors/groups_test.go index 709fdabd8..e2074354e 100644 --- a/src/pkg/selectors/groups_test.go +++ b/src/pkg/selectors/groups_test.go @@ -394,6 +394,8 @@ func (suite *GroupsSelectorSuite) TestGroupsScope_MatchesInfo() { {"file modified before epoch", dspl, user, sel.ModifiedBefore(dttm.Format(now)), assert.False}, {"in library", dspl, user, sel.Library("included-library"), assert.True}, {"not in library", dspl, user, sel.Library("not-included-library"), assert.False}, + {"site id", dspl, user, sel.Site("site1"), assert.True}, + {"web url", dspl, user, sel.Site(user), assert.True}, {"library id", dspl, user, sel.Library("1234"), assert.True}, {"not library id", dspl, user, sel.Library("abcd"), assert.False}, @@ -430,6 +432,7 @@ func (suite *GroupsSelectorSuite) TestGroupsScope_MatchesInfo() { LastReplyAt: mod, DriveName: "included-library", DriveID: "1234", + SiteID: "site1", }, } @@ -457,6 +460,7 @@ func (suite *GroupsSelectorSuite) TestCategory_PathType() { {GroupsLibraryFolder, path.LibrariesCategory}, {GroupsLibraryItem, path.LibrariesCategory}, {GroupsInfoSiteLibraryDrive, path.LibrariesCategory}, + {GroupsInfoSite, path.LibrariesCategory}, } for _, test := range table { suite.Run(test.cat.String(), func() {