diff --git a/pkg/connector/app.go b/pkg/connector/app.go index fa07b9a1..a494754b 100644 --- a/pkg/connector/app.go +++ b/pkg/connector/app.go @@ -376,10 +376,6 @@ func appResource(app *okta.Application) (*v2.Resource, error) { } func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { - var ( - ok bool - email string - ) l := ctxzap.Extract(ctx) if principal.Id.ResourceType != resourceTypeUser.Id && principal.Id.ResourceType != resourceTypeGroup.Id { l.Warn( @@ -399,7 +395,7 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent if response == nil { l.Warn("okta-connector: failed to fetch application user, nil response", zap.String("app_id", appID), zap.String("user_id", userID), zap.Error(err)) - return nil, fmt.Errorf("okta-connector: failed to fetch application user: %s", err.Error()) + return nil, fmt.Errorf("okta-connector: failed to fetch application user: %w", handleOktaResponseError(response, err)) } defer response.Body.Close() errOkta, err := getError(response) @@ -421,7 +417,7 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent } if appUser != nil && userID == appUser.Id { - l.Warn( + l.Debug( "okta-connector: The app specified is already assigned to the user", zap.String("principal_id", principal.Id.String()), zap.String("principal_type", principal.Id.ResourceType), @@ -430,13 +426,14 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent return annotations.New(&v2.GrantAlreadyExists{}), nil } - user, _, err := g.client.User.GetUser(ctx, userID) + user, userResp, err := g.client.User.GetUser(ctx, userID) if err != nil { - return nil, err + return nil, handleOktaResponseError(userResp, err) } profile := *user.Profile - if email, ok = profile[profileFieldEmail].(string); !ok { + email, ok := profile[profileFieldEmail].(string) + if !ok { email = unknownProfileValue } @@ -447,23 +444,24 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent Id: userID, Scope: strings.ToUpper(principal.Id.ResourceType), } - assignedUser, _, err := g.client.Application.AssignUserToApplication(ctx, appID, payload) + assignedUser, assignResp, err := g.client.Application.AssignUserToApplication(ctx, appID, payload) if err != nil { l.Warn( "okta-connector: The app specified cannot be assigned to the user", zap.String("principal_id", principal.Id.String()), zap.String("principal_type", principal.Id.ResourceType), ) - return nil, fmt.Errorf("okta-connector: The app specified cannot be assigned to the user %s", - err.Error()) + return nil, fmt.Errorf("okta-connector: the app specified cannot be assigned to the user: %w", handleOktaResponseError(assignResp, err)) } - l.Warn("App Membership has been created.", + l.Debug("App Membership has been created.", zap.String("userID", assignedUser.Id), zap.String("Status", assignedUser.Status), zap.Time("LastUpdated", *assignedUser.LastUpdated), zap.String("Scope", assignedUser.Scope), ) + + return rateLimitAnnotations(assignResp), nil case resourceTypeGroup.Id: groupID := principal.Id.Resource appGroup, response, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil) @@ -471,7 +469,7 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent if response == nil { l.Warn("okta-connector: failed to fetch application group assignment, nil response", zap.String("app_id", appID), zap.String("group_id", groupID), zap.Error(err)) - return nil, fmt.Errorf("okta-connector: failed to fetch application group assignment: %s", err.Error()) + return nil, fmt.Errorf("okta-connector: failed to fetch application group assignment: %w", handleOktaResponseError(response, err)) } defer response.Body.Close() errOkta, err := getError(response) @@ -493,7 +491,7 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent } if appGroup != nil && groupID == appGroup.Id { - l.Warn( + l.Debug( "okta-connector: The app specified is already assigned to the group", zap.String("principal_id", principal.Id.String()), zap.String("principal_type", principal.Id.ResourceType), @@ -503,20 +501,20 @@ func (g *appResourceType) Grant(ctx context.Context, principal *v2.Resource, ent } payload := okta.ApplicationGroupAssignment{} - assignedGroup, _, err := g.client.Application.CreateApplicationGroupAssignment(ctx, appID, groupID, payload) + assignedGroup, createResp, err := g.client.Application.CreateApplicationGroupAssignment(ctx, appID, groupID, payload) if err != nil { - return nil, err + return nil, handleOktaResponseError(createResp, err) } - l.Warn("App Membership has been created.", + l.Debug("App Membership has been created.", zap.String("userID", assignedGroup.Id), zap.Time("LastUpdated", *assignedGroup.LastUpdated), ) + + return rateLimitAnnotations(createResp), nil default: return nil, fmt.Errorf("okta-connector: invalid grant resource type: %s", principal.Id.ResourceType) } - - return nil, nil } func (g *appResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) { @@ -538,59 +536,61 @@ func (g *appResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotati userID := principal.Id.Resource _, resp, err := g.client.Application.GetApplicationUser(ctx, appID, userID, nil) if err != nil { - if resp != nil && resp.StatusCode == http.StatusNotFound { - l.Debug( - "okta-connector: revoke: user does not have app membership", - zap.String("principal_id", principal.Id.String()), - zap.String("principal_type", principal.Id.ResourceType), - ) - return annotations.New(&v2.GrantAlreadyRevoked{}), nil - } - l.Warn( - "okta-connector: user does not have app membership", - zap.String("principal_id", principal.Id.String()), - zap.String("principal_type", principal.Id.ResourceType), - ) - return nil, fmt.Errorf("okta-connector: user does not have app membership: %s", err.Error()) + return appRevokeNotFoundOrError(l, principal, resp, err, "user") } response, err := g.client.Application.DeleteApplicationUser(ctx, appID, userID, nil) if err != nil { - return nil, fmt.Errorf("okta-connector: failed to remove user from application: %s", err.Error()) + return nil, fmt.Errorf("okta-connector: failed to remove user from application: %w", handleOktaResponseError(response, err)) } + logAppMembershipRevoked(l, response) - if response != nil && response.StatusCode == http.StatusNoContent { - l.Warn("Membership has been revoked", - zap.String("Status", response.Status), - ) - } + return rateLimitAnnotations(response), nil case resourceTypeGroup.Id: groupID := principal.Id.Resource - _, _, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil) + _, groupResp, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil) if err != nil { - l.Warn( - "okta-connector: group does not have app membership", - zap.String("principal_id", principal.Id.String()), - zap.String("principal_type", principal.Id.ResourceType), - ) - return nil, fmt.Errorf("okta-connector: group does not have app membership: %s", err.Error()) + return appRevokeNotFoundOrError(l, principal, groupResp, err, "group") } response, err := g.client.Application.DeleteApplicationGroupAssignment(ctx, appID, groupID) if err != nil { - return nil, fmt.Errorf("okta-connector: failed to remove group from application: %s", err.Error()) + return nil, fmt.Errorf("okta-connector: failed to remove group from application: %w", handleOktaResponseError(response, err)) } + logAppMembershipRevoked(l, response) - if response != nil && response.StatusCode == http.StatusNoContent { - l.Warn("Membership has been revoked", - zap.String("Status", response.Status), - ) - } + return rateLimitAnnotations(response), nil default: return nil, fmt.Errorf("okta-connector: invalid grant resource type: %s", principal.Id.ResourceType) } +} + +// appRevokeNotFoundOrError classifies a failed pre-delete existence check ("user" or +// "group" kind): already-gone yields GrantAlreadyRevoked, else the wrapped lookup error. +func appRevokeNotFoundOrError(l *zap.Logger, principal *v2.Resource, resp *okta.Response, err error, kind string) (annotations.Annotations, error) { + if isRevokeNotFoundError(resp, err) { + l.Debug( + fmt.Sprintf("okta-connector: revoke: %s does not have app membership", kind), + zap.String("principal_id", principal.Id.String()), + zap.String("principal_type", principal.Id.ResourceType), + ) + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } - return nil, nil + l.Warn( + fmt.Sprintf("okta-connector: %s does not have app membership", kind), + zap.String("principal_id", principal.Id.String()), + zap.String("principal_type", principal.Id.ResourceType), + ) + return nil, fmt.Errorf("okta-connector: %s does not have app membership: %w", kind, handleOktaResponseError(resp, err)) +} + +// logAppMembershipRevoked logs the NoContent delete confirmation shared by both +// app revoke branches. +func logAppMembershipRevoked(l *zap.Logger, resp *okta.Response) { + if resp != nil && resp.StatusCode == http.StatusNoContent { + l.Debug("Membership has been revoked", zap.String("Status", resp.Status)) + } } func (o *appResourceType) Get(ctx context.Context, resourceId *v2.ResourceId, parentResourceId *v2.ResourceId) (*v2.Resource, annotations.Annotations, error) { diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index d32b6de2..eac6a2ee 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -33,6 +33,10 @@ const oktaURLScheme = "https" // oktaSDKAuthSentinel activates the SDK's Bearer auth path; the oktaauth RoundTripper substitutes the real DPoP/Bearer token per request. const oktaSDKAuthSentinel = "dpop-managed" +// oktaRateLimitMaxBackoffSeconds covers Okta's ~60s rate-limit reset window; the bigger +// fix is classifying 429 as Unavailable (rateLimitError) so provisioning retries at all. +const oktaRateLimitMaxBackoffSeconds = 60 + type Okta struct { client *okta.Client clientV5 *oktav5.APIClient @@ -429,6 +433,7 @@ func New(ctx context.Context, cc *cfg.Okta, opts *cli.ConnectorOpts) (connectorb okta.WithCache(cc.Cache), okta.WithCacheTti(cacheTTI), okta.WithCacheTtl(cacheTTL), + okta.WithRateLimitMaxBackOff(oktaRateLimitMaxBackoffSeconds), ) if err != nil { return nil, nil, err @@ -479,6 +484,7 @@ func New(ctx context.Context, cc *cfg.Okta, opts *cli.ConnectorOpts) (connectorb okta.WithCache(cc.Cache), okta.WithCacheTti(cacheTTI), okta.WithCacheTtl(cacheTTL), + okta.WithRateLimitMaxBackOff(oktaRateLimitMaxBackoffSeconds), ) if err != nil { return nil, nil, err diff --git a/pkg/connector/error_classification_test.go b/pkg/connector/error_classification_test.go new file mode 100644 index 00000000..1d112a8a --- /dev/null +++ b/pkg/connector/error_classification_test.go @@ -0,0 +1,324 @@ +package connector + +import ( + "context" + "errors" + "net/http" + "net/url" + "strconv" + "strings" + "testing" + "time" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/annotations" + "github.com/okta/okta-sdk-golang/v2/okta" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +const ( + testGroupID = "00g1abc2def3GHI4jk5" + testAppID = "0oa1abc2def3GHI4jk5" +) + +// rateLimitedStep builds a 429 response with the headers the v2 SDK's own retry +// logic requires (Get429BackoffTime); missing/invalid ones make it fail with a +// header-parse error instead of reproducing the "too many requests" sentinel. +func rateLimitedStep(method, path string) oktaRequestStep { + now := time.Now().UTC() + return oktaRequestStep{ + method: method, + path: path, + statusCode: http.StatusTooManyRequests, + headers: map[string]string{ + "Date": now.Format(http.TimeFormat), + "X-Rate-Limit-Limit": "20", + "X-Rate-Limit-Remaining": "0", + "X-Rate-Limit-Reset": strconv.FormatInt(now.Add(2*time.Second).Unix(), 10), + }, + body: `{"errorCode":"E0000047","errorSummary":"API call exceeded rate limit"}`, + } +} + +// testUserPrincipal is the user principal shared by group-membership and app-access tests. +func testUserPrincipal() *v2.Resource { + return &v2.Resource{Id: &v2.ResourceId{ResourceType: resourceTypeUser.Id, Resource: testOktaUserID}} +} + +func groupMembershipEntitlement() *v2.Entitlement { + return &v2.Entitlement{Resource: &v2.Resource{Id: &v2.ResourceId{ResourceType: resourceTypeGroup.Id, Resource: testGroupID}}} +} + +func appGroupPrincipal() *v2.Resource { + return &v2.Resource{Id: &v2.ResourceId{ResourceType: resourceTypeGroup.Id, Resource: testGroupID}} +} + +func appAccessEntitlement() *v2.Entitlement { + return &v2.Entitlement{Resource: &v2.Resource{Id: &v2.ResourceId{ResourceType: resourceTypeApp.Id, Resource: testAppID}}} +} + +func oktaAppUserAssignedResponse() string { + return `{"id":"` + testOktaUserID + `","status":"ACTIVE","scope":"USER","lastUpdated":"2024-01-01T00:00:00.000Z"}` +} + +func oktaAppGroupAssignmentResponse() string { + return `{"id":"` + testGroupID + `","lastUpdated":"2024-01-01T00:00:00.000Z"}` +} + +func newTestAppBuilder(client *okta.Client) *appResourceType { + return appBuilder("", "", false, nil, client) +} + +// TestRateLimitClassification proves the wiring end to end: a 429 exhausting the +// v2 SDK's own retries reaches the connector as codes.Unavailable, not codes.Unknown. +func TestRateLimitClassification(t *testing.T) { + t.Run("group grant", func(t *testing.T) { + client := newScriptedOktaClient(t, + rateLimitedStep(http.MethodPut, "/api/v1/groups/"+testGroupID+"/users/"+testOktaUserID), + ) + + _, err := groupBuilder(&Okta{client: client}).Grant(t.Context(), testUserPrincipal(), groupMembershipEntitlement()) + if status.Code(err) != codes.Unavailable { + t.Fatalf("Grant() status = %s, want %s (error: %v)", status.Code(err), codes.Unavailable, err) + } + }) + + t.Run("group revoke", func(t *testing.T) { + client := newScriptedOktaClient(t, + rateLimitedStep(http.MethodDelete, "/api/v1/groups/"+testGroupID+"/users/"+testOktaUserID), + ) + grant := &v2.Grant{Principal: testUserPrincipal(), Entitlement: groupMembershipEntitlement()} + + _, err := groupBuilder(&Okta{client: client}).Revoke(t.Context(), grant) + if status.Code(err) != codes.Unavailable { + t.Fatalf("Revoke() status = %s, want %s (error: %v)", status.Code(err), codes.Unavailable, err) + } + }) + + t.Run("app grant", func(t *testing.T) { + client := newScriptedOktaClient(t, + rateLimitedStep(http.MethodGet, "/api/v1/apps/"+testAppID+"/users/"+testOktaUserID), + ) + + _, err := newTestAppBuilder(client).Grant(t.Context(), testUserPrincipal(), appAccessEntitlement()) + if status.Code(err) != codes.Unavailable { + t.Fatalf("Grant() status = %s, want %s (error: %v)", status.Code(err), codes.Unavailable, err) + } + }) +} + +func TestRevokeIdempotency(t *testing.T) { + t.Run("app revoke: missing user is already revoked", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: http.MethodGet, path: "/api/v1/apps/" + testAppID + "/users/" + testOktaUserID, statusCode: http.StatusNotFound, body: oktaNotFoundResponse()}, + ) + grant := &v2.Grant{Principal: testUserPrincipal(), Entitlement: appAccessEntitlement()} + + annos, err := newTestAppBuilder(client).Revoke(t.Context(), grant) + if err != nil { + t.Fatalf("Revoke() error: %v", err) + } + if !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke() annotations = %v, want GrantAlreadyRevoked", annos) + } + }) + + t.Run("app revoke: missing group is already revoked", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: http.MethodGet, path: "/api/v1/apps/" + testAppID + "/groups/" + testGroupID, statusCode: http.StatusNotFound, body: oktaNotFoundResponse()}, + ) + grant := &v2.Grant{Principal: appGroupPrincipal(), Entitlement: appAccessEntitlement()} + + annos, err := newTestAppBuilder(client).Revoke(t.Context(), grant) + if err != nil { + t.Fatalf("Revoke() error: %v", err) + } + if !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke() annotations = %v, want GrantAlreadyRevoked", annos) + } + }) + + t.Run("group revoke: missing membership is already revoked", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: http.MethodDelete, path: "/api/v1/groups/" + testGroupID + "/users/" + testOktaUserID, statusCode: http.StatusNotFound, body: oktaNotFoundResponse()}, + ) + grant := &v2.Grant{Principal: testUserPrincipal(), Entitlement: groupMembershipEntitlement()} + + annos, err := groupBuilder(&Okta{client: client}).Revoke(t.Context(), grant) + if err != nil { + t.Fatalf("Revoke() error: %v", err) + } + if !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke() annotations = %v, want GrantAlreadyRevoked", annos) + } + }) +} + +func TestAppGrantIdempotency(t *testing.T) { + t.Run("missing app user proceeds to assign", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: http.MethodGet, path: "/api/v1/apps/" + testAppID + "/users/" + testOktaUserID, statusCode: http.StatusNotFound, body: oktaNotFoundResponse()}, + oktaRequestStep{method: http.MethodGet, path: "/api/v1/users/" + testOktaUserID, statusCode: http.StatusOK, body: oktaUserResponse(userStatusActive)}, + oktaRequestStep{method: http.MethodPost, path: "/api/v1/apps/" + testAppID + "/users", statusCode: http.StatusOK, body: oktaAppUserAssignedResponse()}, + ) + + if _, err := newTestAppBuilder(client).Grant(t.Context(), testUserPrincipal(), appAccessEntitlement()); err != nil { + t.Fatalf("Grant() error: %v", err) + } + }) + + t.Run("already assigned user is a no-op", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: http.MethodGet, path: "/api/v1/apps/" + testAppID + "/users/" + testOktaUserID, statusCode: http.StatusOK, body: oktaAppUserAssignedResponse()}, + ) + + annos, err := newTestAppBuilder(client).Grant(t.Context(), testUserPrincipal(), appAccessEntitlement()) + if err != nil { + t.Fatalf("Grant() error: %v", err) + } + if !annos.Contains(&v2.GrantAlreadyExists{}) { + t.Fatalf("Grant() annotations = %v, want GrantAlreadyExists", annos) + } + }) + + t.Run("missing app group proceeds to assign", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: http.MethodGet, path: "/api/v1/apps/" + testAppID + "/groups/" + testGroupID, statusCode: http.StatusNotFound, body: oktaNotFoundResponse()}, + oktaRequestStep{method: http.MethodPut, path: "/api/v1/apps/" + testAppID + "/groups/" + testGroupID, statusCode: http.StatusOK, body: oktaAppGroupAssignmentResponse()}, + ) + + if _, err := newTestAppBuilder(client).Grant(t.Context(), appGroupPrincipal(), appAccessEntitlement()); err != nil { + t.Fatalf("Grant() error: %v", err) + } + }) +} + +// TestHandleOktaResponseErrorClassification is a pure unit test (no server) that +// pins the pre-existing classification behavior alongside the new 429 case, so a +// future change to rate-limit handling can't silently regress the others. +func TestHandleOktaResponseErrorClassification(t *testing.T) { + tests := []struct { + name string + resp *okta.Response + err error + want codes.Code + }{ + { + name: "okta not-found error code maps to NotFound", + resp: &okta.Response{Response: &http.Response{StatusCode: http.StatusNotFound}}, + err: &okta.Error{ErrorCode: "E0000007"}, + want: codes.NotFound, + }, + { + name: "5xx status maps to Unavailable", + resp: &okta.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}}, + err: errors.New("server error"), + want: codes.Unavailable, + }, + { + name: "context deadline exceeded maps to DeadlineExceeded", + err: &url.Error{Op: "Put", URL: "https://example.okta.com", Err: context.DeadlineExceeded}, + want: codes.DeadlineExceeded, + }, + { + name: "429 status with response maps to Unavailable", + resp: &okta.Response{Response: &http.Response{StatusCode: http.StatusTooManyRequests}}, + err: errors.New("unexpected status code: 429"), + want: codes.Unavailable, + }, + { + name: "too many requests sentinel without response maps to Unavailable", + err: errors.New("too many requests"), + want: codes.Unavailable, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := handleOktaResponseError(tt.resp, tt.err) + if got := status.Code(err); got != tt.want { + t.Errorf("handleOktaResponseError() status = %s, want %s (error: %v)", got, tt.want, err) + } + }) + } +} + +// TestServerErrorPreservesOktaErrorText proves a 5xx no longer discards the original +// Okta error (and its x-okta-request-id) behind the generic "server error" status. +func TestServerErrorPreservesOktaErrorText(t *testing.T) { + base := errors.New("server error, x-okta-request-id=test-request-id") + resp := &okta.Response{Response: &http.Response{StatusCode: http.StatusInternalServerError}} + + err := handleOktaResponseError(resp, base) + if status.Code(err) != codes.Unavailable { + t.Fatalf("handleOktaResponseError() status = %s, want %s (error: %v)", status.Code(err), codes.Unavailable, err) + } + if !errors.Is(err, base) { + t.Fatalf("handleOktaResponseError() lost the original Okta error: %v", err) + } + if !strings.Contains(err.Error(), "x-okta-request-id=test-request-id") { + t.Fatalf("handleOktaResponseError() error text lost the Okta request id: %v", err) + } +} + +// TestRevokeAcceptsEitherNotFoundSignal proves all three revoke paths treat an HTTP 404 +// and a classified codes.NotFound as the same idempotent outcome, even when only one of +// the two signals is present in the response. +func TestRevokeAcceptsEitherNotFoundSignal(t *testing.T) { + type revoker func(t *testing.T, client *okta.Client) (annotations.Annotations, error) + + appUserRevoke := func(t *testing.T, client *okta.Client) (annotations.Annotations, error) { + grant := &v2.Grant{Principal: testUserPrincipal(), Entitlement: appAccessEntitlement()} + return newTestAppBuilder(client).Revoke(t.Context(), grant) + } + appGroupRevoke := func(t *testing.T, client *okta.Client) (annotations.Annotations, error) { + grant := &v2.Grant{Principal: appGroupPrincipal(), Entitlement: appAccessEntitlement()} + return newTestAppBuilder(client).Revoke(t.Context(), grant) + } + groupRevoke := func(t *testing.T, client *okta.Client) (annotations.Annotations, error) { + grant := &v2.Grant{Principal: testUserPrincipal(), Entitlement: groupMembershipEntitlement()} + return groupBuilder(&Okta{client: client}).Revoke(t.Context(), grant) + } + + paths := []struct { + name string + method string + path string + revoke revoker + }{ + {"app user revoke", http.MethodGet, "/api/v1/apps/" + testAppID + "/users/" + testOktaUserID, appUserRevoke}, + {"app group revoke", http.MethodGet, "/api/v1/apps/" + testAppID + "/groups/" + testGroupID, appGroupRevoke}, + {"group revoke", http.MethodDelete, "/api/v1/groups/" + testGroupID + "/users/" + testOktaUserID, groupRevoke}, + } + + for _, p := range paths { + t.Run(p.name+": 404 status with an unrelated error body", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: p.method, path: p.path, statusCode: http.StatusNotFound, body: oktaLifecycleErrorResponse()}, + ) + + annos, err := p.revoke(t, client) + if err != nil { + t.Fatalf("Revoke() error: %v", err) + } + if !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke() annotations = %v, want GrantAlreadyRevoked", annos) + } + }) + + t.Run(p.name+": non-404 status with a not-found error body", func(t *testing.T) { + client := newScriptedOktaClient(t, + oktaRequestStep{method: p.method, path: p.path, statusCode: http.StatusBadRequest, body: oktaNotFoundResponse()}, + ) + + annos, err := p.revoke(t, client) + if err != nil { + t.Fatalf("Revoke() error: %v", err) + } + if !annos.Contains(&v2.GrantAlreadyRevoked{}) { + t.Fatalf("Revoke() annotations = %v, want GrantAlreadyRevoked", annos) + } + }) + } +} diff --git a/pkg/connector/group.go b/pkg/connector/group.go index 5df25536..d8fe504d 100644 --- a/pkg/connector/group.go +++ b/pkg/connector/group.go @@ -464,7 +464,7 @@ func (g *groupResourceType) Grant(ctx context.Context, principal *v2.Resource, e l.Debug("Membership has been created") } - return nil, nil + return rateLimitAnnotations(response), nil } func (g *groupResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotations.Annotations, error) { @@ -485,16 +485,24 @@ func (g *groupResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annota response, err := g.connector.client.Group.RemoveUserFromGroup(ctx, groupId, userId) if err != nil { + if isRevokeNotFoundError(response, err) { + l.Debug( + "okta-connector: revoke: user does not have group membership", + zap.String("principal_id", principal.Id.String()), + zap.String("principal_type", principal.Id.ResourceType), + ) + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } return nil, handleOktaResponseError(response, err) } if response != nil { - l.Warn("Membership has been revoked", zap.String("Status", response.Status)) + l.Debug("Membership has been revoked", zap.String("Status", response.Status)) } else { - l.Warn("Membership has been revoked") + l.Debug("Membership has been revoked") } - return nil, nil + return rateLimitAnnotations(response), nil } func (o *groupResourceType) Get(ctx context.Context, resourceId *v2.ResourceId, parentResourceId *v2.ResourceId) (*v2.Resource, annotations.Annotations, error) { diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index abf2c8f4..352b58a8 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -6,10 +6,13 @@ import ( "errors" "fmt" "io" + "net/http" "net/url" "strings" + "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/pagination" + "github.com/conductorone/baton-sdk/pkg/ratelimit" "github.com/conductorone/baton-sdk/pkg/uhttp" "github.com/okta/okta-sdk-golang/v2/okta" "github.com/okta/okta-sdk-golang/v2/okta/query" @@ -142,6 +145,12 @@ func handleOktaResponseError(resp *okta.Response, err error) error { return status.Error(codes.DeadlineExceeded, "request timeout") } + // A 429 exhausting the v2 SDK's own retries drops the response, leaving only the + // "too many requests" sentinel; check that before the response-based paths below. + if isRateLimitError(resp, err) { + return rateLimitError(resp, err) + } + var oktaApiError *okta.Error if errors.As(err, &oktaApiError) { grpcErrCode, ok := oktaErrToGRPCError[oktaApiError.ErrorCode] @@ -159,6 +168,47 @@ func handleOktaResponseError(resp *okta.Response, err error) error { return err } +// isRevokeNotFoundError reports whether a revoke's failed check means the assignment is +// already gone: an HTTP 404, or a classified codes.NotFound — both mean the same thing. +func isRevokeNotFoundError(resp *okta.Response, err error) bool { + if resp != nil && resp.StatusCode == http.StatusNotFound { + return true + } + return status.Code(handleOktaResponseError(resp, err)) == codes.NotFound +} + +// isRateLimitError reports a 429 from the response status, or from the "too many +// requests" sentinel once the v2 SDK exhausts its own retries and drops the response. +func isRateLimitError(resp *okta.Response, err error) bool { + if resp != nil && resp.StatusCode == http.StatusTooManyRequests { + return true + } + return err != nil && strings.Contains(err.Error(), "too many requests") +} + +// rateLimitError classifies rate limits as codes.Unavailable, not ResourceExhausted: +// baton-sdk's provisioning retryer only waits and retries on Unavailable/DeadlineExceeded. +func rateLimitError(resp *okta.Response, err error) error { + if resp != nil && resp.Response != nil { + return uhttp.WrapErrorsWithRateLimitInfo(codes.Unavailable, resp.Response, err) + } + return uhttp.WrapErrors(codes.Unavailable, "rate limited by Okta", err) +} + +// rateLimitAnnotations extracts rate-limit info from a successful response, nil-safe. +// desc can be a typed-nil *v2.RateLimitDescription that WithRateLimiting won't catch, +// so the nil check happens here instead. +func rateLimitAnnotations(resp *okta.Response) annotations.Annotations { + var annos annotations.Annotations + if resp == nil || resp.Response == nil { + return annos + } + if desc, err := ratelimit.ExtractRateLimitData(resp.StatusCode, &resp.Header); err == nil && desc != nil { + annos.WithRateLimiting(desc) + } + return annos +} + // apiValidationFailedErrorCode covers every Create User validation failure, so a // duplicate login has to be confirmed from errorCauses before treating it as one. // https://developer.okta.com/docs/reference/error-codes/#E0000001 diff --git a/pkg/connector/user_deprovisioning_test.go b/pkg/connector/user_deprovisioning_test.go index 31042966..7b0b1d9f 100644 --- a/pkg/connector/user_deprovisioning_test.go +++ b/pkg/connector/user_deprovisioning_test.go @@ -26,6 +26,7 @@ type oktaRequestStep struct { query map[string]string statusCode int body string + headers map[string]string } func newScriptedOktaClient(t *testing.T, steps ...oktaRequestStep) *okta.Client { @@ -56,6 +57,9 @@ func newScriptedOktaClient(t *testing.T, steps ...oktaRequestStep) *okta.Client t.Errorf("request %d query %s = %q, want %q", next, key, got, want) } } + for key, value := range step.headers { + w.Header().Set(key, value) + } writeOktaTestResponse(w, step.statusCode, step.body) })) t.Cleanup(func() {