From a915cd1dc14ec4793ab56d99b2b4ab887b130f4f Mon Sep 17 00:00:00 2001 From: ryanfkeepers Date: Tue, 30 Jan 2024 14:41:37 -0700 Subject: [PATCH] fix mailbox user not found handling --- src/internal/m365/service/exchange/enabled.go | 9 +- src/pkg/services/m365/api/graph/errors.go | 7 +- .../services/m365/api/graph/errors_test.go | 94 ++++++++++++++++++- src/pkg/services/m365/api/users.go | 20 ++-- src/pkg/services/m365/api/users_test.go | 57 +++++------ 5 files changed, 135 insertions(+), 52 deletions(-) diff --git a/src/internal/m365/service/exchange/enabled.go b/src/internal/m365/service/exchange/enabled.go index bd377acd3..d15d01bab 100644 --- a/src/internal/m365/service/exchange/enabled.go +++ b/src/internal/m365/service/exchange/enabled.go @@ -23,7 +23,8 @@ func IsServiceEnabled( ) (bool, error) { _, err := gmi.GetMailInbox(ctx, resource) if err != nil { - if err := api.EvaluateMailboxError(err); err != nil { + ignorable := api.IsMailboxErrorIgnorable(err) + if !ignorable { logger.CtxErr(ctx, err).Error("getting user's mail folder") return false, clues.Stack(err) } @@ -54,10 +55,10 @@ func GetMailboxInfo( // First check whether the user is able to access their inbox. inbox, err := gmb.GetMailInbox(ctx, userID) if err != nil { - if err := api.EvaluateMailboxError(clues.Stack(err)); err != nil { + ignorable := api.IsMailboxErrorIgnorable(clues.Stack(err)) + if !ignorable { logger.CtxErr(ctx, err).Error("getting user's mail folder") - - return mi, err + return mi, clues.Stack(err) } logger.Ctx(ctx).Info("resource owner does not have a mailbox enabled") diff --git a/src/pkg/services/m365/api/graph/errors.go b/src/pkg/services/m365/api/graph/errors.go index b3bd4b740..2c5ddb408 100644 --- a/src/pkg/services/m365/api/graph/errors.go +++ b/src/pkg/services/m365/api/graph/errors.go @@ -127,6 +127,8 @@ var ( // error categorization // --------------------------------------------------------------------------- +var ErrUserNotFound = clues.New("user not found") + func stackWithCoreErr(ctx context.Context, err error, traceDepth int) error { if err == nil { return nil @@ -141,7 +143,10 @@ func stackWithCoreErr(ctx context.Context, err error, traceDepth int) error { case isErrApplicationThrottled(ode, err): err = clues.Stack(core.ErrApplicationThrottled, err) case isErrUserNotFound(ode, err): - err = clues.Stack(core.ErrNotFound, err) + // stack both userNotFound and notFound to ensure some graph api + // internals can distinguish between the two cases (where possible). + // layers above the api should still handle only the core notFound. + err = clues.Stack(ErrUserNotFound, core.ErrNotFound, err) case isErrResourceLocked(ode, err): err = clues.Stack(core.ErrResourceNotAccessible, err) case isErrInsufficientAuthorization(ode, err): diff --git a/src/pkg/services/m365/api/graph/errors_test.go b/src/pkg/services/m365/api/graph/errors_test.go index 19b113a80..47c6b3904 100644 --- a/src/pkg/services/m365/api/graph/errors_test.go +++ b/src/pkg/services/m365/api/graph/errors_test.go @@ -1111,7 +1111,6 @@ func (suite *GraphErrorsUnitSuite) TestToErrByRespCode() { for _, test := range table { suite.Run(test.name, func() { t := suite.T() - err := toErrByRespCode(parseODataErr(test.err), test.err) if test.expectNoStack { @@ -1122,3 +1121,96 @@ func (suite *GraphErrorsUnitSuite) TestToErrByRespCode() { }) } } + +func (suite *GraphErrorsUnitSuite) TestIsErrItemAlreadyExists() { + table := []struct { + name string + err error + expect assert.BoolAssertionFunc + }{ + { + name: "nil", + err: nil, + expect: assert.False, + }, + { + name: "non-matching", + err: assert.AnError, + expect: assert.False, + }, + { + name: "non-matching oDataErr", + err: graphTD.ODataErrWithMsg("InvalidRequest", "item already exists"), + expect: assert.False, + }, + { + name: "matching oDataErr code", + err: graphTD.ODataInner(string(nameAlreadyExists)), + expect: assert.True, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + ode := parseODataErr(test.err) + test.expect(suite.T(), isErrItemAlreadyExists(ode, test.err)) + }) + } +} + +func (suite *GraphErrorsUnitSuite) TestStackWithCoreErr() { + table := []struct { + name string + err error + expect []error + }{ + { + name: "bad jwt", + err: graphTD.ODataErr(string(invalidAuthenticationToken)), + expect: []error{core.ErrAuthTokenExpired}, + }, + { + name: "throttled", + err: graphTD.ODataErr(string(ApplicationThrottled)), + expect: []error{core.ErrApplicationThrottled}, + }, + { + name: "user not found", + err: graphTD.ODataErrWithMsg(string(ResourceNotFound), "User not found"), + expect: []error{ErrUserNotFound, core.ErrNotFound}, + }, + { + name: "resource locked", + err: graphTD.ODataErr(string(NotAllowed)), + expect: []error{core.ErrResourceNotAccessible}, + }, + { + name: "insufficient auth", + err: graphTD.ODataErr(string(AuthorizationRequestDenied)), + expect: []error{core.ErrInsufficientAuthorization}, + }, + { + name: "already exists", + err: graphTD.ODataInner(string(nameAlreadyExists)), + expect: []error{core.ErrAlreadyExists}, + }, + { + name: "not found", + err: graphTD.ODataErr(string(ItemNotFound)), + expect: []error{core.ErrNotFound}, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + result := stackWithCoreErr(ctx, test.err, 1) + + for _, ex := range test.expect { + assert.ErrorIs(t, result, ex, clues.ToCore(result)) + } + }) + } +} diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index f664f3dc9..2f0a6e790 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -185,26 +185,18 @@ func appendIfErr(errs []error, err error) []error { return append(errs, err) } -// EvaluateMailboxError checks whether the provided error can be interpreted +// IsMailboxErrorIgnorable checks whether the provided error can be interpreted // as "user does not have a mailbox", or whether it is some other error. If // the former (no mailbox), returns nil, otherwise returns an error. -func EvaluateMailboxError(err error) error { - if err == nil { - return nil - } - +func IsMailboxErrorIgnorable(err error) bool { // must occur before MailFolderNotFound, due to overlapping cases. - if errors.Is(err, core.ErrResourceNotAccessible) { - return err + if errors.Is(err, core.ErrResourceNotAccessible) || errors.Is(err, graph.ErrUserNotFound) { + return false } - if errors.Is(err, core.ErrNotFound) || + return err != nil && (errors.Is(err, core.ErrNotFound) || graph.IsErrExchangeMailFolderNotFound(err) || - graph.IsErrAuthenticationError(err) { - return nil - } - - return err + graph.IsErrAuthenticationError(err)) } // IsAnyErrMailboxNotFound inspects the secondary errors inside MailboxInfo and diff --git a/src/pkg/services/m365/api/users_test.go b/src/pkg/services/m365/api/users_test.go index 388d150a4..48ff275b4 100644 --- a/src/pkg/services/m365/api/users_test.go +++ b/src/pkg/services/m365/api/users_test.go @@ -70,54 +70,47 @@ func (suite *UsersUnitSuite) TestEvaluateMailboxError() { table := []struct { name string err error - expect func(t *testing.T, err error) + expect assert.BoolAssertionFunc }{ { - name: "nil", - err: nil, - expect: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, + name: "nil", + err: nil, + expect: assert.False, }, { - name: "mail inbox err - user not found", - err: core.ErrNotFound, - expect: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, + name: "user not found - corso sentinel", + err: graph.ErrUserNotFound, + expect: assert.False, }, { - name: "mail inbox err - resourceLocked", - err: core.ErrResourceNotAccessible, - expect: func(t *testing.T, err error) { - assert.ErrorIs(t, err, core.ErrResourceNotAccessible, clues.ToCore(err)) - }, + name: "not found - corso sentinel", + err: core.ErrNotFound, + expect: assert.True, }, { - name: "mail inbox err - user not found", - err: graphTD.ODataErr(string(graph.MailboxNotEnabledForRESTAPI)), - expect: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, + name: "mailbox not enabled - graph api error", + err: graphTD.ODataErr(string(graph.MailboxNotEnabledForRESTAPI)), + expect: assert.True, }, { - name: "mail inbox err - authenticationError", - err: graphTD.ODataErr(string(graph.AuthenticationError)), - expect: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, + name: "resourceLocked", + err: core.ErrResourceNotAccessible, + expect: assert.False, }, { - name: "mail inbox err - other error", - err: graphTD.ODataErrWithMsg("somecode", "somemessage"), - expect: func(t *testing.T, err error) { - assert.Error(t, err, clues.ToCore(err)) - }, + name: "authenticationError", + err: graphTD.ODataErr(string(graph.AuthenticationError)), + expect: assert.True, + }, + { + name: "other error", + err: graphTD.ODataErrWithMsg("somecode", "somemessage"), + expect: assert.False, }, } for _, test := range table { suite.Run(test.name, func() { - test.expect(suite.T(), EvaluateMailboxError(test.err)) + test.expect(suite.T(), IsMailboxErrorIgnorable(test.err)) }) } }