diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a9ddfde5..c6f5693f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Handle OLE conversion errors when trying to fetch attachments - Fix uploading large attachments for emails and calendar +- Return a ServiceNotEnabled error when a tenant has no active SharePoint license. ## [v0.9.0] (beta) - 2023-06-05 diff --git a/src/internal/m365/discovery/discovery.go b/src/internal/m365/discovery/discovery.go index cba4a25a7..fd6d073e1 100644 --- a/src/internal/m365/discovery/discovery.go +++ b/src/internal/m365/discovery/discovery.go @@ -29,6 +29,10 @@ type getWithInfoer interface { GetInfoer } +type GetDefaultDriver interface { + GetDefaultDrive(ctx context.Context, userID string) (models.Driveable, error) +} + type getAller interface { GetAll(ctx context.Context, errs *fault.Bus) ([]models.Userable, error) } diff --git a/src/internal/m365/graph/errors.go b/src/internal/m365/graph/errors.go index cbc82080c..a6a0c7fe1 100644 --- a/src/internal/m365/graph/errors.go +++ b/src/internal/m365/graph/errors.go @@ -37,7 +37,7 @@ const ( // @microsoft.graph.conflictBehavior=fail finds a conflicting file. nameAlreadyExists errorCode = "nameAlreadyExists" quotaExceeded errorCode = "ErrorQuotaExceeded" - requestResourceNotFound errorCode = "Request_ResourceNotFound" + RequestResourceNotFound errorCode = "Request_ResourceNotFound" resourceNotFound errorCode = "ResourceNotFound" resyncRequired errorCode = "ResyncRequired" // alt: resyncRequired syncFolderNotFound errorCode = "ErrorSyncFolderNotFound" @@ -56,17 +56,16 @@ const ( type errorMessage string const ( - IOErrDuringRead errorMessage = "IO error during request payload read" + IOErrDuringRead errorMessage = "IO error during request payload read" + MysiteURLNotFound errorMessage = "unable to retrieve user's mysite url" + MysiteNotFound errorMessage = "user's mysite not found" + NoSPLicense errorMessage = "Tenant does not have a SPO license" ) const ( - mysiteURLNotFound = "unable to retrieve user's mysite url" - mysiteNotFound = "user's mysite not found" -) - -const ( - LabelsMalware = "malware_detected" - LabelsMysiteNotFound = "mysite_not_found" + LabelsMalware = "malware_detected" + LabelsMysiteNotFound = "mysite_not_found" + LabelsNoSharePointLicense = "no_sharepoint_license" // LabelsSkippable is used to determine if an error is skippable LabelsSkippable = "skippable_errors" @@ -132,7 +131,7 @@ func IsErrExchangeMailFolderNotFound(err error) bool { } func IsErrUserNotFound(err error) bool { - return hasErrorCode(err, requestResourceNotFound) + return hasErrorCode(err, RequestResourceNotFound) } func IsErrResourceNotFound(err error) bool { @@ -297,11 +296,17 @@ func setLabels(err *clues.Err, msg string) *clues.Err { return nil } - ml := strings.ToLower(msg) - if strings.Contains(ml, mysiteNotFound) || strings.Contains(ml, mysiteURLNotFound) { + f := filters.Contains([]string{msg}) + + if f.Compare(string(MysiteNotFound)) || + f.Compare(string(MysiteURLNotFound)) { err = err.Label(LabelsMysiteNotFound) } + if f.Compare(string(NoSPLicense)) { + err = err.Label(LabelsNoSharePointLicense) + } + if IsMalware(err) { err = err.Label(LabelsMalware) } diff --git a/src/internal/m365/graph/errors_test.go b/src/internal/m365/graph/errors_test.go index 714677179..7a415c8c4 100644 --- a/src/internal/m365/graph/errors_test.go +++ b/src/internal/m365/graph/errors_test.go @@ -33,6 +33,16 @@ func odErr(code string) *odataerrors.ODataError { return odErr } +func odErrMsg(code, message string) *odataerrors.ODataError { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(&code) + merr.SetMessage(&message) + odErr.SetError(merr) + + return odErr +} + func (suite *GraphErrorsUnitSuite) TestIsErrConnectionReset() { table := []struct { name string @@ -223,7 +233,7 @@ func (suite *GraphErrorsUnitSuite) TestIsErrUserNotFound() { }, { name: "request resource not found oDataErr", - err: odErr(string(requestResourceNotFound)), + err: odErr(string(RequestResourceNotFound)), expect: assert.True, }, } @@ -423,3 +433,56 @@ func (suite *GraphErrorsUnitSuite) TestIsErrCannotOpenFileAttachment() { }) } } + +func (suite *GraphErrorsUnitSuite) TestGraphStack_labels() { + table := []struct { + name string + err error + expect []string + }{ + { + name: "nil", + err: nil, + expect: []string{}, + }, + { + name: "not-odata", + err: assert.AnError, + expect: []string{}, + }, + { + name: "oDataErr matches no labels", + err: odErr("code"), + expect: []string{}, + }, + { + name: "mysite not found", + err: odErrMsg("code", string(MysiteNotFound)), + expect: []string{}, + }, + { + name: "mysite url not found", + err: odErrMsg("code", string(MysiteURLNotFound)), + expect: []string{}, + }, + { + name: "no sp license", + err: odErrMsg("code", string(NoSPLicense)), + expect: []string{}, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + defer flush() + + result := Stack(ctx, test.err) + + for _, e := range test.expect { + assert.True(t, clues.HasLabel(result, e), clues.ToCore(result)) + } + }) + } +} diff --git a/src/pkg/services/m365/api/drive_pager.go b/src/pkg/services/m365/api/drive_pager.go index 8199dc8e8..7d06be397 100644 --- a/src/pkg/services/m365/api/drive_pager.go +++ b/src/pkg/services/m365/api/drive_pager.go @@ -292,8 +292,8 @@ func GetAllDrives( for i := 0; i <= maxRetryCount; i++ { page, err = pager.GetPage(ctx) if err != nil { - if clues.HasLabel(err, graph.LabelsMysiteNotFound) { - logger.Ctx(ctx).Infof("resource owner does not have a drive") + if clues.HasLabel(err, graph.LabelsMysiteNotFound) || clues.HasLabel(err, graph.LabelsNoSharePointLicense) { + logger.CtxErr(ctx, err).Infof("resource owner does not have a drive") return make([]models.Driveable, 0), nil // no license or drives. } diff --git a/src/pkg/services/m365/api/mail_test.go b/src/pkg/services/m365/api/mail_test.go index dcc9df9a7..a328de9c1 100644 --- a/src/pkg/services/m365/api/mail_test.go +++ b/src/pkg/services/m365/api/mail_test.go @@ -203,8 +203,7 @@ func TestMailAPIIntgSuite(t *testing.T) { suite.Run(t, &MailAPIIntgSuite{ Suite: tester.NewIntegrationSuite( t, - [][]string{tester.M365AcctCredEnvs}, - ), + [][]string{tester.M365AcctCredEnvs}), }) } @@ -219,7 +218,7 @@ func (suite *MailAPIIntgSuite) SetupSuite() { suite.ac, err = mock.NewClient(m365) require.NoError(t, err, clues.ToCore(err)) - suite.user = tester.M365UserID(suite.T()) + suite.user = tester.M365UserID(t) } func getJSONObject(t *testing.T, thing serialization.Parsable) map[string]interface{} { diff --git a/src/pkg/services/m365/api/users.go b/src/pkg/services/m365/api/users.go index 07d2430ac..286cc52ff 100644 --- a/src/pkg/services/m365/api/users.go +++ b/src/pkg/services/m365/api/users.go @@ -183,7 +183,7 @@ func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { // check whether the user is able to access their onedrive drive. // if they cannot, we can assume they are ineligible for onedrive backups. if _, err := c.GetDefaultDrive(ctx, userID); err != nil { - if !clues.HasLabel(err, graph.LabelsMysiteNotFound) { + if !clues.HasLabel(err, graph.LabelsMysiteNotFound) || clues.HasLabel(err, graph.LabelsNoSharePointLicense) { logger.CtxErr(ctx, err).Error("getting user's drive") return nil, graph.Wrap(ctx, err, "getting user's drive") } diff --git a/src/pkg/services/m365/m365.go b/src/pkg/services/m365/m365.go index 305a10bbf..ffffd3625 100644 --- a/src/pkg/services/m365/m365.go +++ b/src/pkg/services/m365/m365.go @@ -73,12 +73,12 @@ func UsersCompatNoInfo(ctx context.Context, acct account.Account) ([]*UserNoInfo // UserHasMailbox returns true if the user has an exchange mailbox enabled // false otherwise, and a nil pointer and an error in case of error func UserHasMailbox(ctx context.Context, acct account.Account, userID string) (bool, error) { - uapi, err := makeUserAPI(acct) + ac, err := makeAC(acct) if err != nil { - return false, clues.Wrap(err, "getting mailbox").WithClues(ctx) + return false, clues.Stack(err).WithClues(ctx) } - _, err = uapi.GetMailInbox(ctx, userID) + _, err = ac.Users().GetMailInbox(ctx, userID) if err != nil { // we consider this a non-error case, since it // answers the question the caller is asking. @@ -103,16 +103,20 @@ func UserHasMailbox(ctx context.Context, acct account.Account, userID string) (b // UserHasDrives returns true if the user has any drives // false otherwise, and a nil pointer and an error in case of error func UserHasDrives(ctx context.Context, acct account.Account, userID string) (bool, error) { - uapi, err := makeUserAPI(acct) + ac, err := makeAC(acct) if err != nil { - return false, clues.Wrap(err, "getting drives").WithClues(ctx) + return false, clues.Stack(err).WithClues(ctx) } - _, err = uapi.GetDefaultDrive(ctx, userID) + return checkUserHasDrives(ctx, ac.Users(), userID) +} + +func checkUserHasDrives(ctx context.Context, dgdd discovery.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) { + if clues.HasLabel(err, graph.LabelsMysiteNotFound) || clues.HasLabel(err, graph.LabelsNoSharePointLicense) { return false, nil } @@ -130,12 +134,12 @@ func UserHasDrives(ctx context.Context, acct account.Account, userID string) (bo // TODO: Remove this once we remove `Info` from `Users` and instead rely on the `GetUserInfo` API // to get user information func usersNoInfo(ctx context.Context, acct account.Account, errs *fault.Bus) ([]*UserNoInfo, error) { - uapi, err := makeUserAPI(acct) + ac, err := makeAC(acct) if err != nil { - return nil, clues.Wrap(err, "getting users").WithClues(ctx) + return nil, clues.Stack(err).WithClues(ctx) } - us, err := discovery.Users(ctx, uapi, errs) + us, err := discovery.Users(ctx, ac.Users(), errs) if err != nil { return nil, err } @@ -162,12 +166,12 @@ func usersNoInfo(ctx context.Context, acct account.Account, errs *fault.Bus) ([] // Users returns a list of users in the specified M365 tenant func Users(ctx context.Context, acct account.Account, errs *fault.Bus) ([]*User, error) { - uapi, err := makeUserAPI(acct) + ac, err := makeAC(acct) if err != nil { - return nil, clues.Wrap(err, "getting users").WithClues(ctx) + return nil, clues.Stack(err).WithClues(ctx) } - us, err := discovery.Users(ctx, uapi, errs) + us, err := discovery.Users(ctx, ac.Users(), errs) if err != nil { return nil, err } @@ -197,7 +201,7 @@ func Users(ctx context.Context, acct account.Account, errs *fault.Bus) ([]*User, func parseUser(item models.Userable) (*User, error) { if item.GetUserPrincipalName() == nil { return nil, clues.New("user missing principal name"). - With("user_id", *item.GetId()) // TODO: pii + With("user_id", ptr.Val(item.GetId())) } u := &User{ @@ -215,12 +219,12 @@ func GetUserInfo( acct account.Account, userID string, ) (*api.UserInfo, error) { - uapi, err := makeUserAPI(acct) + ac, err := makeAC(acct) if err != nil { - return nil, clues.Wrap(err, "getting user info").WithClues(ctx) + return nil, clues.Stack(err).WithClues(ctx) } - ui, err := discovery.UserInfo(ctx, uapi, userID) + ui, err := discovery.UserInfo(ctx, ac.Users(), userID) if err != nil { return nil, err } @@ -249,9 +253,26 @@ type Site struct { // Sites returns a list of Sites in a specified M365 tenant func Sites(ctx context.Context, acct account.Account, errs *fault.Bus) ([]*Site, error) { - sites, err := discovery.Sites(ctx, acct, errs) + ac, err := makeAC(acct) if err != nil { - return nil, clues.Wrap(err, "initializing M365 api connection") + return nil, clues.Stack(err).WithClues(ctx) + } + + return getAllSites(ctx, ac.Sites()) +} + +type getAllSiteser interface { + GetAll(ctx context.Context, errs *fault.Bus) ([]models.Siteable, error) +} + +func getAllSites(ctx context.Context, gas getAllSiteser) ([]*Site, error) { + sites, err := gas.GetAll(ctx, fault.New(true)) + if err != nil { + if clues.HasLabel(err, graph.LabelsNoSharePointLicense) { + return nil, clues.Stack(graph.ErrServiceNotEnabled, err) + } + + return nil, clues.Wrap(err, "retrieving sites") } ret := make([]*Site, 0, len(sites)) @@ -304,16 +325,16 @@ func SitesMap( // helpers // --------------------------------------------------------------------------- -func makeUserAPI(acct account.Account) (api.Users, error) { +func makeAC(acct account.Account) (api.Client, error) { creds, err := acct.M365Config() if err != nil { - return api.Users{}, clues.Wrap(err, "getting m365 account creds") + return api.Client{}, clues.Wrap(err, "getting m365 account creds") } cli, err := api.NewClient(creds) if err != nil { - return api.Users{}, clues.Wrap(err, "constructing api client") + return api.Client{}, clues.Wrap(err, "constructing api client") } - return cli.Users(), nil + return cli, nil } diff --git a/src/pkg/services/m365/m365_test.go b/src/pkg/services/m365/m365_test.go index 27a385d5d..e77338dea 100644 --- a/src/pkg/services/m365/m365_test.go +++ b/src/pkg/services/m365/m365_test.go @@ -1,17 +1,22 @@ -package m365_test +package m365 import ( + "context" "testing" "github.com/alcionai/clues" + "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/discovery" + "github.com/alcionai/corso/src/internal/m365/graph" "github.com/alcionai/corso/src/internal/tester" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/path" - "github.com/alcionai/corso/src/pkg/services/m365" ) type M365IntegrationSuite struct { @@ -22,8 +27,7 @@ func TestM365IntegrationSuite(t *testing.T) { suite.Run(t, &M365IntegrationSuite{ Suite: tester.NewIntegrationSuite( t, - [][]string{tester.M365AcctCredEnvs}, - ), + [][]string{tester.M365AcctCredEnvs}), }) } @@ -35,7 +39,7 @@ func (suite *M365IntegrationSuite) TestUsers() { acct := tester.NewM365Account(suite.T()) - users, err := m365.Users(ctx, acct, fault.New(true)) + users, err := Users(ctx, acct, fault.New(true)) assert.NoError(t, err, clues.ToCore(err)) assert.NotEmpty(t, users) @@ -59,7 +63,7 @@ func (suite *M365IntegrationSuite) TestUsersCompat_HasNoInfo() { acct := tester.NewM365Account(suite.T()) - users, err := m365.UsersCompatNoInfo(ctx, acct) + users, err := UsersCompatNoInfo(ctx, acct) assert.NoError(t, err, clues.ToCore(err)) assert.NotEmpty(t, users) @@ -85,7 +89,7 @@ func (suite *M365IntegrationSuite) TestGetUserInfo() { uid = tester.M365UserID(t) ) - info, err := m365.GetUserInfo(ctx, acct, uid) + info, err := GetUserInfo(ctx, acct, uid) require.NoError(t, err, clues.ToCore(err)) require.NotNil(t, info) require.NotEmpty(t, info) @@ -112,7 +116,7 @@ func (suite *M365IntegrationSuite) TestUserHasMailbox() { uid = tester.M365UserID(t) ) - enabled, err := m365.UserHasMailbox(ctx, acct, uid) + enabled, err := UserHasMailbox(ctx, acct, uid) require.NoError(t, err, clues.ToCore(err)) assert.True(t, enabled) } @@ -128,7 +132,7 @@ func (suite *M365IntegrationSuite) TestUserHasDrive() { uid = tester.M365UserID(t) ) - enabled, err := m365.UserHasDrives(ctx, acct, uid) + enabled, err := UserHasDrives(ctx, acct, uid) require.NoError(t, err, clues.ToCore(err)) assert.True(t, enabled) } @@ -139,14 +143,14 @@ func (suite *M365IntegrationSuite) TestSites() { ctx, flush := tester.NewContext(t) defer flush() - acct := tester.NewM365Account(suite.T()) + acct := tester.NewM365Account(t) - sites, err := m365.Sites(ctx, acct, fault.New(true)) + sites, err := Sites(ctx, acct, fault.New(true)) assert.NoError(t, err, clues.ToCore(err)) assert.NotEmpty(t, sites) for _, s := range sites { - suite.Run("site", func() { + suite.Run("site_"+s.ID, func() { t := suite.T() assert.NotEmpty(t, s.WebURL) assert.NotEmpty(t, s.ID) @@ -154,3 +158,204 @@ func (suite *M365IntegrationSuite) TestSites() { }) } } + +type m365UnitSuite struct { + tester.Suite +} + +func TestM365UnitSuite(t *testing.T) { + suite.Run(t, &m365UnitSuite{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 *m365UnitSuite) TestCheckUserHasDrives() { + table := []struct { + name string + mock func(context.Context) discovery.GetDefaultDriver + expect assert.BoolAssertionFunc + expectErr func(*testing.T, error) + }{ + { + name: "ok", + mock: func(ctx context.Context) discovery.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) discovery.GetDefaultDriver { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(ptr.To("code")) + merr.SetMessage(ptr.To(string(graph.MysiteNotFound))) + odErr.SetError(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) discovery.GetDefaultDriver { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(ptr.To("code")) + merr.SetMessage(ptr.To(string(graph.MysiteURLNotFound))) + odErr.SetError(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) discovery.GetDefaultDriver { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(ptr.To("code")) + merr.SetMessage(ptr.To(string(graph.NoSPLicense))) + odErr.SetError(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) discovery.GetDefaultDriver { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(ptr.To(string(graph.RequestResourceNotFound))) + merr.SetMessage(ptr.To("message")) + odErr.SetError(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) discovery.GetDefaultDriver { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(ptr.To("code")) + merr.SetMessage(ptr.To("message")) + odErr.SetError(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) + }) + } +} + +type mockGAS struct { + response []models.Siteable + err error +} + +func (m mockGAS) GetAll(context.Context, *fault.Bus) ([]models.Siteable, error) { + return m.response, m.err +} + +func (suite *m365UnitSuite) TestGetAllSites() { + table := []struct { + name string + mock func(context.Context) getAllSiteser + expectErr func(*testing.T, error) + }{ + { + name: "ok", + mock: func(ctx context.Context) getAllSiteser { + return mockGAS{[]models.Siteable{}, nil} + }, + expectErr: func(t *testing.T, err error) { + assert.NoError(t, err, clues.ToCore(err)) + }, + }, + { + name: "no sharepoint license", + mock: func(ctx context.Context) getAllSiteser { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(ptr.To("code")) + merr.SetMessage(ptr.To(string(graph.NoSPLicense))) + odErr.SetError(merr) + + return mockGAS{nil, graph.Stack(ctx, odErr)} + }, + expectErr: func(t *testing.T, err error) { + assert.ErrorIs(t, err, graph.ErrServiceNotEnabled, clues.ToCore(err)) + }, + }, + { + name: "arbitrary error", + mock: func(ctx context.Context) getAllSiteser { + odErr := odataerrors.NewODataError() + merr := odataerrors.NewMainError() + merr.SetCode(ptr.To("code")) + merr.SetMessage(ptr.To("message")) + odErr.SetError(merr) + + return mockGAS{nil, graph.Stack(ctx, odErr)} + }, + 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() + + gas := test.mock(ctx) + + _, err := getAllSites(ctx, gas) + test.expectErr(t, err) + }) + } +}