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

Cleanup old user info code (#4189)

Browse Source 
<!-- PR description-->

---

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

- [ ] ✅ Yes, it's included
- [ ] 🕐 Yes, but in a later PR
- [x] ⛔ No

#### Type of change

<!--- Please check the type of change your PR introduces: --->
- [ ] 🌻 Feature
- [ ] 🐛 Bugfix
- [ ] 🗺️ Documentation
- [ ] 🤖 Supportability/Tests
- [ ] 💻 CI/Deployment
- [x] 🧹 Tech Debt/Cleanup

#### Issue(s)

<!-- Can reference multiple issues. Use one of the following "magic words" - "closes, fixes" to auto-close the Github issue. -->
* #<issue>

#### Test Plan

<!-- How will this be tested prior to merging.-->
- [ ] 💪 Manual
- [x] ⚡ Unit test
- [ ] 💚 E2E
This commit is contained in:
Abhishek Pandey 2023-09-08 14:11:58 +05:30 committed by GitHub
parent 9bedee9c72
commit 35ccb985df
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 42 additions and 484 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
src/pkg/services/m365/api/user_info.go
View File

@ -5,18 +5,8 @@ import (
"github.com/alcionai/corso/src/internal/common/str"
"github.com/alcionai/corso/src/internal/common/tform"
"github.com/alcionai/corso/src/pkg/path"
)
// ---------------------------------------------------------------------------
// User Info
// ---------------------------------------------------------------------------
type UserInfo struct {
ServicesEnabled map[path.ServiceType]struct{}
Mailbox MailboxInfo
}
type MailboxInfo struct {
Purpose string
ArchiveFolder string
@ -59,15 +49,6 @@ type WorkingHours struct {
}
}
func newUserInfo() *UserInfo {
return &UserInfo{
ServicesEnabled: map[path.ServiceType]struct{}{
path.ExchangeService: {},
path.OneDriveService: {},
},
}
}
func ParseMailboxSettings(
settings models.Userable,
mi MailboxInfo,

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

@ -15,8 +15,6 @@ import (
"github.com/alcionai/corso/src/internal/common/ptr"
"github.com/alcionai/corso/src/internal/m365/graph"
"github.com/alcionai/corso/src/pkg/fault"
"github.com/alcionai/corso/src/pkg/logger"
"github.com/alcionai/corso/src/pkg/path"
)
// Variables
@ -169,86 +167,6 @@ func appendIfErr(errs []error, err error) []error {
return append(errs, err)
}
// ---------------------------------------------------------------------------
// Info
// ---------------------------------------------------------------------------
// TODO(pandeyabs): Remove this once the SDK users have migrated to new APIs
func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) {
var (
// Assume all services are enabled
// then filter down to only services the user has enabled
userInfo = newUserInfo()
mailFolderFound = true
)
// 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) && !clues.HasLabel(err, graph.LabelsNoSharePointLicense) {
logger.CtxErr(ctx, err).Error("getting user's default drive")
return nil, graph.Wrap(ctx, err, "getting user's default drive info")
}
logger.Ctx(ctx).Info("resource owner does not have a drive")
delete(userInfo.ServicesEnabled, path.OneDriveService)
}
// check whether the user is able to access their inbox.
// if they cannot, we can assume they are ineligible for exchange backups.
inbx, err := c.GetMailInbox(ctx, userID)
if err != nil {
if err := EvaluateMailboxError(graph.Stack(ctx, err)); err != nil {
logger.CtxErr(ctx, err).Error("getting user's mail folder")
return nil, err
}
logger.Ctx(ctx).Info("resource owner does not have a mailbox enabled")
delete(userInfo.ServicesEnabled, path.ExchangeService)
mailFolderFound = false
}
// check whether the user has accessible mailbox settings.
// if they do, aggregate them in the MailboxInfo
mi := MailboxInfo{
ErrGetMailBoxSetting: []error{},
}
if !mailFolderFound {
mi.ErrGetMailBoxSetting = append(mi.ErrGetMailBoxSetting, ErrMailBoxNotFound)
userInfo.Mailbox = mi
return userInfo, nil
}
mboxSettings, err := c.GetMailboxSettings(ctx, userID)
if err != nil {
logger.CtxErr(ctx, err).Info("err getting user's mailbox settings")
if !graph.IsErrAccessDenied(err) {
return nil, graph.Wrap(ctx, err, "getting user's mailbox settings")
}
mi.ErrGetMailBoxSetting = append(mi.ErrGetMailBoxSetting, clues.New("access denied"))
} else {
mi = ParseMailboxSettings(mboxSettings, mi)
}
err = c.GetFirstInboxMessage(ctx, userID, ptr.Val(inbx.GetId()))
if err != nil {
if !graph.IsErrQuotaExceeded(err) {
return nil, clues.Stack(err)
}
mi.QuotaExceeded = graph.IsErrQuotaExceeded(err)
}
userInfo.Mailbox = mi
return userInfo, nil
}
// EvaluateMailboxError 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.

