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] ✅ Yes, it's included

## Type of change

- [x] 🌻 Feature

## Issue(s)

* #2094

## Test Plan

- [x] 💪 Manual
- [x] ⚡ Unit test

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
 

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
 	}

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

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

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

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
 }

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<email> value<id>
 	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
 	}

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)

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
 	}