From 4f9793a8242bfa5f66a94c1a57feaa0797b3d777 Mon Sep 17 00:00:00 2001 From: ashmrtn <3891298+ashmrtn@users.noreply.github.com> Date: Tue, 27 Jun 2023 11:59:33 -0700 Subject: [PATCH] Handle broader set of user not found error codes (#3678) graph has been inconsistent about what error code is returned when the user doesn't exist. Expand the set of things we check so that hopefully we'll get them all Always return an error if the user doesn't exist as determined by checking for exchange folders during GetInfo calls Also add a little more stack trace info to returned errors --- #### Does this PR need a docs update or release note? - [ ] :white_check_mark: Yes, it's included - [ ] :clock1: Yes, but in a later PR - [x] :no_entry: No #### Type of change - [ ] :sunflower: Feature - [x] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [ ] :broom: Tech Debt/Cleanup #### Issue(s) * fixes #3672 #### Test Plan - [x] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/m365/discovery/discovery_test.go | 2 +- src/internal/m365/graph/errors.go | 31 +++++++++++++++---- src/internal/m365/graph/errors_test.go | 26 ++++++++++++++++ src/pkg/services/m365/api/users.go | 9 ++++-- 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/src/internal/m365/discovery/discovery_test.go b/src/internal/m365/discovery/discovery_test.go index f8648cd84..f4ae315f3 100644 --- a/src/internal/m365/discovery/discovery_test.go +++ b/src/internal/m365/discovery/discovery_test.go @@ -214,7 +214,7 @@ func (suite *DiscoveryIntgSuite) TestUserInfo() { ServicesEnabled: map[path.ServiceType]struct{}{}, Mailbox: api.MailboxInfo{}, }, - expectErr: require.NoError, + expectErr: require.Error, }, } for _, test := range table { diff --git a/src/internal/m365/graph/errors.go b/src/internal/m365/graph/errors.go index a6a0c7fe1..5bd5cc2bd 100644 --- a/src/internal/m365/graph/errors.go +++ b/src/internal/m365/graph/errors.go @@ -38,11 +38,15 @@ const ( nameAlreadyExists errorCode = "nameAlreadyExists" quotaExceeded errorCode = "ErrorQuotaExceeded" RequestResourceNotFound errorCode = "Request_ResourceNotFound" - resourceNotFound errorCode = "ResourceNotFound" - resyncRequired errorCode = "ResyncRequired" // alt: resyncRequired - syncFolderNotFound errorCode = "ErrorSyncFolderNotFound" - syncStateInvalid errorCode = "SyncStateInvalid" - syncStateNotFound errorCode = "SyncStateNotFound" + // Returned when we try to get the inbox of a user that doesn't exist. + resourceNotFound errorCode = "ResourceNotFound" + // Some datacenters are returning this when we try to get the inbox of a user + // that doesn't exist. + invalidUser errorCode = "ErrorInvalidUser" + resyncRequired errorCode = "ResyncRequired" + syncFolderNotFound errorCode = "ErrorSyncFolderNotFound" + syncStateInvalid errorCode = "SyncStateInvalid" + syncStateNotFound errorCode = "SyncStateNotFound" // This error occurs when an attempt is made to create a folder that has // the same name as another folder in the same parent. Such duplicate folder // names are not allowed by graph. @@ -131,7 +135,22 @@ func IsErrExchangeMailFolderNotFound(err error) bool { } func IsErrUserNotFound(err error) bool { - return hasErrorCode(err, RequestResourceNotFound) + if hasErrorCode(err, RequestResourceNotFound, invalidUser) { + return true + } + + if hasErrorCode(err, resourceNotFound) { + var odErr odataerrors.ODataErrorable + if !errors.As(err, &odErr) { + return false + } + + mainMsg, _, _ := errData(odErr) + + return strings.Contains(strings.ToLower(mainMsg), "user") + } + + return false } func IsErrResourceNotFound(err error) bool { diff --git a/src/internal/m365/graph/errors_test.go b/src/internal/m365/graph/errors_test.go index 7a415c8c4..ffc1b85d6 100644 --- a/src/internal/m365/graph/errors_test.go +++ b/src/internal/m365/graph/errors_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/suite" + "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/pkg/fault" ) @@ -231,11 +232,36 @@ func (suite *GraphErrorsUnitSuite) TestIsErrUserNotFound() { err: odErr("fnords"), expect: assert.False, }, + { + name: "non-matching resource not found", + err: func() error { + res := odErr(string(resourceNotFound)) + res.GetError().SetMessage(ptr.To("Calendar not found")) + + return res + }(), + expect: assert.False, + }, { name: "request resource not found oDataErr", err: odErr(string(RequestResourceNotFound)), expect: assert.True, }, + { + name: "invalid user oDataErr", + err: odErr(string(invalidUser)), + expect: assert.True, + }, + { + name: "resource not found oDataErr", + err: func() error { + res := odErr(string(resourceNotFound)) + res.GetError().SetMessage(ptr.To("User not found")) + + return res + }(), + expect: assert.True, + }, } for _, test := range table { suite.Run(test.name, func() { diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index 286cc52ff..1f21384be 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -200,12 +200,14 @@ func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { if graph.IsErrUserNotFound(err) { logger.CtxErr(ctx, err).Error("user not found") - return nil, clues.Stack(graph.ErrResourceOwnerNotFound, err) + // FIXME(ashmrtn): This shouldn't need clues.Wrap. + return nil, clues.Wrap(clues.Stack(graph.ErrResourceOwnerNotFound, err), "") } if !graph.IsErrExchangeMailFolderNotFound(err) { logger.CtxErr(ctx, err).Error("getting user's mail folder") - return nil, err + // FIXME(ashmrtn): This should use clues.Stack. + return nil, clues.Wrap(err, "") } logger.Ctx(ctx).Info("resource owner does not have a mailbox enabled") @@ -243,7 +245,8 @@ func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { err = c.getFirstInboxMessage(ctx, userID, ptr.Val(inbx.GetId())) if err != nil { if !graph.IsErrQuotaExceeded(err) { - return nil, err + // FIXME(ashmrtn): This should use clues.Stack. + return nil, clues.Wrap(err, "") } userInfo.Mailbox.QuotaExceeded = graph.IsErrQuotaExceeded(err)