From 9bedee9c72ca094941ee1f5c49e624503227f72c Mon Sep 17 00:00:00 2001 From: Abhishek Pandey Date: Fri, 8 Sep 2023 13:28:38 +0530 Subject: [PATCH] Wire service isolation changes into SDK code (#4186) Wires `IsServiceEnabled` code into `UserHasMailbox`, `UserHasDrive`, and `UserGetMailboxInfo` SDK APIs. --- #### 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 - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Supportability/Tests - [ ] :computer: CI/Deployment - [x] :broom: Tech Debt/Cleanup #### Issue(s) * https://github.com/alcionai/corso/issues/3844 #### Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [x] :green_heart: E2E --- src/internal/m365/service/exchange/enabled.go | 2 +- .../m365/service/exchange/enabled_test.go | 2 +- src/pkg/services/m365/api/users.go | 17 +- src/pkg/services/m365/api/users_test.go | 37 +++ src/pkg/services/m365/m365.go | 5 - src/pkg/services/m365/users.go | 42 +-- src/pkg/services/m365/users_test.go | 258 +++++++----------- 7 files changed, 168 insertions(+), 195 deletions(-) diff --git a/src/internal/m365/service/exchange/enabled.go b/src/internal/m365/service/exchange/enabled.go index d7a330a28..e4fcede35 100644 --- a/src/internal/m365/service/exchange/enabled.go +++ b/src/internal/m365/service/exchange/enabled.go @@ -64,7 +64,7 @@ func GetMailboxInfo( mi.ErrGetMailBoxSetting = append( mi.ErrGetMailBoxSetting, - api.ErrMailBoxSettingsNotFound) + api.ErrMailBoxNotFound) return mi, nil } diff --git a/src/internal/m365/service/exchange/enabled_test.go b/src/internal/m365/service/exchange/enabled_test.go index 279637733..33798994b 100644 --- a/src/internal/m365/service/exchange/enabled_test.go +++ b/src/internal/m365/service/exchange/enabled_test.go @@ -189,7 +189,7 @@ func (suite *EnabledUnitSuite) TestGetMailboxInfo() { mi := api.MailboxInfo{} mi.ErrGetMailBoxSetting = append( mi.ErrGetMailBoxSetting, - api.ErrMailBoxSettingsNotFound) + api.ErrMailBoxNotFound) return mi }, diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index f20b9dfc1..18437b8e7 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -9,6 +9,7 @@ import ( msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/microsoftgraph/msgraph-sdk-go/users" + "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/common/idname" "github.com/alcionai/corso/src/internal/common/ptr" @@ -20,7 +21,7 @@ import ( // Variables var ( - ErrMailBoxSettingsNotFound = clues.New("mailbox settings not found") + ErrMailBoxNotFound = clues.New("mailbox not found") ErrMailBoxSettingsAccessDenied = clues.New("mailbox settings access denied") ) @@ -215,7 +216,7 @@ func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { } if !mailFolderFound { - mi.ErrGetMailBoxSetting = append(mi.ErrGetMailBoxSetting, ErrMailBoxSettingsNotFound) + mi.ErrGetMailBoxSetting = append(mi.ErrGetMailBoxSetting, ErrMailBoxNotFound) userInfo.Mailbox = mi return userInfo, nil @@ -268,6 +269,18 @@ func EvaluateMailboxError(err error) error { return err } +// IsAnyErrMailboxNotFound inspects the secondary errors inside MailboxInfo and +// determines whether the resource has a mailbox. +func IsAnyErrMailboxNotFound(errs []error) bool { + for _, err := range errs { + if errors.Is(err, ErrMailBoxNotFound) { + return true + } + } + + return false +} + func (c Users) GetMailboxSettings( ctx context.Context, userID string, diff --git a/src/pkg/services/m365/api/users_test.go b/src/pkg/services/m365/api/users_test.go index 7ce620ebf..aee817413 100644 --- a/src/pkg/services/m365/api/users_test.go +++ b/src/pkg/services/m365/api/users_test.go @@ -117,6 +117,43 @@ func (suite *UsersUnitSuite) TestEvaluateMailboxError() { } } +func (suite *UsersUnitSuite) TestIsAnyErrMailboxNotFound() { + table := []struct { + name string + errs []error + expect bool + }{ + { + name: "no errors", + errs: nil, + expect: false, + }, + { + name: "mailbox not found error", + errs: []error{ + clues.New("an error"), + api.ErrMailBoxNotFound, + clues.New("an error"), + }, + expect: true, + }, + { + name: "other errors", + errs: []error{ + clues.New("an error"), + api.ErrMailBoxSettingsAccessDenied, + clues.New("an error"), + }, + expect: false, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + assert.Equal(suite.T(), test.expect, api.IsAnyErrMailboxNotFound(test.errs)) + }) + } +} + type UsersIntgSuite struct { tester.Suite its intgTesterSetup diff --git a/src/pkg/services/m365/m365.go b/src/pkg/services/m365/m365.go index 6bb8125c4..48d08980b 100644 --- a/src/pkg/services/m365/m365.go +++ b/src/pkg/services/m365/m365.go @@ -4,7 +4,6 @@ import ( "context" "github.com/alcionai/clues" - "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/control" @@ -17,10 +16,6 @@ import ( // interfaces & structs // --------------------------------------------------------------------------- -type getDefaultDriver interface { - GetDefaultDrive(ctx context.Context, userID string) (models.Driveable, error) -} - type getAller[T any] interface { GetAll(ctx context.Context, errs *fault.Bus) ([]T, error) } diff --git a/src/pkg/services/m365/users.go b/src/pkg/services/m365/users.go index ee3d99336..8a3935c25 100644 --- a/src/pkg/services/m365/users.go +++ b/src/pkg/services/m365/users.go @@ -7,7 +7,8 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/alcionai/corso/src/internal/common/ptr" - "github.com/alcionai/corso/src/internal/m365/graph" + "github.com/alcionai/corso/src/internal/m365/service/exchange" + "github.com/alcionai/corso/src/internal/m365/service/onedrive" "github.com/alcionai/corso/src/pkg/account" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" @@ -66,16 +67,20 @@ func UserHasMailbox(ctx context.Context, acct account.Account, userID string) (b return false, clues.Stack(err).WithClues(ctx) } - _, err = ac.Users().GetMailInbox(ctx, userID) - if err != nil { - if err := api.EvaluateMailboxError(err); err != nil { - return false, clues.Stack(err) - } + return exchange.IsServiceEnabled(ctx, ac.Users(), userID) +} - return false, nil +func UserGetMailboxInfo( + ctx context.Context, + acct account.Account, + userID string, +) (api.MailboxInfo, error) { + ac, err := makeAC(ctx, acct, path.ExchangeService) + if err != nil { + return api.MailboxInfo{}, clues.Stack(err).WithClues(ctx) } - return true, nil + return exchange.GetMailboxInfo(ctx, ac.Users(), userID) } // UserHasDrives returns true if the user has any drives @@ -86,26 +91,7 @@ func UserHasDrives(ctx context.Context, acct account.Account, userID string) (bo return false, clues.Stack(err).WithClues(ctx) } - return checkUserHasDrives(ctx, ac.Users(), userID) -} - -func checkUserHasDrives(ctx context.Context, dgdd getDefaultDriver, userID string) (bool, error) { - _, err := dgdd.GetDefaultDrive(ctx, userID) - if err != nil { - // we consider this a non-error case, since it - // answers the question the caller is asking. - if clues.HasLabel(err, graph.LabelsMysiteNotFound) || clues.HasLabel(err, graph.LabelsNoSharePointLicense) { - return false, nil - } - - if graph.IsErrUserNotFound(err) { - return false, clues.Stack(graph.ErrResourceOwnerNotFound, err) - } - - return false, clues.Stack(err) - } - - return true, nil + return onedrive.IsServiceEnabled(ctx, ac.Users(), userID) } // usersNoInfo returns a list of users in the specified M365 tenant - with no info diff --git a/src/pkg/services/m365/users_test.go b/src/pkg/services/m365/users_test.go index 78a27e111..344b9be7b 100644 --- a/src/pkg/services/m365/users_test.go +++ b/src/pkg/services/m365/users_test.go @@ -1,18 +1,14 @@ package m365 import ( - "context" "testing" "github.com/alcionai/clues" "github.com/google/uuid" - "github.com/microsoftgraph/msgraph-sdk-go/models" - "github.com/microsoftgraph/msgraph-sdk-go/models/odataerrors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" - "github.com/alcionai/corso/src/internal/common/ptr" "github.com/alcionai/corso/src/internal/m365/graph" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/internal/tester/tconfig" @@ -94,34 +90,115 @@ func (suite *userIntegrationSuite) TestUsersCompat_HasNoInfo() { func (suite *userIntegrationSuite) TestUserHasMailbox() { t := suite.T() + acct := tconfig.NewM365Account(t) + userID := tconfig.M365UserID(t) - ctx, flush := tester.NewContext(t) - defer flush() + table := []struct { + name string + user string + expect bool + }{ + { + name: "user with no mailbox", + user: "a53c26f7-5100-4acb-a910-4d20960b2c19", // User: testevents@10rqc2.onmicrosoft.com + expect: false, + }, + { + name: "user with mailbox", + user: userID, + expect: true, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() - var ( - acct = tconfig.NewM365Account(t) - uid = tconfig.M365UserID(t) - ) + ctx, flush := tester.NewContext(t) + defer flush() - enabled, err := UserHasMailbox(ctx, acct, uid) - require.NoError(t, err, clues.ToCore(err)) - assert.True(t, enabled) + enabled, err := UserHasMailbox(ctx, acct, test.user) + require.NoError(t, err, clues.ToCore(err)) + assert.Equal(t, test.expect, enabled) + }) + } } func (suite *userIntegrationSuite) TestUserHasDrive() { t := suite.T() + acct := tconfig.NewM365Account(t) + userID := tconfig.M365UserID(t) - ctx, flush := tester.NewContext(t) - defer flush() + table := []struct { + name string + user string + expect bool + }{ + { + name: "user without drive", + user: "a53c26f7-5100-4acb-a910-4d20960b2c19", // User: testevents@10rqc2.onmicrosoft.com + expect: false, + }, + { + name: "user with drive", + user: userID, + expect: true, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() - var ( - acct = tconfig.NewM365Account(t) - uid = tconfig.M365UserID(t) - ) + ctx, flush := tester.NewContext(t) + defer flush() - enabled, err := UserHasDrives(ctx, acct, uid) - require.NoError(t, err, clues.ToCore(err)) - assert.True(t, enabled) + enabled, err := UserHasDrives(ctx, acct, test.user) + require.NoError(t, err, clues.ToCore(err)) + assert.Equal(t, test.expect, enabled) + }) + } +} + +func (suite *userIntegrationSuite) TestUserGetMailboxInfo() { + t := suite.T() + acct := tconfig.NewM365Account(t) + + table := []struct { + name string + user string + expect func(t *testing.T, info api.MailboxInfo) + expectErr require.ErrorAssertionFunc + }{ + { + name: "shared mailbox", + user: "bb1a2049-3fc1-4fdc-93b8-7a14f63dd0db", // User: neha-test-shared-mailbox@10rqc2.onmicrosoft.com + expect: func(t *testing.T, info api.MailboxInfo) { + require.NotNil(t, info) + assert.Equal(t, "shared", info.Purpose) + }, + expectErr: require.NoError, + }, + { + name: "user with no mailbox", + user: "a53c26f7-5100-4acb-a910-4d20960b2c19", // User: testevents@10rqc2.onmicrosoft.com + expect: func(t *testing.T, info api.MailboxInfo) { + require.NotNil(t, info) + assert.Contains(t, info.ErrGetMailBoxSetting, api.ErrMailBoxNotFound) + }, + expectErr: require.NoError, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + info, err := UserGetMailboxInfo(ctx, acct, test.user) + test.expectErr(t, err, clues.ToCore(err)) + test.expect(t, info) + }) + } } func (suite *userIntegrationSuite) TestUsers_InvalidCredentials() { @@ -228,7 +305,7 @@ func (suite *userIntegrationSuite) TestGetUserInfo_userWithoutDrive() { expect: &api.UserInfo{ ServicesEnabled: map[path.ServiceType]struct{}{}, Mailbox: api.MailboxInfo{ - ErrGetMailBoxSetting: []error{api.ErrMailBoxSettingsNotFound}, + ErrGetMailBoxSetting: []error{api.ErrMailBoxNotFound}, }, }, }, @@ -262,138 +339,3 @@ func (suite *userIntegrationSuite) TestGetUserInfo_userWithoutDrive() { }) } } - -// --------------------------------------------------------------------------- -// Unit -// --------------------------------------------------------------------------- - -type userUnitSuite struct { - tester.Suite -} - -func TestUserUnitSuite(t *testing.T) { - suite.Run(t, &userUnitSuite{Suite: tester.NewUnitSuite(t)}) -} - -type mockDGDD struct { - response models.Driveable - err error -} - -func (m mockDGDD) GetDefaultDrive(context.Context, string) (models.Driveable, error) { - return m.response, m.err -} - -func (suite *userUnitSuite) TestCheckUserHasDrives() { - table := []struct { - name string - mock func(context.Context) getDefaultDriver - expect assert.BoolAssertionFunc - expectErr func(*testing.T, error) - }{ - { - name: "ok", - mock: func(ctx context.Context) getDefaultDriver { - return mockDGDD{models.NewDrive(), nil} - }, - expect: assert.True, - expectErr: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, - }, - { - name: "mysite not found", - mock: func(ctx context.Context) getDefaultDriver { - odErr := odataerrors.NewODataError() - merr := odataerrors.NewMainError() - merr.SetCode(ptr.To("code")) - merr.SetMessage(ptr.To(string(graph.MysiteNotFound))) - odErr.SetErrorEscaped(merr) - - return mockDGDD{nil, graph.Stack(ctx, odErr)} - }, - expect: assert.False, - expectErr: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, - }, - { - name: "mysite URL not found", - mock: func(ctx context.Context) getDefaultDriver { - odErr := odataerrors.NewODataError() - merr := odataerrors.NewMainError() - merr.SetCode(ptr.To("code")) - merr.SetMessage(ptr.To(string(graph.MysiteURLNotFound))) - odErr.SetErrorEscaped(merr) - - return mockDGDD{nil, graph.Stack(ctx, odErr)} - }, - expect: assert.False, - expectErr: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, - }, - { - name: "no sharepoint license", - mock: func(ctx context.Context) getDefaultDriver { - odErr := odataerrors.NewODataError() - merr := odataerrors.NewMainError() - merr.SetCode(ptr.To("code")) - merr.SetMessage(ptr.To(string(graph.NoSPLicense))) - odErr.SetErrorEscaped(merr) - - return mockDGDD{nil, graph.Stack(ctx, odErr)} - }, - expect: assert.False, - expectErr: func(t *testing.T, err error) { - assert.NoError(t, err, clues.ToCore(err)) - }, - }, - { - name: "user not found", - mock: func(ctx context.Context) getDefaultDriver { - odErr := odataerrors.NewODataError() - merr := odataerrors.NewMainError() - merr.SetCode(ptr.To(string(graph.RequestResourceNotFound))) - merr.SetMessage(ptr.To("message")) - odErr.SetErrorEscaped(merr) - - return mockDGDD{nil, graph.Stack(ctx, odErr)} - }, - expect: assert.False, - expectErr: func(t *testing.T, err error) { - assert.Error(t, err, clues.ToCore(err)) - }, - }, - { - name: "arbitrary error", - mock: func(ctx context.Context) getDefaultDriver { - odErr := odataerrors.NewODataError() - merr := odataerrors.NewMainError() - merr.SetCode(ptr.To("code")) - merr.SetMessage(ptr.To("message")) - odErr.SetErrorEscaped(merr) - - return mockDGDD{nil, graph.Stack(ctx, odErr)} - }, - expect: assert.False, - expectErr: func(t *testing.T, err error) { - assert.Error(t, err, clues.ToCore(err)) - }, - }, - } - for _, test := range table { - suite.Run(test.name, func() { - t := suite.T() - - ctx, flush := tester.NewContext(t) - defer flush() - - dgdd := test.mock(ctx) - - ok, err := checkUserHasDrives(ctx, dgdd, "foo") - test.expect(t, ok, "has drives flag") - test.expectErr(t, err) - }) - } -}