From d8c284399bf1f4cfb55cc7356d783897b7ece66f Mon Sep 17 00:00:00 2001 From: Sacha Al Himdani Date: Fri, 31 Jul 2026 15:32:49 +0200 Subject: [PATCH] Soft-fail Microsoft 365 MFA when registration fetch fails AuditLog.Read.All is not available on every tenant. Keep importing accounts with MFA unknown and log the error instead of failing the whole source fetch. Signed-off-by: Sacha Al Himdani --- pkg/accessreview/drivers/microsoft_365.go | 18 ++- .../drivers/microsoft_365_test.go | 108 +++++++++++++++++- pkg/connector/provider/microsoft_365.go | 4 +- 3 files changed, 125 insertions(+), 5 deletions(-) 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)