From c407226c05c32ae6f0e2b89ae10d0c297b1634cb Mon Sep 17 00:00:00 2001 From: Idan Varsano Date: Wed, 24 Aug 2022 15:26:12 -0400 Subject: [PATCH] Repo Sync: If all errors are warnings, delete repos with bad permissions (#40690) * Prevent stopping sync if a querying for repos in Bitbucket project key returns a 'fatal' error --- internal/repos/syncer.go | 33 +++++++++++--- internal/repos/syncer_test.go | 84 +++++++++++++++++++++++++++++++++++ lib/errors/warning.go | 5 +++ 3 files changed, 117 insertions(+), 5 deletions(-) diff --git a/internal/repos/syncer.go b/internal/repos/syncer.go index a45e5c0070d..8576a32fdbf 100644 --- a/internal/repos/syncer.go +++ b/internal/repos/syncer.go @@ -636,7 +636,6 @@ func (s *Syncer) SyncExternalService( continue } - for _, r := range diff.Repos() { seen[r.ID] = struct{}{} } @@ -661,7 +660,7 @@ func (s *Syncer) SyncExternalService( } // We don't delete any repos of site-level external services if there were any - // errors during a sync. + // non-warning errors during a sync. // // Only user or organization external services will delete // repos in a sync run with fatal errors. @@ -669,14 +668,38 @@ func (s *Syncer) SyncExternalService( // Site-level external services can own lots of repos and are managed by site admins. // It's preferable to have them fix any invalidated token manually rather than deleting the repos automatically. deleted := 0 - if errs == nil || (!svc.IsSiteOwned() && fatal(errs)) { + + // If all of our errors are warnings and either Forbidden or Unauthorized, + // we want to proceed with the deletion. This is to be able to properly sync + // repos (by removing ones if code-host permissions have changed). + abortDeletion := false + if errs != nil { + var ref errors.MultiError + if errors.As(errs, &ref) { + for _, e := range ref.Errors() { + if errors.IsWarning(e) { + baseError := errors.Unwrap(e) + if !errcode.IsForbidden(baseError) && !errcode.IsUnauthorized(baseError) { + abortDeletion = true + break + } + continue + } + abortDeletion = true + break + } + } + } + + if !abortDeletion || (!svc.IsSiteOwned() && fatal(errs)) { // Remove associations and any repos that are no longer associated with any // external service. // // We don't want to delete all repos that weren't seen if we had a lot of // spurious errors since that could cause lots of repos to be deleted, only to be - // added the next sync. We delete only if we had no errors or we had one of the - // fatal errors. + // added the next sync. We delete only if we had no errors, + // or all of our errors are warnings and either Forbidden or Unauthorized, + // or we had one of the fatal errors and the service is not site owned. var deletedErr error deleted, deletedErr = s.delete(ctx, svc, seen) if deletedErr != nil { diff --git a/internal/repos/syncer_test.go b/internal/repos/syncer_test.go index cb46fa121eb..e328f158034 100644 --- a/internal/repos/syncer_test.go +++ b/internal/repos/syncer_test.go @@ -268,6 +268,31 @@ func testSyncerSync(s repos.Store) func(*testing.T) { svcs: []*types.ExternalService{tc.svc}, err: "bad credentials", }, + testCase{ + // If the source is unauthorized with a warning rather than an error, + // the sync will continue. If the warning error is unauthorized, the + // corresponding repos will be deleted as it's seen as permissions changes. + name: string(tc.repo.Name) + "/unauthorized-with-warning", + sourcer: repos.NewFakeSourcer(nil, + repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(&repos.ErrUnauthorized{})), + ), + store: s, + stored: types.Repos{tc.repo.With( + typestest.Opt.RepoSources(tc.svc.URN()), + )}, + now: clock.Now, + diff: repos.Diff{ + Deleted: types.Repos{ + tc.repo.With(func(r *types.Repo) { + r.Sources = map[string]*types.SourceInfo{} + r.DeletedAt = clock.Time(0) + r.UpdatedAt = clock.Time(0) + }), + }, + }, + svcs: []*types.ExternalService{tc.svc}, + err: "bad credentials", + }, testCase{ // If the source is forbidden we should treat this as if zero repos were returned // as it indicates that the source no longer has access to its repos @@ -286,6 +311,31 @@ func testSyncerSync(s repos.Store) func(*testing.T) { svcs: []*types.ExternalService{tc.svc}, err: "forbidden", }, + testCase{ + // If the source is forbidden with a warning rather than an error, + // the sync will continue. If the warning error is forbidden, the + // corresponding repos will be deleted as it's seen as permissions changes. + name: string(tc.repo.Name) + "/forbidden-with-warning", + sourcer: repos.NewFakeSourcer(nil, + repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(&repos.ErrForbidden{})), + ), + store: s, + stored: types.Repos{tc.repo.With( + typestest.Opt.RepoSources(tc.svc.URN()), + )}, + now: clock.Now, + diff: repos.Diff{ + Deleted: types.Repos{ + tc.repo.With(func(r *types.Repo) { + r.Sources = map[string]*types.SourceInfo{} + r.DeletedAt = clock.Time(0) + r.UpdatedAt = clock.Time(0) + }), + }, + }, + svcs: []*types.ExternalService{tc.svc}, + err: "forbidden", + }, testCase{ // If the source account has been suspended we should treat this as if zero repos were returned as it indicates // that the source no longer has access to its repos @@ -304,6 +354,22 @@ func testSyncerSync(s repos.Store) func(*testing.T) { svcs: []*types.ExternalService{tc.svc}, err: "account suspended", }, + testCase{ + // If the source is account suspended with a warning rather than an error, + // the sync will terminate. This is the only warning error that the sync will abort + name: string(tc.repo.Name) + "/accountsuspended-with-warning", + sourcer: repos.NewFakeSourcer(nil, + repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(&repos.ErrAccountSuspended{})), + ), + store: s, + stored: types.Repos{tc.repo.With( + typestest.Opt.RepoSources(tc.svc.URN()), + )}, + now: clock.Now, + diff: diff, + svcs: []*types.ExternalService{tc.svc}, + err: "account suspended", + }, testCase{ // Test that spurious errors don't cause deletions. name: string(tc.repo.Name) + "/spurious-error", @@ -321,6 +387,24 @@ func testSyncerSync(s repos.Store) func(*testing.T) { svcs: []*types.ExternalService{tc.svc}, err: io.EOF.Error(), }, + testCase{ + // If the source is a spurious error with a warning rather than an error, + // the sync will continue. However, no repos will be deleted. + name: string(tc.repo.Name) + "/spurious-error-with-warning", + sourcer: repos.NewFakeSourcer(nil, + repos.NewFakeSource(tc.svc.Clone(), errors.NewWarningError(io.EOF)), + ), + store: s, + stored: types.Repos{tc.repo.With( + typestest.Opt.RepoSources(tc.svc.URN()), + )}, + now: clock.Now, + diff: repos.Diff{Unmodified: types.Repos{tc.repo.With( + typestest.Opt.RepoSources(tc.svc.URN()), + )}}, + svcs: []*types.ExternalService{tc.svc}, + err: io.EOF.Error(), + }, testCase{ // It's expected that there could be multiple stored sources but only one will ever be returned // by the code host as it can't know about others. diff --git a/lib/errors/warning.go b/lib/errors/warning.go index 61d36a15a92..3972affc0b0 100644 --- a/lib/errors/warning.go +++ b/lib/errors/warning.go @@ -69,6 +69,11 @@ func (w *warning) IsWarning() bool { return true } +// Unwrap returns the underlying error of the warning. +func (w *warning) Unwrap() error { + return w.error +} + // As will return true if the target is of type warning. // // However, this method is not invoked when `errors.As` is invoked. See note in the docstring of the