From b3d4b4687b4bbe187be236631a130c01d924236b Mon Sep 17 00:00:00 2001 From: Keepers Date: Fri, 20 Jan 2023 13:09:59 -0700 Subject: [PATCH] discovery api, filter guest and external users (#2188) ## Description Adds the api client pkg pattern to the connector/ discovery package. Most code changes are plain lift-n-shift, with minor clean-ups along the way. User retrieval is now filtered to only include member and on-premise accounts. ## Does this PR need a docs update or release note? - [x] :white_check_mark: Yes, it's included ## Type of change - [x] :sunflower: Feature ## Issue(s) * #2094 ## Test Plan - [x] :muscle: Manual - [x] :zap: Unit test --- CHANGELOG.md | 2 + src/internal/connector/data_collections.go | 8 +- src/internal/connector/discovery/api/api.go | 57 ++++++ src/internal/connector/discovery/api/users.go | 166 ++++++++++++++++++ .../{discovery_test.go => api/users_test.go} | 12 +- src/internal/connector/discovery/discovery.go | 137 ++++----------- src/internal/connector/graph_connector.go | 14 +- .../connector/graph_connector_test.go | 5 +- src/pkg/services/m365/m365.go | 2 +- 9 files changed, 281 insertions(+), 122 deletions(-) create mode 100644 src/internal/connector/discovery/api/api.go create mode 100644 src/internal/connector/discovery/api/users.go rename src/internal/connector/discovery/{discovery_test.go => api/users_test.go} (84%) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07c061f12..248260c0a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Check if the user specified for an exchange backup operation has a mailbox. - Handle case where user's drive has not been initialized - Inline attachments (e.g. copy/paste ) are discovered and backed up correctly ([#2163](https://github.com/alcionai/corso/issues/2163)) +- Guest and External users (for cloud accounts) and non-on-premise users (for systems that use on-prem AD syncs) are now excluded from backup and restore operations. + ## [v0.1.0] (alpha) - 2023-01-13 diff --git a/src/internal/connector/data_collections.go b/src/internal/connector/data_collections.go index 2ba2e59a4..7eaf1c517 100644 --- a/src/internal/connector/data_collections.go +++ b/src/internal/connector/data_collections.go @@ -8,8 +8,8 @@ import ( "github.com/pkg/errors" "github.com/alcionai/corso/src/internal/connector/discovery" + "github.com/alcionai/corso/src/internal/connector/discovery/api" "github.com/alcionai/corso/src/internal/connector/exchange" - "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/onedrive" "github.com/alcionai/corso/src/internal/connector/sharepoint" "github.com/alcionai/corso/src/internal/connector/support" @@ -44,7 +44,7 @@ func (gc *GraphConnector) DataCollections( return nil, err } - serviceEnabled, err := checkServiceEnabled(ctx, gc.Service, path.ServiceType(sels.Service), sels.DiscreteOwner) + serviceEnabled, err := checkServiceEnabled(ctx, gc.Owners.Users(), path.ServiceType(sels.Service), sels.DiscreteOwner) if err != nil { return nil, err } @@ -138,7 +138,7 @@ func verifyBackupInputs(sels selectors.Selector, userPNs, siteIDs []string) erro func checkServiceEnabled( ctx context.Context, - gs graph.Servicer, + au api.Users, service path.ServiceType, resource string, ) (bool, error) { @@ -147,7 +147,7 @@ func checkServiceEnabled( return true, nil } - _, info, err := discovery.User(ctx, gs, resource) + _, info, err := discovery.User(ctx, au, resource) if err != nil { return false, err } diff --git a/src/internal/connector/discovery/api/api.go b/src/internal/connector/discovery/api/api.go new file mode 100644 index 000000000..20fc1b25d --- /dev/null +++ b/src/internal/connector/discovery/api/api.go @@ -0,0 +1,57 @@ +package api + +import ( + "github.com/pkg/errors" + + "github.com/alcionai/corso/src/internal/connector/graph" + "github.com/alcionai/corso/src/pkg/account" +) + +// --------------------------------------------------------------------------- +// interfaces +// --------------------------------------------------------------------------- + +// Client is used to fulfill the interface for discovery +// queries that are traditionally backed by GraphAPI. A +// struct is used in this case, instead of deferring to +// pure function wrappers, so that the boundary separates the +// granular implementation of the graphAPI and kiota away +// from the exchange package's broader intents. +type Client struct { + Credentials account.M365Config + + // The stable service is re-usable for any non-paged request. + // This allows us to maintain performance across async requests. + stable graph.Servicer +} + +// NewClient produces a new exchange api client. Must be used in +// place of creating an ad-hoc client struct. +func NewClient(creds account.M365Config) (Client, error) { + s, err := newService(creds) + if err != nil { + return Client{}, err + } + + return Client{creds, s}, nil +} + +// service generates a new service. Used for paged and other long-running +// requests instead of the client's stable service, so that in-flight state +// within the adapter doesn't get clobbered +func (c Client) service() (*graph.Service, error) { + return newService(c.Credentials) +} + +func newService(creds account.M365Config) (*graph.Service, error) { + adapter, err := graph.CreateAdapter( + creds.AzureTenantID, + creds.AzureClientID, + creds.AzureClientSecret, + ) + if err != nil { + return nil, errors.Wrap(err, "generating graph api service client") + } + + return graph.NewService(adapter), nil +} diff --git a/src/internal/connector/discovery/api/users.go b/src/internal/connector/discovery/api/users.go new file mode 100644 index 000000000..ff41ee06f --- /dev/null +++ b/src/internal/connector/discovery/api/users.go @@ -0,0 +1,166 @@ +package api + +import ( + "context" + + 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/connector/graph" + "github.com/alcionai/corso/src/internal/connector/support" + "github.com/alcionai/corso/src/pkg/path" +) + +// --------------------------------------------------------------------------- +// controller +// --------------------------------------------------------------------------- + +func (c Client) Users() Users { + return Users{c} +} + +// Users is an interface-compliant provider of the client. +type Users struct { + Client +} + +// --------------------------------------------------------------------------- +// structs +// --------------------------------------------------------------------------- + +type UserInfo struct { + DiscoveredServices map[path.ServiceType]struct{} +} + +func newUserInfo() *UserInfo { + return &UserInfo{ + DiscoveredServices: map[path.ServiceType]struct{}{ + path.ExchangeService: {}, + path.OneDriveService: {}, + }, + } +} + +// --------------------------------------------------------------------------- +// methods +// --------------------------------------------------------------------------- + +const ( + userSelectID = "id" + userSelectPrincipalName = "userPrincipalName" + userSelectDisplayName = "displayName" +) + +// Filter out both guest users, and (for on-prem installations) non-synced users. +// The latter filter makes an assumption that no on-prem users are guests; this might +// require more fine-tuned controls in the future. +// https://stackoverflow.com/questions/64044266/error-message-unsupported-or-invalid-query-filter-clause-specified-for-property +// +//nolint:lll +var userFilterNoGuests = "onPremisesSyncEnabled eq true OR userType eq 'Member'" + +func userOptions(fs *string) *users.UsersRequestBuilderGetRequestConfiguration { + return &users.UsersRequestBuilderGetRequestConfiguration{ + QueryParameters: &users.UsersRequestBuilderGetQueryParameters{ + Select: []string{userSelectID, userSelectPrincipalName, userSelectDisplayName}, + Filter: fs, + }, + } +} + +// GetAll retrieves all users. +func (c Users) GetAll(ctx context.Context) ([]models.Userable, error) { + service, err := c.service() + if err != nil { + return nil, err + } + + resp, err := service.Client().Users().Get(ctx, userOptions(&userFilterNoGuests)) + if err != nil { + return nil, support.ConnectorStackErrorTraceWrap(err, "getting all users") + } + + iter, err := msgraphgocore.NewPageIterator( + resp, + service.Adapter(), + models.CreateUserCollectionResponseFromDiscriminatorValue) + if err != nil { + return nil, support.ConnectorStackErrorTraceWrap(err, "constructing user iterator") + } + + var ( + iterErrs error + us = make([]models.Userable, 0) + ) + + iterator := func(item any) bool { + u, err := validateUser(item) + if err != nil { + iterErrs = support.WrapAndAppend("validating user", err, iterErrs) + } else { + us = append(us, u) + } + + return true + } + + if err := iter.Iterate(ctx, iterator); err != nil { + return nil, support.ConnectorStackErrorTraceWrap(err, "iterating all users") + } + + return us, iterErrs +} + +func (c Users) GetByID(ctx context.Context, userID string) (models.Userable, error) { + user, err := c.stable.Client().UsersById(userID).Get(ctx, nil) + if err != nil { + return nil, support.ConnectorStackErrorTraceWrap(err, "getting user by id") + } + + return user, nil +} + +func (c Users) GetInfo(ctx context.Context, userID string) (*UserInfo, error) { + // Assume all services are enabled + // then filter down to only services the user has enabled + userInfo := newUserInfo() + + // TODO: OneDrive + + _, err := c.stable.Client().UsersById(userID).MailFolders().Get(ctx, nil) + if err != nil { + if !graph.IsErrExchangeMailFolderNotFound(err) { + return nil, support.ConnectorStackErrorTraceWrap(err, "getting user's exchange mailfolders") + } + + delete(userInfo.DiscoveredServices, path.ExchangeService) + } + + return userInfo, nil +} + +// --------------------------------------------------------------------------- +// helpers +// --------------------------------------------------------------------------- + +// validateUser ensures the item is a Userable, and contains the necessary +// identifiers that we handle with all users. +// returns the item as a Userable model. +func validateUser(item any) (models.Userable, error) { + m, ok := item.(models.Userable) + if !ok { + return nil, errors.Errorf("expected Userable, got %T", item) + } + + if m.GetId() == nil { + return nil, errors.Errorf("missing ID") + } + + if m.GetUserPrincipalName() == nil { + return nil, errors.New("missing principalName") + } + + return m, nil +} diff --git a/src/internal/connector/discovery/discovery_test.go b/src/internal/connector/discovery/api/users_test.go similarity index 84% rename from src/internal/connector/discovery/discovery_test.go rename to src/internal/connector/discovery/api/users_test.go index e9349ba55..160806cf6 100644 --- a/src/internal/connector/discovery/discovery_test.go +++ b/src/internal/connector/discovery/api/users_test.go @@ -1,4 +1,4 @@ -package discovery +package api import ( "reflect" @@ -8,15 +8,15 @@ import ( "github.com/stretchr/testify/suite" ) -type DiscoverySuite struct { +type UsersUnitSuite struct { suite.Suite } -func TestDiscoverySuite(t *testing.T) { - suite.Run(t, new(DiscoverySuite)) +func TestUsersUnitSuite(t *testing.T) { + suite.Run(t, new(UsersUnitSuite)) } -func (suite *DiscoverySuite) TestParseUser() { +func (suite *UsersUnitSuite) TestValidateUser() { t := suite.T() name := "testuser" @@ -60,7 +60,7 @@ func (suite *DiscoverySuite) TestParseUser() { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := parseUser(tt.args) + got, err := validateUser(tt.args) if (err != nil) != tt.wantErr { t.Errorf("parseUser() error = %v, wantErr %v", err, tt.wantErr) return diff --git a/src/internal/connector/discovery/discovery.go b/src/internal/connector/discovery/discovery.go index a9f2266d1..3f75f74c4 100644 --- a/src/internal/connector/discovery/discovery.go +++ b/src/internal/connector/discovery/discovery.go @@ -3,125 +3,52 @@ 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" - "github.com/alcionai/corso/src/pkg/path" + "github.com/alcionai/corso/src/internal/connector/discovery/api" ) -const ( - userSelectID = "id" - userSelectPrincipalName = "userPrincipalName" - userSelectDisplayName = "displayName" -) +// --------------------------------------------------------------------------- +// interfaces +// --------------------------------------------------------------------------- -func Users(ctx context.Context, gs graph.Servicer, 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 +type getAller interface { + GetAll(context.Context) ([]models.Userable, error) } -type UserInfo struct { - DiscoveredServices map[path.ServiceType]struct{} +type getter interface { + GetByID(context.Context, string) (models.Userable, error) } -func User(ctx context.Context, gs graph.Servicer, userID string) (models.Userable, *UserInfo, error) { - user, err := gs.Client().UsersById(userID).Get(ctx, nil) +type getInfoer interface { + GetInfo(context.Context, string) (*api.UserInfo, error) +} + +type getWithInfoer interface { + getter + getInfoer +} + +// --------------------------------------------------------------------------- +// api +// --------------------------------------------------------------------------- + +// Users fetches all users in the tenant. +func Users(ctx context.Context, ga getAller) ([]models.Userable, error) { + return ga.GetAll(ctx) +} + +func User(ctx context.Context, gwi getWithInfoer, userID string) (models.Userable, *api.UserInfo, error) { + u, err := gwi.GetByID(ctx, userID) if err != nil { - return nil, nil, errors.Wrapf( - err, - "retrieving resource for tenant: %s", - support.ConnectorStackErrorTrace(err), - ) + return nil, nil, errors.Wrap(err, "getting user") } - // Assume all services are enabled - userInfo := &UserInfo{ - DiscoveredServices: map[path.ServiceType]struct{}{ - path.ExchangeService: {}, - path.OneDriveService: {}, - }, - } - - // Discover which services the user has enabled - - // Exchange: Query `MailFolders` - _, err = gs.Client().UsersById(userID).MailFolders().Get(ctx, nil) + ui, err := gwi.GetInfo(ctx, userID) if err != nil { - if !graph.IsErrExchangeMailFolderNotFound(err) { - return nil, nil, errors.Wrapf( - err, - "retrieving mail folders for tenant: %s", - support.ConnectorStackErrorTrace(err), - ) - } - - delete(userInfo.DiscoveredServices, path.ExchangeService) + return nil, nil, errors.Wrap(err, "getting user info") } - // TODO: OneDrive - - return user, userInfo, nil -} - -// 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 + return u, ui, nil } diff --git a/src/internal/connector/graph_connector.go b/src/internal/connector/graph_connector.go index e047a017b..68f03d048 100644 --- a/src/internal/connector/graph_connector.go +++ b/src/internal/connector/graph_connector.go @@ -15,6 +15,7 @@ import ( "golang.org/x/exp/maps" "github.com/alcionai/corso/src/internal/connector/discovery" + "github.com/alcionai/corso/src/internal/connector/discovery/api" "github.com/alcionai/corso/src/internal/connector/exchange" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/onedrive" @@ -37,7 +38,9 @@ import ( // GraphRequestAdapter from the msgraph-sdk-go. Additional fields are for // bookkeeping and interfacing with other component. type GraphConnector struct { - Service graph.Servicer + Service graph.Servicer + Owners api.Client + tenant string Users map[string]string // key value Sites map[string]string // key value @@ -74,12 +77,15 @@ func NewGraphConnector(ctx context.Context, acct account.Account, r resource) (* credentials: m365, } - gService, err := gc.createService() + gc.Service, err = gc.createService() if err != nil { return nil, errors.Wrap(err, "creating service connection") } - gc.Service = gService + gc.Owners, err = api.NewClient(m365) + if err != nil { + return nil, errors.Wrap(err, "creating api client") + } // TODO(ashmrtn): When selectors only encapsulate a single resource owner that // is not a wildcard don't populate users or sites when making the connector. @@ -121,7 +127,7 @@ func (gc *GraphConnector) setTenantUsers(ctx context.Context) error { ctx, end := D.Span(ctx, "gc:setTenantUsers") defer end() - users, err := discovery.Users(ctx, gc.Service, gc.tenant) + users, err := discovery.Users(ctx, gc.Owners.Users()) if err != nil { return err } diff --git a/src/internal/connector/graph_connector_test.go b/src/internal/connector/graph_connector_test.go index d635ee6f9..71eff095a 100644 --- a/src/internal/connector/graph_connector_test.go +++ b/src/internal/connector/graph_connector_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/suite" "golang.org/x/exp/maps" + "github.com/alcionai/corso/src/internal/connector/discovery/api" "github.com/alcionai/corso/src/internal/connector/graph" "github.com/alcionai/corso/src/internal/connector/mockconnector" "github.com/alcionai/corso/src/internal/connector/support" @@ -174,10 +175,10 @@ func (suite *GraphConnectorIntegrationSuite) TestSetTenantUsers() { ctx, flush := tester.NewContext() defer flush() - service, err := newConnector.createService() + owners, err := api.NewClient(suite.connector.credentials) require.NoError(suite.T(), err) - newConnector.Service = service + newConnector.Owners = owners suite.Empty(len(newConnector.Users)) err = newConnector.setTenantUsers(ctx) diff --git a/src/pkg/services/m365/m365.go b/src/pkg/services/m365/m365.go index 19da0a21f..ba9cc2a59 100644 --- a/src/pkg/services/m365/m365.go +++ b/src/pkg/services/m365/m365.go @@ -25,7 +25,7 @@ func Users(ctx context.Context, m365Account account.Account) ([]*User, error) { return nil, errors.Wrap(err, "could not initialize M365 graph connection") } - users, err := discovery.Users(ctx, gc.Service, m365Account.ID()) + users, err := discovery.Users(ctx, gc.Owners.Users()) if err != nil { return nil, err }