164
src/pkg/services/m365/api/users_test.go
View File

@ -4,15 +4,12 @@ import (
"testing"
"github.com/alcionai/clues"
"github.com/h2non/gock"
"github.com/microsoftgraph/msgraph-sdk-go/models"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"github.com/alcionai/corso/src/internal/m365/graph"
"github.com/alcionai/corso/src/internal/tester"
"github.com/alcionai/corso/src/internal/tester/tconfig"
"github.com/alcionai/corso/src/pkg/services/m365/api"
)
@ -153,164 +150,3 @@ func (suite *UsersUnitSuite) TestIsAnyErrMailboxNotFound() {
})
}
}
type UsersIntgSuite struct {
tester.Suite
its intgTesterSetup
}
func TestUsersIntgSuite(t *testing.T) {
suite.Run(t, &UsersIntgSuite{
Suite: tester.NewIntegrationSuite(
t,
[][]string{tconfig.M365AcctCredEnvs}),
})
}
func (suite *UsersIntgSuite) SetupSuite() {
suite.its = newIntegrationTesterSetup(suite.T())
}
func (suite *UsersIntgSuite) TestUsers_GetInfo_errors() {
table := []struct {
name string
setGocks func(t *testing.T)
expectErr func(t *testing.T, err error)
}{
{
name: "default drive err - mysite not found",
setGocks: func(t *testing.T) {
interceptV1Path("users", "user", "drive").
Reply(400).
JSON(parseableToMap(t, odErrMsg("anycode", string(graph.MysiteNotFound))))
interceptV1Path("users", "user", "mailFolders", api.MailInbox).
Reply(400).
JSON(parseableToMap(t, odErr(string(graph.ResourceNotFound))))
},
expectErr: func(t *testing.T, err error) {
assert.NoError(t, err, clues.ToCore(err))
},
},
{
name: "default drive err - no sharepoint license",
setGocks: func(t *testing.T) {
interceptV1Path("users", "user", "drive").
Reply(400).
JSON(parseableToMap(t, odErrMsg("anycode", string(graph.NoSPLicense))))
interceptV1Path("users", "user", "mailFolders", api.MailInbox).
Reply(400).
JSON(parseableToMap(t, odErr(string(graph.ResourceNotFound))))
},
expectErr: func(t *testing.T, err error) {
assert.NoError(t, err, clues.ToCore(err))
},
},
{
name: "default drive err - other error",
setGocks: func(t *testing.T) {
interceptV1Path("users", "user", "drive").
Reply(400).
JSON(parseableToMap(t, odErrMsg("somecode", "somemessage")))
},
expectErr: func(t *testing.T, err error) {
assert.Error(t, err, clues.ToCore(err))
},
},
{
name: "mail inbox err - user not found",
setGocks: func(t *testing.T) {
interceptV1Path("users", "user", "drive").
Reply(200).
JSON(parseableToMap(t, models.NewDrive()))
interceptV1Path("users", "user", "mailFolders", api.MailInbox).
Reply(400).
JSON(parseableToMap(t, odErr(string(graph.RequestResourceNotFound))))
},
expectErr: func(t *testing.T, err error) {
assert.ErrorIs(t, err, graph.ErrResourceOwnerNotFound, clues.ToCore(err))
},
},
{
name: "mail inbox err - user not found",
setGocks: func(t *testing.T) {
interceptV1Path("users", "user", "drive").
Reply(200).
JSON(parseableToMap(t, models.NewDrive()))
interceptV1Path("users", "user", "mailFolders", api.MailInbox).
Reply(400).
JSON(parseableToMap(t, odErr(string(graph.MailboxNotEnabledForRESTAPI))))
},
expectErr: func(t *testing.T, err error) {
assert.NoError(t, err, clues.ToCore(err))
},
},
{
name: "mail inbox err - authenticationError",
setGocks: func(t *testing.T) {
interceptV1Path("users", "user", "drive").
Reply(200).
JSON(parseableToMap(t, models.NewDrive()))
interceptV1Path("users", "user", "mailFolders", api.MailInbox).
Reply(400).
JSON(parseableToMap(t, odErr(string(graph.AuthenticationError))))
},
expectErr: func(t *testing.T, err error) {
assert.NoError(t, err, clues.ToCore(err))
},
},
{
name: "mail inbox err - other error",
setGocks: func(t *testing.T) {
interceptV1Path("users", "user", "drive").
Reply(200).
JSON(parseableToMap(t, models.NewDrive()))
interceptV1Path("users", "user", "mailFolders", api.MailInbox).
Reply(400).
JSON(parseableToMap(t, odErrMsg("somecode", "somemessage")))
},
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()
defer gock.Off()
test.setGocks(t)
_, err := suite.its.gockAC.Users().GetInfo(ctx, "user")
test.expectErr(t, err)
})
}
}
func (suite *UsersIntgSuite) TestUsers_GetInfo_quotaExceeded() {
t := suite.T()
ctx, flush := tester.NewContext(t)
defer flush()
defer gock.Off()
gock.EnableNetworking()
gock.New(graphAPIHostURL).
// Wildcard match on the inbox folder ID.
Get(v1APIURLPath("users", suite.its.user.id, "mailFolders", "(.*)", "messages", "delta")).
Reply(403).
SetHeaders(
map[string]string{
"Content-Type": "application/json; odata.metadata=minimal; " +
"odata.streaming=true; IEEE754Compatible=false; charset=utf-8",
},
).
BodyString(`{"error":{"code":"ErrorQuotaExceeded","message":"The process failed to get the correct properties."}}`)
output, err := suite.its.gockAC.Users().GetInfo(ctx, suite.its.user.id)
require.NoError(t, err, clues.ToCore(err))
assert.True(t, output.Mailbox.QuotaExceeded)
}

