larryh/corso
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

handle no sharepoint license err (#3673)

Browse Source 
adds handling for graph api responses
where the tenant does not have an active
sharepoint license.  For user's drives, this
will fall under the same "service not enabled"
behavior we use to determine if the drive can
get backed up.  For sites, this causes the
"get all sites" call to return a serviceNotEnabled error.

---

#### Does this PR need a docs update or release note?

- [x] ✅ Yes, it's included

#### Type of change

- [x] 🐛 Bugfix

#### Issue(s)

* #3671

#### Test Plan

- [x] ⚡ Unit test
- [x] 💚 E2E
This commit is contained in:
Keepers 2023-06-26 20:26:47 -06:00 committed by GitHub
parent 9cbd46aabe
commit 0451e933d5
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
9 changed files with 352 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -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

4
src/internal/m365/discovery/discovery.go
View File

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

23
src/internal/m365/graph/errors.go
View File

@ -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"
@ -57,16 +57,15 @@ type errorMessage string
const (
IOErrDuringRead errorMessage = "IO error during request payload read"
)
const (
mysiteURLNotFound = "unable to retrieve user's mysite url"
mysiteNotFound = "user's mysite not found"
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 (
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)
}

65
src/internal/m365/graph/errors_test.go
View File

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

4
src/pkg/services/m365/api/drive_pager.go
View File

@ -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.
}

5
src/pkg/services/m365/api/mail_test.go
View File

@ -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{} {

2
src/pkg/services/m365/api/users.go
View File

@ -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")
}

67
src/pkg/services/m365/m365.go
View File

@ -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
}

229
src/pkg/services/m365/m365_test.go
View File

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