fix mailbox user not found handling

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, clues.Stack(err)
-			return mi, err
 		}
 
 		logger.Ctx(ctx).Info("resource owner does not have a mailbox enabled")

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):

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))
+			}
+		})
+	}
+}

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 {
+func IsMailboxErrorIgnorable(err error) bool {
-	if err == nil {
-		return nil
-	}
-
 	// must occur before MailFolderNotFound, due to overlapping cases.
-	if errors.Is(err, core.ErrResourceNotAccessible) {
+	if errors.Is(err, core.ErrResourceNotAccessible) || errors.Is(err, graph.ErrUserNotFound) {
-		return err
+		return false
 	}
 
-	if errors.Is(err, core.ErrNotFound) ||
+	return err != nil && (errors.Is(err, core.ErrNotFound) ||
 		graph.IsErrExchangeMailFolderNotFound(err) ||
-		graph.IsErrAuthenticationError(err) {
+		graph.IsErrAuthenticationError(err))
-		return nil
-	}
-
-	return err
 }
 
 // IsAnyErrMailboxNotFound inspects the secondary errors inside MailboxInfo and

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",
+			name:   "nil",
-			err:  nil,
+			err:    nil,
-			expect: func(t *testing.T, err error) {
+			expect: assert.False,
-				assert.NoError(t, err, clues.ToCore(err))
-			},
 		},
 		{
-			name: "mail inbox err - user not found",
+			name:   "user not found - corso sentinel",
-			err:  core.ErrNotFound,
+			err:    graph.ErrUserNotFound,
-			expect: func(t *testing.T, err error) {
+			expect: assert.False,
-				assert.NoError(t, err, clues.ToCore(err))
-			},
 		},
 		{
-			name: "mail inbox err - resourceLocked",
+			name:   "not found - corso sentinel",
-			err:  core.ErrResourceNotAccessible,
+			err:    core.ErrNotFound,
-			expect: func(t *testing.T, err error) {
+			expect: assert.True,
-				assert.ErrorIs(t, err, core.ErrResourceNotAccessible, clues.ToCore(err))
-			},
 		},
 		{
-			name: "mail inbox err - user not found",
+			name:   "mailbox not enabled - graph api error",
-			err:  graphTD.ODataErr(string(graph.MailboxNotEnabledForRESTAPI)),
+			err:    graphTD.ODataErr(string(graph.MailboxNotEnabledForRESTAPI)),
-			expect: func(t *testing.T, err error) {
+			expect: assert.True,
-				assert.NoError(t, err, clues.ToCore(err))
-			},
 		},
 		{
-			name: "mail inbox err - authenticationError",
+			name:   "resourceLocked",
-			err:  graphTD.ODataErr(string(graph.AuthenticationError)),
+			err:    core.ErrResourceNotAccessible,
-			expect: func(t *testing.T, err error) {
+			expect: assert.False,
-				assert.NoError(t, err, clues.ToCore(err))
-			},
 		},
 		{
-			name: "mail inbox err - other error",
+			name:   "authenticationError",
-			err:  graphTD.ODataErrWithMsg("somecode", "somemessage"),
+			err:    graphTD.ODataErr(string(graph.AuthenticationError)),
-			expect: func(t *testing.T, err error) {
+			expect: assert.True,
-				assert.Error(t, err, clues.ToCore(err))
+		},
-			},
+		{
+			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))
 		})
 	}
 }