88
src/pkg/services/m365/users.go
View File

@ -15,39 +15,17 @@ import (
"github.com/alcionai/corso/src/pkg/services/m365/api"
)
// User is the minimal information required to identify and display a user.
type User struct {
PrincipalName string
ID string
Name string
Info api.UserInfo
}
// UserNoInfo is the minimal information required to identify and display a user.
// TODO: Remove this once `UsersCompatNoInfo` is removed
// TODO(pandeyabs): Rename this to User now that `Info` support has been removed.
type UserNoInfo struct {
PrincipalName string
ID string
Name string
}
// UsersCompat returns a list of users in the specified M365 tenant.
// TODO(ashmrtn): Remove when upstream consumers of the SDK support the fault
// package.
func UsersCompat(ctx context.Context, acct account.Account) ([]*User, error) {
errs := fault.New(true)
us, err := Users(ctx, acct, errs)
if err != nil {
return nil, err
}
return us, errs.Failure()
}
// UsersCompatNoInfo returns a list of users in the specified M365 tenant.
// TODO: Remove this once `Info` is removed from the `User` struct and callers
// have switched over
// TODO(pandeyabs): Rename this to Users now that `Info` support has been removed. Would
// need corresponding changes in SDK consumers.
func UsersCompatNoInfo(ctx context.Context, acct account.Account) ([]*UserNoInfo, error) {
errs := fault.New(true)
@ -95,8 +73,6 @@ func UserHasDrives(ctx context.Context, acct account.Account, userID string) (bo
}
// usersNoInfo returns a list of users in the specified M365 tenant - with no info
// 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) {
ac, err := makeAC(ctx, acct, path.UnknownService)
if err != nil {
@ -128,47 +104,14 @@ func usersNoInfo(ctx context.Context, acct account.Account, errs *fault.Bus) ([]
return ret, nil
}
// Users returns a list of users in the specified M365 tenant
func Users(ctx context.Context, acct account.Account, errs *fault.Bus) ([]*User, error) {
ac, err := makeAC(ctx, acct, path.ExchangeService)
if err != nil {
return nil, clues.Stack(err).WithClues(ctx)
}
us, err := ac.Users().GetAll(ctx, errs)
if err != nil {
return nil, err
}
ret := make([]*User, 0, len(us))
for _, u := range us {
pu, err := parseUser(u)
if err != nil {
return nil, clues.Wrap(err, "formatting user data")
}
userInfo, err := ac.Users().GetInfo(ctx, pu.ID)
if err != nil {
return nil, clues.Wrap(err, "getting user details")
}
pu.Info = *userInfo
ret = append(ret, pu)
}
return ret, nil
}
// parseUser extracts information from `models.Userable` we care about
func parseUser(item models.Userable) (*User, error) {
func parseUser(item models.Userable) (*UserNoInfo, error) {
if item.GetUserPrincipalName() == nil {
return nil, clues.New("user missing principal name").
With("user_id", ptr.Val(item.GetId()))
}
u := &User{
u := &UserNoInfo{
PrincipalName: ptr.Val(item.GetUserPrincipalName()),
ID: ptr.Val(item.GetId()),
Name: ptr.Val(item.GetDisplayName()),
@ -176,24 +119,3 @@ func parseUser(item models.Userable) (*User, error) {
return u, nil
}
// UserInfo returns the corso-specific set of user metadata.
// TODO(pandeyabs): Remove support for this API. SDK users would be using
// per service API calls - UserHasMailbox, UserGetMailboxInfo, UserHasDrive, etc.
func GetUserInfo(
ctx context.Context,
acct account.Account,
userID string,
) (*api.UserInfo, error) {
ac, err := makeAC(ctx, acct, path.ExchangeService)
if err != nil {
return nil, clues.Stack(err).WithClues(ctx)
}
ui, err := ac.Users().GetInfo(ctx, userID)
if err != nil {
return nil, err
}
return ui, nil
}

155
src/pkg/services/m365/users_test.go
View File

@ -14,8 +14,6 @@ import (
"github.com/alcionai/corso/src/internal/tester/tconfig"
"github.com/alcionai/corso/src/pkg/account"
"github.com/alcionai/corso/src/pkg/credentials"
"github.com/alcionai/corso/src/pkg/fault"
"github.com/alcionai/corso/src/pkg/path"
"github.com/alcionai/corso/src/pkg/services/m365/api"
)
@ -41,7 +39,7 @@ func (suite *userIntegrationSuite) SetupSuite() {
suite.acct = tconfig.NewM365Account(suite.T())
}
func (suite *userIntegrationSuite) TestUsers() {
func (suite *userIntegrationSuite) TestUsersCompat_HasNoInfo() {
t := suite.T()
ctx, flush := tester.NewContext(t)
@ -49,28 +47,6 @@ func (suite *userIntegrationSuite) TestUsers() {
graph.InitializeConcurrencyLimiter(ctx, true, 4)
users, err := Users(ctx, suite.acct, fault.New(true))
assert.NoError(t, err, clues.ToCore(err))
assert.NotEmpty(t, users)
for _, u := range users {
suite.Run("user_"+u.ID, func() {
t := suite.T()
assert.NotEmpty(t, u.ID)
assert.NotEmpty(t, u.PrincipalName)
assert.NotEmpty(t, u.Name)
assert.NotEmpty(t, u.Info)
})
}
}
func (suite *userIntegrationSuite) TestUsersCompat_HasNoInfo() {
t := suite.T()
ctx, flush := tester.NewContext(t)
defer flush()
acct := tconfig.NewM365Account(suite.T())
users, err := UsersCompatNoInfo(ctx, acct)
@ -132,16 +108,19 @@ func (suite *userIntegrationSuite) TestUserHasDrive() {
name string
user string
expect bool
expectErr require.ErrorAssertionFunc
}{
{
name: "user without drive",
user: "a53c26f7-5100-4acb-a910-4d20960b2c19", // User: testevents@10rqc2.onmicrosoft.com
expect: false,
expectErr: require.NoError,
},
{
name: "user with drive",
user: userID,
expect: true,
expectErr: require.NoError,
},
}
for _, test := range table {
@ -152,7 +131,7 @@ func (suite *userIntegrationSuite) TestUserHasDrive() {
defer flush()
enabled, err := UserHasDrives(ctx, acct, test.user)
require.NoError(t, err, clues.ToCore(err))
test.expectErr(t, err, clues.ToCore(err))
assert.Equal(t, test.expect, enabled)
})
}
@ -161,6 +140,7 @@ func (suite *userIntegrationSuite) TestUserHasDrive() {
func (suite *userIntegrationSuite) TestUserGetMailboxInfo() {
t := suite.T()
acct := tconfig.NewM365Account(t)
userID := tconfig.M365UserID(t)
table := []struct {
name string
@ -177,6 +157,15 @@ func (suite *userIntegrationSuite) TestUserGetMailboxInfo() {
},
expectErr: require.NoError,
},
{
name: "user mailbox",
user: userID,
expect: func(t *testing.T, info api.MailboxInfo) {
require.NotNil(t, info)
assert.Equal(t, "user", info.Purpose)
},
expectErr: require.NoError,
},
{
name: "user with no mailbox",
user: "a53c26f7-5100-4acb-a910-4d20960b2c19", // User: testevents@10rqc2.onmicrosoft.com
@ -186,6 +175,18 @@ func (suite *userIntegrationSuite) TestUserGetMailboxInfo() {
},
expectErr: require.NoError,
},
{
name: "invalid user",
user: uuid.NewString(),
expect: func(t *testing.T, info api.MailboxInfo) {
mi := api.MailboxInfo{
ErrGetMailBoxSetting: []error{},
}
require.Equal(t, mi, info)
},
expectErr: require.Error,
},
}
for _, test := range table {
suite.Run(test.name, func() {
@ -233,109 +234,9 @@ func (suite *userIntegrationSuite) TestUsers_InvalidCredentials() {
ctx, flush := tester.NewContext(t)
defer flush()
users, err := Users(ctx, test.acct(t), fault.New(true))
users, err := UsersCompatNoInfo(ctx, test.acct(t))
assert.Empty(t, users, "returned some users")
assert.NotNil(t, err)
})
}
}
func (suite *userIntegrationSuite) TestGetUserInfo() {
table := []struct {
name string
user string
expect *api.UserInfo
expectErr require.ErrorAssertionFunc
}{
{
name: "standard test user",
user: tconfig.M365UserID(suite.T()),
expect: &api.UserInfo{
ServicesEnabled: map[path.ServiceType]struct{}{
path.ExchangeService: {},
path.OneDriveService: {},
},
Mailbox: api.MailboxInfo{
Purpose: "user",
ErrGetMailBoxSetting: nil,
},
},
expectErr: require.NoError,
},
{
name: "user does not exist",
user: uuid.NewString(),
expect: &api.UserInfo{
ServicesEnabled: map[path.ServiceType]struct{}{},
Mailbox: api.MailboxInfo{},
},
expectErr: require.Error,
},
}
for _, test := range table {
suite.Run(test.name, func() {
t := suite.T()
ctx, flush := tester.NewContext(t)
defer flush()
result, err := GetUserInfo(ctx, suite.acct, test.user)
test.expectErr(t, err, clues.ToCore(err))
if err != nil {
return
}
assert.Equal(t, test.expect.ServicesEnabled, result.ServicesEnabled)
})
}
}
func (suite *userIntegrationSuite) TestGetUserInfo_userWithoutDrive() {
userID := tconfig.M365UserID(suite.T())
table := []struct {
name string
user string
expect *api.UserInfo
}{
{
name: "user without drive and exchange",
user: "a53c26f7-5100-4acb-a910-4d20960b2c19", // User: testevents@10rqc2.onmicrosoft.com
expect: &api.UserInfo{
ServicesEnabled: map[path.ServiceType]struct{}{},
Mailbox: api.MailboxInfo{
ErrGetMailBoxSetting: []error{api.ErrMailBoxNotFound},
},
},
},
{
name: "user with drive and exchange",
user: userID,
expect: &api.UserInfo{
ServicesEnabled: map[path.ServiceType]struct{}{
path.ExchangeService: {},
path.OneDriveService: {},
},
Mailbox: api.MailboxInfo{
Purpose: "user",
ErrGetMailBoxSetting: []error{},
},
},
},
}
for _, test := range table {
suite.Run(test.name, func() {
t := suite.T()
ctx, flush := tester.NewContext(t)
defer flush()
result, err := GetUserInfo(ctx, suite.acct, test.user)
require.NoError(t, err, clues.ToCore(err))
assert.Equal(t, test.expect.ServicesEnabled, result.ServicesEnabled)
assert.Equal(t, test.expect.Mailbox.ErrGetMailBoxSetting, result.Mailbox.ErrGetMailBoxSetting)
assert.Equal(t, test.expect.Mailbox.Purpose, result.Mailbox.Purpose)
})
}
}