From 5533ada9beb2b803ea6d0893d00ceb055dccb439 Mon Sep 17 00:00:00 2001 From: Vaibhav Kamra Date: Mon, 5 Dec 2022 17:45:51 -0800 Subject: [PATCH] Refactor user query and return user display name (#1679) ## Description Move user discovery into a `discovery` package. Also use that to return user display name from `m365.Users()` call. This will also allow us to remove `Users` from the GC struct going forward. If this pattern works - will follow up with a similar change for `Sites()`. ## Type of change - [x] :sunflower: Feature - [ ] :bug: Bugfix - [ ] :world_map: Documentation - [ ] :robot: Test - [ ] :computer: CI/Deployment - [ ] :hamster: Trivial/Minor ## Test Plan - [ ] :muscle: Manual - [x] :zap: Unit test - [ ] :green_heart: E2E --- src/internal/connector/discovery/discovery.go | 83 +++++++++++++++++++ .../connector/discovery/discovery_test.go | 73 ++++++++++++++++ .../exchange/exchange_service_test.go | 7 -- .../connector/exchange/query_options.go | 17 ---- .../connector/exchange/service_query.go | 12 --- src/internal/connector/graph_connector.go | 31 ++----- src/pkg/services/m365/m365.go | 61 ++++++++++---- src/pkg/services/m365/m365_test.go | 34 ++------ 8 files changed, 218 insertions(+), 100 deletions(-) create mode 100644 src/internal/connector/discovery/discovery.go create mode 100644 src/internal/connector/discovery/discovery_test.go diff --git a/src/internal/connector/discovery/discovery.go b/src/internal/connector/discovery/discovery.go new file mode 100644 index 000000000..e29880c77 --- /dev/null +++ b/src/internal/connector/discovery/discovery.go @@ -0,0 +1,83 @@ +package discovery + +import ( + "context" + + msgraphgocore "github.com/microsoftgraph/msgraph-sdk-go-core" + "github.com/microsoftgraph/msgraph-sdk-go/models" + msuser "github.com/microsoftgraph/msgraph-sdk-go/users" + "github.com/pkg/errors" + + "github.com/alcionai/corso/src/internal/connector/graph" + "github.com/alcionai/corso/src/internal/connector/support" +) + +const ( + userSelectID = "id" + userSelectPrincipalName = "userPrincipalName" + userSelectDisplayName = "displayName" +) + +func Users(ctx context.Context, gs graph.Service, tenantID string) ([]models.Userable, error) { + users := make([]models.Userable, 0) + + options := &msuser.UsersRequestBuilderGetRequestConfiguration{ + QueryParameters: &msuser.UsersRequestBuilderGetQueryParameters{ + Select: []string{userSelectID, userSelectPrincipalName, userSelectDisplayName}, + }, + } + + response, err := gs.Client().Users().Get(ctx, options) + if err != nil { + return nil, errors.Wrapf( + err, + "retrieving resources for tenant %s: %s", + tenantID, + support.ConnectorStackErrorTrace(err), + ) + } + + iter, err := msgraphgocore.NewPageIterator(response, gs.Adapter(), + models.CreateUserCollectionResponseFromDiscriminatorValue) + if err != nil { + return nil, errors.Wrap(err, support.ConnectorStackErrorTrace(err)) + } + + var iterErrs error + + callbackFunc := func(item interface{}) bool { + u, err := parseUser(item) + if err != nil { + iterErrs = support.WrapAndAppend("discovering users: ", err, iterErrs) + return true + } + + users = append(users, u) + + return true + } + + if err := iter.Iterate(ctx, callbackFunc); err != nil { + return nil, errors.Wrap(err, support.ConnectorStackErrorTrace(err)) + } + + return users, iterErrs +} + +// parseUser extracts information from `models.Userable` we care about +func parseUser(item interface{}) (models.Userable, error) { + m, ok := item.(models.Userable) + if !ok { + return nil, errors.New("iteration retrieved non-User item") + } + + if m.GetId() == nil { + return nil, errors.Errorf("no ID for User") + } + + if m.GetUserPrincipalName() == nil { + return nil, errors.Errorf("no principal name for User: %s", *m.GetId()) + } + + return m, nil +} diff --git a/src/internal/connector/discovery/discovery_test.go b/src/internal/connector/discovery/discovery_test.go new file mode 100644 index 000000000..e9349ba55 --- /dev/null +++ b/src/internal/connector/discovery/discovery_test.go @@ -0,0 +1,73 @@ +package discovery + +import ( + "reflect" + "testing" + + "github.com/microsoftgraph/msgraph-sdk-go/models" + "github.com/stretchr/testify/suite" +) + +type DiscoverySuite struct { + suite.Suite +} + +func TestDiscoverySuite(t *testing.T) { + suite.Run(t, new(DiscoverySuite)) +} + +func (suite *DiscoverySuite) TestParseUser() { + t := suite.T() + + name := "testuser" + email := "testuser@foo.com" + id := "testID" + user := models.NewUser() + user.SetUserPrincipalName(&email) + user.SetDisplayName(&name) + user.SetId(&id) + + tests := []struct { + name string + args interface{} + want models.Userable + wantErr bool + }{ + { + name: "Invalid type", + args: string("invalid type"), + wantErr: true, + }, + { + name: "No ID", + args: models.NewUser(), + wantErr: true, + }, + { + name: "No user principal name", + args: func() *models.User { + u := models.NewUser() + u.SetId(&id) + return u + }(), + wantErr: true, + }, + { + name: "Valid User", + args: user, + want: user, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseUser(tt.args) + if (err != nil) != tt.wantErr { + t.Errorf("parseUser() error = %v, wantErr %v", err, tt.wantErr) + return + } + if !reflect.DeepEqual(got, tt.want) { + t.Errorf("parseUser() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/src/internal/connector/exchange/exchange_service_test.go b/src/internal/connector/exchange/exchange_service_test.go index 2cef9836e..f9881f3dd 100644 --- a/src/internal/connector/exchange/exchange_service_test.go +++ b/src/internal/connector/exchange/exchange_service_test.go @@ -5,7 +5,6 @@ import ( "testing" "time" - absser "github.com/microsoft/kiota-abstractions-go/serialization" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -252,12 +251,6 @@ func (suite *ExchangeServiceSuite) TestGraphQueryFunctions() { name: "GraphQuery: Get All Folders", function: GetAllFolderNamesForUser, }, - { - name: "GraphQuery: Get All Users", - function: func(ctx context.Context, gs graph.Service, toss string) (absser.Parsable, error) { - return GetAllUsersForTenant(ctx, gs) - }, - }, { name: "GraphQuery: Get All ContactFolders", function: GetAllContactFolderNamesForUser, diff --git a/src/internal/connector/exchange/query_options.go b/src/internal/connector/exchange/query_options.go index e3272063f..19b21c884 100644 --- a/src/internal/connector/exchange/query_options.go +++ b/src/internal/connector/exchange/query_options.go @@ -4,7 +4,6 @@ import ( "fmt" abs "github.com/microsoft/kiota-abstractions-go" - msuser "github.com/microsoftgraph/msgraph-sdk-go/users" mscalendars "github.com/microsoftgraph/msgraph-sdk-go/users/item/calendars" mscevents "github.com/microsoftgraph/msgraph-sdk-go/users/item/calendars/item/events" mscontactfolder "github.com/microsoftgraph/msgraph-sdk-go/users/item/contactfolders" @@ -391,22 +390,6 @@ func optionsForContacts(moreOps []string) (*mscontacts.ContactsRequestBuilderGet return options, nil } -func optionsForUsers(moreOps []string) (*msuser.UsersRequestBuilderGetRequestConfiguration, error) { - selecting, err := buildOptions(moreOps, users) - if err != nil { - return nil, err - } - - requestParams := &msuser.UsersRequestBuilderGetQueryParameters{ - Select: selecting, - } - options := &msuser.UsersRequestBuilderGetRequestConfiguration{ - QueryParameters: requestParams, - } - - return options, nil -} - // buildOptions - Utility Method for verifying if select options are valid for the m365 object type // @return is a pair. The first is a string literal of allowable options based on the object type, // the second is an error. An error is returned if an unsupported option or optionIdentifier was used diff --git a/src/internal/connector/exchange/service_query.go b/src/internal/connector/exchange/service_query.go index 8d7b952e8..e321fa210 100644 --- a/src/internal/connector/exchange/service_query.go +++ b/src/internal/connector/exchange/service_query.go @@ -74,18 +74,6 @@ func GetAllContactFolderNamesForUser(ctx context.Context, gs graph.Service, user return gs.Client().UsersById(user).ContactFolders().Get(ctx, options) } -// GetAllUsersForTenant makes a GraphQuery request retrieving all the users in the tenant. -func GetAllUsersForTenant(ctx context.Context, gs graph.Service) (absser.Parsable, error) { - selecting := []string{"userPrincipalName"} - - options, err := optionsForUsers(selecting) - if err != nil { - return nil, err - } - - return gs.Client().Users().Get(ctx, options) -} - // GetAllEvents for User. Default returns EventResponseCollection for future events. // of the time that the call was made. 'calendar' option must be present to gain // access to additional data map in future calls. diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index 18520a037..e7aec953a 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -14,6 +14,7 @@ import ( "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/pkg/errors" + "github.com/alcionai/corso/src/internal/connector/discovery" "github.com/alcionai/corso/src/internal/connector/exchange" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/onedrive" @@ -154,38 +155,20 @@ func (gc *GraphConnector) setTenantUsers(ctx context.Context) error { ctx, end := D.Span(ctx, "gc:setTenantUsers") defer end() - users, err := getResources( - ctx, - gc.graphService, - gc.tenant, - exchange.GetAllUsersForTenant, - models.CreateUserCollectionResponseFromDiscriminatorValue, - identifyUser, - ) + users, err := discovery.Users(ctx, gc.graphService, gc.tenant) if err != nil { return err } - gc.Users = users + gc.Users = make(map[string]string, len(users)) + + for _, u := range users { + gc.Users[*u.GetUserPrincipalName()] = *u.GetId() + } return nil } -// Transforms an interface{} into a key,value pair representing -// userPrincipalName:userID. -func identifyUser(item any) (string, string, error) { - m, ok := item.(models.Userable) - if !ok { - return "", "", errors.New("iteration retrieved non-User item") - } - - if m.GetUserPrincipalName() == nil { - return "", "", errors.Errorf("no principal name for User: %s", *m.GetId()) - } - - return *m.GetUserPrincipalName(), *m.GetId(), nil -} - // GetUsers returns the email address of users within tenant. func (gc *GraphConnector) GetUsers() []string { return buildFromMap(true, gc.Users) diff --git a/src/pkg/services/m365/m365.go b/src/pkg/services/m365/m365.go index 6415b456b..714106e15 100644 --- a/src/pkg/services/m365/m365.go +++ b/src/pkg/services/m365/m365.go @@ -3,41 +3,59 @@ package m365 import ( "context" + "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/connector" + "github.com/alcionai/corso/src/internal/connector/discovery" "github.com/alcionai/corso/src/pkg/account" ) +type User struct { + PrincipalName string + ID string + Name string +} + // Users returns a list of users in the specified M365 tenant // TODO: Implement paging support -func Users(ctx context.Context, m365Account account.Account) ([]string, error) { +func Users(ctx context.Context, m365Account account.Account) ([]*User, error) { gc, err := connector.NewGraphConnector(ctx, m365Account, connector.Users) if err != nil { return nil, errors.Wrap(err, "could not initialize M365 graph connection") } - return gc.GetUsers(), nil + users, err := discovery.Users(ctx, gc.Service(), m365Account.ID()) + if err != nil { + return nil, err + } + + ret := make([]*User, 0, len(users)) + + for _, u := range users { + pu, err := parseUser(u) + if err != nil { + return nil, err + } + + ret = append(ret, pu) + } + + return ret, nil } -// UserIDs returns a list of user IDs for the specified M365 tenant -// TODO: Implement paging support func UserIDs(ctx context.Context, m365Account account.Account) ([]string, error) { - gc, err := connector.NewGraphConnector(ctx, m365Account, connector.Users) + users, err := Users(ctx, m365Account) if err != nil { - return nil, errors.Wrap(err, "could not initialize M365 graph connection") + return nil, err } - return gc.GetUsersIds(), nil -} - -func GetEmailAndUserID(ctx context.Context, m365Account account.Account) (map[string]string, error) { - gc, err := connector.NewGraphConnector(ctx, m365Account, connector.Users) - if err != nil { - return nil, errors.Wrap(err, "could not initialize M365 graph connection") + ret := make([]string, 0, len(users)) + for _, u := range users { + ret = append(ret, u.ID) } - return gc.Users, nil + return ret, nil } // Sites returns a list of SharePoint sites in the specified M365 tenant @@ -50,3 +68,18 @@ func Sites(ctx context.Context, m365Account account.Account) ([]string, error) { return gc.GetSites(), nil } + +// parseUser extracts information from `models.Userable` we care about +func parseUser(item models.Userable) (*User, error) { + if item.GetUserPrincipalName() == nil { + return nil, errors.Errorf("no principal name for User: %s", *item.GetId()) + } + + u := &User{PrincipalName: *item.GetUserPrincipalName(), ID: *item.GetId()} + + if item.GetDisplayName() != nil { + u.Name = *item.GetDisplayName() + } + + return u, nil +} diff --git a/src/pkg/services/m365/m365_test.go b/src/pkg/services/m365/m365_test.go index 01bc50c9b..8bea6f707 100644 --- a/src/pkg/services/m365/m365_test.go +++ b/src/pkg/services/m365/m365_test.go @@ -3,6 +3,7 @@ package m365 import ( "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -40,30 +41,11 @@ func (suite *M365IntegrationSuite) TestUsers() { require.NotNil(suite.T(), users) require.Greater(suite.T(), len(users), 0) -} - -func (suite *M365IntegrationSuite) TestUserIDs() { - ctx, flush := tester.NewContext() - defer flush() - - acct := tester.NewM365Account(suite.T()) - - ids, err := UserIDs(ctx, acct) - require.NoError(suite.T(), err) - - require.NotNil(suite.T(), ids) - require.Greater(suite.T(), len(ids), 0) -} - -func (suite *M365IntegrationSuite) TestGetEmailAndUserID() { - ctx, flush := tester.NewContext() - defer flush() - - acct := tester.NewM365Account(suite.T()) - - ids, err := GetEmailAndUserID(ctx, acct) - require.NoError(suite.T(), err) - - require.NotNil(suite.T(), ids) - require.Greater(suite.T(), len(ids), 0) + + for _, u := range users { + suite.T().Log(u) + assert.NotEmpty(suite.T(), u.ID) + assert.NotEmpty(suite.T(), u.PrincipalName) + assert.NotEmpty(suite.T(), u.Name) + } }