From 27ff840be855ccc1af19c0f7d73c538ab099c6dd Mon Sep 17 00:00:00 2001 From: Keepers Date: Tue, 5 Dec 2023 18:26:08 -0700 Subject: [PATCH] match specific error message for resourcre locked (#4806) Matches a specific error message returned by graph when an account does not have support for microsoft teams. --- #### Does this PR need a docs update or release note? - [x] :no_entry: No #### Type of change - [x] :bug: Bugfix #### Test Plan - [x] :zap: Unit test - [x] :green_heart: E2E --- src/pkg/filters/filters.go | 16 +++++ src/pkg/filters/filters_test.go | 58 ++++++++++++++++ src/pkg/services/m365/api/graph/errors.go | 26 ++++++- .../services/m365/api/graph/errors_test.go | 11 +++ src/pkg/services/m365/api/groups.go | 16 ++--- src/pkg/services/m365/api/groups_test.go | 69 +++++++++++++++++++ 6 files changed, 186 insertions(+), 10 deletions(-) diff --git a/src/pkg/filters/filters.go b/src/pkg/filters/filters.go index c697203b7..648fcb82b 100644 --- a/src/pkg/filters/filters.go +++ b/src/pkg/filters/filters.go @@ -361,6 +361,22 @@ func newFilter(c comparator, targets, normTargets []string, negate bool) Filter // Comparisons // ---------------------------------------------------------------------------------------------------- +// Must returns true if ALL provided filters match the input. +// Returns false if no filters are provided +func Must(input string, filters ...Filter) bool { + if len(filters) == 0 { + return false + } + + for _, f := range filters { + if !f.Compare(input) { + return false + } + } + + return true +} + // CompareAny checks whether any one of all the provided // inputs passes the filter. // diff --git a/src/pkg/filters/filters_test.go b/src/pkg/filters/filters_test.go index ac8820551..98aaa33ae 100644 --- a/src/pkg/filters/filters_test.go +++ b/src/pkg/filters/filters_test.go @@ -554,6 +554,64 @@ func (suite *FiltersSuite) TestPathEquals_NormalizedTargets() { } } +func (suite *FiltersSuite) TestMust() { + table := []struct { + name string + filters []filters.Filter + expect assert.BoolAssertionFunc + }{ + { + name: "no filters", + filters: []filters.Filter{}, + expect: assert.False, + }, + { + name: "one matching filter", + filters: []filters.Filter{filters.Equal([]string{"fnords"})}, + expect: assert.True, + }, + { + name: "one non-matching filter", + filters: []filters.Filter{filters.Equal([]string{"smarf"})}, + expect: assert.False, + }, + { + name: "multiple matching filter", + filters: []filters.Filter{ + filters.Equal([]string{"fnords"}), + filters.Equal([]string{"FNORDS"}), + filters.Equal([]string{"fNORDs"}), + }, + expect: assert.True, + }, + { + name: "one non-matching filter among many matching", + filters: []filters.Filter{ + filters.Equal([]string{"fnords"}), + filters.Equal([]string{"FNORDS"}), + filters.Equal([]string{"smarf"}), + }, + expect: assert.False, + }, + { + name: "all non-matching filters", + filters: []filters.Filter{ + filters.Equal([]string{"smarf"}), + filters.Equal([]string{"SMARF"}), + filters.Equal([]string{"sMARf"}), + }, + expect: assert.False, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + test.expect( + suite.T(), + filters.Must("fnords", test.filters...)) + }) + } +} + // --------------------------------------------------------------------------- // pii handling // --------------------------------------------------------------------------- diff --git a/src/pkg/services/m365/api/graph/errors.go b/src/pkg/services/m365/api/graph/errors.go index 1333a82fc..8cfcfb157 100644 --- a/src/pkg/services/m365/api/graph/errors.go +++ b/src/pkg/services/m365/api/graph/errors.go @@ -276,7 +276,12 @@ func IsErrSiteNotFound(err error) bool { func IsErrResourceLocked(err error) bool { return errors.Is(err, ErrResourceLocked) || hasInnerErrorCode(err, ResourceLocked) || - hasErrorCode(err, NotAllowed) + hasErrorCode(err, NotAllowed) || + errMessageMatchesAllFilters( + err, + filters.In([]string{"the service principal for resource"}), + filters.In([]string{"this indicate that a subscription within the tenant has lapsed"}), + filters.In([]string{"preventing tokens from being issued for it"})) } // --------------------------------------------------------------------------- @@ -358,6 +363,25 @@ func hasErrorMessage(err error, msgs ...errorMessage) bool { return filters.In(cs).Compare(msg) } +// only use this as a last resort. Prefer the code or statuscode if possible. +func errMessageMatchesAllFilters(err error, fs ...filters.Filter) bool { + if err == nil { + return false + } + + var oDataError odataerrors.ODataErrorable + if !errors.As(err, &oDataError) { + return false + } + + msg, ok := ptr.ValOK(oDataError.GetErrorEscaped().GetMessage()) + if !ok { + return false + } + + return filters.Must(msg, fs...) +} + // Wrap is a helper function that extracts ODataError metadata from // the error. If the error is not an ODataError type, returns the error. func Wrap(ctx context.Context, e error, msg string) *clues.Err { diff --git a/src/pkg/services/m365/api/graph/errors_test.go b/src/pkg/services/m365/api/graph/errors_test.go index 0c6b9ee27..f1801cbc6 100644 --- a/src/pkg/services/m365/api/graph/errors_test.go +++ b/src/pkg/services/m365/api/graph/errors_test.go @@ -877,6 +877,17 @@ func (suite *GraphErrorsUnitSuite) TestIsErrResourceLocked() { err: innerMatch, expect: assert.True, }, + { + name: "matching oDataErr message", + err: graphTD.ODataErrWithMsg( + string(AuthenticationError), + "AADSTS500014: The service principal for resource 'beefe6b7-f5df-413d-ac2d-abf1e3fd9c0b' "+ + "is disabled. This indicate that a subscription within the tenant has lapsed, or that the "+ + "administrator for this tenant has disabled the application, preventing tokens from being "+ + "issued for it. Trace ID: dead78e1-0830-4edf-bea7-f0a445620100 Correlation ID: "+ + "deadbeef-7f1e-4578-8215-36004a2c935c Timestamp: 2023-12-05 19:31:01Z"), + expect: assert.True, + }, { name: "matching err sentinel", err: ErrResourceLocked, diff --git a/src/pkg/services/m365/api/groups.go b/src/pkg/services/m365/api/groups.go index 60431d101..cfccfed47 100644 --- a/src/pkg/services/m365/api/groups.go +++ b/src/pkg/services/m365/api/groups.go @@ -110,20 +110,18 @@ func (c Groups) GetByID( identifier string, _ CallConfig, // matching standards ) (models.Groupable, error) { - service, err := c.Service(c.counter) - if err != nil { - return nil, err - } - ctx = clues.Add(ctx, "resource_identifier", identifier) - var group models.Groupable + var ( + group models.Groupable + err error + ) // prefer lookup by id, but fallback to lookup by display name, // even in the case of a uuid, just in case the display name itself // is a uuid. if uuidRE.MatchString(identifier) { - group, err = service. + group, err = c.Stable. Client(). Groups(). ByGroupId(identifier). @@ -149,7 +147,7 @@ func (c Groups) GetByID( }, } - resp, err := service.Client().Groups().Get(ctx, opts) + resp, err := c.Stable.Client().Groups().Get(ctx, opts) if err == nil { return getGroupFromResponse(ctx, resp) } @@ -169,7 +167,7 @@ func (c Groups) GetByID( }, } - resp, err := service.Client().Groups().Get(ctx, opts) + resp, err := c.Stable.Client().Groups().Get(ctx, opts) if err != nil { if graph.IsErrResourceLocked(err) { err = clues.Stack(graph.ErrResourceLocked, err) diff --git a/src/pkg/services/m365/api/groups_test.go b/src/pkg/services/m365/api/groups_test.go index fb66a2a31..f0ebc51e1 100644 --- a/src/pkg/services/m365/api/groups_test.go +++ b/src/pkg/services/m365/api/groups_test.go @@ -5,6 +5,7 @@ import ( "github.com/alcionai/clues" "github.com/google/uuid" + "github.com/h2non/gock" "github.com/microsoftgraph/msgraph-sdk-go/models" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -15,6 +16,7 @@ import ( "github.com/alcionai/corso/src/internal/tester/tconfig" "github.com/alcionai/corso/src/pkg/fault" "github.com/alcionai/corso/src/pkg/services/m365/api/graph" + graphTD "github.com/alcionai/corso/src/pkg/services/m365/api/graph/testdata" ) type GroupUnitSuite struct { @@ -218,3 +220,70 @@ func (suite *GroupsIntgSuite) TestGroups_GetByID() { }) } } + +func (suite *GroupsIntgSuite) TestGroups_GetByID_mockResourceLockedErrs() { + gID := uuid.NewString() + + table := []struct { + name string + id string + setup func(t *testing.T) + }{ + { + name: "by name", + id: "g", + setup: func(t *testing.T) { + err := graphTD.ODataErr(string(graph.NotAllowed)) + + interceptV1Path("groups"). + Reply(403). + JSON(graphTD.ParseableToMap(t, err)) + }, + }, + { + name: "by id", + id: gID, + setup: func(t *testing.T) { + err := graphTD.ODataErr(string(graph.NotAllowed)) + + interceptV1Path("groups", gID). + Reply(403). + JSON(graphTD.ParseableToMap(t, err)) + }, + }, + { + name: "by id - matches error message", + id: gID, + setup: func(t *testing.T) { + err := graphTD.ODataErrWithMsg( + string(graph.AuthenticationError), + "AADSTS500014: The service principal for resource 'beefe6b7-f5df-413d-ac2d-abf1e3fd9c0b' "+ + "is disabled. This indicate that a subscription within the tenant has lapsed, or that the "+ + "administrator for this tenant has disabled the application, preventing tokens from being "+ + "issued for it. Trace ID: dead78e1-0830-4edf-bea7-f0a445620100 Correlation ID: "+ + "deadbeef-7f1e-4578-8215-36004a2c935c Timestamp: 2023-12-05 19:31:01Z") + + interceptV1Path("groups", gID). + Reply(403). + JSON(graphTD.ParseableToMap(t, err)) + }, + }, + } + for _, test := range table { + suite.Run(test.name, func() { + t := suite.T() + + ctx, flush := tester.NewContext(t) + + defer flush() + defer gock.Off() + + test.setup(t) + + _, err := suite.its.gockAC. + Groups(). + GetByID(ctx, test.id, CallConfig{}) + require.ErrorIs(t, err, graph.ErrResourceLocked, clues.ToCore(err)) + }) + } +}