diff --git a/pkg/accessreview/drivers/microsoft_365.go b/pkg/accessreview/drivers/microsoft_365.go index b1e0fe979..f6fbc8a19 100644 --- a/pkg/accessreview/drivers/microsoft_365.go +++ b/pkg/accessreview/drivers/microsoft_365.go @@ -23,6 +23,7 @@ package drivers import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -31,6 +32,7 @@ import ( "strings" "time" + "go.gearno.de/kit/log" "go.probo.inc/probo/pkg/coredata" ) @@ -39,6 +41,7 @@ import ( // HTTP client (Bearer token). type Microsoft365Driver struct { httpClient *http.Client + logger *log.Logger } var _ Driver = (*Microsoft365Driver)(nil) @@ -68,7 +71,7 @@ var adminRoleDisplayNames = map[string]bool{ "Authentication Administrator": true, } -func NewMicrosoft365Driver(httpClient *http.Client) *Microsoft365Driver { +func NewMicrosoft365Driver(httpClient *http.Client, logger *log.Logger) *Microsoft365Driver { return &Microsoft365Driver{ httpClient: &http.Client{ Transport: &retryRoundTripper{ @@ -76,6 +79,7 @@ func NewMicrosoft365Driver(httpClient *http.Client) *Microsoft365Driver { maxRetries: 3, }, }, + logger: logger, } } @@ -157,9 +161,19 @@ func (d *Microsoft365Driver) ListAccounts(ctx context.Context) ([]AccountRecord, return nil, fmt.Errorf("cannot list users: %w", err) } + // MFA registration details require AuditLog.Read.All. A failure here + // must not abort the account fetch — leave MFA unknown so the campaign + // can still proceed. Context cancel/deadline still fail the fetch so a + // timed-out source does not commit an incomplete result as success. mfaStatuses, err := d.listMFAStatuses(ctx) if err != nil { - return nil, fmt.Errorf("cannot list MFA statuses: %w", err) + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil, fmt.Errorf("cannot list MFA statuses: %w", err) + } + + d.logger.ErrorCtx(ctx, "cannot list microsoft 365 MFA statuses, reporting MFA unknown", log.Error(err)) + + mfaStatuses = map[string]coredata.MFAStatus{} } records := make([]AccountRecord, 0, len(users)) diff --git a/pkg/accessreview/drivers/microsoft_365_test.go b/pkg/accessreview/drivers/microsoft_365_test.go index 595625a6f..bf0a7451e 100644 --- a/pkg/accessreview/drivers/microsoft_365_test.go +++ b/pkg/accessreview/drivers/microsoft_365_test.go @@ -22,11 +22,15 @@ package drivers import ( "context" + "io" + "net/http" "os" + "strings" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.gearno.de/kit/log" "go.probo.inc/probo/pkg/coredata" ) @@ -36,7 +40,7 @@ func TestMicrosoft365Driver(t *testing.T) { rec := newRecorder(t, "testdata/microsoft_365", "MICROSOFT_365_TOKEN") client := newVCRClient(rec, bearerAuth(os.Getenv("MICROSOFT_365_TOKEN"))) - driver := NewMicrosoft365Driver(client) + driver := NewMicrosoft365Driver(client, log.NewLogger(log.WithName("test"))) records, err := driver.ListAccounts(context.Background()) require.NoError(t, err) require.Len(t, records, 4) @@ -83,3 +87,105 @@ func TestMicrosoft365Driver(t *testing.T) { require.NotNil(t, dana.Active) assert.False(t, *dana.Active) } + +func TestMicrosoft365Driver_MFAFetchFailureLeavesUnknown(t *testing.T) { + t.Parallel() + + client := &http.Client{ + Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) { + header := http.Header{"Content-Type": []string{"application/json"}} + + switch { + case strings.HasSuffix(r.URL.Path, "/directoryRoles"): + return &http.Response{ + StatusCode: http.StatusOK, + Header: header, + Body: io.NopCloser(strings.NewReader(`{"value":[]}`)), + }, nil + case strings.HasSuffix(r.URL.Path, "/users"): + return &http.Response{ + StatusCode: http.StatusOK, + Header: header, + Body: io.NopCloser(strings.NewReader(`{ + "value":[{ + "id":"user-1", + "userPrincipalName":"alice@example.com", + "mail":"alice@example.com", + "displayName":"Alice", + "accountEnabled":true + }] + }`)), + }, nil + case strings.Contains(r.URL.Path, "userRegistrationDetails"): + return &http.Response{ + StatusCode: http.StatusForbidden, + Header: header, + Body: io.NopCloser(strings.NewReader(`{ + "error":{ + "code":"Authentication_RequestFromNonPremiumTenantOrB2CTenant", + "message":"Tenant is not a B2C tenant and doesn't have premium license" + } + }`)), + }, nil + default: + return &http.Response{ + StatusCode: http.StatusNotFound, + Header: header, + Body: io.NopCloser(strings.NewReader(`{}`)), + }, nil + } + }), + } + + driver := NewMicrosoft365Driver(client, log.NewLogger(log.WithName("test"))) + records, err := driver.ListAccounts(context.Background()) + require.NoError(t, err) + require.Len(t, records, 1) + assert.Equal(t, "alice@example.com", records[0].Email) + assert.Equal(t, coredata.MFAStatusUnknown, records[0].MFAStatus) +} + +func TestMicrosoft365Driver_MFAFetchCanceledPropagates(t *testing.T) { + t.Parallel() + + client := &http.Client{ + Transport: roundTripperFunc(func(r *http.Request) (*http.Response, error) { + header := http.Header{"Content-Type": []string{"application/json"}} + + switch { + case strings.HasSuffix(r.URL.Path, "/directoryRoles"): + return &http.Response{ + StatusCode: http.StatusOK, + Header: header, + Body: io.NopCloser(strings.NewReader(`{"value":[]}`)), + }, nil + case strings.HasSuffix(r.URL.Path, "/users"): + return &http.Response{ + StatusCode: http.StatusOK, + Header: header, + Body: io.NopCloser(strings.NewReader(`{ + "value":[{ + "id":"user-1", + "userPrincipalName":"alice@example.com", + "mail":"alice@example.com", + "displayName":"Alice", + "accountEnabled":true + }] + }`)), + }, nil + case strings.Contains(r.URL.Path, "userRegistrationDetails"): + return nil, context.Canceled + default: + return &http.Response{ + StatusCode: http.StatusNotFound, + Header: header, + Body: io.NopCloser(strings.NewReader(`{}`)), + }, nil + } + }), + } + + driver := NewMicrosoft365Driver(client, log.NewLogger(log.WithName("test"))) + _, err := driver.ListAccounts(context.Background()) + require.ErrorIs(t, err, context.Canceled) +} diff --git a/pkg/connector/provider/microsoft_365.go b/pkg/connector/provider/microsoft_365.go index acb159adc..5d68d367d 100644 --- a/pkg/connector/provider/microsoft_365.go +++ b/pkg/connector/provider/microsoft_365.go @@ -48,8 +48,8 @@ func microsoft365Registration() *Registration { "https://graph.microsoft.com/Directory.Read.All", "https://graph.microsoft.com/RoleManagement.Read.Directory", }, - NewDriver: func(_ context.Context, c *http.Client, _ *coredata.Connector, _ *log.Logger) (drivers.Driver, error) { - return drivers.NewMicrosoft365Driver(c), nil + NewDriver: func(_ context.Context, c *http.Client, _ *coredata.Connector, logger *log.Logger) (drivers.Driver, error) { + return drivers.NewMicrosoft365Driver(c, logger.Named("microsoft365")), nil }, NewNameResolver: func(_ context.Context, c *http.Client, _ *coredata.Connector, _ *log.Logger) drivers.NameResolver { return drivers.NewMicrosoft365NameResolver(c)