From c479230e95fe28a8ddffc1f1c44542c2d3a2734a Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 16:17:10 -0300 Subject: [PATCH 01/28] fix: classify DocuSign's hourly-limit error as retryable, not fatal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pylon #11445 (The Trade Desk): initial full sync fails non-recoverably once an account exceeds DocuSign's hourly API-call budget. DocuSign signals this via a JSON error-body errorCode (HOURLY_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400 — DocuSign is mid-migration to 429 and documents parsing errorCode instead of relying on status code. uhttp.GrpcCodeFromHTTPStatus maps 400 to codes.InvalidArgument, which the SDK's sync-retry loop (pkg/sync's Retryer, wired to SyncResourcesOp/SyncGrantsOp) treats as fatal — it only waits and retries on Unavailable/DeadlineExceeded — so an otherwise-recoverable rate limit surfaced as a permanent sync failure with no checkpoint resume. doRequestCommon now recognizes this errorCode independent of HTTP status (so it keeps working once DocuSign flips to 429) and reclassifies it as codes.Unavailable with a RateLimitDescription attached via status details, so the SDK's existing retry loop picks it up and the sync pauses/resumes instead of failing outright. --- pkg/client/helper.go | 73 ++++++++++++++++ pkg/client/helper_test.go | 177 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 250 insertions(+) create mode 100644 pkg/client/helper_test.go diff --git a/pkg/client/helper.go b/pkg/client/helper.go index 45286c93..39aeac3d 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -6,14 +6,80 @@ import ( "fmt" "net/http" "net/url" + "time" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/ratelimit" "github.com/conductorone/baton-sdk/pkg/uhttp" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + "google.golang.org/protobuf/types/known/timestamppb" ) const DefaultPageSize = 100 +// docusignHourlyRateLimitErrorCode is the eSignature API's JSON error-body errorCode for +// "the account's hourly API-call budget is exhausted" — confirmed against a real account +// (Pylon #11445). DocuSign returns this as HTTP 400 today and is mid-migration to 429 +// (DocuSign's own guidance is to key off errorCode, not HTTP status, for exactly this +// reason), so detection below checks the body field independent of resp.StatusCode. +const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" + +// docusignRateLimitDefaultResetWindow is used when DocuSign's response carries no +// X-RateLimit-Reset (or equivalent) header to derive a reset time from — the limit this +// error names is hourly, so an hour is the sane default, matching the spirit of (but +// longer than) uhttp/ratelimit's own 60s default for a headerless 429. +const docusignRateLimitDefaultResetWindow = time.Hour + +// rateLimitErrorFromResponse recognizes docusignHourlyRateLimitErrorCode in errTarget (the +// same *ErrorResponse instance uhttp.WithErrorResponse already unmarshaled the error body +// into before returning origErr — no re-parsing needed) and, if matched, returns a +// codes.Unavailable error carrying a RateLimitDescription. This matters because +// uhttp.GrpcCodeFromHTTPStatus maps this error's current HTTP 400 to codes.InvalidArgument, +// which the SDK's sync-retry loop (pkg/sync's Retryer, wired to SyncResourcesOp/ +// SyncGrantsOp) does not retry — it only waits and retries on Unavailable/DeadlineExceeded, +// so an otherwise-recoverable rate limit was surfacing as a fatal, non-resumable sync +// failure. Returns nil (unchanged behavior) when errTarget isn't this specific eSignature +// error shape, or the errorCode doesn't match — including every ClmErrorResponse-based CLM +// call, which is a distinct error envelope this func never matches. +func rateLimitErrorFromResponse(resp *http.Response, errTarget uhttp.ErrorResponse, origErr error) error { + er, ok := errTarget.(*ErrorResponse) + if !ok || er.ErrorCode != docusignHourlyRateLimitErrorCode { + return nil + } + + desc, _ := ratelimit.ExtractRateLimitData(resp.StatusCode, &resp.Header) + resetAt := timestamppb.New(time.Now().Add(docusignRateLimitDefaultResetWindow)) + var limit, remaining int64 + if desc != nil { + // ExtractRateLimitData always returns a non-nil ResetAt — timestamppb.New of the + // zero time.Time when no reset header matched, not nil — so a nil check alone + // would wrongly accept that zero value over the sane default above. + if resetAtTime := desc.GetResetAt().AsTime(); !resetAtTime.IsZero() { + resetAt = desc.GetResetAt() + } + limit = desc.GetLimit() + remaining = desc.GetRemaining() + } + + st := status.New(codes.Unavailable, origErr.Error()) + withDetails, detailsErr := st.WithDetails(v2.RateLimitDescription_builder{ + Status: v2.RateLimitDescription_STATUS_OVERLIMIT, + Limit: limit, + Remaining: remaining, + ResetAt: resetAt, + }.Build()) + if detailsErr != nil { + // WithDetails only fails for a codes.OK status or a detail that can't marshal to + // an Any — neither applies here (fixed codes.Unavailable, a well-formed proto + // message) — but fall back to the plain Unavailable classification (still + // retryable) rather than losing that reclassification entirely if it somehow does. + return st.Err() + } + return withDetails.Err() +} + // BuildURL combines the base API URL with a formatted endpoint path. func buildURL(base, path string, params ...any) (*url.URL, error) { baseURL, err := url.Parse(base) @@ -39,6 +105,13 @@ func doRequestCommon(wrapper *uhttp.BaseHttpClient, req *http.Request, res any, opts = append(opts, uhttp.WithErrorResponse(errTarget)) resp, err := wrapper.Do(req, opts...) if err != nil { + // resp is non-nil here whenever the error came from a well-formed non-2xx HTTP + // response (as opposed to a network/transport failure) — see wrapper.Do. + if resp != nil { + if rlErr := rateLimitErrorFromResponse(resp, errTarget, err); rlErr != nil { + return resp.Header, nil, rlErr + } + } return nil, nil, err } defer resp.Body.Close() diff --git a/pkg/client/helper_test.go b/pkg/client/helper_test.go new file mode 100644 index 00000000..aa4fc23c --- /dev/null +++ b/pkg/client/helper_test.go @@ -0,0 +1,177 @@ +package client + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "testing" + "time" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/uhttp" + "golang.org/x/oauth2" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +// TestRateLimitErrorFromResponse is a regression test for Pylon #11445: DocuSign signals +// "hourly API-call budget exhausted" via a JSON error body (errorCode +// HOURLY_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400, which uhttp.GrpcCodeFromHTTPStatus maps +// to codes.InvalidArgument — a code the SDK's sync-retry loop treats as fatal, not +// retryable, so a real customer's initial full sync failed outright instead of pausing and +// resuming. rateLimitErrorFromResponse must re-classify exactly this case as +// codes.Unavailable (which the SDK does retry) carrying a RateLimitDescription, and leave +// every other error (including CLM's distinct error envelope) untouched. +func TestRateLimitErrorFromResponse(t *testing.T) { + origErr := errors.New("400 Bad Request") + + t.Run("matches on errorCode regardless of HTTP status", func(t *testing.T) { + for _, statusCode := range []int{http.StatusBadRequest, http.StatusTooManyRequests} { + resp := &http.Response{ + StatusCode: statusCode, + Header: http.Header{}, + } + errTarget := &ErrorResponse{ + ErrorCode: docusignHourlyRateLimitErrorCode, + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", + } + + got := rateLimitErrorFromResponse(resp, errTarget, origErr) + if got == nil { + t.Fatalf("status %d: expected a rate-limit error, got nil", statusCode) + } + st, ok := status.FromError(got) + if !ok { + t.Fatalf("status %d: expected a gRPC status error, got %v", statusCode, got) + } + if st.Code() != codes.Unavailable { + t.Errorf("status %d: expected codes.Unavailable, got %v", statusCode, st.Code()) + } + + var desc *v2.RateLimitDescription + for _, d := range st.Details() { + if rl, ok := d.(*v2.RateLimitDescription); ok { + desc = rl + } + } + if desc == nil { + t.Fatalf("status %d: expected a RateLimitDescription in the error's status details, got %+v", statusCode, st.Details()) + } + if desc.GetStatus() != v2.RateLimitDescription_STATUS_OVERLIMIT { + t.Errorf("status %d: expected STATUS_OVERLIMIT, got %v", statusCode, desc.GetStatus()) + } + if desc.GetResetAt() == nil || desc.GetResetAt().AsTime().Before(time.Now()) { + t.Errorf("status %d: expected a future ResetAt when no header is present, got %v", statusCode, desc.GetResetAt()) + } + } + }) + + t.Run("prefers the X-Ratelimit-Reset header over the default window", func(t *testing.T) { + wantResetAt := time.Now().Add(5 * time.Minute).Truncate(time.Second) + resp := &http.Response{ + StatusCode: http.StatusBadRequest, + Header: http.Header{ + "X-Ratelimit-Reset": []string{strconv.FormatInt(wantResetAt.Unix(), 10)}, + }, + } + errTarget := &ErrorResponse{ErrorCode: docusignHourlyRateLimitErrorCode} + + got := rateLimitErrorFromResponse(resp, errTarget, origErr) + if got == nil { + t.Fatal("expected a rate-limit error, got nil") + } + st, _ := status.FromError(got) + var desc *v2.RateLimitDescription + for _, d := range st.Details() { + if rl, ok := d.(*v2.RateLimitDescription); ok { + desc = rl + } + } + if desc == nil { + t.Fatal("expected a RateLimitDescription in the error's status details") + } + if got, want := desc.GetResetAt().AsTime().Unix(), wantResetAt.Unix(); got != want { + t.Errorf("expected ResetAt derived from the header (%d), got %d", want, got) + } + }) + + t.Run("does not match an unrelated errorCode", func(t *testing.T) { + resp := &http.Response{StatusCode: http.StatusBadRequest, Header: http.Header{}} + errTarget := &ErrorResponse{ErrorCode: "USER_LACKS_PERMISSIONS"} + + if got := rateLimitErrorFromResponse(resp, errTarget, origErr); got != nil { + t.Errorf("expected nil for an unrelated errorCode, got %v", got) + } + }) + + t.Run("does not match CLM's distinct error envelope", func(t *testing.T) { + resp := &http.Response{StatusCode: http.StatusBadRequest, Header: http.Header{}} + // ClmErrorResponse is a different type from *ErrorResponse even if some CLM error + // happened to carry the same string in an analogous field — the type assertion + // alone must reject it, since this function's evidence is eSignature-specific. + errTarget := &ClmErrorResponse{} + + if got := rateLimitErrorFromResponse(resp, errTarget, origErr); got != nil { + t.Errorf("expected nil for a non-eSignature error envelope, got %v", got) + } + }) +} + +// TestGetUsers_ClassifiesHourlyRateLimitAsRetryable is an end-to-end regression test for +// Pylon #11445, exercising the real request path (GetUsers -> doRequestCommon -> +// rateLimitErrorFromResponse) against a mock server that returns DocuSign's actual +// observed 400 body, rather than calling rateLimitErrorFromResponse directly. +func TestGetUsers_ClassifiesHourlyRateLimitAsRetryable(t *testing.T) { + mockServer := httptest.NewServer(nil) + defer mockServer.Close() + + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + if r.URL.Path == "/oauth/userinfo" { + _ = json.NewEncoder(w).Encode(UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + return + } + // GetUsers -> /restapi/v2.1/accounts/{id}/users + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(ErrorResponse{ + ErrorCode: docusignHourlyRateLimitErrorCode, + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", + }) + }) + + mockServerURL, _ := url.Parse(mockServer.URL) + transport := &rewriteTransport{target: mockServerURL, base: http.DefaultTransport} + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: transport}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + c := NewClient(context.Background(), false, tokenSource, "", "", wrapper) + + _, _, _, err := c.GetUsers(context.Background(), PageOptions{}) + if err == nil { + t.Fatal("expected GetUsers to surface the hourly rate-limit error, got nil") + } + if got := status.Code(err); got != codes.Unavailable { + t.Fatalf("expected codes.Unavailable (retryable by the SDK's sync-retry loop), got %v: %v", got, err) + } + st, _ := status.FromError(err) + var desc *v2.RateLimitDescription + for _, d := range st.Details() { + if rl, ok := d.(*v2.RateLimitDescription); ok { + desc = rl + } + } + if desc == nil { + t.Fatalf("expected the error to carry a RateLimitDescription, got details: %+v", st.Details()) + } + if desc.GetStatus() != v2.RateLimitDescription_STATUS_OVERLIMIT { + t.Errorf("expected STATUS_OVERLIMIT, got %v", desc.GetStatus()) + } +} From 08e6adbfdb7a49d2a9d9ed3f67c73c21c72c1f2f Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 16:53:07 -0300 Subject: [PATCH 02/28] fix: avoid per-user GetUserDetails in Grants for active users (Pylon #11445) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit userBuilder.Grants() called GetUserDetails once per user, every sync — real N+1 amplification contributing to accounts hitting DocuSign's hourly call budget. The list response (GetUsers) already carries the user's permission profile name and status, both already captured on the resource's profile during List(). Grants now resolves the profile ID via GetPermissionProfiles (one account-wide call, already served from uhttp's default GET cache on repeat calls within a sync) instead, gated on status == Active — the same distinction GetUserDetails.PermissionProfileID-empty already relies on, not a new assumption. Falls back to the original per-user GetUserDetails call unchanged whenever the user isn't Active, the profile field is absent, the cached name no longer resolves (renamed/deleted since listing), or the GetPermissionProfiles call fails. An earlier version of this fix cached GetPermissionProfiles for the builder's lifetime via sync.Once — dropped after review found it silently diverged from the old code for non-active users (no status check at all) and duplicated caching uhttp's GET client already provides by default. --- pkg/connector/helper.go | 14 ++- pkg/connector/users.go | 53 ++++++++--- pkg/connector/users_test.go | 172 ++++++++++++++++++++++++++++++++++++ 3 files changed, 225 insertions(+), 14 deletions(-) create mode 100644 pkg/connector/users_test.go diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index e4532098..e7e07391 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -12,11 +12,19 @@ import ( // Shared profile/field map keys, reused across builders (and the AccountCreationSchema // field map in connector.go) to avoid repeated string literals (golangci-lint: goconst). const ( - profileFieldEmail = "email" - profileFieldUsername = "username" - profileFieldGroupName = "group_name" + profileFieldEmail = "email" + profileFieldUsername = "username" + profileFieldGroupName = "group_name" + profileFieldPermission = "permission" + profileFieldStatus = "status" ) +// userStatusActive is the DocuSign UserStatus value this connector treats as "active" — +// used to gate userBuilder.Grants' list-response fast path on the same active/non-active +// distinction GetUserDetails' PermissionProfileID-empty check already relies on (see +// users.go), not a new assumption. +const userStatusActive = "Active" + // parsePageToken deserializes the Baton token and returns the Bag and page number for upstream. func parsePageToken(i string, resourceID *v2.ResourceId) (*pagination.Bag, string, error) { b := &pagination.Bag{} diff --git a/pkg/connector/users.go b/pkg/connector/users.go index b6e3ee72..7ebbb2b6 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -74,16 +74,49 @@ func (b *userBuilder) Entitlements(_ context.Context, _ *v2.Resource, _ rs.SyncO } // Grants assigns permissions to users based on their DocuSign settings. +// +// Fast path: an Active user's permission-profile NAME was already captured on the +// resource's profile during List() (parseIntoUserResource's profileFieldPermission +// field) from the same GetUsers list response every sync already pages through. +// Resolving that name to an ID via GetPermissionProfiles (one account-wide call, +// already served from uhttp's default GET cache on every call after the first in this +// sync — see pkg/client/clm_client.go's WithNoCache usage for this repo's opt-out +// convention when a fresh read matters, which this doesn't need) avoids a per-user +// GetUserDetails call for the common case — the N+1 pattern Pylon #11445 flagged as +// contributing to DocuSign's hourly rate limit. +// +// Gated on status == Active for the exact same reason GetUserDetails.PermissionProfileID +// is empty for non-active users below (that pre-existing rule, not a new assumption) — +// so a non-active user, an unresolvable name (profile renamed/deleted since listing), or +// a failed GetPermissionProfiles call all fall through to the always-correct per-user +// GetUserDetails path unchanged from before this fast path existed. func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { - var grants []*v2.Grant - var annos annotations.Annotations userID := resource.Id + profile := rs.GetProfile(resource) + + if status, ok := rs.GetProfileStringValue(profile, profileFieldStatus); ok && status == userStatusActive { + if name, ok := rs.GetProfileStringValue(profile, profileFieldPermission); ok && name != "" { + if profiles, annos, err := b.client.GetPermissionProfiles(ctx); err == nil { + for _, p := range profiles { + if p.PermissionProfileName == name { + newGrant := grant.NewGrant( + &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: p.PermissionProfileId}}, + permissionProfileAssignedTag, + userID, + ) + return []*v2.Grant{newGrant}, &rs.SyncOpResults{Annotations: annos}, nil + } + } + } + } + } userDetail, annotation, err := b.client.GetUserDetails(ctx, userID.Resource) if err != nil { return nil, nil, fmt.Errorf("failed to fetch details for %s: %w", userID.Resource, err) } + var annos annotations.Annotations for _, annon := range annotation { annos.Append(annon) } @@ -102,9 +135,7 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy } newGrant := grant.NewGrant(permissionProfileResource, permissionProfileAssignedTag, userID) - grants = append(grants, newGrant) - - return grants, &rs.SyncOpResults{Annotations: annos}, nil + return []*v2.Grant{newGrant}, &rs.SyncOpResults{Annotations: annos}, nil } // CreateAccountCapabilityDetails declares support for account provisioning without a password. @@ -226,7 +257,7 @@ func newUserBuilder(client *client.Client) *userBuilder { func parseIntoUserResource(user *client.User) (*v2.Resource, error) { var userStatus v2.UserTrait_Status_Status switch user.UserStatus { - case "Active": + case userStatusActive: userStatus = v2.UserTrait_Status_STATUS_ENABLED case "Disabled", "ActivationRequired", "ActivationSent": userStatus = v2.UserTrait_Status_STATUS_DISABLED @@ -237,11 +268,11 @@ func parseIntoUserResource(user *client.User) (*v2.Resource, error) { } profile := map[string]any{ - "userName": user.UserName, - profileFieldEmail: user.Email, - "isAdmin": user.IsAdmin, - "permission": user.Permission, - "status": user.UserStatus, + "userName": user.UserName, + profileFieldEmail: user.Email, + "isAdmin": user.IsAdmin, + profileFieldPermission: user.Permission, + profileFieldStatus: user.UserStatus, } userTraits := []rs.UserTraitOption{ diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go new file mode 100644 index 00000000..7269ee7d --- /dev/null +++ b/pkg/connector/users_test.go @@ -0,0 +1,172 @@ +package connector + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/conductorone/baton-docusign/pkg/client" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/uhttp" + "golang.org/x/oauth2" +) + +// rewriteTransport rewrites all outgoing request URLs to the given target host — +// mirrors pkg/client/client_test.go's helper of the same name (test files aren't +// importable across packages, so this is a small, deliberate duplicate). +type rewriteTransport struct { + target *url.URL +} + +func (t *rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) { + req = req.Clone(req.Context()) + req.URL.Scheme = t.target.Scheme + req.URL.Host = t.target.Host + return http.DefaultTransport.RoundTrip(req) +} + +// usersTestServer wires a *client.Client to a mock server handling /oauth/userinfo, +// GET permission_profiles, and GET users/{id} — everything userBuilder.Grants needs +// across both its fast path and its GetUserDetails fallback. +func newUsersTestClient(t *testing.T, profiles []client.PermissionProfile, userDetails map[string]client.UserDetail) *client.Client { + t.Helper() + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch r.URL.Path { + case "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case "/restapi/v2.1/accounts/acct-1/permission_profiles": + _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{PermissionProfiles: profiles}) + default: + // GET .../users/{id} + const prefix = "/restapi/v2.1/accounts/acct-1/users/" + if len(r.URL.Path) > len(prefix) && r.URL.Path[:len(prefix)] == prefix { + userID := r.URL.Path[len(prefix):] + if detail, ok := userDetails[userID]; ok { + _ = json.NewEncoder(w).Encode(detail) + return + } + } + http.NotFound(w, r) + } + }) + + mockServerURL, _ := url.Parse(mockServer.URL) + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL}}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + return client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) +} + +func userResourceWithProfile(t *testing.T, id, status, permission string) *v2.Resource { + t.Helper() + res, err := rs.NewUserResource(id, userResourceType, id, nil, rs.WithResourceProfile(map[string]any{ + profileFieldStatus: status, + profileFieldPermission: permission, + })) + if err != nil { + t.Fatalf("NewUserResource: %v", err) + } + return res +} + +func TestUserBuilder_Grants_FastPath_ActiveUserWithKnownProfile(t *testing.T) { + c := newUsersTestClient(t, []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + {PermissionProfileId: "pp-2", PermissionProfileName: "DocuSign Viewer"}, + }, nil) // no user-details fixtures — a fallback call here would 404 and fail the test + b := newUserBuilder(c) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + grants, res, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if res == nil { + t.Fatal("expected non-nil SyncOpResults") + } + if len(grants) != 1 { + t.Fatalf("expected exactly 1 grant, got %d: %+v", len(grants), grants) + } + if got := grants[0].Entitlement.Resource.Id.Resource; got != "pp-1" { + t.Errorf("expected grant against permission profile pp-1, got %s", got) + } +} + +func TestUserBuilder_Grants_FallsBackWhenNotActive(t *testing.T) { + // A non-active user must go through GetUserDetails, exactly like before this fast + // path existed — this is the regression test for the review finding that the fast + // path could otherwise grant a profile to a disabled/closed user the old code + // would have correctly skipped. + c := newUsersTestClient(t, []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, map[string]client.UserDetail{ + "user-1": {UserID: "user-1", PermissionProfileID: ""}, // matches "non-active users have no PP" + }) + b := newUserBuilder(c) + resource := userResourceWithProfile(t, "user-1", "Disabled", "DocuSign Admin") + + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 0 { + t.Errorf("expected zero grants for a disabled user, got %d: %+v", len(grants), grants) + } +} + +func TestUserBuilder_Grants_FallsBackWhenProfileNameUnresolvable(t *testing.T) { + // The cached profile name no longer matches any current permission profile (e.g. + // renamed/deleted since this user was listed) — must fall back, not just drop the + // grant silently. + c := newUsersTestClient(t, []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, map[string]client.UserDetail{ + "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, + }) + b := newUserBuilder(c) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "A Since-Renamed Profile") + + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("expected the GetUserDetails fallback to resolve pp-1, got %+v", grants) + } +} + +func TestUserBuilder_Grants_FallsBackWhenProfileFieldMissing(t *testing.T) { + // An identity-only or otherwise profile-less resource must not panic or skip the + // grant — it should behave exactly as it did before the fast path existed. + c := newUsersTestClient(t, []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, map[string]client.UserDetail{ + "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, + }) + b := newUserBuilder(c) + resource, err := rs.NewUserResource("user-1", userResourceType, "user-1", nil) + if err != nil { + t.Fatalf("NewUserResource: %v", err) + } + + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("expected the GetUserDetails fallback to resolve pp-1, got %+v", grants) + } +} From 3010626db6dd7f69bb18e12102fb57cf8aac0b16 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 17:12:35 -0300 Subject: [PATCH 03/28] fix: two deep-code-review findings on the rate-limit/N+1 fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. rateLimitErrorFromResponse no longer trusts header-derived Remaining/ ResetAt for DocuSign's hourly-limit error. DocuSign documents no dedicated headers for this limit (detection has to go through the error body at all) — any generic X-RateLimit-*/Ratelimit-* headers present most plausibly describe an unrelated shorter-window (burst) limit. The SDK's Retryer divides the wait by a nonzero Remaining, so trusting the wrong bucket's headers could produce a short retry delay that keeps re-hitting an account still over its hourly budget. Always uses the fixed ~1h window now — safe by construction, if coarser. 2. userBuilder.Grants now propagates a GetPermissionProfiles failure directly when it's already the reclassified rate-limit error (codes.Unavailable), instead of falling through to GetUserDetails — which would hit the identical limit and double the failing calls per active user, exactly the amplification this fix exists to reduce. Both caught by a deep-code-review pass on the full PR diff. --- pkg/client/helper.go | 30 ++++++++++------------- pkg/client/helper_test.go | 22 +++++++++++++---- pkg/connector/users.go | 24 +++++++++++++++---- pkg/connector/users_test.go | 48 ++++++++++++++++++++++++++++++++----- 4 files changed, 91 insertions(+), 33 deletions(-) diff --git a/pkg/client/helper.go b/pkg/client/helper.go index 39aeac3d..48583c6d 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -43,32 +43,26 @@ const docusignRateLimitDefaultResetWindow = time.Hour // failure. Returns nil (unchanged behavior) when errTarget isn't this specific eSignature // error shape, or the errorCode doesn't match — including every ClmErrorResponse-based CLM // call, which is a distinct error envelope this func never matches. +// +// Deliberately does NOT read ratelimit.ExtractRateLimitData's header-derived +// Limit/Remaining/ResetAt for this specific error: DocuSign's docs describe no dedicated +// headers for this hourly/daily-scoped limit (detection has to go through the error body +// at all), so any generic X-RateLimit-*/Ratelimit-* headers present on this response most +// plausibly describe an unrelated shorter-window limit (e.g. a burst counter), not the +// hourly one that actually produced this error. Trusting them anyway risks the SDK's +// Retryer (vendor pkg/retry/retry.go) computing a short wait off a nonzero Remaining from +// the wrong bucket and hammering an account that's still over its hourly budget. Always +// uses the fixed hourly default window instead — safe by construction, if coarser. func rateLimitErrorFromResponse(resp *http.Response, errTarget uhttp.ErrorResponse, origErr error) error { er, ok := errTarget.(*ErrorResponse) if !ok || er.ErrorCode != docusignHourlyRateLimitErrorCode { return nil } - desc, _ := ratelimit.ExtractRateLimitData(resp.StatusCode, &resp.Header) - resetAt := timestamppb.New(time.Now().Add(docusignRateLimitDefaultResetWindow)) - var limit, remaining int64 - if desc != nil { - // ExtractRateLimitData always returns a non-nil ResetAt — timestamppb.New of the - // zero time.Time when no reset header matched, not nil — so a nil check alone - // would wrongly accept that zero value over the sane default above. - if resetAtTime := desc.GetResetAt().AsTime(); !resetAtTime.IsZero() { - resetAt = desc.GetResetAt() - } - limit = desc.GetLimit() - remaining = desc.GetRemaining() - } - st := status.New(codes.Unavailable, origErr.Error()) withDetails, detailsErr := st.WithDetails(v2.RateLimitDescription_builder{ - Status: v2.RateLimitDescription_STATUS_OVERLIMIT, - Limit: limit, - Remaining: remaining, - ResetAt: resetAt, + Status: v2.RateLimitDescription_STATUS_OVERLIMIT, + ResetAt: timestamppb.New(time.Now().Add(docusignRateLimitDefaultResetWindow)), }.Build()) if detailsErr != nil { // WithDetails only fails for a codes.OK status or a detail that can't marshal to diff --git a/pkg/client/helper_test.go b/pkg/client/helper_test.go index aa4fc23c..aad134bc 100644 --- a/pkg/client/helper_test.go +++ b/pkg/client/helper_test.go @@ -70,12 +70,21 @@ func TestRateLimitErrorFromResponse(t *testing.T) { } }) - t.Run("prefers the X-Ratelimit-Reset header over the default window", func(t *testing.T) { - wantResetAt := time.Now().Add(5 * time.Minute).Truncate(time.Second) + t.Run("ignores rate-limit headers and always uses the fixed hourly window", func(t *testing.T) { + // DocuSign documents no dedicated headers for this hourly/daily-scoped limit — + // any generic X-RateLimit-*/Ratelimit-* headers present most plausibly describe + // an unrelated shorter-window limit (e.g. a burst counter), not the hourly one + // that produced this error. Trusting them would risk the SDK's Retryer computing + // a too-short wait off the wrong bucket's Remaining and re-hitting an account + // that's still over its hourly budget (a deep-code-review finding on this PR) — + // so a header claiming an imminent reset must NOT shorten the wait below the + // fixed default window. + soonResetAt := time.Now().Add(5 * time.Minute) resp := &http.Response{ StatusCode: http.StatusBadRequest, Header: http.Header{ - "X-Ratelimit-Reset": []string{strconv.FormatInt(wantResetAt.Unix(), 10)}, + "X-Ratelimit-Reset": []string{strconv.FormatInt(soonResetAt.Unix(), 10)}, + "X-Ratelimit-Remaining": []string{"5"}, }, } errTarget := &ErrorResponse{ErrorCode: docusignHourlyRateLimitErrorCode} @@ -94,8 +103,11 @@ func TestRateLimitErrorFromResponse(t *testing.T) { if desc == nil { t.Fatal("expected a RateLimitDescription in the error's status details") } - if got, want := desc.GetResetAt().AsTime().Unix(), wantResetAt.Unix(); got != want { - t.Errorf("expected ResetAt derived from the header (%d), got %d", want, got) + if desc.GetRemaining() != 0 { + t.Errorf("expected Remaining to stay 0 (header-derived value ignored), got %d", desc.GetRemaining()) + } + if resetAt := desc.GetResetAt().AsTime(); resetAt.Before(soonResetAt.Add(time.Minute)) { + t.Errorf("expected ResetAt to use the ~1h default window, not the header's near-term value: got %v", resetAt) } }) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 7ebbb2b6..9335e4c5 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -10,6 +10,8 @@ import ( "github.com/conductorone/baton-sdk/pkg/connectorbuilder" "github.com/conductorone/baton-sdk/pkg/types/grant" rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "google.golang.org/grpc/codes" + grpcstatus "google.golang.org/grpc/status" ) var _ connectorbuilder.AccountManagerV2 = &userBuilder{} @@ -89,14 +91,29 @@ func (b *userBuilder) Entitlements(_ context.Context, _ *v2.Resource, _ rs.SyncO // is empty for non-active users below (that pre-existing rule, not a new assumption) — // so a non-active user, an unresolvable name (profile renamed/deleted since listing), or // a failed GetPermissionProfiles call all fall through to the always-correct per-user -// GetUserDetails path unchanged from before this fast path existed. +// GetUserDetails path unchanged from before this fast path existed. The one exception: +// if GetPermissionProfiles itself fails because the account is already rate-limited +// (codes.Unavailable — see pkg/client/helper.go), GetUserDetails below would hit the +// identical limit, so that error is propagated directly instead of also burning that +// call — otherwise every active user would pay for two failing calls instead of one +// while the account is already over budget, exactly the amplification this fix exists +// to reduce. func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { userID := resource.Id profile := rs.GetProfile(resource) - if status, ok := rs.GetProfileStringValue(profile, profileFieldStatus); ok && status == userStatusActive { + var annos annotations.Annotations + + if userStatus, ok := rs.GetProfileStringValue(profile, profileFieldStatus); ok && userStatus == userStatusActive { if name, ok := rs.GetProfileStringValue(profile, profileFieldPermission); ok && name != "" { - if profiles, annos, err := b.client.GetPermissionProfiles(ctx); err == nil { + profiles, profileAnnos, err := b.client.GetPermissionProfiles(ctx) + if err != nil && grpcstatus.Code(err) == codes.Unavailable { + return nil, nil, err + } + if err == nil { + for _, a := range profileAnnos { + annos.Append(a) + } for _, p := range profiles { if p.PermissionProfileName == name { newGrant := grant.NewGrant( @@ -116,7 +133,6 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy return nil, nil, fmt.Errorf("failed to fetch details for %s: %w", userID.Resource, err) } - var annos annotations.Annotations for _, annon := range annotation { annos.Append(annon) } diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 7269ee7d..b705e734 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -13,6 +13,8 @@ import ( rs "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/conductorone/baton-sdk/pkg/uhttp" "golang.org/x/oauth2" + "google.golang.org/grpc/codes" + grpcstatus "google.golang.org/grpc/status" ) // rewriteTransport rewrites all outgoing request URLs to the given target host — @@ -31,8 +33,10 @@ func (t *rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) // usersTestServer wires a *client.Client to a mock server handling /oauth/userinfo, // GET permission_profiles, and GET users/{id} — everything userBuilder.Grants needs -// across both its fast path and its GetUserDetails fallback. -func newUsersTestClient(t *testing.T, profiles []client.PermissionProfile, userDetails map[string]client.UserDetail) *client.Client { +// across both its fast path and its GetUserDetails fallback. If forcePermissionProfilesRateLimit +// is set, the permission_profiles endpoint returns DocuSign's real hourly-rate-limit body +// instead of the profiles list, regardless of what's in profiles. +func newUsersTestClient(t *testing.T, profiles []client.PermissionProfile, userDetails map[string]client.UserDetail, forcePermissionProfilesRateLimit bool) *client.Client { t.Helper() mockServer := httptest.NewServer(nil) t.Cleanup(mockServer.Close) @@ -49,6 +53,14 @@ func newUsersTestClient(t *testing.T, profiles []client.PermissionProfile, userD }, }) case "/restapi/v2.1/accounts/acct-1/permission_profiles": + if forcePermissionProfilesRateLimit { + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ + ErrorCode: "HOURLY_APIINVOCATION_LIMIT_EXCEEDED", + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", + }) + return + } _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{PermissionProfiles: profiles}) default: // GET .../users/{id} @@ -86,7 +98,7 @@ func TestUserBuilder_Grants_FastPath_ActiveUserWithKnownProfile(t *testing.T) { c := newUsersTestClient(t, []client.PermissionProfile{ {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, {PermissionProfileId: "pp-2", PermissionProfileName: "DocuSign Viewer"}, - }, nil) // no user-details fixtures — a fallback call here would 404 and fail the test + }, nil, false) // no user-details fixtures — a fallback call here would 404 and fail the test b := newUserBuilder(c) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") @@ -114,7 +126,7 @@ func TestUserBuilder_Grants_FallsBackWhenNotActive(t *testing.T) { {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, map[string]client.UserDetail{ "user-1": {UserID: "user-1", PermissionProfileID: ""}, // matches "non-active users have no PP" - }) + }, false) b := newUserBuilder(c) resource := userResourceWithProfile(t, "user-1", "Disabled", "DocuSign Admin") @@ -135,7 +147,7 @@ func TestUserBuilder_Grants_FallsBackWhenProfileNameUnresolvable(t *testing.T) { {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, map[string]client.UserDetail{ "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, - }) + }, false) b := newUserBuilder(c) resource := userResourceWithProfile(t, "user-1", userStatusActive, "A Since-Renamed Profile") @@ -148,6 +160,30 @@ func TestUserBuilder_Grants_FallsBackWhenProfileNameUnresolvable(t *testing.T) { } } +// TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls is a regression test +// for a deep-code-review finding: if GetPermissionProfiles fails because the account is +// already rate-limited (the exact scenario Pylon #11445 is about), falling through to +// GetUserDetails would hit the identical limit and double the failing calls per active +// user instead of reducing them. Grants must propagate that error directly. +func TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls(t *testing.T) { + c := newUsersTestClient(t, nil, map[string]client.UserDetail{ + // If Grants incorrectly falls through to GetUserDetails, this fixture would let + // it "succeed" and hide the bug — present specifically so the fallthrough case + // would be caught if it fired instead of propagating. + "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, + }, true) + b := newUserBuilder(c) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err == nil { + t.Fatal("expected Grants to propagate the rate-limit error, got nil") + } + if got := grpcstatus.Code(err); got != codes.Unavailable { + t.Fatalf("expected codes.Unavailable, got %v: %v", got, err) + } +} + func TestUserBuilder_Grants_FallsBackWhenProfileFieldMissing(t *testing.T) { // An identity-only or otherwise profile-less resource must not panic or skip the // grant — it should behave exactly as it did before the fast path existed. @@ -155,7 +191,7 @@ func TestUserBuilder_Grants_FallsBackWhenProfileFieldMissing(t *testing.T) { {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, map[string]client.UserDetail{ "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, - }) + }, false) b := newUserBuilder(c) resource, err := rs.NewUserResource("user-1", userResourceType, "user-1", nil) if err != nil { From 6353c9ff6f2dfc82610f3aa34ae98fd858d40561 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 17:18:10 -0300 Subject: [PATCH 04/28] refactor: address remaining deep-code-review findings on rate-limit/N+1 fix - Rename rateLimitErrorFromResponse -> reclassifyHourlyRateLimitError; the old name read as a getter, not "this replaces the error's gRPC code." - doRequestCommon's doc comment now says it can reclassify this one specific error instead of leaving that undocumented. - Extract userBuilder.Grants' fast path into tryFastPathGrant, replacing the 3-level-nested if/for with guard clauses and an explicit (grant, annos, err, handled) result the caller branches on directly. - Extract permissionProfileIDByName (pkg/connector/helper.go), shared by the fast path and permissionProfilesBuilder.Revoke's default-profile lookup, which had independently duplicated the same linear scan. - Inline the single-use newGrant temp var in Grants' slow-path tail. --- pkg/client/helper.go | 12 ++- pkg/client/helper_test.go | 18 ++--- pkg/connector/helper.go | 14 ++++ pkg/connector/permission_profiles.go | 11 +-- pkg/connector/users.go | 109 +++++++++++++++------------ 5 files changed, 96 insertions(+), 68 deletions(-) diff --git a/pkg/client/helper.go b/pkg/client/helper.go index 48583c6d..c45e2640 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -32,7 +32,7 @@ const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" // longer than) uhttp/ratelimit's own 60s default for a headerless 429. const docusignRateLimitDefaultResetWindow = time.Hour -// rateLimitErrorFromResponse recognizes docusignHourlyRateLimitErrorCode in errTarget (the +// reclassifyHourlyRateLimitError recognizes docusignHourlyRateLimitErrorCode in errTarget (the // same *ErrorResponse instance uhttp.WithErrorResponse already unmarshaled the error body // into before returning origErr — no re-parsing needed) and, if matched, returns a // codes.Unavailable error carrying a RateLimitDescription. This matters because @@ -53,7 +53,7 @@ const docusignRateLimitDefaultResetWindow = time.Hour // Retryer (vendor pkg/retry/retry.go) computing a short wait off a nonzero Remaining from // the wrong bucket and hammering an account that's still over its hourly budget. Always // uses the fixed hourly default window instead — safe by construction, if coarser. -func rateLimitErrorFromResponse(resp *http.Response, errTarget uhttp.ErrorResponse, origErr error) error { +func reclassifyHourlyRateLimitError(resp *http.Response, errTarget uhttp.ErrorResponse, origErr error) error { er, ok := errTarget.(*ErrorResponse) if !ok || er.ErrorCode != docusignHourlyRateLimitErrorCode { return nil @@ -91,6 +91,12 @@ func buildURL(base, path string, params ...any) (*url.URL, error) { // DoRequestCommon executes the HTTP request and handles rate limit annotations. // errTarget receives the parsed error body on non-2xx responses (e.g. &ErrorResponse{} // for eSignature, &ClmErrorResponse{} for CLM) since the two APIs use different error envelopes. +// +// On the error path, one specific eSignature error (DocuSign's hourly API-call-budget +// error — see reclassifyHourlyRateLimitError) has its gRPC code silently overridden from +// whatever uhttp.GrpcCodeFromHTTPStatus would otherwise produce to codes.Unavailable, so +// the SDK's sync-retry loop treats it as retryable instead of fatal. Every other error is +// returned unchanged. func doRequestCommon(wrapper *uhttp.BaseHttpClient, req *http.Request, res any, errTarget uhttp.ErrorResponse) (http.Header, annotations.Annotations, error) { opts := []uhttp.DoOption{} if res != nil { @@ -102,7 +108,7 @@ func doRequestCommon(wrapper *uhttp.BaseHttpClient, req *http.Request, res any, // resp is non-nil here whenever the error came from a well-formed non-2xx HTTP // response (as opposed to a network/transport failure) — see wrapper.Do. if resp != nil { - if rlErr := rateLimitErrorFromResponse(resp, errTarget, err); rlErr != nil { + if rlErr := reclassifyHourlyRateLimitError(resp, errTarget, err); rlErr != nil { return resp.Header, nil, rlErr } } diff --git a/pkg/client/helper_test.go b/pkg/client/helper_test.go index aad134bc..317b2a33 100644 --- a/pkg/client/helper_test.go +++ b/pkg/client/helper_test.go @@ -18,15 +18,15 @@ import ( "google.golang.org/grpc/status" ) -// TestRateLimitErrorFromResponse is a regression test for Pylon #11445: DocuSign signals +// TestReclassifyHourlyRateLimitError is a regression test for Pylon #11445: DocuSign signals // "hourly API-call budget exhausted" via a JSON error body (errorCode // HOURLY_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400, which uhttp.GrpcCodeFromHTTPStatus maps // to codes.InvalidArgument — a code the SDK's sync-retry loop treats as fatal, not // retryable, so a real customer's initial full sync failed outright instead of pausing and -// resuming. rateLimitErrorFromResponse must re-classify exactly this case as +// resuming. reclassifyHourlyRateLimitError must re-classify exactly this case as // codes.Unavailable (which the SDK does retry) carrying a RateLimitDescription, and leave // every other error (including CLM's distinct error envelope) untouched. -func TestRateLimitErrorFromResponse(t *testing.T) { +func TestReclassifyHourlyRateLimitError(t *testing.T) { origErr := errors.New("400 Bad Request") t.Run("matches on errorCode regardless of HTTP status", func(t *testing.T) { @@ -40,7 +40,7 @@ func TestRateLimitErrorFromResponse(t *testing.T) { ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", } - got := rateLimitErrorFromResponse(resp, errTarget, origErr) + got := reclassifyHourlyRateLimitError(resp, errTarget, origErr) if got == nil { t.Fatalf("status %d: expected a rate-limit error, got nil", statusCode) } @@ -89,7 +89,7 @@ func TestRateLimitErrorFromResponse(t *testing.T) { } errTarget := &ErrorResponse{ErrorCode: docusignHourlyRateLimitErrorCode} - got := rateLimitErrorFromResponse(resp, errTarget, origErr) + got := reclassifyHourlyRateLimitError(resp, errTarget, origErr) if got == nil { t.Fatal("expected a rate-limit error, got nil") } @@ -115,7 +115,7 @@ func TestRateLimitErrorFromResponse(t *testing.T) { resp := &http.Response{StatusCode: http.StatusBadRequest, Header: http.Header{}} errTarget := &ErrorResponse{ErrorCode: "USER_LACKS_PERMISSIONS"} - if got := rateLimitErrorFromResponse(resp, errTarget, origErr); got != nil { + if got := reclassifyHourlyRateLimitError(resp, errTarget, origErr); got != nil { t.Errorf("expected nil for an unrelated errorCode, got %v", got) } }) @@ -127,7 +127,7 @@ func TestRateLimitErrorFromResponse(t *testing.T) { // alone must reject it, since this function's evidence is eSignature-specific. errTarget := &ClmErrorResponse{} - if got := rateLimitErrorFromResponse(resp, errTarget, origErr); got != nil { + if got := reclassifyHourlyRateLimitError(resp, errTarget, origErr); got != nil { t.Errorf("expected nil for a non-eSignature error envelope, got %v", got) } }) @@ -135,8 +135,8 @@ func TestRateLimitErrorFromResponse(t *testing.T) { // TestGetUsers_ClassifiesHourlyRateLimitAsRetryable is an end-to-end regression test for // Pylon #11445, exercising the real request path (GetUsers -> doRequestCommon -> -// rateLimitErrorFromResponse) against a mock server that returns DocuSign's actual -// observed 400 body, rather than calling rateLimitErrorFromResponse directly. +// reclassifyHourlyRateLimitError) against a mock server that returns DocuSign's actual +// observed 400 body, rather than calling reclassifyHourlyRateLimitError directly. func TestGetUsers_ClassifiesHourlyRateLimitAsRetryable(t *testing.T) { mockServer := httptest.NewServer(nil) defer mockServer.Close() diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index e7e07391..18f61776 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -3,6 +3,7 @@ package connector import ( "strings" + "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/pagination" "google.golang.org/grpc/codes" @@ -83,6 +84,19 @@ func isOptInFeatureUnavailableError(err error) bool { } } +// permissionProfileIDByName returns the ID of the permission profile named name, and +// whether one was found — shared by userBuilder.Grants' list-response fast path and +// permissionProfilesBuilder.Revoke's default-profile lookup, which independently +// duplicated this same linear scan before this helper existed. +func permissionProfileIDByName(profiles []client.PermissionProfile, name string) (string, bool) { + for _, p := range profiles { + if p.PermissionProfileName == name { + return p.PermissionProfileId, true + } + } + return "", false +} + // clmIDFromHref extracts the trailing path segment from a CLM object's Href — CLM's // Object API schemas expose a Href field ("Uri where the object can be retrieved") but // no separate opaque Id field, so this is the closest thing to a native ID CLM exposes. diff --git a/pkg/connector/permission_profiles.go b/pkg/connector/permission_profiles.go index ca8c8184..3eb41029 100644 --- a/pkg/connector/permission_profiles.go +++ b/pkg/connector/permission_profiles.go @@ -117,15 +117,8 @@ func (p *permissionProfilesBuilder) Revoke(ctx context.Context, grantObj *v2.Gra return profileAnnos, fmt.Errorf("failed to get permission profiles: %w", err) } - var defaultProfileID string - for _, profile := range permissionProfiles { - if profile.PermissionProfileName == defaultPermissionProfileName { - defaultProfileID = profile.PermissionProfileId - break - } - } - - if defaultProfileID == "" { + defaultProfileID, ok := permissionProfileIDByName(permissionProfiles, defaultPermissionProfileName) + if !ok { return profileAnnos, fmt.Errorf("default permission profile '%s' not found in account", defaultPermissionProfileName) } diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 9335e4c5..6ad8c893 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -77,55 +77,20 @@ func (b *userBuilder) Entitlements(_ context.Context, _ *v2.Resource, _ rs.SyncO // Grants assigns permissions to users based on their DocuSign settings. // -// Fast path: an Active user's permission-profile NAME was already captured on the -// resource's profile during List() (parseIntoUserResource's profileFieldPermission -// field) from the same GetUsers list response every sync already pages through. -// Resolving that name to an ID via GetPermissionProfiles (one account-wide call, -// already served from uhttp's default GET cache on every call after the first in this -// sync — see pkg/client/clm_client.go's WithNoCache usage for this repo's opt-out -// convention when a fresh read matters, which this doesn't need) avoids a per-user -// GetUserDetails call for the common case — the N+1 pattern Pylon #11445 flagged as -// contributing to DocuSign's hourly rate limit. -// -// Gated on status == Active for the exact same reason GetUserDetails.PermissionProfileID -// is empty for non-active users below (that pre-existing rule, not a new assumption) — -// so a non-active user, an unresolvable name (profile renamed/deleted since listing), or -// a failed GetPermissionProfiles call all fall through to the always-correct per-user -// GetUserDetails path unchanged from before this fast path existed. The one exception: -// if GetPermissionProfiles itself fails because the account is already rate-limited -// (codes.Unavailable — see pkg/client/helper.go), GetUserDetails below would hit the -// identical limit, so that error is propagated directly instead of also burning that -// call — otherwise every active user would pay for two failing calls instead of one -// while the account is already over budget, exactly the amplification this fix exists -// to reduce. +// Tries tryFastPathGrant first (an Active user's permission-profile NAME, already +// captured on the resource's profile during List(), resolved via one account-wide +// GetPermissionProfiles call instead of a per-user GetUserDetails call — the N+1 +// pattern Pylon #11445 flagged as contributing to DocuSign's hourly rate limit) and +// falls back to the always-correct per-user GetUserDetails path unchanged from before +// that fast path existed whenever it declines to handle the request (see its own doc). func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { userID := resource.Id - profile := rs.GetProfile(resource) - - var annos annotations.Annotations - if userStatus, ok := rs.GetProfileStringValue(profile, profileFieldStatus); ok && userStatus == userStatusActive { - if name, ok := rs.GetProfileStringValue(profile, profileFieldPermission); ok && name != "" { - profiles, profileAnnos, err := b.client.GetPermissionProfiles(ctx) - if err != nil && grpcstatus.Code(err) == codes.Unavailable { - return nil, nil, err - } - if err == nil { - for _, a := range profileAnnos { - annos.Append(a) - } - for _, p := range profiles { - if p.PermissionProfileName == name { - newGrant := grant.NewGrant( - &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: p.PermissionProfileId}}, - permissionProfileAssignedTag, - userID, - ) - return []*v2.Grant{newGrant}, &rs.SyncOpResults{Annotations: annos}, nil - } - } - } + if newGrant, annos, err, handled := b.tryFastPathGrant(ctx, resource, userID); handled { + if err != nil { + return nil, nil, err } + return []*v2.Grant{newGrant}, &rs.SyncOpResults{Annotations: annos}, nil } userDetail, annotation, err := b.client.GetUserDetails(ctx, userID.Resource) @@ -133,6 +98,7 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy return nil, nil, fmt.Errorf("failed to fetch details for %s: %w", userID.Resource, err) } + var annos annotations.Annotations for _, annon := range annotation { annos.Append(annon) } @@ -150,8 +116,57 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy }, } - newGrant := grant.NewGrant(permissionProfileResource, permissionProfileAssignedTag, userID) - return []*v2.Grant{newGrant}, &rs.SyncOpResults{Annotations: annos}, nil + return []*v2.Grant{ + grant.NewGrant(permissionProfileResource, permissionProfileAssignedTag, userID), + }, &rs.SyncOpResults{Annotations: annos}, nil +} + +// tryFastPathGrant is Grants' fast path: an Active user's permission-profile NAME, +// already captured on the resource's profile during List(), resolved via +// GetPermissionProfiles (one account-wide call, already served from uhttp's default GET +// cache on every call after the first in this sync — see pkg/client/clm_client.go's +// WithNoCache usage for this repo's opt-out convention when a fresh read matters, which +// this doesn't need) instead of a per-user GetUserDetails call. +// +// handled=false means "no decision, fall back to Grants' original GetUserDetails path +// unchanged" — covers a non-active user, a missing profile field, an unresolvable name +// (profile renamed/deleted since listing), or a GetPermissionProfiles failure unrelated +// to rate limiting. handled=true with a non-nil err means GetPermissionProfiles hit the +// same rate limit GetUserDetails would also hit (codes.Unavailable — see +// pkg/client/helper.go); propagate it directly rather than also burning that call — +// otherwise every active user would pay for two failing calls instead of one while the +// account is already over budget, exactly the amplification this fix exists to reduce. +// handled=true with a nil err means the grant was resolved. +func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resource, userID *v2.ResourceId) (*v2.Grant, annotations.Annotations, error, bool) { + profile := rs.GetProfile(resource) + + userStatus, ok := rs.GetProfileStringValue(profile, profileFieldStatus) + if !ok || userStatus != userStatusActive { + return nil, nil, nil, false + } + name, ok := rs.GetProfileStringValue(profile, profileFieldPermission) + if !ok || name == "" { + return nil, nil, nil, false + } + + profiles, annos, err := b.client.GetPermissionProfiles(ctx) + if err != nil { + if grpcstatus.Code(err) == codes.Unavailable { + return nil, nil, err, true + } + return nil, nil, nil, false + } + + id, ok := permissionProfileIDByName(profiles, name) + if !ok { + return nil, nil, nil, false + } + newGrant := grant.NewGrant( + &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: id}}, + permissionProfileAssignedTag, + userID, + ) + return newGrant, annos, nil, true } // CreateAccountCapabilityDetails declares support for account provisioning without a password. From 4d071e627b1859512994d5e569db9aa3b8b06f8b Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 19:27:59 -0300 Subject: [PATCH 05/28] fix: address 11 bot review comments on PR #68 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix a real bug in the prior "narrow the Unavailable check" fix itself: isReclassifiedRateLimitError checked only "does a RateLimitDescription exist" — but uhttp's own WrapErrorsWithRateLimitInfo attaches one (usually STATUS_UNSPECIFIED) to every non-2xx response unconditionally, since ratelimit.ExtractRateLimitData never errors. That made the check match almost any error, not just genuine rate limits. Now requires Status == STATUS_OVERLIMIT specifically. Caught by the two new fallback tests below, which failed against the buggy version. - Drop the now-unused resp param from reclassifyHourlyRateLimitError, and fix its and docusignRateLimitDefaultResetWindow's now-stale doc comments (both referenced header-derivation that was removed in the prior commit). - permissionProfileIDByName now requires a non-empty ID, restoring a check the code it replaced (permission_profiles.go's inline Revoke lookup) had. - tryFastPathGrant logs at Debug and stops forwarding GetPermissionProfiles' annotations on a non-rate-limit failure/cache-miss path — those calls are served from uhttp's GET cache after the first in a sync, so forwarding them on every subsequent user would replay an increasingly stale rate-limit snapshot instead of the fresh per-request data GetUserDetails used to supply. - Added client.User.PermissionProfileID (unconfirmed against a live account — additive/optional, matches UserDetail's existing field): tryFastPathGrant prefers it directly when present, skipping GetPermissionProfiles and the name lookup entirely. - New tests: GetPermissionProfiles failing with a non-rate-limit error (403) and with a plain 503 must both still fall back to GetUserDetails, not propagate; the direct-profile-ID fast path skips the API call entirely. --- pkg/client/helper.go | 13 +++-- pkg/client/helper_test.go | 94 +++++++++----------------------- pkg/client/models.go | 7 +++ pkg/connector/helper.go | 38 +++++++++++-- pkg/connector/users.go | 72 +++++++++++++++++------- pkg/connector/users_test.go | 106 ++++++++++++++++++++++++++++++++---- 6 files changed, 220 insertions(+), 110 deletions(-) diff --git a/pkg/client/helper.go b/pkg/client/helper.go index c45e2640..cfc1835c 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -26,10 +26,11 @@ const DefaultPageSize = 100 // reason), so detection below checks the body field independent of resp.StatusCode. const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" -// docusignRateLimitDefaultResetWindow is used when DocuSign's response carries no -// X-RateLimit-Reset (or equivalent) header to derive a reset time from — the limit this -// error names is hourly, so an hour is the sane default, matching the spirit of (but -// longer than) uhttp/ratelimit's own 60s default for a headerless 429. +// docusignRateLimitDefaultResetWindow is the fixed wait this connector asks the SDK's +// retry loop to use for docusignHourlyRateLimitErrorCode — applied unconditionally, not +// just as a fallback (see reclassifyHourlyRateLimitError's doc for why response headers +// are deliberately never consulted for this error). The limit this error names is +// hourly, so an hour is the sane, safe choice. const docusignRateLimitDefaultResetWindow = time.Hour // reclassifyHourlyRateLimitError recognizes docusignHourlyRateLimitErrorCode in errTarget (the @@ -53,7 +54,7 @@ const docusignRateLimitDefaultResetWindow = time.Hour // Retryer (vendor pkg/retry/retry.go) computing a short wait off a nonzero Remaining from // the wrong bucket and hammering an account that's still over its hourly budget. Always // uses the fixed hourly default window instead — safe by construction, if coarser. -func reclassifyHourlyRateLimitError(resp *http.Response, errTarget uhttp.ErrorResponse, origErr error) error { +func reclassifyHourlyRateLimitError(errTarget uhttp.ErrorResponse, origErr error) error { er, ok := errTarget.(*ErrorResponse) if !ok || er.ErrorCode != docusignHourlyRateLimitErrorCode { return nil @@ -108,7 +109,7 @@ func doRequestCommon(wrapper *uhttp.BaseHttpClient, req *http.Request, res any, // resp is non-nil here whenever the error came from a well-formed non-2xx HTTP // response (as opposed to a network/transport failure) — see wrapper.Do. if resp != nil { - if rlErr := reclassifyHourlyRateLimitError(resp, errTarget, err); rlErr != nil { + if rlErr := reclassifyHourlyRateLimitError(errTarget, err); rlErr != nil { return resp.Header, nil, rlErr } } diff --git a/pkg/client/helper_test.go b/pkg/client/helper_test.go index 317b2a33..b75bf72a 100644 --- a/pkg/client/helper_test.go +++ b/pkg/client/helper_test.go @@ -7,7 +7,6 @@ import ( "net/http" "net/http/httptest" "net/url" - "strconv" "testing" "time" @@ -29,71 +28,27 @@ import ( func TestReclassifyHourlyRateLimitError(t *testing.T) { origErr := errors.New("400 Bad Request") - t.Run("matches on errorCode regardless of HTTP status", func(t *testing.T) { - for _, statusCode := range []int{http.StatusBadRequest, http.StatusTooManyRequests} { - resp := &http.Response{ - StatusCode: statusCode, - Header: http.Header{}, - } - errTarget := &ErrorResponse{ - ErrorCode: docusignHourlyRateLimitErrorCode, - ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", - } - - got := reclassifyHourlyRateLimitError(resp, errTarget, origErr) - if got == nil { - t.Fatalf("status %d: expected a rate-limit error, got nil", statusCode) - } - st, ok := status.FromError(got) - if !ok { - t.Fatalf("status %d: expected a gRPC status error, got %v", statusCode, got) - } - if st.Code() != codes.Unavailable { - t.Errorf("status %d: expected codes.Unavailable, got %v", statusCode, st.Code()) - } - - var desc *v2.RateLimitDescription - for _, d := range st.Details() { - if rl, ok := d.(*v2.RateLimitDescription); ok { - desc = rl - } - } - if desc == nil { - t.Fatalf("status %d: expected a RateLimitDescription in the error's status details, got %+v", statusCode, st.Details()) - } - if desc.GetStatus() != v2.RateLimitDescription_STATUS_OVERLIMIT { - t.Errorf("status %d: expected STATUS_OVERLIMIT, got %v", statusCode, desc.GetStatus()) - } - if desc.GetResetAt() == nil || desc.GetResetAt().AsTime().Before(time.Now()) { - t.Errorf("status %d: expected a future ResetAt when no header is present, got %v", statusCode, desc.GetResetAt()) - } - } - }) - - t.Run("ignores rate-limit headers and always uses the fixed hourly window", func(t *testing.T) { - // DocuSign documents no dedicated headers for this hourly/daily-scoped limit — - // any generic X-RateLimit-*/Ratelimit-* headers present most plausibly describe - // an unrelated shorter-window limit (e.g. a burst counter), not the hourly one - // that produced this error. Trusting them would risk the SDK's Retryer computing - // a too-short wait off the wrong bucket's Remaining and re-hitting an account - // that's still over its hourly budget (a deep-code-review finding on this PR) — - // so a header claiming an imminent reset must NOT shorten the wait below the - // fixed default window. - soonResetAt := time.Now().Add(5 * time.Minute) - resp := &http.Response{ - StatusCode: http.StatusBadRequest, - Header: http.Header{ - "X-Ratelimit-Reset": []string{strconv.FormatInt(soonResetAt.Unix(), 10)}, - "X-Ratelimit-Remaining": []string{"5"}, - }, + t.Run("matches on errorCode and always uses the fixed hourly window", func(t *testing.T) { + // The function takes no status code or headers at all — it can only ever see + // errTarget's parsed body, so there's nothing header-derived to test here beyond + // confirming the fixed window is what gets used. + errTarget := &ErrorResponse{ + ErrorCode: docusignHourlyRateLimitErrorCode, + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", } - errTarget := &ErrorResponse{ErrorCode: docusignHourlyRateLimitErrorCode} - got := reclassifyHourlyRateLimitError(resp, errTarget, origErr) + got := reclassifyHourlyRateLimitError(errTarget, origErr) if got == nil { t.Fatal("expected a rate-limit error, got nil") } - st, _ := status.FromError(got) + st, ok := status.FromError(got) + if !ok { + t.Fatalf("expected a gRPC status error, got %v", got) + } + if st.Code() != codes.Unavailable { + t.Errorf("expected codes.Unavailable, got %v", st.Code()) + } + var desc *v2.RateLimitDescription for _, d := range st.Details() { if rl, ok := d.(*v2.RateLimitDescription); ok { @@ -101,33 +56,34 @@ func TestReclassifyHourlyRateLimitError(t *testing.T) { } } if desc == nil { - t.Fatal("expected a RateLimitDescription in the error's status details") + t.Fatalf("expected a RateLimitDescription in the error's status details, got %+v", st.Details()) + } + if desc.GetStatus() != v2.RateLimitDescription_STATUS_OVERLIMIT { + t.Errorf("expected STATUS_OVERLIMIT, got %v", desc.GetStatus()) } if desc.GetRemaining() != 0 { - t.Errorf("expected Remaining to stay 0 (header-derived value ignored), got %d", desc.GetRemaining()) + t.Errorf("expected Remaining to be 0 (matches OVERLIMIT, no header data is ever read), got %d", desc.GetRemaining()) } - if resetAt := desc.GetResetAt().AsTime(); resetAt.Before(soonResetAt.Add(time.Minute)) { - t.Errorf("expected ResetAt to use the ~1h default window, not the header's near-term value: got %v", resetAt) + if desc.GetResetAt() == nil || desc.GetResetAt().AsTime().Before(time.Now().Add(50*time.Minute)) { + t.Errorf("expected a ResetAt roughly an hour out, got %v", desc.GetResetAt()) } }) t.Run("does not match an unrelated errorCode", func(t *testing.T) { - resp := &http.Response{StatusCode: http.StatusBadRequest, Header: http.Header{}} errTarget := &ErrorResponse{ErrorCode: "USER_LACKS_PERMISSIONS"} - if got := reclassifyHourlyRateLimitError(resp, errTarget, origErr); got != nil { + if got := reclassifyHourlyRateLimitError(errTarget, origErr); got != nil { t.Errorf("expected nil for an unrelated errorCode, got %v", got) } }) t.Run("does not match CLM's distinct error envelope", func(t *testing.T) { - resp := &http.Response{StatusCode: http.StatusBadRequest, Header: http.Header{}} // ClmErrorResponse is a different type from *ErrorResponse even if some CLM error // happened to carry the same string in an analogous field — the type assertion // alone must reject it, since this function's evidence is eSignature-specific. errTarget := &ClmErrorResponse{} - if got := reclassifyHourlyRateLimitError(resp, errTarget, origErr); got != nil { + if got := reclassifyHourlyRateLimitError(errTarget, origErr); got != nil { t.Errorf("expected nil for a non-eSignature error envelope, got %v", got) } }) diff --git a/pkg/client/models.go b/pkg/client/models.go index 92eb026c..dbd2ce7d 100644 --- a/pkg/client/models.go +++ b/pkg/client/models.go @@ -49,6 +49,13 @@ type User struct { UserStatus string `json:"userStatus"` IsAdmin string `json:"isAdmin"` Permission string `json:"permissionProfileName"` + // PermissionProfileID mirrors UserDetail.PermissionProfileID's json tag on the + // chance the list-users response includes it alongside permissionProfileName — + // not confirmed against a live account (no DocuSign tenant available to verify). + // If DocuSign's list response doesn't actually send this field, it just stays + // empty and callers fall back to their existing name-based/GetUserDetails paths + // unchanged — this field is additive, never required. + PermissionProfileID string `json:"permissionProfileId"` } type GroupsResponse struct { diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 18f61776..5dfdaff3 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -16,8 +16,9 @@ const ( profileFieldEmail = "email" profileFieldUsername = "username" profileFieldGroupName = "group_name" - profileFieldPermission = "permission" - profileFieldStatus = "status" + profileFieldPermission = "permission" + profileFieldStatus = "status" + profileFieldPermissionID = "permission_profile_id" ) // userStatusActive is the DocuSign UserStatus value this connector treats as "active" — @@ -26,6 +27,33 @@ const ( // users.go), not a new assumption. const userStatusActive = "Active" +// isReclassifiedRateLimitError reports whether err represents a genuine rate-limit +// overlimit — either DocuSign's hourly error (pkg/client/helper.go's +// reclassifyHourlyRateLimitError) or a plain HTTP 429 uhttp's own +// WrapErrorsWithRateLimitInfo already classifies this way — identified by a +// RateLimitDescription with Status == STATUS_OVERLIMIT specifically, not merely the +// presence of a RateLimitDescription at all: uhttp's wrapper.go attaches one to every +// non-2xx response unconditionally (ratelimit.ExtractRateLimitData never errors, so +// WrapErrorsWithRateLimitInfo's `if err == nil { st.WithDetails(description) }` always +// runs), almost always with Status left at its unset zero value — so a bare presence +// check would false-positive-match ordinary unrelated errors (403s, validation errors, +// anything non-2xx). codes.Unavailable alone is also too broad on its own: uhttp maps a +// plain 503 or a transient network failure to it too, and those are genuinely different +// failures a caller may want to handle differently (e.g. still fall back to another +// endpoint) rather than treat as an account already over its rate-limit budget. +func isReclassifiedRateLimitError(err error) bool { + st, ok := status.FromError(err) + if !ok { + return false + } + for _, d := range st.Details() { + if rl, ok := d.(*v2.RateLimitDescription); ok && rl.GetStatus() == v2.RateLimitDescription_STATUS_OVERLIMIT { + return true + } + } + return false +} + // parsePageToken deserializes the Baton token and returns the Bag and page number for upstream. func parsePageToken(i string, resourceID *v2.ResourceId) (*pagination.Bag, string, error) { b := &pagination.Bag{} @@ -87,10 +115,12 @@ func isOptInFeatureUnavailableError(err error) bool { // permissionProfileIDByName returns the ID of the permission profile named name, and // whether one was found — shared by userBuilder.Grants' list-response fast path and // permissionProfilesBuilder.Revoke's default-profile lookup, which independently -// duplicated this same linear scan before this helper existed. +// duplicated this same linear scan before this helper existed. Requires a non-empty ID, +// matching Revoke's original inline check (`if defaultProfileID == ""`) that a name match +// with no usable ID counts as not found, not as an empty-string result. func permissionProfileIDByName(profiles []client.PermissionProfile, name string) (string, bool) { for _, p := range profiles { - if p.PermissionProfileName == name { + if p.PermissionProfileName == name && p.PermissionProfileId != "" { return p.PermissionProfileId, true } } diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 6ad8c893..7bb78d71 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -10,8 +10,8 @@ import ( "github.com/conductorone/baton-sdk/pkg/connectorbuilder" "github.com/conductorone/baton-sdk/pkg/types/grant" rs "github.com/conductorone/baton-sdk/pkg/types/resource" - "google.golang.org/grpc/codes" - grpcstatus "google.golang.org/grpc/status" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" ) var _ connectorbuilder.AccountManagerV2 = &userBuilder{} @@ -121,22 +121,36 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy }, &rs.SyncOpResults{Annotations: annos}, nil } -// tryFastPathGrant is Grants' fast path: an Active user's permission-profile NAME, -// already captured on the resource's profile during List(), resolved via -// GetPermissionProfiles (one account-wide call, already served from uhttp's default GET -// cache on every call after the first in this sync — see pkg/client/clm_client.go's -// WithNoCache usage for this repo's opt-out convention when a fresh read matters, which -// this doesn't need) instead of a per-user GetUserDetails call. +// tryFastPathGrant is Grants' fast path for an Active user, avoiding the per-user +// GetUserDetails call Pylon #11445 flagged as contributing to DocuSign's hourly rate +// limit. Two ways it can resolve the grant without that call, both already captured on +// the resource's profile during List(): +// - Preferred: client.User.PermissionProfileID directly, if the list response included +// it (unconfirmed against a live account — see that field's doc) — no API call at all. +// - Otherwise: the permission-profile NAME, resolved to an ID via GetPermissionProfiles +// (one account-wide call, already served from uhttp's default GET cache on every call +// after the first in this sync — see pkg/client/clm_client.go's WithNoCache usage for +// this repo's opt-out convention when a fresh read matters, which this doesn't need). // // handled=false means "no decision, fall back to Grants' original GetUserDetails path // unchanged" — covers a non-active user, a missing profile field, an unresolvable name // (profile renamed/deleted since listing), or a GetPermissionProfiles failure unrelated // to rate limiting. handled=true with a non-nil err means GetPermissionProfiles hit the -// same rate limit GetUserDetails would also hit (codes.Unavailable — see -// pkg/client/helper.go); propagate it directly rather than also burning that call — -// otherwise every active user would pay for two failing calls instead of one while the -// account is already over budget, exactly the amplification this fix exists to reduce. -// handled=true with a nil err means the grant was resolved. +// same rate limit GetUserDetails would also hit — identified specifically via +// isReclassifiedRateLimitError, not codes.Unavailable alone (that code is broader — +// uhttp also maps a plain 503 or a transient network failure to it, and those should +// still fall back rather than fail every active user's Grants for the rest of the sync). +// Propagating the rate-limit case directly, rather than also falling back, avoids every +// active user paying for two failing calls instead of one while the account is already +// over budget — exactly the amplification this fix exists to reduce. handled=true with a +// nil err means the grant was resolved. +// +// Never forwards GetPermissionProfiles' annotations: after the first real call in a +// sync, repeat calls are served from uhttp's GET cache, which replays that first +// response's rate-limit snapshot verbatim — forwarding it on every subsequent active +// user would feed the SDK's self-throttling rate limiter a frozen, increasingly stale +// signal instead of the fresh per-request data GetUserDetails supplied before this fast +// path existed. func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resource, userID *v2.ResourceId) (*v2.Grant, annotations.Annotations, error, bool) { profile := rs.GetProfile(resource) @@ -144,16 +158,31 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc if !ok || userStatus != userStatusActive { return nil, nil, nil, false } + + // If the list response happened to include the profile ID directly (unconfirmed + // against a live account — see client.User.PermissionProfileID's doc), this skips + // GetPermissionProfiles entirely: no API call, no name lookup, no cache dependency. + if id, ok := rs.GetProfileStringValue(profile, profileFieldPermissionID); ok && id != "" { + newGrant := grant.NewGrant( + &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: id}}, + permissionProfileAssignedTag, + userID, + ) + return newGrant, nil, nil, true + } + name, ok := rs.GetProfileStringValue(profile, profileFieldPermission) if !ok || name == "" { return nil, nil, nil, false } - profiles, annos, err := b.client.GetPermissionProfiles(ctx) + profiles, _, err := b.client.GetPermissionProfiles(ctx) if err != nil { - if grpcstatus.Code(err) == codes.Unavailable { + if isReclassifiedRateLimitError(err) { return nil, nil, err, true } + ctxzap.Extract(ctx).Debug("baton-docusign: GetPermissionProfiles failed, falling back to per-user GetUserDetails for this Grants call", + zap.String("user_id", userID.Resource), zap.Error(err)) return nil, nil, nil, false } @@ -166,7 +195,7 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc permissionProfileAssignedTag, userID, ) - return newGrant, annos, nil, true + return newGrant, nil, nil, true } // CreateAccountCapabilityDetails declares support for account provisioning without a password. @@ -299,11 +328,12 @@ func parseIntoUserResource(user *client.User) (*v2.Resource, error) { } profile := map[string]any{ - "userName": user.UserName, - profileFieldEmail: user.Email, - "isAdmin": user.IsAdmin, - profileFieldPermission: user.Permission, - profileFieldStatus: user.UserStatus, + "userName": user.UserName, + profileFieldEmail: user.Email, + "isAdmin": user.IsAdmin, + profileFieldPermission: user.Permission, + profileFieldStatus: user.UserStatus, + profileFieldPermissionID: user.PermissionProfileID, } userTraits := []rs.UserTraitOption{ diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index b705e734..c56b6aa8 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -31,12 +31,20 @@ func (t *rewriteTransport) RoundTrip(req *http.Request) (*http.Response, error) return http.DefaultTransport.RoundTrip(req) } +// Forced-error modes for newUsersTestClient's permission_profiles endpoint. +const ( + permissionProfilesOK = "" + permissionProfilesRateLimit = "rate_limit" // DocuSign's real hourly-limit body + permissionProfilesForbidden = "forbidden" // a generic, unrelated failure + permissionProfilesServiceUnavailable = "service_unavailable" // a plain 503, no rate-limit body at all +) + // usersTestServer wires a *client.Client to a mock server handling /oauth/userinfo, // GET permission_profiles, and GET users/{id} — everything userBuilder.Grants needs -// across both its fast path and its GetUserDetails fallback. If forcePermissionProfilesRateLimit -// is set, the permission_profiles endpoint returns DocuSign's real hourly-rate-limit body -// instead of the profiles list, regardless of what's in profiles. -func newUsersTestClient(t *testing.T, profiles []client.PermissionProfile, userDetails map[string]client.UserDetail, forcePermissionProfilesRateLimit bool) *client.Client { +// across both its fast path and its GetUserDetails fallback. forcedPermissionProfilesError +// selects what the permission_profiles endpoint returns instead of the profiles list — +// see the permissionProfiles* constants above. +func newUsersTestClient(t *testing.T, profiles []client.PermissionProfile, userDetails map[string]client.UserDetail, forcedPermissionProfilesError string) *client.Client { t.Helper() mockServer := httptest.NewServer(nil) t.Cleanup(mockServer.Close) @@ -53,13 +61,25 @@ func newUsersTestClient(t *testing.T, profiles []client.PermissionProfile, userD }, }) case "/restapi/v2.1/accounts/acct-1/permission_profiles": - if forcePermissionProfilesRateLimit { + switch forcedPermissionProfilesError { + case permissionProfilesRateLimit: w.WriteHeader(http.StatusBadRequest) _ = json.NewEncoder(w).Encode(client.ErrorResponse{ ErrorCode: "HOURLY_APIINVOCATION_LIMIT_EXCEEDED", ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", }) return + case permissionProfilesForbidden: + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ + ErrorCode: "USER_LACKS_PERMISSIONS", + ErrorMessage: "The user does not have permission to access permission profiles.", + }) + return + case permissionProfilesServiceUnavailable: + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) + return } _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{PermissionProfiles: profiles}) default: @@ -98,7 +118,7 @@ func TestUserBuilder_Grants_FastPath_ActiveUserWithKnownProfile(t *testing.T) { c := newUsersTestClient(t, []client.PermissionProfile{ {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, {PermissionProfileId: "pp-2", PermissionProfileName: "DocuSign Viewer"}, - }, nil, false) // no user-details fixtures — a fallback call here would 404 and fail the test + }, nil, permissionProfilesOK) // no user-details fixtures — a fallback call here would 404 and fail the test b := newUserBuilder(c) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") @@ -126,7 +146,7 @@ func TestUserBuilder_Grants_FallsBackWhenNotActive(t *testing.T) { {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, map[string]client.UserDetail{ "user-1": {UserID: "user-1", PermissionProfileID: ""}, // matches "non-active users have no PP" - }, false) + }, permissionProfilesOK) b := newUserBuilder(c) resource := userResourceWithProfile(t, "user-1", "Disabled", "DocuSign Admin") @@ -147,7 +167,7 @@ func TestUserBuilder_Grants_FallsBackWhenProfileNameUnresolvable(t *testing.T) { {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, map[string]client.UserDetail{ "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, - }, false) + }, permissionProfilesOK) b := newUserBuilder(c) resource := userResourceWithProfile(t, "user-1", userStatusActive, "A Since-Renamed Profile") @@ -171,7 +191,7 @@ func TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls(t *testing // it "succeed" and hide the bug — present specifically so the fallthrough case // would be caught if it fired instead of propagating. "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, - }, true) + }, permissionProfilesRateLimit) b := newUserBuilder(c) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") @@ -184,6 +204,72 @@ func TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls(t *testing } } +// TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure covers the +// other half of the fast path's error handling: a GetPermissionProfiles failure that +// ISN'T the reclassified rate-limit error (e.g. a permissions/scope issue) must still +// fall through to GetUserDetails and resolve the grant, not propagate the error. +func TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure(t *testing.T) { + c := newUsersTestClient(t, nil, map[string]client.UserDetail{ + "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, + }, permissionProfilesForbidden) + b := newUserBuilder(c) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("expected the GetUserDetails fallback to resolve pp-1, got %+v", grants) + } +} + +// TestUserBuilder_Grants_FallsBackOnServiceUnavailable is a regression test for a +// deep-code-review finding: codes.Unavailable is broader than "already rate-limited" — +// uhttp also maps a plain HTTP 503 to it. A 503 from GetPermissionProfiles (no +// RateLimitDescription attached, unlike the reclassified rate-limit error) must fall +// through to GetUserDetails, not be mistaken for the rate-limit case and propagated. +func TestUserBuilder_Grants_FallsBackOnServiceUnavailable(t *testing.T) { + c := newUsersTestClient(t, nil, map[string]client.UserDetail{ + "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, + }, permissionProfilesServiceUnavailable) + b := newUserBuilder(c) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("expected the GetUserDetails fallback to resolve pp-1, got %+v", grants) + } +} + +// TestUserBuilder_Grants_FastPath_PrefersDirectProfileIDOverName covers the +// PermissionProfileID-on-the-list-response path: when present, it must skip +// GetPermissionProfiles entirely (no profiles fixture is provided — a call would 404 and +// fail this test) and use the ID directly. +func TestUserBuilder_Grants_FastPath_PrefersDirectProfileIDOverName(t *testing.T) { + c := newUsersTestClient(t, nil, nil, permissionProfilesOK) + b := newUserBuilder(c) + resource, err := rs.NewUserResource("user-1", userResourceType, "user-1", nil, rs.WithResourceProfile(map[string]any{ + profileFieldStatus: userStatusActive, + profileFieldPermission: "DocuSign Admin", + profileFieldPermissionID: "pp-1", + })) + if err != nil { + t.Fatalf("NewUserResource: %v", err) + } + + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("expected the direct profile ID to resolve pp-1 with no API call, got %+v", grants) + } +} + func TestUserBuilder_Grants_FallsBackWhenProfileFieldMissing(t *testing.T) { // An identity-only or otherwise profile-less resource must not panic or skip the // grant — it should behave exactly as it did before the fast path existed. @@ -191,7 +277,7 @@ func TestUserBuilder_Grants_FallsBackWhenProfileFieldMissing(t *testing.T) { {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, map[string]client.UserDetail{ "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, - }, false) + }, permissionProfilesOK) b := newUserBuilder(c) resource, err := rs.NewUserResource("user-1", userResourceType, "user-1", nil) if err != nil { From 07f84f97994a3c94513580a6166831145d05bd3c Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 20:36:46 -0300 Subject: [PATCH 06/28] fix: drop ticket/review-process references from code comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ticket numbers (Pylon #11445) and "review finding"/"deep-code-review finding" framing belong in the PR description, not the source — they rot and don't help a future reader. Keep only the durable technical reasoning behind each choice. --- pkg/client/helper.go | 8 ++++---- pkg/client/helper_test.go | 19 +++++++++---------- pkg/connector/users.go | 14 +++++++------- pkg/connector/users_test.go | 24 +++++++++++------------- 4 files changed, 31 insertions(+), 34 deletions(-) diff --git a/pkg/client/helper.go b/pkg/client/helper.go index cfc1835c..3eee6548 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -20,10 +20,10 @@ import ( const DefaultPageSize = 100 // docusignHourlyRateLimitErrorCode is the eSignature API's JSON error-body errorCode for -// "the account's hourly API-call budget is exhausted" — confirmed against a real account -// (Pylon #11445). DocuSign returns this as HTTP 400 today and is mid-migration to 429 -// (DocuSign's own guidance is to key off errorCode, not HTTP status, for exactly this -// reason), so detection below checks the body field independent of resp.StatusCode. +// "the account's hourly API-call budget is exhausted" — confirmed against a real account. +// DocuSign returns this as HTTP 400 today and is mid-migration to 429 (DocuSign's own +// guidance is to key off errorCode, not HTTP status, for exactly this reason), so +// detection below checks the body field independent of resp.StatusCode. const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" // docusignRateLimitDefaultResetWindow is the fixed wait this connector asks the SDK's diff --git a/pkg/client/helper_test.go b/pkg/client/helper_test.go index b75bf72a..5887384d 100644 --- a/pkg/client/helper_test.go +++ b/pkg/client/helper_test.go @@ -17,14 +17,13 @@ import ( "google.golang.org/grpc/status" ) -// TestReclassifyHourlyRateLimitError is a regression test for Pylon #11445: DocuSign signals -// "hourly API-call budget exhausted" via a JSON error body (errorCode -// HOURLY_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400, which uhttp.GrpcCodeFromHTTPStatus maps -// to codes.InvalidArgument — a code the SDK's sync-retry loop treats as fatal, not -// retryable, so a real customer's initial full sync failed outright instead of pausing and -// resuming. reclassifyHourlyRateLimitError must re-classify exactly this case as -// codes.Unavailable (which the SDK does retry) carrying a RateLimitDescription, and leave -// every other error (including CLM's distinct error envelope) untouched. +// TestReclassifyHourlyRateLimitError: DocuSign signals "hourly API-call budget exhausted" +// via a JSON error body (errorCode HOURLY_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400, +// which uhttp.GrpcCodeFromHTTPStatus maps to codes.InvalidArgument — a code the SDK's +// sync-retry loop treats as fatal, not retryable, so a full sync fails outright instead +// of pausing and resuming. reclassifyHourlyRateLimitError must re-classify exactly this +// case as codes.Unavailable (which the SDK does retry) carrying a RateLimitDescription, +// and leave every other error (including CLM's distinct error envelope) untouched. func TestReclassifyHourlyRateLimitError(t *testing.T) { origErr := errors.New("400 Bad Request") @@ -89,8 +88,8 @@ func TestReclassifyHourlyRateLimitError(t *testing.T) { }) } -// TestGetUsers_ClassifiesHourlyRateLimitAsRetryable is an end-to-end regression test for -// Pylon #11445, exercising the real request path (GetUsers -> doRequestCommon -> +// TestGetUsers_ClassifiesHourlyRateLimitAsRetryable is an end-to-end regression test, +// exercising the real request path (GetUsers -> doRequestCommon -> // reclassifyHourlyRateLimitError) against a mock server that returns DocuSign's actual // observed 400 body, rather than calling reclassifyHourlyRateLimitError directly. func TestGetUsers_ClassifiesHourlyRateLimitAsRetryable(t *testing.T) { diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 7bb78d71..e8b89690 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -79,10 +79,10 @@ func (b *userBuilder) Entitlements(_ context.Context, _ *v2.Resource, _ rs.SyncO // // Tries tryFastPathGrant first (an Active user's permission-profile NAME, already // captured on the resource's profile during List(), resolved via one account-wide -// GetPermissionProfiles call instead of a per-user GetUserDetails call — the N+1 -// pattern Pylon #11445 flagged as contributing to DocuSign's hourly rate limit) and -// falls back to the always-correct per-user GetUserDetails path unchanged from before -// that fast path existed whenever it declines to handle the request (see its own doc). +// GetPermissionProfiles call instead of a per-user GetUserDetails call — avoiding the +// N+1 pattern that contributes to DocuSign's hourly rate limit) and falls back to the +// always-correct per-user GetUserDetails path unchanged from before that fast path +// existed whenever it declines to handle the request (see its own doc). func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { userID := resource.Id @@ -122,9 +122,9 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy } // tryFastPathGrant is Grants' fast path for an Active user, avoiding the per-user -// GetUserDetails call Pylon #11445 flagged as contributing to DocuSign's hourly rate -// limit. Two ways it can resolve the grant without that call, both already captured on -// the resource's profile during List(): +// GetUserDetails call that contributes to DocuSign's hourly rate limit. Two ways it can +// resolve the grant without that call, both already captured on the resource's profile +// during List(): // - Preferred: client.User.PermissionProfileID directly, if the list response included // it (unconfirmed against a live account — see that field's doc) — no API call at all. // - Otherwise: the permission-profile NAME, resolved to an ID via GetPermissionProfiles diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index c56b6aa8..5a08ca92 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -139,9 +139,8 @@ func TestUserBuilder_Grants_FastPath_ActiveUserWithKnownProfile(t *testing.T) { func TestUserBuilder_Grants_FallsBackWhenNotActive(t *testing.T) { // A non-active user must go through GetUserDetails, exactly like before this fast - // path existed — this is the regression test for the review finding that the fast - // path could otherwise grant a profile to a disabled/closed user the old code - // would have correctly skipped. + // path existed — otherwise the fast path could grant a profile to a disabled/closed + // user that GetUserDetails would correctly skip. c := newUsersTestClient(t, []client.PermissionProfile{ {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, map[string]client.UserDetail{ @@ -180,11 +179,10 @@ func TestUserBuilder_Grants_FallsBackWhenProfileNameUnresolvable(t *testing.T) { } } -// TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls is a regression test -// for a deep-code-review finding: if GetPermissionProfiles fails because the account is -// already rate-limited (the exact scenario Pylon #11445 is about), falling through to -// GetUserDetails would hit the identical limit and double the failing calls per active -// user instead of reducing them. Grants must propagate that error directly. +// TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls: if +// GetPermissionProfiles fails because the account is already rate-limited, falling +// through to GetUserDetails would hit the identical limit and double the failing calls +// per active user instead of reducing them. Grants must propagate that error directly. func TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls(t *testing.T) { c := newUsersTestClient(t, nil, map[string]client.UserDetail{ // If Grants incorrectly falls through to GetUserDetails, this fixture would let @@ -224,11 +222,11 @@ func TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure(t * } } -// TestUserBuilder_Grants_FallsBackOnServiceUnavailable is a regression test for a -// deep-code-review finding: codes.Unavailable is broader than "already rate-limited" — -// uhttp also maps a plain HTTP 503 to it. A 503 from GetPermissionProfiles (no -// RateLimitDescription attached, unlike the reclassified rate-limit error) must fall -// through to GetUserDetails, not be mistaken for the rate-limit case and propagated. +// TestUserBuilder_Grants_FallsBackOnServiceUnavailable: codes.Unavailable is broader +// than "already rate-limited" — uhttp also maps a plain HTTP 503 to it. A 503 from +// GetPermissionProfiles (no RateLimitDescription attached, unlike the reclassified +// rate-limit error) must fall through to GetUserDetails, not be mistaken for the +// rate-limit case and propagated. func TestUserBuilder_Grants_FallsBackOnServiceUnavailable(t *testing.T) { c := newUsersTestClient(t, nil, map[string]client.UserDetail{ "user-1": {UserID: "user-1", PermissionProfileID: "pp-1"}, From a93f3f21d492b15a0ab07b0fe349a8a3a6bf90c2 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 21:04:43 -0300 Subject: [PATCH 07/28] fix: treat ambiguous permission-profile names as not-found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit permissionProfileIDByName picked the first name match, but DocuSign doesn't guarantee PermissionProfileName is unique per account — two profiles sharing a name would silently grant/revoke the wrong one. Both callers already have a safe fallback for "not found", so treat an ambiguous match the same way instead of guessing. Also: fix an inaccurate test comment describing the wrong 404 path, and document that the fast path's call-amplification win depends on uhttp's GET cache being enabled. --- pkg/connector/helper.go | 26 +++++++++++----- pkg/connector/helper_test.go | 57 ++++++++++++++++++++++++++++++++++++ pkg/connector/users.go | 7 +++++ pkg/connector/users_test.go | 6 ++-- 4 files changed, 87 insertions(+), 9 deletions(-) diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 5dfdaff3..7b44d9e1 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -13,9 +13,9 @@ import ( // Shared profile/field map keys, reused across builders (and the AccountCreationSchema // field map in connector.go) to avoid repeated string literals (golangci-lint: goconst). const ( - profileFieldEmail = "email" - profileFieldUsername = "username" - profileFieldGroupName = "group_name" + profileFieldEmail = "email" + profileFieldUsername = "username" + profileFieldGroupName = "group_name" profileFieldPermission = "permission" profileFieldStatus = "status" profileFieldPermissionID = "permission_profile_id" @@ -113,18 +113,30 @@ func isOptInFeatureUnavailableError(err error) bool { } // permissionProfileIDByName returns the ID of the permission profile named name, and -// whether one was found — shared by userBuilder.Grants' list-response fast path and -// permissionProfilesBuilder.Revoke's default-profile lookup, which independently +// whether exactly one was found — shared by userBuilder.Grants' list-response fast path +// and permissionProfilesBuilder.Revoke's default-profile lookup, which independently // duplicated this same linear scan before this helper existed. Requires a non-empty ID, // matching Revoke's original inline check (`if defaultProfileID == ""`) that a name match // with no usable ID counts as not found, not as an empty-string result. +// +// DocuSign does not guarantee PermissionProfileName is unique per account, so two +// profiles sharing a name is treated the same as zero matches (not found) rather than +// picking the first: both callers have a safe fallback for "not found" (Grants' caller +// falls back to the per-user GetUserDetails path; Revoke's caller surfaces a clear +// error), whereas silently guessing wrong here would grant or revoke the wrong profile. func permissionProfileIDByName(profiles []client.PermissionProfile, name string) (string, bool) { + id := "" + matches := 0 for _, p := range profiles { if p.PermissionProfileName == name && p.PermissionProfileId != "" { - return p.PermissionProfileId, true + id = p.PermissionProfileId + matches++ } } - return "", false + if matches != 1 { + return "", false + } + return id, true } // clmIDFromHref extracts the trailing path segment from a CLM object's Href — CLM's diff --git a/pkg/connector/helper_test.go b/pkg/connector/helper_test.go index 034ed990..f9923e2b 100644 --- a/pkg/connector/helper_test.go +++ b/pkg/connector/helper_test.go @@ -4,6 +4,7 @@ import ( "errors" "testing" + "github.com/conductorone/baton-docusign/pkg/client" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -32,3 +33,59 @@ func TestIsOptInFeatureUnavailableError(t *testing.T) { }) } } + +func TestPermissionProfileIDByName(t *testing.T) { + tests := []struct { + name string + profiles []client.PermissionProfile + lookup string + wantID string + wantOK bool + }{ + { + name: "single match", + profiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + lookup: "DocuSign Admin", + wantID: "pp-1", + wantOK: true, + }, + { + name: "no match", + profiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + lookup: "Nonexistent", + wantOK: false, + }, + { + name: "match with no usable ID counts as not found", + profiles: []client.PermissionProfile{ + {PermissionProfileId: "", PermissionProfileName: "DocuSign Admin"}, + }, + lookup: "DocuSign Admin", + wantOK: false, + }, + { + // DocuSign does not guarantee PermissionProfileName is unique per account — + // picking the first match here would risk granting/revoking the wrong + // profile, so an ambiguous name must be treated as not found. + name: "ambiguous name is treated as not found, not the first match", + profiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "Custom Profile"}, + {PermissionProfileId: "pp-2", PermissionProfileName: "Custom Profile"}, + }, + lookup: "Custom Profile", + wantOK: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotID, gotOK := permissionProfileIDByName(tt.profiles, tt.lookup) + if gotOK != tt.wantOK || (gotOK && gotID != tt.wantID) { + t.Errorf("permissionProfileIDByName(%v, %q) = (%q, %v), want (%q, %v)", tt.profiles, tt.lookup, gotID, gotOK, tt.wantID, tt.wantOK) + } + }) + } +} diff --git a/pkg/connector/users.go b/pkg/connector/users.go index e8b89690..55ac57bd 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -131,6 +131,13 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy // (one account-wide call, already served from uhttp's default GET cache on every call // after the first in this sync — see pkg/client/clm_client.go's WithNoCache usage for // this repo's opt-out convention when a fresh read matters, which this doesn't need). +// This path's call-amplification win depends entirely on that cache being enabled and +// large enough to hold the response for the rest of the sync: an operator running with +// BATON_DISABLE_HTTP_CACHE=true, BATON_HTTP_CACHE_BACKEND=noop, or a small enough +// BATON_HTTP_CACHE_TTL/size budget gets one GetPermissionProfiles call per active user +// instead — the same 1:1 amplification as the GetUserDetails path this fast path exists +// to avoid, just against a different endpoint. Silent, not incorrect: Grants still +// resolves correctly either way. // // handled=false means "no decision, fall back to Grants' original GetUserDetails path // unchanged" — covers a non-active user, a missing profile field, an unresolvable name diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 5a08ca92..805df18f 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -245,8 +245,10 @@ func TestUserBuilder_Grants_FallsBackOnServiceUnavailable(t *testing.T) { // TestUserBuilder_Grants_FastPath_PrefersDirectProfileIDOverName covers the // PermissionProfileID-on-the-list-response path: when present, it must skip -// GetPermissionProfiles entirely (no profiles fixture is provided — a call would 404 and -// fail this test) and use the ID directly. +// GetPermissionProfiles entirely and use the ID directly. If it fell through instead, +// GetPermissionProfiles would succeed with an empty list (no profiles fixture is +// provided), fail to resolve "DocuSign Admin" by name, and fall through again to +// GetUserDetails — which 404s (no userDetails fixture either) and fails this test. func TestUserBuilder_Grants_FastPath_PrefersDirectProfileIDOverName(t *testing.T) { c := newUsersTestClient(t, nil, nil, permissionProfilesOK) b := newUserBuilder(c) From 598b2766cd346f55ab46d3ace857128ae8eba387 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 11 Aug 2026 21:19:21 -0300 Subject: [PATCH 08/28] fix: distinguish ambiguous from missing default permission profile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit permissionProfileIDByName's !ok now covers two different causes (zero matches vs. an ambiguous name), but Revoke's error message only ever said "not found" — misleading an operator into looking for a missing profile that actually exists twice under the same name. --- pkg/connector/permission_profiles.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/pkg/connector/permission_profiles.go b/pkg/connector/permission_profiles.go index 3eb41029..00ba8b17 100644 --- a/pkg/connector/permission_profiles.go +++ b/pkg/connector/permission_profiles.go @@ -119,6 +119,19 @@ func (p *permissionProfilesBuilder) Revoke(ctx context.Context, grantObj *v2.Gra defaultProfileID, ok := permissionProfileIDByName(permissionProfiles, defaultPermissionProfileName) if !ok { + // permissionProfileIDByName also returns false when the name matches more than + // one profile (ambiguous, not missing) — distinguish the two here so the error + // doesn't send an operator looking for a missing profile that actually exists + // twice under the same name. + matches := 0 + for _, p := range permissionProfiles { + if p.PermissionProfileName == defaultPermissionProfileName { + matches++ + } + } + if matches > 1 { + return profileAnnos, fmt.Errorf("default permission profile '%s' is ambiguous: %d profiles share that name in this account", defaultPermissionProfileName, matches) + } return profileAnnos, fmt.Errorf("default permission profile '%s' not found in account", defaultPermissionProfileName) } From a050118e1e7fed4a77acb58ef221687f238b6ad7 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 12 Aug 2026 12:10:28 -0300 Subject: [PATCH 09/28] fix: permissionProfileIDByName exposes match count, drops duplicate scan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revoke's "ambiguous vs missing" recount block used a loop variable `p` that shadowed the method receiver (harmless today, but govet's shadow check is disabled in this repo's config, so a future use wouldn't be caught) and duplicated permissionProfileIDByName's matching condition without the `PermissionProfileId != ""` requirement — a profile with a matching name but no usable ID would count toward "ambiguous" here but not toward the helper's own match count, so the two could disagree. permissionProfileIDByName now returns the match count directly instead of an ok bool, so Revoke reads it straight off the single scan the helper already does instead of re-scanning with a diverging condition. --- pkg/connector/helper.go | 23 ++++------------ pkg/connector/helper_test.go | 41 +++++++++++++--------------- pkg/connector/permission_profiles.go | 20 ++++---------- pkg/connector/users.go | 4 +-- 4 files changed, 32 insertions(+), 56 deletions(-) diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 7b44d9e1..58f01f33 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -112,19 +112,11 @@ func isOptInFeatureUnavailableError(err error) bool { } } -// permissionProfileIDByName returns the ID of the permission profile named name, and -// whether exactly one was found — shared by userBuilder.Grants' list-response fast path -// and permissionProfilesBuilder.Revoke's default-profile lookup, which independently -// duplicated this same linear scan before this helper existed. Requires a non-empty ID, -// matching Revoke's original inline check (`if defaultProfileID == ""`) that a name match -// with no usable ID counts as not found, not as an empty-string result. -// -// DocuSign does not guarantee PermissionProfileName is unique per account, so two -// profiles sharing a name is treated the same as zero matches (not found) rather than -// picking the first: both callers have a safe fallback for "not found" (Grants' caller -// falls back to the per-user GetUserDetails path; Revoke's caller surfaces a clear -// error), whereas silently guessing wrong here would grant or revoke the wrong profile. -func permissionProfileIDByName(profiles []client.PermissionProfile, name string) (string, bool) { +// permissionProfileIDByName returns the ID of the profile named name (requiring a +// non-empty ID) and how many matched, so callers can tell "not found" (0) from +// "ambiguous" (2+) — names aren't guaranteed unique per account — without a second scan +// of their own. id is only meaningful when matches == 1. +func permissionProfileIDByName(profiles []client.PermissionProfile, name string) (string, int) { id := "" matches := 0 for _, p := range profiles { @@ -133,10 +125,7 @@ func permissionProfileIDByName(profiles []client.PermissionProfile, name string) matches++ } } - if matches != 1 { - return "", false - } - return id, true + return id, matches } // clmIDFromHref extracts the trailing path segment from a CLM object's Href — CLM's diff --git a/pkg/connector/helper_test.go b/pkg/connector/helper_test.go index f9923e2b..ad3c57c1 100644 --- a/pkg/connector/helper_test.go +++ b/pkg/connector/helper_test.go @@ -36,55 +36,52 @@ func TestIsOptInFeatureUnavailableError(t *testing.T) { func TestPermissionProfileIDByName(t *testing.T) { tests := []struct { - name string - profiles []client.PermissionProfile - lookup string - wantID string - wantOK bool + name string + profiles []client.PermissionProfile + lookup string + wantID string + wantMatches int }{ { name: "single match", profiles: []client.PermissionProfile{ {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, - lookup: "DocuSign Admin", - wantID: "pp-1", - wantOK: true, + lookup: "DocuSign Admin", + wantID: "pp-1", + wantMatches: 1, }, { name: "no match", profiles: []client.PermissionProfile{ {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, }, - lookup: "Nonexistent", - wantOK: false, + lookup: "Nonexistent", + wantMatches: 0, }, { - name: "match with no usable ID counts as not found", + name: "match with no usable ID doesn't count", profiles: []client.PermissionProfile{ {PermissionProfileId: "", PermissionProfileName: "DocuSign Admin"}, }, - lookup: "DocuSign Admin", - wantOK: false, + lookup: "DocuSign Admin", + wantMatches: 0, }, { - // DocuSign does not guarantee PermissionProfileName is unique per account — - // picking the first match here would risk granting/revoking the wrong - // profile, so an ambiguous name must be treated as not found. - name: "ambiguous name is treated as not found, not the first match", + name: "ambiguous name reports the match count, not the first match", profiles: []client.PermissionProfile{ {PermissionProfileId: "pp-1", PermissionProfileName: "Custom Profile"}, {PermissionProfileId: "pp-2", PermissionProfileName: "Custom Profile"}, }, - lookup: "Custom Profile", - wantOK: false, + lookup: "Custom Profile", + wantMatches: 2, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotID, gotOK := permissionProfileIDByName(tt.profiles, tt.lookup) - if gotOK != tt.wantOK || (gotOK && gotID != tt.wantID) { - t.Errorf("permissionProfileIDByName(%v, %q) = (%q, %v), want (%q, %v)", tt.profiles, tt.lookup, gotID, gotOK, tt.wantID, tt.wantOK) + gotID, gotMatches := permissionProfileIDByName(tt.profiles, tt.lookup) + if gotMatches != tt.wantMatches || (gotMatches == 1 && gotID != tt.wantID) { + t.Errorf("permissionProfileIDByName(%v, %q) = (%q, %d), want (%q, %d)", tt.profiles, tt.lookup, gotID, gotMatches, tt.wantID, tt.wantMatches) } }) } diff --git a/pkg/connector/permission_profiles.go b/pkg/connector/permission_profiles.go index 00ba8b17..c71ceb9a 100644 --- a/pkg/connector/permission_profiles.go +++ b/pkg/connector/permission_profiles.go @@ -117,21 +117,11 @@ func (p *permissionProfilesBuilder) Revoke(ctx context.Context, grantObj *v2.Gra return profileAnnos, fmt.Errorf("failed to get permission profiles: %w", err) } - defaultProfileID, ok := permissionProfileIDByName(permissionProfiles, defaultPermissionProfileName) - if !ok { - // permissionProfileIDByName also returns false when the name matches more than - // one profile (ambiguous, not missing) — distinguish the two here so the error - // doesn't send an operator looking for a missing profile that actually exists - // twice under the same name. - matches := 0 - for _, p := range permissionProfiles { - if p.PermissionProfileName == defaultPermissionProfileName { - matches++ - } - } - if matches > 1 { - return profileAnnos, fmt.Errorf("default permission profile '%s' is ambiguous: %d profiles share that name in this account", defaultPermissionProfileName, matches) - } + defaultProfileID, matches := permissionProfileIDByName(permissionProfiles, defaultPermissionProfileName) + if matches > 1 { + return profileAnnos, fmt.Errorf("default permission profile '%s' is ambiguous: %d profiles share that name in this account", defaultPermissionProfileName, matches) + } + if matches == 0 { return profileAnnos, fmt.Errorf("default permission profile '%s' not found in account", defaultPermissionProfileName) } diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 55ac57bd..5707bf62 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -193,8 +193,8 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc return nil, nil, nil, false } - id, ok := permissionProfileIDByName(profiles, name) - if !ok { + id, matches := permissionProfileIDByName(profiles, name) + if matches != 1 { return nil, nil, nil, false } newGrant := grant.NewGrant( From c53cde45276bb99a154c2f78187a97dd3bbf5864 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 12 Aug 2026 12:18:24 -0300 Subject: [PATCH 10/28] test: pin the mixed-ID-validity case for permissionProfileIDByName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two profiles sharing a name where only one has a usable ID must resolve to that one (matches == 1), not report ambiguous — the old recount loop this replaced would have miscounted this case as 2. --- pkg/connector/helper_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/connector/helper_test.go b/pkg/connector/helper_test.go index ad3c57c1..1575ed19 100644 --- a/pkg/connector/helper_test.go +++ b/pkg/connector/helper_test.go @@ -76,6 +76,18 @@ func TestPermissionProfileIDByName(t *testing.T) { lookup: "Custom Profile", wantMatches: 2, }, + { + // A same-name profile with no usable ID doesn't count toward "ambiguous" — + // only the one profile with a real ID matters here. + name: "same name, one with no usable ID, resolves to the valid one", + profiles: []client.PermissionProfile{ + {PermissionProfileId: "", PermissionProfileName: "Custom Profile"}, + {PermissionProfileId: "pp-2", PermissionProfileName: "Custom Profile"}, + }, + lookup: "Custom Profile", + wantID: "pp-2", + wantMatches: 1, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 3f3cdf39839ea40683f36246d0c9d3645b266120 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 18 Aug 2026 12:59:05 -0300 Subject: [PATCH 11/28] fix: memoize GetPermissionProfiles per sync; correct retry-window comment - tryFastPathGrant's name-lookup branch relied on uhttp's GET cache to make repeat GetPermissionProfiles calls cheap, but that cache never stores a non-2xx response. A persistent non-rate-limit failure (e.g. a service user lacking permission_profiles read access) re-hit the real API on every Active user instead of once per sync, doubling that user's calls (the failed lookup, then the GetUserDetails fallback) against the same hourly budget this fix exists to protect. Memoizes the call (success or failure) on userBuilder via sync.Once, since Grants() runs concurrently across users sharing one builder per sync. Added a regression test confirming exactly one real call across two Active users during a persistent failure. - docusignRateLimitDefaultResetWindow's doc overstated what the SDK's retry loop actually does with the 1-hour ResetAt: pkg/retry clamps the computed wait to its 60-second MaxDelay default before sleeping, so the net effect is a 60-second retry with unlimited attempts, not an hour of backoff. Corrected the comment; behavior is unchanged (this is an SDK-level gap, not something fixable from this connector). Co-Authored-By: Claude Sonnet 5 --- pkg/client/helper.go | 18 +++++++--- pkg/connector/users.go | 48 ++++++++++++++++++++------- pkg/connector/users_test.go | 66 +++++++++++++++++++++++++++++++++++++ 3 files changed, 116 insertions(+), 16 deletions(-) diff --git a/pkg/client/helper.go b/pkg/client/helper.go index 3eee6548..ca665d67 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -26,11 +26,19 @@ const DefaultPageSize = 100 // detection below checks the body field independent of resp.StatusCode. const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" -// docusignRateLimitDefaultResetWindow is the fixed wait this connector asks the SDK's -// retry loop to use for docusignHourlyRateLimitErrorCode — applied unconditionally, not -// just as a fallback (see reclassifyHourlyRateLimitError's doc for why response headers -// are deliberately never consulted for this error). The limit this error names is -// hourly, so an hour is the sane, safe choice. +// docusignRateLimitDefaultResetWindow is the fixed ResetAt this connector puts on the +// RateLimitDescription for docusignHourlyRateLimitErrorCode — applied unconditionally, +// not just as a fallback (see reclassifyHourlyRateLimitError's doc for why response +// headers are deliberately never consulted for this error). The limit this error names +// is hourly, so an hour is the semantically correct value to report — but it is not +// what the SDK's retry loop actually waits: pkg/sync/parallel_syncer.go constructs its +// Retryer with MaxDelay: 0, which retry.NewRetryer normalizes to a 60-second cap, and +// retry.Retryer.ShouldWaitAndRetry computes a wait from this ResetAt only to then clamp +// it down to that same 60 seconds (`if wait > maxDelay { wait = maxDelay }`). With +// MaxAttempts: 0 (unlimited), the net effect is a 60-second retry with no attempt limit +// for the rest of the hour, not an hour of backoff — still strictly better than the old +// fatal classification, but not a full-hour wait. That gap lives in baton-sdk's retry +// package, not something this connector can change from here. const docusignRateLimitDefaultResetWindow = time.Hour // reclassifyHourlyRateLimitError recognizes docusignHourlyRateLimitErrorCode in errTarget (the diff --git a/pkg/connector/users.go b/pkg/connector/users.go index b137edda..0642521f 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -3,6 +3,7 @@ package connector import ( "context" "fmt" + "sync" "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -27,6 +28,35 @@ type userBuilder struct { // entitlements are skipped, but when permission_profile is excluded the // grants pass is skipped too, since it is this builder's only output. skipPermissionProfileResourceType bool + + // permissionProfilesOnce/permissionProfiles/permissionProfilesErr memoize the one + // account-wide GetPermissionProfiles call tryFastPathGrant's name-lookup branch + // needs, across every Active user's Grants() call in this sync (a *userBuilder is + // constructed once per sync and Grants() runs concurrently across users, so this + // must be shared and safe for concurrent access — hence sync.Once, not a plain + // bool). uhttp's GET cache only ever caches a 200 response, never an error, so + // without this a persistent non-rate-limit failure (e.g. a service user lacking + // permission_profiles read access) would re-hit the real API on every Active user + // instead of once per sync — doubling that user's calls (the failed lookup, then the + // GetUserDetails fallback) against the same hourly budget this fix exists to + // protect. A transient blip is deliberately memoized as a failure for the rest of + // this sync too, not just genuine outages — the fallback path still resolves the + // grant correctly either way, so the only cost is skipping the fast path's call + // savings for this one sync run, in exchange for never amplifying calls during a + // real persistent failure. + permissionProfilesOnce sync.Once + permissionProfiles []client.PermissionProfile + permissionProfilesErr error +} + +// getPermissionProfiles returns the account's permission profiles, calling +// client.GetPermissionProfiles at most once for the lifetime of this userBuilder — see +// the memoization fields' doc on the struct above for why. +func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { + b.permissionProfilesOnce.Do(func() { + b.permissionProfiles, _, b.permissionProfilesErr = b.client.GetPermissionProfiles(ctx) + }) + return b.permissionProfiles, b.permissionProfilesErr } // ResourceType returns the Baton resource type handled by this builder, @@ -153,16 +183,12 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy // - Preferred: client.User.PermissionProfileID directly, if the list response included // it (unconfirmed against a live account — see that field's doc) — no API call at all. // - Otherwise: the permission-profile NAME, resolved to an ID via GetPermissionProfiles -// (one account-wide call, already served from uhttp's default GET cache on every call -// after the first in this sync — see pkg/client/clm_client.go's WithNoCache usage for -// this repo's opt-out convention when a fresh read matters, which this doesn't need). -// This path's call-amplification win depends entirely on that cache being enabled and -// large enough to hold the response for the rest of the sync: an operator running with -// BATON_DISABLE_HTTP_CACHE=true, BATON_HTTP_CACHE_BACKEND=noop, or a small enough -// BATON_HTTP_CACHE_TTL/size budget gets one GetPermissionProfiles call per active user -// instead — the same 1:1 amplification as the GetUserDetails path this fast path exists -// to avoid, just against a different endpoint. Silent, not incorrect: Grants still -// resolves correctly either way. +// — one account-wide call for the whole sync, via getPermissionProfiles' own +// memoization on this builder (see its doc), not uhttp's GET cache: that cache never +// stores a non-2xx response, so relying on it alone would let a persistent failure +// (not just a rate limit — e.g. a service user lacking permission_profiles read +// access) re-hit the real API once per Active user instead of once per sync, the +// same 1:1 amplification as the GetUserDetails path this fast path exists to avoid. // // handled=false means "no decision, fall back to Grants' original GetUserDetails path // unchanged" — covers a non-active user, a missing profile field, an unresolvable name @@ -208,7 +234,7 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc return nil, nil, nil, false } - profiles, _, err := b.client.GetPermissionProfiles(ctx) + profiles, err := b.getPermissionProfiles(ctx) if err != nil { if isReclassifiedRateLimitError(err) { return nil, nil, err, true diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 8115d963..35587591 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -425,6 +425,72 @@ func TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure(t * } } +// TestUserBuilder_Grants_MemoizesPermissionProfilesFailureAcrossUsers is a regression +// test: uhttp's GET cache never stores a non-2xx response, so without its own +// memoization, tryFastPathGrant would re-hit GetPermissionProfiles for every Active user +// during a persistent (non-rate-limit) failure — doubling that user's calls (the failed +// lookup, then the GetUserDetails fallback) instead of the single fallback call this +// fast path is supposed to cost. Two Active users sharing one userBuilder must trigger +// exactly one real GetPermissionProfiles call, with both still resolving their grant via +// the GetUserDetails fallback. +func TestUserBuilder_Grants_MemoizesPermissionProfilesFailureAcrossUsers(t *testing.T) { + var permissionProfilesCalls int32 + + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case "/restapi/v2.1/accounts/acct-1/permission_profiles": + atomic.AddInt32(&permissionProfilesCalls, 1) + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ + ErrorCode: "USER_LACKS_PERMISSIONS", + ErrorMessage: "The user does not have permission to access permission profiles.", + }) + default: + const prefix = "/restapi/v2.1/accounts/acct-1/users/" + if len(r.URL.Path) > len(prefix) && r.URL.Path[:len(prefix)] == prefix { + userID := r.URL.Path[len(prefix):] + _ = json.NewEncoder(w).Encode(client.UserDetail{UserID: userID, PermissionProfileID: "pp-1"}) + return + } + http.NotFound(w, r) + } + }) + + mockServerURL, err := url.Parse(mockServer.URL) + if err != nil { + t.Fatalf("failed to parse mock server URL: %v", err) + } + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) + + b := newUserBuilder(c, false) + for _, userID := range []string{"user-1", "user-2"} { + resource := userResourceWithProfile(t, userID, userStatusActive, "DocuSign Admin") + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants(%s): %v", userID, err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("Grants(%s): expected the GetUserDetails fallback to resolve pp-1, got %+v", userID, grants) + } + } + + if got := atomic.LoadInt32(&permissionProfilesCalls); got != 1 { + t.Errorf("expected exactly 1 GetPermissionProfiles call across both users, got %d", got) + } +} + // TestUserBuilder_Grants_FallsBackOnServiceUnavailable: codes.Unavailable is broader // than "already rate-limited" — uhttp also maps a plain HTTP 503 to it. A 503 from // GetPermissionProfiles (no RateLimitDescription attached, unlike the reclassified From e671a67f18f2a5b5ba63911dac132addd22357b0 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 18 Aug 2026 13:09:52 -0300 Subject: [PATCH 12/28] fix: don't memoize rate-limit/context errors in getPermissionProfiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Caching a reclassified rate-limit error alongside genuine persistent failures was a blocking regression: the SDK's per-action retry loop reuses this same userBuilder across every retry, so once the first call hit the hourly limit, every subsequent retry replayed the cached Unavailable error without ever issuing a real request again — spinning at the retryer's ~60s interval forever instead of recovering once the account's hourly window resets, defeating the entire point of this PR's error reclassification. Only a genuinely persistent failure (e.g. the existing 403 case) is cached now; a reclassified rate-limit error or a context cancellation/deadline error is left uncached so the next caller re-attempts against the real API. Added a regression test asserting GetPermissionProfiles is called on every retry during a rate-limited window, not just once. Co-Authored-By: Claude Sonnet 5 --- pkg/connector/users.go | 66 ++++++++++++++++++++++++++----------- pkg/connector/users_test.go | 60 +++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 20 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 0642521f..ba687b84 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -2,6 +2,7 @@ package connector import ( "context" + "errors" "fmt" "sync" @@ -29,34 +30,59 @@ type userBuilder struct { // grants pass is skipped too, since it is this builder's only output. skipPermissionProfileResourceType bool - // permissionProfilesOnce/permissionProfiles/permissionProfilesErr memoize the one - // account-wide GetPermissionProfiles call tryFastPathGrant's name-lookup branch - // needs, across every Active user's Grants() call in this sync (a *userBuilder is - // constructed once per sync and Grants() runs concurrently across users, so this - // must be shared and safe for concurrent access — hence sync.Once, not a plain - // bool). uhttp's GET cache only ever caches a 200 response, never an error, so - // without this a persistent non-rate-limit failure (e.g. a service user lacking + // permissionProfilesMu/permissionProfilesCached/permissionProfiles/permissionProfilesErr + // memoize the one account-wide GetPermissionProfiles call tryFastPathGrant's + // name-lookup branch needs, across every Active user's Grants() call in this sync (a + // *userBuilder is constructed once per sync and Grants() runs concurrently across + // users, so this must be shared and safe for concurrent access — hence a mutex, not + // a plain bool). uhttp's GET cache only ever caches a 200 response, never an error, + // so without this a persistent non-rate-limit failure (e.g. a service user lacking // permission_profiles read access) would re-hit the real API on every Active user // instead of once per sync — doubling that user's calls (the failed lookup, then the // GetUserDetails fallback) against the same hourly budget this fix exists to - // protect. A transient blip is deliberately memoized as a failure for the rest of - // this sync too, not just genuine outages — the fallback path still resolves the - // grant correctly either way, so the only cost is skipping the fast path's call - // savings for this one sync run, in exchange for never amplifying calls during a - // real persistent failure. - permissionProfilesOnce sync.Once - permissionProfiles []client.PermissionProfile - permissionProfilesErr error + // protect. Only a genuinely persistent failure is cached this way, though — see + // getPermissionProfiles' doc for why a rate-limit or context error is deliberately + // left uncached. + permissionProfilesMu sync.Mutex + permissionProfilesCached bool + permissionProfiles []client.PermissionProfile + permissionProfilesErr error } // getPermissionProfiles returns the account's permission profiles, calling // client.GetPermissionProfiles at most once for the lifetime of this userBuilder — see -// the memoization fields' doc on the struct above for why. +// the memoization fields' doc on the struct above for why — with one deliberate +// exception: a reclassified rate-limit error, or a context-cancellation/deadline error, +// is never cached. Caching either would be actively harmful, not just a missed +// optimization: +// - A rate-limit error is exactly the case reclassifyHourlyRateLimitError's Unavailable +// reclassification exists to make retryable. The SDK's per-action retry loop +// (pkg/sync/parallel_syncer.go, unlimited attempts) reuses this same userBuilder +// across every retry of this action, so caching the error would replay the same +// stale rate-limit failure on every retry forever, without ever issuing a fresh +// request — the sync would spin at the retryer's ~60s interval and never notice the +// account's hourly window has actually reset, defeating the whole point of this fix. +// - A context error only means whichever caller's context happened to win this call +// was already done — not that the account or its permissions are actually broken. +// Caching it would incorrectly drop every other Active user in the sync back to the +// per-user GetUserDetails fallback for the rest of the run. func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { - b.permissionProfilesOnce.Do(func() { - b.permissionProfiles, _, b.permissionProfilesErr = b.client.GetPermissionProfiles(ctx) - }) - return b.permissionProfiles, b.permissionProfilesErr + b.permissionProfilesMu.Lock() + defer b.permissionProfilesMu.Unlock() + + if b.permissionProfilesCached { + return b.permissionProfiles, b.permissionProfilesErr + } + + profiles, _, err := b.client.GetPermissionProfiles(ctx) + if err != nil && (isReclassifiedRateLimitError(err) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) { + return nil, err + } + + b.permissionProfilesCached = true + b.permissionProfiles = profiles + b.permissionProfilesErr = err + return profiles, err } // ResourceType returns the Baton resource type handled by this builder, diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 35587591..baf3ceae 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -405,6 +405,66 @@ func TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls(t *testing } } +// TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure is a regression test: unlike a +// persistent non-rate-limit failure, a reclassified rate-limit error must NOT be cached +// on the builder — caching it would replay the same stale error on every retry of the +// SDK's per-action retry loop (which reuses this same userBuilder), spinning forever at +// the retryer's clamped interval instead of ever re-checking whether the account's +// hourly window has reset. Two Grants() calls against a rate-limited mock, on the same +// builder, must each issue a real GetPermissionProfiles call. +func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { + var permissionProfilesCalls int32 + + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case "/restapi/v2.1/accounts/acct-1/permission_profiles": + atomic.AddInt32(&permissionProfilesCalls, 1) + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ + ErrorCode: "HOURLY_APIINVOCATION_LIMIT_EXCEEDED", + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", + }) + default: + http.NotFound(w, r) + } + }) + + mockServerURL, err := url.Parse(mockServer.URL) + if err != nil { + t.Fatalf("failed to parse mock server URL: %v", err) + } + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) + + b := newUserBuilder(c, false) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + for i := 0; i < 2; i++ { + _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err == nil { + t.Fatalf("call %d: expected Grants to propagate the rate-limit error, got nil", i) + } + if got := grpcstatus.Code(err); got != codes.Unavailable { + t.Fatalf("call %d: expected codes.Unavailable, got %v: %v", i, got, err) + } + } + + if got := atomic.LoadInt32(&permissionProfilesCalls); got != 2 { + t.Errorf("expected GetPermissionProfiles to be called on every retry (2 calls), got %d — the rate-limit error must not be memoized", got) + } +} + // TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure covers the // other half of the fast path's error handling: a GetPermissionProfiles failure that // ISN'T the reclassified rate-limit error (e.g. a permissions/scope issue) must still From 541e958d45a3bdc22c85c5b5fd407addcb7ce27d Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 18 Aug 2026 13:20:49 -0300 Subject: [PATCH 13/28] fix: widen non-cacheable-error carve-out beyond reclassified rate limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isCacheablePermissionProfilesError replaces the previous isReclassifiedRateLimitError-specific check: a plain transient 5xx or network blip also arrives as codes.Unavailable but carries no RateLimitDescription, so it fell outside the old carve-out and was being cached permanently — same bug class as the rate-limit case just fixed, just for ordinary transient failures. Now only PermissionDenied/Unauthenticated/NotFound (mirroring isOptInFeatureUnavailableError's persistent-failure classification) are cacheable; everything else, including context errors, is not. Added regression tests for both the newly-widened case (a plain 503) and the previously-untested context-cancellation path (which now works correctly by construction, since Unknown/Canceled codes were never in the cacheable allowlist). Co-Authored-By: Claude Sonnet 5 --- pkg/connector/users.go | 59 +++++++++++------ pkg/connector/users_test.go | 123 ++++++++++++++++++++++++++++++++++++ 2 files changed, 161 insertions(+), 21 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index ba687b84..dcd60551 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -2,7 +2,6 @@ package connector import ( "context" - "errors" "fmt" "sync" @@ -14,6 +13,8 @@ import ( rs "github.com/conductorone/baton-sdk/pkg/types/resource" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/proto" ) @@ -36,36 +37,52 @@ type userBuilder struct { // *userBuilder is constructed once per sync and Grants() runs concurrently across // users, so this must be shared and safe for concurrent access — hence a mutex, not // a plain bool). uhttp's GET cache only ever caches a 200 response, never an error, - // so without this a persistent non-rate-limit failure (e.g. a service user lacking + // so without this a persistent failure (e.g. a service user lacking // permission_profiles read access) would re-hit the real API on every Active user // instead of once per sync — doubling that user's calls (the failed lookup, then the // GetUserDetails fallback) against the same hourly budget this fix exists to // protect. Only a genuinely persistent failure is cached this way, though — see - // getPermissionProfiles' doc for why a rate-limit or context error is deliberately - // left uncached. + // isCacheablePermissionProfilesError's doc for why a transient failure (a rate limit, + // a plain 5xx/network blip, a context error) is deliberately left uncached. permissionProfilesMu sync.Mutex permissionProfilesCached bool permissionProfiles []client.PermissionProfile permissionProfilesErr error } +// isCacheablePermissionProfilesError reports whether err is a persistent, +// account-configuration-shaped failure safe to cache on userBuilder for the rest of this +// sync — mirrors isOptInFeatureUnavailableError's PermissionDenied/Unauthenticated/ +// NotFound classification (this account's permission_profiles access isn't going to +// change mid-sync), deliberately narrower than that helper: no FailedPrecondition, which +// is specific to CLM discovery's response-shape check and not relevant here. +// +// Everything else is deliberately NOT cacheable — a reclassified rate-limit error +// (codes.Unavailable with a RateLimitDescription), an ordinary transient 5xx/network +// blip (codes.Unavailable with none), a context cancellation/deadline, or any other +// unclassified failure. Caching any of these would be actively harmful, not just a +// missed optimization: the SDK's per-action retry loop (pkg/sync/parallel_syncer.go, +// unlimited attempts) reuses this same userBuilder across every retry of this action, so +// a cached transient error would replay the same stale failure on every retry forever, +// without ever issuing a fresh request to notice the underlying condition — whether an +// hourly rate-limit window resetting or a 503 clearing — has cleared. A context error +// specifically only means whichever caller's context happened to win this call was +// already done, not that the account or its permissions are actually broken; caching it +// would incorrectly drop every other Active user in the sync back to the per-user +// GetUserDetails fallback for the rest of the run. +func isCacheablePermissionProfilesError(err error) bool { + switch status.Code(err) { + case codes.PermissionDenied, codes.Unauthenticated, codes.NotFound: + return true + default: + return false + } +} + // getPermissionProfiles returns the account's permission profiles, calling -// client.GetPermissionProfiles at most once for the lifetime of this userBuilder — see -// the memoization fields' doc on the struct above for why — with one deliberate -// exception: a reclassified rate-limit error, or a context-cancellation/deadline error, -// is never cached. Caching either would be actively harmful, not just a missed -// optimization: -// - A rate-limit error is exactly the case reclassifyHourlyRateLimitError's Unavailable -// reclassification exists to make retryable. The SDK's per-action retry loop -// (pkg/sync/parallel_syncer.go, unlimited attempts) reuses this same userBuilder -// across every retry of this action, so caching the error would replay the same -// stale rate-limit failure on every retry forever, without ever issuing a fresh -// request — the sync would spin at the retryer's ~60s interval and never notice the -// account's hourly window has actually reset, defeating the whole point of this fix. -// - A context error only means whichever caller's context happened to win this call -// was already done — not that the account or its permissions are actually broken. -// Caching it would incorrectly drop every other Active user in the sync back to the -// per-user GetUserDetails fallback for the rest of the run. +// client.GetPermissionProfiles at most once for the lifetime of this userBuilder unless +// the call fails with a non-cacheable (transient) error — see the memoization fields' +// doc on the struct above, and isCacheablePermissionProfilesError's doc, for why. func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { b.permissionProfilesMu.Lock() defer b.permissionProfilesMu.Unlock() @@ -75,7 +92,7 @@ func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.Permi } profiles, _, err := b.client.GetPermissionProfiles(ctx) - if err != nil && (isReclassifiedRateLimitError(err) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)) { + if err != nil && !isCacheablePermissionProfilesError(err) { return nil, err } diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index baf3ceae..0aa0fbe7 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -465,6 +465,129 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { } } +// TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure is the same regression +// as TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure, but for an ordinary +// transient 5xx with no RateLimitDescription at all (isReclassifiedRateLimitError +// returns false for it) — isCacheablePermissionProfilesError must still treat +// codes.Unavailable as non-cacheable regardless of why the error carries that code, or +// this class of failure would disable the fast path for the rest of the sync even after +// the endpoint recovers. +func TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure(t *testing.T) { + var permissionProfilesCalls int32 + + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case "/restapi/v2.1/accounts/acct-1/permission_profiles": + atomic.AddInt32(&permissionProfilesCalls, 1) + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) + case "/restapi/v2.1/accounts/acct-1/users/user-1": + _ = json.NewEncoder(w).Encode(client.UserDetail{UserID: "user-1", PermissionProfileID: "pp-1"}) + default: + http.NotFound(w, r) + } + }) + + mockServerURL, err := url.Parse(mockServer.URL) + if err != nil { + t.Fatalf("failed to parse mock server URL: %v", err) + } + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) + + b := newUserBuilder(c, false) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + for i := 0; i < 2; i++ { + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("call %d: Grants: %v", i, err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("call %d: expected the GetUserDetails fallback to resolve pp-1, got %+v", i, grants) + } + } + + if got := atomic.LoadInt32(&permissionProfilesCalls); got != 2 { + t.Errorf("expected GetPermissionProfiles to be called on every retry (2 calls), got %d — a plain transient 503 must not be memoized either", got) + } +} + +// TestUserBuilder_Grants_DoesNotMemoizeContextError is a regression test for the other +// carve-out case: a context cancellation/deadline only means whichever caller's context +// happened to reach getPermissionProfiles first was already done, not that the account +// or its permissions are broken. Caching it would incorrectly drop every later Active +// user back to the GetUserDetails fallback for the rest of the sync, exactly like the +// rate-limit and service-unavailable cases above. +func TestUserBuilder_Grants_DoesNotMemoizeContextError(t *testing.T) { + var permissionProfilesCalls int32 + + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case "/restapi/v2.1/accounts/acct-1/permission_profiles": + atomic.AddInt32(&permissionProfilesCalls, 1) + _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{ + PermissionProfiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + }) + default: + http.NotFound(w, r) + } + }) + + mockServerURL, err := url.Parse(mockServer.URL) + if err != nil { + t.Fatalf("failed to parse mock server URL: %v", err) + } + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) + + b := newUserBuilder(c, false) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + cancelledCtx, cancel := context.WithCancel(context.Background()) + cancel() // already cancelled before Grants ever runs + if _, _, err := b.Grants(cancelledCtx, resource, rs.SyncOpAttrs{}); err == nil { + t.Fatal("expected Grants to fail with an already-cancelled context, got nil") + } + + // A later Active user with a fresh, valid context must still resolve via the fast + // path — the cancelled attempt above must not have poisoned the builder's cache. + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants with a fresh context: %v", err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("expected the fast path to resolve pp-1, got %+v", grants) + } + if got := atomic.LoadInt32(&permissionProfilesCalls); got == 0 { + t.Error("expected the fresh-context call to actually reach GetPermissionProfiles, got 0 real calls") + } +} + // TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure covers the // other half of the fast path's error handling: a GetPermissionProfiles failure that // ISN'T the reclassified rate-limit error (e.g. a permissions/scope issue) must still From cf9f5fc13af94e5242e079833e2baf44cafd0228 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 18 Aug 2026 13:37:40 -0300 Subject: [PATCH 14/28] fix: bound consecutive transient getPermissionProfiles retries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Leaving every transient failure uncached fixed the stale-retry bug, but introduced a new worst case: a *sustained* outage (not just a blip) would cost every Active user two calls (the failed lookup plus the GetUserDetails fallback) for the whole sync — 2N vs. the N calls this fast path exists to avoid, worse than not having it at all. Adds permissionProfilesTransientFailureThreshold (3): after that many consecutive transient failures, the builder treats it as a sustained outage rather than a blip and caches it, falling back to the pre-fast-path 1-call-per-user cost for the remainder of the sync. Genuine blips still get retried fresh (existing tests asserting a second back-to-back call still pass, since they stay under the threshold). Added a regression test asserting exactly permissionProfilesTransientFailureThreshold real calls occur across more Grants() calls than that. Co-Authored-By: Claude Sonnet 5 --- pkg/connector/users.go | 63 ++++++++++++++++++++++++++----------- pkg/connector/users_test.go | 62 ++++++++++++++++++++++++++++++++++++ 2 files changed, 106 insertions(+), 19 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index dcd60551..3073cb07 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -31,25 +31,45 @@ type userBuilder struct { // grants pass is skipped too, since it is this builder's only output. skipPermissionProfileResourceType bool - // permissionProfilesMu/permissionProfilesCached/permissionProfiles/permissionProfilesErr - // memoize the one account-wide GetPermissionProfiles call tryFastPathGrant's - // name-lookup branch needs, across every Active user's Grants() call in this sync (a - // *userBuilder is constructed once per sync and Grants() runs concurrently across - // users, so this must be shared and safe for concurrent access — hence a mutex, not - // a plain bool). uhttp's GET cache only ever caches a 200 response, never an error, - // so without this a persistent failure (e.g. a service user lacking - // permission_profiles read access) would re-hit the real API on every Active user - // instead of once per sync — doubling that user's calls (the failed lookup, then the - // GetUserDetails fallback) against the same hourly budget this fix exists to - // protect. Only a genuinely persistent failure is cached this way, though — see - // isCacheablePermissionProfilesError's doc for why a transient failure (a rate limit, - // a plain 5xx/network blip, a context error) is deliberately left uncached. - permissionProfilesMu sync.Mutex - permissionProfilesCached bool - permissionProfiles []client.PermissionProfile - permissionProfilesErr error + // permissionProfilesMu/permissionProfilesCached/permissionProfiles/permissionProfilesErr/ + // permissionProfilesTransientFails memoize the one account-wide + // GetPermissionProfiles call tryFastPathGrant's name-lookup branch needs, across + // every Active user's Grants() call in this sync (a *userBuilder is constructed once + // per sync and Grants() runs concurrently across users, so this must be shared and + // safe for concurrent access — hence a mutex, not a plain bool). uhttp's GET cache + // only ever caches a 200 response, never an error, so without this a persistent + // failure (e.g. a service user lacking permission_profiles read access) would re-hit + // the real API on every Active user instead of once per sync — doubling that user's + // calls (the failed lookup, then the GetUserDetails fallback) against the same + // hourly budget this fix exists to protect. + // + // A genuinely persistent failure (see isCacheablePermissionProfilesError's doc) is + // cached immediately. A transient-shaped failure (a rate limit, a plain 5xx/network + // blip, a context error) is deliberately NOT cached on the first attempt — caching + // it would replay a stale error forever instead of ever re-checking whether the + // condition cleared — but permissionProfilesTransientFails bounds the resulting + // worst case: after permissionProfilesTransientFailureThreshold consecutive + // transient failures, the call is treated as a sustained outage rather than a blip + // and cached anyway, so the 2-calls-per-user cost (failed lookup + GetUserDetails + // fallback) only applies to the first few Active users in the sync, not all of + // them — the rest fall back at the same 1-call-per-user cost this fast path existed + // before. + permissionProfilesMu sync.Mutex + permissionProfilesCached bool + permissionProfiles []client.PermissionProfile + permissionProfilesErr error + permissionProfilesTransientFails int } +// permissionProfilesTransientFailureThreshold is how many consecutive transient +// getPermissionProfiles failures this builder tolerates (each retried against the real +// API) before treating the failure as a sustained outage and caching it anyway — see +// the memoization fields' doc on the struct above. Chosen to still give a genuine blip +// (a single dropped request, one 503 during a brief window) more than one chance to +// clear before falling back to the pre-fast-path 1-call-per-user cost for the rest of +// the sync. +const permissionProfilesTransientFailureThreshold = 3 + // isCacheablePermissionProfilesError reports whether err is a persistent, // account-configuration-shaped failure safe to cache on userBuilder for the rest of this // sync — mirrors isOptInFeatureUnavailableError's PermissionDenied/Unauthenticated/ @@ -82,7 +102,9 @@ func isCacheablePermissionProfilesError(err error) bool { // getPermissionProfiles returns the account's permission profiles, calling // client.GetPermissionProfiles at most once for the lifetime of this userBuilder unless // the call fails with a non-cacheable (transient) error — see the memoization fields' -// doc on the struct above, and isCacheablePermissionProfilesError's doc, for why. +// doc on the struct above, and isCacheablePermissionProfilesError's doc, for why — and +// even then, only up to permissionProfilesTransientFailureThreshold consecutive times +// before that transient failure is cached too, bounding the worst-case call cost. func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { b.permissionProfilesMu.Lock() defer b.permissionProfilesMu.Unlock() @@ -93,7 +115,10 @@ func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.Permi profiles, _, err := b.client.GetPermissionProfiles(ctx) if err != nil && !isCacheablePermissionProfilesError(err) { - return nil, err + b.permissionProfilesTransientFails++ + if b.permissionProfilesTransientFails < permissionProfilesTransientFailureThreshold { + return nil, err + } } b.permissionProfilesCached = true diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 0aa0fbe7..05f2b592 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -524,6 +524,68 @@ func TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure(t *testing.T } } +// TestUserBuilder_Grants_BoundsTransientFailureRetries is a regression test for the +// worst case of leaving transient failures uncached: without a cap, a *sustained* +// outage (not just a blip) would cost every Active user in the sync two calls (the +// failed lookup plus the GetUserDetails fallback) instead of the one call the fast path +// would otherwise avoid — worse than not having the fast path at all. After +// permissionProfilesTransientFailureThreshold consecutive transient failures, the +// builder must stop re-attempting the real endpoint and fall back at one call per user +// for the remainder of the sync, like the persistent-failure case. +func TestUserBuilder_Grants_BoundsTransientFailureRetries(t *testing.T) { + var permissionProfilesCalls int32 + + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch r.URL.Path { + case "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case "/restapi/v2.1/accounts/acct-1/permission_profiles": + atomic.AddInt32(&permissionProfilesCalls, 1) + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) + case "/restapi/v2.1/accounts/acct-1/users/user-1": + _ = json.NewEncoder(w).Encode(client.UserDetail{UserID: "user-1", PermissionProfileID: "pp-1"}) + default: + http.NotFound(w, r) + } + }) + + mockServerURL, err := url.Parse(mockServer.URL) + if err != nil { + t.Fatalf("failed to parse mock server URL: %v", err) + } + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) + + b := newUserBuilder(c, false) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + const totalUsers = permissionProfilesTransientFailureThreshold + 2 + for i := 0; i < totalUsers; i++ { + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("call %d: Grants: %v", i, err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("call %d: expected the GetUserDetails fallback to resolve pp-1, got %+v", i, grants) + } + } + + if got := atomic.LoadInt32(&permissionProfilesCalls); got != permissionProfilesTransientFailureThreshold { + t.Errorf("expected exactly %d real GetPermissionProfiles calls (retried up to the threshold, then cached), got %d across %d Grants calls", + permissionProfilesTransientFailureThreshold, got, totalUsers) + } +} + // TestUserBuilder_Grants_DoesNotMemoizeContextError is a regression test for the other // carve-out case: a context cancellation/deadline only means whichever caller's context // happened to reach getPermissionProfiles first was already done, not that the account From 9b273809c5f6234f4bf7ece7f4f5f5c24496671e Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 18 Aug 2026 13:48:19 -0300 Subject: [PATCH 15/28] fix: exempt rate-limit and context errors from the transient-failure threshold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The threshold added in the previous commit applied uniformly to every non-cacheable error, including the reclassified rate-limit error — so after permissionProfilesTransientFailureThreshold consecutive rate-limit hits, it got cached anyway, reintroducing the exact unrecoverable-sync regression already fixed twice earlier in this PR: the SDK's per-action retry loop would replay the cached codes.Unavailable forever, never re-checking whether the hourly window reset. A rate-limit error now always returns uncached regardless of the counter, no matter how many times it recurs. Also exempts context cancellation/deadline errors from incrementing the counter at all (not just from being cached) — per isCacheablePermissionProfilesError's own reasoning, an unlucky run of cancellations doesn't mean the endpoint is degraded, so it shouldn't accumulate toward disabling the fast path on an otherwise-healthy account. Extended TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure and TestUserBuilder_Grants_DoesNotMemoizeContextError to loop past the threshold, proving both exemptions hold no matter how many times they recur (the previous 2-call versions passed by coincidence, since 2 is below the threshold of 3). Co-Authored-By: Claude Sonnet 5 --- pkg/connector/users.go | 40 +++++++++++++++++++++++++++---------- pkg/connector/users_test.go | 29 +++++++++++++++++---------- 2 files changed, 49 insertions(+), 20 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 3073cb07..d1d40d2a 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -2,6 +2,7 @@ package connector import ( "context" + "errors" "fmt" "sync" @@ -44,16 +45,19 @@ type userBuilder struct { // hourly budget this fix exists to protect. // // A genuinely persistent failure (see isCacheablePermissionProfilesError's doc) is - // cached immediately. A transient-shaped failure (a rate limit, a plain 5xx/network - // blip, a context error) is deliberately NOT cached on the first attempt — caching - // it would replay a stale error forever instead of ever re-checking whether the - // condition cleared — but permissionProfilesTransientFails bounds the resulting + // cached immediately. A transient-shaped failure (a plain 5xx/network blip, or any + // other unclassified error) is deliberately NOT cached on the first attempt — + // caching it would replay a stale error forever instead of ever re-checking whether + // the condition cleared — but permissionProfilesTransientFails bounds the resulting // worst case: after permissionProfilesTransientFailureThreshold consecutive - // transient failures, the call is treated as a sustained outage rather than a blip - // and cached anyway, so the 2-calls-per-user cost (failed lookup + GetUserDetails - // fallback) only applies to the first few Active users in the sync, not all of - // them — the rest fall back at the same 1-call-per-user cost this fast path existed - // before. + // transient failures of THAT kind, the call is treated as a sustained outage rather + // than a blip and cached anyway, so the 2-calls-per-user cost (failed lookup + + // GetUserDetails fallback) only applies to the first few Active users in the sync, + // not all of them — the rest fall back at the same 1-call-per-user cost this fast + // path existed before. A reclassified rate-limit error and a context + // cancellation/deadline are exempt from this counter entirely — see + // getPermissionProfiles' doc for why counting either toward the threshold would be + // actively harmful, not just a missed optimization. permissionProfilesMu sync.Mutex permissionProfilesCached bool permissionProfiles []client.PermissionProfile @@ -104,7 +108,20 @@ func isCacheablePermissionProfilesError(err error) bool { // the call fails with a non-cacheable (transient) error — see the memoization fields' // doc on the struct above, and isCacheablePermissionProfilesError's doc, for why — and // even then, only up to permissionProfilesTransientFailureThreshold consecutive times -// before that transient failure is cached too, bounding the worst-case call cost. +// before that transient failure is cached too, bounding the worst-case call cost. Two +// exceptions never count toward that threshold and are never cached no matter how many +// times they recur: +// - A reclassified rate-limit error: unlike an ordinary transient blip, this failure +// has a known, bounded resolution (the hourly window resetting), so it's always +// worth a real retry — caching it would replay the same stale codes.Unavailable on +// every retry of the SDK's per-action retry loop (unlimited attempts, same builder +// reused) forever, never re-checking whether the window has actually reset. This is +// the exact regression the isCacheablePermissionProfilesError allowlist already +// exists to prevent; the threshold must not reintroduce it via a different path. +// - A context cancellation/deadline: only means whichever caller's context won this +// attempt was already done, not that the endpoint is actually degraded. An unlucky +// run of cancellations shouldn't accumulate toward disabling the fast path on an +// otherwise-healthy account. func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { b.permissionProfilesMu.Lock() defer b.permissionProfilesMu.Unlock() @@ -115,6 +132,9 @@ func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.Permi profiles, _, err := b.client.GetPermissionProfiles(ctx) if err != nil && !isCacheablePermissionProfilesError(err) { + if isReclassifiedRateLimitError(err) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil, err + } b.permissionProfilesTransientFails++ if b.permissionProfilesTransientFails < permissionProfilesTransientFailureThreshold { return nil, err diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 05f2b592..ff9c0d5c 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -410,8 +410,11 @@ func TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls(t *testing // on the builder — caching it would replay the same stale error on every retry of the // SDK's per-action retry loop (which reuses this same userBuilder), spinning forever at // the retryer's clamped interval instead of ever re-checking whether the account's -// hourly window has reset. Two Grants() calls against a rate-limited mock, on the same -// builder, must each issue a real GetPermissionProfiles call. +// hourly window has reset. Loops well past permissionProfilesTransientFailureThreshold: +// the rate-limit exemption must hold regardless of how many consecutive times it +// recurs, unlike an ordinary transient failure that IS eventually cached (see +// TestUserBuilder_Grants_BoundsTransientFailureRetries) — every call here must issue a +// real GetPermissionProfiles request. func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { var permissionProfilesCalls int32 @@ -450,7 +453,8 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") - for i := 0; i < 2; i++ { + const attempts = permissionProfilesTransientFailureThreshold + 2 + for i := 0; i < attempts; i++ { _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) if err == nil { t.Fatalf("call %d: expected Grants to propagate the rate-limit error, got nil", i) @@ -460,8 +464,8 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { } } - if got := atomic.LoadInt32(&permissionProfilesCalls); got != 2 { - t.Errorf("expected GetPermissionProfiles to be called on every retry (2 calls), got %d — the rate-limit error must not be memoized", got) + if got := atomic.LoadInt32(&permissionProfilesCalls); got != attempts { + t.Errorf("expected GetPermissionProfiles called on every one of %d retries, got %d — the rate-limit error must never be memoized, even past the threshold", attempts, got) } } @@ -630,14 +634,19 @@ func TestUserBuilder_Grants_DoesNotMemoizeContextError(t *testing.T) { b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") - cancelledCtx, cancel := context.WithCancel(context.Background()) - cancel() // already cancelled before Grants ever runs - if _, _, err := b.Grants(cancelledCtx, resource, rs.SyncOpAttrs{}); err == nil { - t.Fatal("expected Grants to fail with an already-cancelled context, got nil") + // Cancelled attempts must not count toward permissionProfilesTransientFailureThreshold + // either — loop past it to prove a run of unrelated cancellations can't accumulate + // into caching a failure on an otherwise-healthy account. + for i := 0; i < permissionProfilesTransientFailureThreshold+2; i++ { + cancelledCtx, cancel := context.WithCancel(context.Background()) + cancel() // already cancelled before Grants ever runs + if _, _, err := b.Grants(cancelledCtx, resource, rs.SyncOpAttrs{}); err == nil { + t.Fatalf("call %d: expected Grants to fail with an already-cancelled context, got nil", i) + } } // A later Active user with a fresh, valid context must still resolve via the fast - // path — the cancelled attempt above must not have poisoned the builder's cache. + // path — none of the cancelled attempts above must have poisoned the builder's cache. grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) if err != nil { t.Fatalf("Grants with a fresh context: %v", err) From dc5be5d8ca572888b96034f3b601ec0c1f9fc582 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 18 Aug 2026 14:20:06 -0300 Subject: [PATCH 16/28] refactor: dedupe grant construction and test mock-server scaffolding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - users.go: extracted newPermissionProfileGrant, replacing 3 identical inline permission_profile-grant constructions (Grants' fallback, tryFastPathGrant's direct-ID branch, and its name-resolved branch). - users_test.go: extracted newCountingPermissionProfilesClient, replacing 5 near-identical ~35-line mock-server setups (differing only in what the permission_profiles endpoint returns) across the memoization regression tests. No behavior change — same assertions, same fixtures, just without the repeated boilerplate. Co-Authored-By: Claude Sonnet 5 --- pkg/connector/users.go | 34 +++--- pkg/connector/users_test.go | 217 ++++++++++-------------------------- 2 files changed, 75 insertions(+), 176 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index d1d40d2a..829831af 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -252,18 +252,22 @@ func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.Sy return nil, nil, nil } - permissionProfileResource := &v2.Resource{ - Id: &v2.ResourceId{ - ResourceType: permissionProfilesResourceType.Id, - Resource: permissionProfileID, - }, - } - return []*v2.Grant{ - grant.NewGrant(permissionProfileResource, permissionProfileAssignedTag, userID), + newPermissionProfileGrant(permissionProfileID, userID), }, &rs.SyncOpResults{Annotations: annos}, nil } +// newPermissionProfileGrant builds the permission_profile grant for userID against +// permissionProfileID — the one shape Grants' fallback and both of tryFastPathGrant's +// resolution branches all construct. +func newPermissionProfileGrant(permissionProfileID string, userID *v2.ResourceId) *v2.Grant { + return grant.NewGrant( + &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: permissionProfileID}}, + permissionProfileAssignedTag, + userID, + ) +} + // tryFastPathGrant is Grants' fast path for an Active user, avoiding the per-user // GetUserDetails call that contributes to DocuSign's hourly rate limit. Two ways it can // resolve the grant without that call, both already captured on the resource's profile @@ -309,12 +313,7 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc // against a live account — see client.User.PermissionProfileID's doc), this skips // GetPermissionProfiles entirely: no API call, no name lookup, no cache dependency. if id, ok := rs.GetProfileStringValue(profile, profileFieldPermissionID); ok && id != "" { - newGrant := grant.NewGrant( - &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: id}}, - permissionProfileAssignedTag, - userID, - ) - return newGrant, nil, nil, true + return newPermissionProfileGrant(id, userID), nil, nil, true } name, ok := rs.GetProfileStringValue(profile, profileFieldPermission) @@ -336,12 +335,7 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc if matches != 1 { return nil, nil, nil, false } - newGrant := grant.NewGrant( - &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: id}}, - permissionProfileAssignedTag, - userID, - ) - return newGrant, nil, nil, true + return newPermissionProfileGrant(id, userID), nil, nil, true } // CreateAccountCapabilityDetails declares support for account provisioning without a password. diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index ff9c0d5c..ceb55793 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -405,18 +405,15 @@ func TestUserBuilder_Grants_PropagatesRateLimitInsteadOfDoublingCalls(t *testing } } -// TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure is a regression test: unlike a -// persistent non-rate-limit failure, a reclassified rate-limit error must NOT be cached -// on the builder — caching it would replay the same stale error on every retry of the -// SDK's per-action retry loop (which reuses this same userBuilder), spinning forever at -// the retryer's clamped interval instead of ever re-checking whether the account's -// hourly window has reset. Loops well past permissionProfilesTransientFailureThreshold: -// the rate-limit exemption must hold regardless of how many consecutive times it -// recurs, unlike an ordinary transient failure that IS eventually cached (see -// TestUserBuilder_Grants_BoundsTransientFailureRetries) — every call here must issue a -// real GetPermissionProfiles request. -func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { - var permissionProfilesCalls int32 +// newCountingPermissionProfilesClient wires a *client.Client whose permission_profiles +// endpoint invokes respond on every request (after incrementing the returned counter) +// and whose users/{id} endpoint always resolves to PermissionProfileID "pp-1" — shared +// setup for the tests below, each of which only differs in what the permission_profiles +// endpoint returns and asserts how many times it was actually called across multiple +// Grants() calls sharing one userBuilder. +func newCountingPermissionProfilesClient(t *testing.T, respond func(w http.ResponseWriter)) (*client.Client, *int32) { + t.Helper() + var calls int32 mockServer := httptest.NewServer(nil) t.Cleanup(mockServer.Close) @@ -431,13 +428,15 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { }, }) case "/restapi/v2.1/accounts/acct-1/permission_profiles": - atomic.AddInt32(&permissionProfilesCalls, 1) - w.WriteHeader(http.StatusBadRequest) - _ = json.NewEncoder(w).Encode(client.ErrorResponse{ - ErrorCode: "HOURLY_APIINVOCATION_LIMIT_EXCEEDED", - ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", - }) + atomic.AddInt32(&calls, 1) + respond(w) default: + const prefix = "/restapi/v2.1/accounts/acct-1/users/" + if len(r.URL.Path) > len(prefix) && r.URL.Path[:len(prefix)] == prefix { + userID := r.URL.Path[len(prefix):] + _ = json.NewEncoder(w).Encode(client.UserDetail{UserID: userID, PermissionProfileID: "pp-1"}) + return + } http.NotFound(w, r) } }) @@ -448,7 +447,27 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { } wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) - c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) + return client.NewClient(context.Background(), false, tokenSource, "", "", wrapper), &calls +} + +// TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure is a regression test: unlike a +// persistent non-rate-limit failure, a reclassified rate-limit error must NOT be cached +// on the builder — caching it would replay the same stale error on every retry of the +// SDK's per-action retry loop (which reuses this same userBuilder), spinning forever at +// the retryer's clamped interval instead of ever re-checking whether the account's +// hourly window has reset. Loops well past permissionProfilesTransientFailureThreshold: +// the rate-limit exemption must hold regardless of how many consecutive times it +// recurs, unlike an ordinary transient failure that IS eventually cached (see +// TestUserBuilder_Grants_BoundsTransientFailureRetries) — every call here must issue a +// real GetPermissionProfiles request. +func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ + ErrorCode: "HOURLY_APIINVOCATION_LIMIT_EXCEEDED", + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", + }) + }) b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") @@ -464,7 +483,7 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { } } - if got := atomic.LoadInt32(&permissionProfilesCalls); got != attempts { + if got := atomic.LoadInt32(permissionProfilesCalls); got != attempts { t.Errorf("expected GetPermissionProfiles called on every one of %d retries, got %d — the rate-limit error must never be memoized, even past the threshold", attempts, got) } } @@ -477,39 +496,11 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { // this class of failure would disable the fast path for the rest of the sync even after // the endpoint recovers. func TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure(t *testing.T) { - var permissionProfilesCalls int32 - - mockServer := httptest.NewServer(nil) - t.Cleanup(mockServer.Close) - mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/oauth/userinfo": - _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ - Sub: "service-account-user-id", - Accounts: []client.AccountInfo{ - {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, - }, - }) - case "/restapi/v2.1/accounts/acct-1/permission_profiles": - atomic.AddInt32(&permissionProfilesCalls, 1) - w.WriteHeader(http.StatusServiceUnavailable) - _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) - case "/restapi/v2.1/accounts/acct-1/users/user-1": - _ = json.NewEncoder(w).Encode(client.UserDetail{UserID: "user-1", PermissionProfileID: "pp-1"}) - default: - http.NotFound(w, r) - } + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) }) - mockServerURL, err := url.Parse(mockServer.URL) - if err != nil { - t.Fatalf("failed to parse mock server URL: %v", err) - } - wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) - tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) - c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) - b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") @@ -523,7 +514,7 @@ func TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure(t *testing.T } } - if got := atomic.LoadInt32(&permissionProfilesCalls); got != 2 { + if got := atomic.LoadInt32(permissionProfilesCalls); got != 2 { t.Errorf("expected GetPermissionProfiles to be called on every retry (2 calls), got %d — a plain transient 503 must not be memoized either", got) } } @@ -537,39 +528,11 @@ func TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure(t *testing.T // builder must stop re-attempting the real endpoint and fall back at one call per user // for the remainder of the sync, like the persistent-failure case. func TestUserBuilder_Grants_BoundsTransientFailureRetries(t *testing.T) { - var permissionProfilesCalls int32 - - mockServer := httptest.NewServer(nil) - t.Cleanup(mockServer.Close) - mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/oauth/userinfo": - _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ - Sub: "service-account-user-id", - Accounts: []client.AccountInfo{ - {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, - }, - }) - case "/restapi/v2.1/accounts/acct-1/permission_profiles": - atomic.AddInt32(&permissionProfilesCalls, 1) - w.WriteHeader(http.StatusServiceUnavailable) - _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) - case "/restapi/v2.1/accounts/acct-1/users/user-1": - _ = json.NewEncoder(w).Encode(client.UserDetail{UserID: "user-1", PermissionProfileID: "pp-1"}) - default: - http.NotFound(w, r) - } + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) }) - mockServerURL, err := url.Parse(mockServer.URL) - if err != nil { - t.Fatalf("failed to parse mock server URL: %v", err) - } - wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) - tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) - c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) - b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") @@ -584,7 +547,7 @@ func TestUserBuilder_Grants_BoundsTransientFailureRetries(t *testing.T) { } } - if got := atomic.LoadInt32(&permissionProfilesCalls); got != permissionProfilesTransientFailureThreshold { + if got := atomic.LoadInt32(permissionProfilesCalls); got != permissionProfilesTransientFailureThreshold { t.Errorf("expected exactly %d real GetPermissionProfiles calls (retried up to the threshold, then cached), got %d across %d Grants calls", permissionProfilesTransientFailureThreshold, got, totalUsers) } @@ -597,40 +560,14 @@ func TestUserBuilder_Grants_BoundsTransientFailureRetries(t *testing.T) { // user back to the GetUserDetails fallback for the rest of the sync, exactly like the // rate-limit and service-unavailable cases above. func TestUserBuilder_Grants_DoesNotMemoizeContextError(t *testing.T) { - var permissionProfilesCalls int32 - - mockServer := httptest.NewServer(nil) - t.Cleanup(mockServer.Close) - mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/oauth/userinfo": - _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ - Sub: "service-account-user-id", - Accounts: []client.AccountInfo{ - {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, - }, - }) - case "/restapi/v2.1/accounts/acct-1/permission_profiles": - atomic.AddInt32(&permissionProfilesCalls, 1) - _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{ - PermissionProfiles: []client.PermissionProfile{ - {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, - }, - }) - default: - http.NotFound(w, r) - } + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{ + PermissionProfiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + }) }) - mockServerURL, err := url.Parse(mockServer.URL) - if err != nil { - t.Fatalf("failed to parse mock server URL: %v", err) - } - wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) - tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) - c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) - b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") @@ -654,7 +591,7 @@ func TestUserBuilder_Grants_DoesNotMemoizeContextError(t *testing.T) { if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { t.Errorf("expected the fast path to resolve pp-1, got %+v", grants) } - if got := atomic.LoadInt32(&permissionProfilesCalls); got == 0 { + if got := atomic.LoadInt32(permissionProfilesCalls); got == 0 { t.Error("expected the fresh-context call to actually reach GetPermissionProfiles, got 0 real calls") } } @@ -688,46 +625,14 @@ func TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure(t * // exactly one real GetPermissionProfiles call, with both still resolving their grant via // the GetUserDetails fallback. func TestUserBuilder_Grants_MemoizesPermissionProfilesFailureAcrossUsers(t *testing.T) { - var permissionProfilesCalls int32 - - mockServer := httptest.NewServer(nil) - t.Cleanup(mockServer.Close) - mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - switch r.URL.Path { - case "/oauth/userinfo": - _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ - Sub: "service-account-user-id", - Accounts: []client.AccountInfo{ - {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, - }, - }) - case "/restapi/v2.1/accounts/acct-1/permission_profiles": - atomic.AddInt32(&permissionProfilesCalls, 1) - w.WriteHeader(http.StatusForbidden) - _ = json.NewEncoder(w).Encode(client.ErrorResponse{ - ErrorCode: "USER_LACKS_PERMISSIONS", - ErrorMessage: "The user does not have permission to access permission profiles.", - }) - default: - const prefix = "/restapi/v2.1/accounts/acct-1/users/" - if len(r.URL.Path) > len(prefix) && r.URL.Path[:len(prefix)] == prefix { - userID := r.URL.Path[len(prefix):] - _ = json.NewEncoder(w).Encode(client.UserDetail{UserID: userID, PermissionProfileID: "pp-1"}) - return - } - http.NotFound(w, r) - } + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusForbidden) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ + ErrorCode: "USER_LACKS_PERMISSIONS", + ErrorMessage: "The user does not have permission to access permission profiles.", + }) }) - mockServerURL, err := url.Parse(mockServer.URL) - if err != nil { - t.Fatalf("failed to parse mock server URL: %v", err) - } - wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) - tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) - c := client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) - b := newUserBuilder(c, false) for _, userID := range []string{"user-1", "user-2"} { resource := userResourceWithProfile(t, userID, userStatusActive, "DocuSign Admin") @@ -740,7 +645,7 @@ func TestUserBuilder_Grants_MemoizesPermissionProfilesFailureAcrossUsers(t *test } } - if got := atomic.LoadInt32(&permissionProfilesCalls); got != 1 { + if got := atomic.LoadInt32(permissionProfilesCalls); got != 1 { t.Errorf("expected exactly 1 GetPermissionProfiles call across both users, got %d", got) } } From 4041c8251a4279390f9eedbfb6cf38797f04e60b Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Fri, 21 Aug 2026 14:35:49 -0300 Subject: [PATCH 17/28] fix: also detect hourly rate limit via docs-quoted error message Match the published account hourly-limit message in addition to the live-confirmed errorCode, and correct comments about X-RateLimit headers. Co-authored-by: Cursor --- pkg/client/helper.go | 70 +++++++++++++++++++++++++-------------- pkg/client/helper_test.go | 21 ++++++++++++ 2 files changed, 66 insertions(+), 25 deletions(-) diff --git a/pkg/client/helper.go b/pkg/client/helper.go index ca665d67..ae389726 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/url" + "strings" "time" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -20,19 +21,28 @@ import ( const DefaultPageSize = 100 // docusignHourlyRateLimitErrorCode is the eSignature API's JSON error-body errorCode for -// "the account's hourly API-call budget is exhausted" — confirmed against a real account. -// DocuSign returns this as HTTP 400 today and is mid-migration to 429 (DocuSign's own -// guidance is to key off errorCode, not HTTP status, for exactly this reason), so -// detection below checks the body field independent of resp.StatusCode. +// the account's hourly API-call budget being exhausted — confirmed against a real account. +// The published eSignature "rules and resource limits" / error-codes pages quote the +// human message (see docusignHourlyRateLimitErrorMessage) and related envelope-scoped +// codes (e.g. Hourly_APIInvocation_Envelope_Limit_Exceeded), but do not list this exact +// account-level errorCode string; detection matches either. DocuSign returns this as +// HTTP 400 today and is mid-migration to 429 (DocuSign's own guidance is to key off +// errorCode / the error body, not HTTP status), so detection below ignores StatusCode. const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" +// docusignHourlyRateLimitErrorMessage is the account hourly-limit error text quoted by +// DocuSign's eSignature API rules-and-resource-limits docs ("If you exceed the API rate +// limit, you will receive the error: …"). Matched as a substring of ErrorResponse.message +// so a future errorCode rename still classifies correctly when the published message stays. +const docusignHourlyRateLimitErrorMessage = "The maximum number of hourly API invocations has been exceeded" + // docusignRateLimitDefaultResetWindow is the fixed ResetAt this connector puts on the -// RateLimitDescription for docusignHourlyRateLimitErrorCode — applied unconditionally, +// RateLimitDescription for the account hourly-limit error — applied unconditionally, // not just as a fallback (see reclassifyHourlyRateLimitError's doc for why response -// headers are deliberately never consulted for this error). The limit this error names -// is hourly, so an hour is the semantically correct value to report — but it is not -// what the SDK's retry loop actually waits: pkg/sync/parallel_syncer.go constructs its -// Retryer with MaxDelay: 0, which retry.NewRetryer normalizes to a 60-second cap, and +// headers are not used for ResetAt here). The limit this error names is hourly, so an +// hour is the semantically correct value to report — but it is not what the SDK's retry +// loop actually waits: pkg/sync/parallel_syncer.go constructs its Retryer with +// MaxDelay: 0, which retry.NewRetryer normalizes to a 60-second cap, and // retry.Retryer.ShouldWaitAndRetry computes a wait from this ResetAt only to then clamp // it down to that same 60 seconds (`if wait > maxDelay { wait = maxDelay }`). With // MaxAttempts: 0 (unlimited), the net effect is a 60-second retry with no attempt limit @@ -41,30 +51,30 @@ const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" // package, not something this connector can change from here. const docusignRateLimitDefaultResetWindow = time.Hour -// reclassifyHourlyRateLimitError recognizes docusignHourlyRateLimitErrorCode in errTarget (the -// same *ErrorResponse instance uhttp.WithErrorResponse already unmarshaled the error body -// into before returning origErr — no re-parsing needed) and, if matched, returns a -// codes.Unavailable error carrying a RateLimitDescription. This matters because +// reclassifyHourlyRateLimitError recognizes the account hourly API-invocation limit in +// errTarget (the same *ErrorResponse instance uhttp.WithErrorResponse already unmarshaled +// the error body into before returning origErr — no re-parsing needed) and, if matched, +// returns a codes.Unavailable error carrying a RateLimitDescription. This matters because // uhttp.GrpcCodeFromHTTPStatus maps this error's current HTTP 400 to codes.InvalidArgument, // which the SDK's sync-retry loop (pkg/sync's Retryer, wired to SyncResourcesOp/ // SyncGrantsOp) does not retry — it only waits and retries on Unavailable/DeadlineExceeded, // so an otherwise-recoverable rate limit was surfacing as a fatal, non-resumable sync // failure. Returns nil (unchanged behavior) when errTarget isn't this specific eSignature -// error shape, or the errorCode doesn't match — including every ClmErrorResponse-based CLM -// call, which is a distinct error envelope this func never matches. +// error shape, or neither the live-confirmed errorCode nor the docs-quoted message match — +// including every ClmErrorResponse-based CLM call, which is a distinct error envelope +// this func never matches. // -// Deliberately does NOT read ratelimit.ExtractRateLimitData's header-derived -// Limit/Remaining/ResetAt for this specific error: DocuSign's docs describe no dedicated -// headers for this hourly/daily-scoped limit (detection has to go through the error body -// at all), so any generic X-RateLimit-*/Ratelimit-* headers present on this response most -// plausibly describe an unrelated shorter-window limit (e.g. a burst counter), not the -// hourly one that actually produced this error. Trusting them anyway risks the SDK's -// Retryer (vendor pkg/retry/retry.go) computing a short wait off a nonzero Remaining from -// the wrong bucket and hammering an account that's still over its hourly budget. Always -// uses the fixed hourly default window instead — safe by construction, if coarser. +// Deliberately does NOT use ratelimit.ExtractRateLimitData's header-derived +// Limit/Remaining/ResetAt for ResetAt on this error. The eSignature rules-and-limits +// docs do map X-RateLimit-* to the account hourly budget and X-BurstLimit-* to the +// separate 30-second burst window — but on an already-over-limit response Remaining is +// uninformative, and trusting ResetAt/Remaining without knowing which limiter produced +// the body risks the SDK's Retryer (vendor pkg/retry/retry.go) computing a short wait +// off the burst window and hammering an account that's still over its hourly budget. +// Always uses the fixed hourly default window instead — safe by construction, if coarser. func reclassifyHourlyRateLimitError(errTarget uhttp.ErrorResponse, origErr error) error { er, ok := errTarget.(*ErrorResponse) - if !ok || er.ErrorCode != docusignHourlyRateLimitErrorCode { + if !ok || !isHourlyAPIInvocationLimitError(er) { return nil } @@ -83,6 +93,16 @@ func reclassifyHourlyRateLimitError(errTarget uhttp.ErrorResponse, origErr error return withDetails.Err() } +// isHourlyAPIInvocationLimitError reports whether er is DocuSign's account-level hourly +// API-invocation budget error — matching the live-confirmed errorCode and/or the message +// text quoted in the eSignature rules-and-resource-limits docs. +func isHourlyAPIInvocationLimitError(er *ErrorResponse) bool { + if er.ErrorCode == docusignHourlyRateLimitErrorCode { + return true + } + return strings.Contains(er.ErrorMessage, docusignHourlyRateLimitErrorMessage) +} + // BuildURL combines the base API URL with a formatted endpoint path. func buildURL(base, path string, params ...any) (*url.URL, error) { baseURL, err := url.Parse(base) diff --git a/pkg/client/helper_test.go b/pkg/client/helper_test.go index 5887384d..5dae1b1c 100644 --- a/pkg/client/helper_test.go +++ b/pkg/client/helper_test.go @@ -76,6 +76,27 @@ func TestReclassifyHourlyRateLimitError(t *testing.T) { } }) + t.Run("matches on the docs-quoted message when errorCode differs", func(t *testing.T) { + // Published rules-and-limits docs quote the message text for the account hourly + // limit but do not list HOURLY_APIINVOCATION_LIMIT_EXCEEDED; accept either signal. + errTarget := &ErrorResponse{ + ErrorCode: "SOME_FUTURE_HOURLY_CODE", + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", + } + + got := reclassifyHourlyRateLimitError(errTarget, origErr) + if got == nil { + t.Fatal("expected a rate-limit error from the docs-quoted message, got nil") + } + st, ok := status.FromError(got) + if !ok { + t.Fatalf("expected a gRPC status error, got %v", got) + } + if st.Code() != codes.Unavailable { + t.Errorf("expected codes.Unavailable, got %v", st.Code()) + } + }) + t.Run("does not match CLM's distinct error envelope", func(t *testing.T) { // ClmErrorResponse is a different type from *ErrorResponse even if some CLM error // happened to carry the same string in an analogous field — the type assertion From 5733e42c9f28150561540f8313b01105f3b22603 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 25 Aug 2026 17:36:11 -0300 Subject: [PATCH 18/28] fix: key the permission-profiles memoization on SyncID, not process lifetime MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit userBuilder is registered once via ResourceSyncers and reused for the connector process's lifetime, not reconstructed per sync — in service/ hosted mode that's many syncs sharing one builder instance. The cache only checked "have I fetched this at all", so the first sync's snapshot (or its cached failure) would silently serve every later sync forever. Threads SyncOpAttrs.SyncID through Grants() -> tryFastPathGrant -> getPermissionProfiles and resets the cache (and the transient-failure counter) whenever the SyncID changes. --- pkg/connector/users.go | 66 +++++++++++++++++++++++-------------- pkg/connector/users_test.go | 37 +++++++++++++++++++++ 2 files changed, 79 insertions(+), 24 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 829831af..f607febb 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -32,17 +32,24 @@ type userBuilder struct { // grants pass is skipped too, since it is this builder's only output. skipPermissionProfileResourceType bool - // permissionProfilesMu/permissionProfilesCached/permissionProfiles/permissionProfilesErr/ - // permissionProfilesTransientFails memoize the one account-wide - // GetPermissionProfiles call tryFastPathGrant's name-lookup branch needs, across - // every Active user's Grants() call in this sync (a *userBuilder is constructed once - // per sync and Grants() runs concurrently across users, so this must be shared and - // safe for concurrent access — hence a mutex, not a plain bool). uhttp's GET cache - // only ever caches a 200 response, never an error, so without this a persistent - // failure (e.g. a service user lacking permission_profiles read access) would re-hit - // the real API on every Active user instead of once per sync — doubling that user's - // calls (the failed lookup, then the GetUserDetails fallback) against the same - // hourly budget this fix exists to protect. + // permissionProfilesMu/permissionProfilesSyncID/permissionProfilesCached/ + // permissionProfiles/permissionProfilesErr/permissionProfilesTransientFails memoize + // the one account-wide GetPermissionProfiles call tryFastPathGrant's name-lookup + // branch needs, across every Active user's Grants() call within a single sync. This + // builder is registered once via ResourceSyncers and reused for the lifetime of the + // connector process (baton-sdk's connectorbuilder.NewConnector stores the returned + // syncers once; see vendor/.../pkg/connectorbuilder/connectorbuilder.go) — in + // service/hosted mode that's many syncs, not one — so the cache is keyed on + // permissionProfilesSyncID (from SyncOpAttrs.SyncID, threaded through from Grants()) + // rather than trusted for the process's whole lifetime: a mismatch means a new sync + // has started and the memo is stale, resetting both the cached result and the + // transient-failure counter below. Grants() runs concurrently across users within + // one sync, so this must stay safe for concurrent access — hence a mutex, not a + // plain bool. uhttp's GET cache only ever caches a 200 response, never an error, so + // without this a persistent failure (e.g. a service user lacking permission_profiles + // read access) would re-hit the real API on every Active user instead of once per + // sync — doubling that user's calls (the failed lookup, then the GetUserDetails + // fallback) against the same hourly budget this fix exists to protect. // // A genuinely persistent failure (see isCacheablePermissionProfilesError's doc) is // cached immediately. A transient-shaped failure (a plain 5xx/network blip, or any @@ -59,6 +66,7 @@ type userBuilder struct { // getPermissionProfiles' doc for why counting either toward the threshold would be // actively harmful, not just a missed optimization. permissionProfilesMu sync.Mutex + permissionProfilesSyncID string permissionProfilesCached bool permissionProfiles []client.PermissionProfile permissionProfilesErr error @@ -104,13 +112,14 @@ func isCacheablePermissionProfilesError(err error) bool { } // getPermissionProfiles returns the account's permission profiles, calling -// client.GetPermissionProfiles at most once for the lifetime of this userBuilder unless -// the call fails with a non-cacheable (transient) error — see the memoization fields' -// doc on the struct above, and isCacheablePermissionProfilesError's doc, for why — and -// even then, only up to permissionProfilesTransientFailureThreshold consecutive times -// before that transient failure is cached too, bounding the worst-case call cost. Two -// exceptions never count toward that threshold and are never cached no matter how many -// times they recur: +// client.GetPermissionProfiles at most once per sync (keyed by syncID — see the +// memoization fields' doc on the struct above for why this builder can't just trust the +// cache for its whole process lifetime) unless the call fails with a non-cacheable +// (transient) error — see isCacheablePermissionProfilesError's doc — and even then, only +// up to permissionProfilesTransientFailureThreshold consecutive times before that +// transient failure is cached too, bounding the worst-case call cost. Two exceptions +// never count toward that threshold and are never cached no matter how many times they +// recur: // - A reclassified rate-limit error: unlike an ordinary transient blip, this failure // has a known, bounded resolution (the hourly window resetting), so it's always // worth a real retry — caching it would replay the same stale codes.Unavailable on @@ -122,13 +131,22 @@ func isCacheablePermissionProfilesError(err error) bool { // attempt was already done, not that the endpoint is actually degraded. An unlucky // run of cancellations shouldn't accumulate toward disabling the fast path on an // otherwise-healthy account. -func (b *userBuilder) getPermissionProfiles(ctx context.Context) ([]client.PermissionProfile, error) { +func (b *userBuilder) getPermissionProfiles(ctx context.Context, syncID string) ([]client.PermissionProfile, error) { b.permissionProfilesMu.Lock() defer b.permissionProfilesMu.Unlock() - if b.permissionProfilesCached { + if b.permissionProfilesCached && b.permissionProfilesSyncID == syncID { return b.permissionProfiles, b.permissionProfilesErr } + if b.permissionProfilesSyncID != syncID { + // A new sync started (or this is the first call ever): the previous sync's + // cached result/error and transient-failure count no longer apply. Reset both + // so this sync gets its own full permissionProfilesTransientFailureThreshold + // chances rather than inheriting a count left over from a prior sync's outage. + b.permissionProfilesSyncID = syncID + b.permissionProfilesCached = false + b.permissionProfilesTransientFails = 0 + } profiles, _, err := b.client.GetPermissionProfiles(ctx) if err != nil && !isCacheablePermissionProfilesError(err) { @@ -226,10 +244,10 @@ func (b *userBuilder) Entitlements(_ context.Context, _ *v2.Resource, _ rs.SyncO // rate limit) and falls back to the always-correct per-user GetUserDetails path, // unchanged from before that fast path existed, whenever it declines to handle the // request (see tryFastPathGrant's own doc). -func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { +func (b *userBuilder) Grants(ctx context.Context, resource *v2.Resource, attrs rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { userID := resource.Id - if newGrant, annos, err, handled := b.tryFastPathGrant(ctx, resource, userID); handled { + if newGrant, annos, err, handled := b.tryFastPathGrant(ctx, resource, userID, attrs.SyncID); handled { if err != nil { return nil, nil, err } @@ -301,7 +319,7 @@ func newPermissionProfileGrant(permissionProfileID string, userID *v2.ResourceId // user would feed the SDK's self-throttling rate limiter a frozen, increasingly stale // signal instead of the fresh per-request data GetUserDetails supplied before this fast // path existed. -func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resource, userID *v2.ResourceId) (*v2.Grant, annotations.Annotations, error, bool) { +func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resource, userID *v2.ResourceId, syncID string) (*v2.Grant, annotations.Annotations, error, bool) { profile := rs.GetProfile(resource) userStatus, ok := rs.GetProfileStringValue(profile, profileFieldStatus) @@ -321,7 +339,7 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc return nil, nil, nil, false } - profiles, err := b.getPermissionProfiles(ctx) + profiles, err := b.getPermissionProfiles(ctx, syncID) if err != nil { if isReclassifiedRateLimitError(err) { return nil, nil, err, true diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index ceb55793..bb7fe7ea 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -650,6 +650,43 @@ func TestUserBuilder_Grants_MemoizesPermissionProfilesFailureAcrossUsers(t *test } } +// TestUserBuilder_Grants_PermissionProfilesCacheIsPerSync is a regression test: this +// builder is registered once via ResourceSyncers and reused for the connector process's +// lifetime, not reconstructed per sync (see the memoization fields' doc on userBuilder), +// so a cache that only ever checked "have I fetched permission profiles at all" would +// keep serving the first sync's snapshot (or its cached failure) to every later sync on +// a long-lived connector process — never noticing a profile renamed/added/removed, or a +// prior persistent failure's underlying cause having been fixed. Two Grants() calls with +// different SyncOpAttrs.SyncID values (same builder, same user) must each issue their +// own GetPermissionProfiles call. +func TestUserBuilder_Grants_PermissionProfilesCacheIsPerSync(t *testing.T) { + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{ + PermissionProfiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + }) + }) + + b := newUserBuilder(c, false) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + for _, syncID := range []string{"sync-1", "sync-2"} { + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{SyncID: syncID}) + if err != nil { + t.Fatalf("Grants(syncID=%s): %v", syncID, err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("Grants(syncID=%s): expected the fast path to resolve pp-1, got %+v", syncID, grants) + } + } + + if got := atomic.LoadInt32(permissionProfilesCalls); got != 2 { + t.Errorf("expected 1 GetPermissionProfiles call per distinct SyncID (2 total), got %d — "+ + "the cache is leaking across syncs", got) + } +} + // TestUserBuilder_Grants_FallsBackOnServiceUnavailable: codes.Unavailable is broader // than "already rate-limited" — uhttp also maps a plain HTTP 503 to it. A 503 from // GetPermissionProfiles (no RateLimitDescription attached, unlike the reclassified From 5ca06eab337f90eb25bf92de678681582e184cfa Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 25 Aug 2026 17:36:11 -0300 Subject: [PATCH 19/28] fix: bypass the shared HTTP GET cache for GetPermissionProfiles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaced by testing the SyncID-keyed memoization fix: even after that fix correctly decided a new sync needed a fresh call, uhttp's GET cache was still serving the previous sync's cached HTTP response underneath it. userBuilder already calls this at most once per sync, so a cached response here can only ever be a stale snapshot from a prior sync on the same long-lived connector process — never a real saved call. --- pkg/client/client.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index ed033d7d..8565b53e 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -641,6 +641,11 @@ func (c *Client) GetUserByEmail(ctx context.Context, userEmail string) (*User, a // Typically, DocuSign accounts have a limited number of permission profiles (< 50), so this is acceptable. // // Returns: all permission profiles, annotations, error. +// GetPermissionProfiles bypasses the shared HTTP GET cache: userBuilder calls this at +// most once per sync (see its memoization fields' doc in pkg/connector/users.go), so by +// the time this request fires, that caller has already decided a fresh call is needed — +// a cached response here could only ever serve a stale snapshot left over from a +// previous sync on the same long-lived connector process, never save a real call. func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile, annotations.Annotations, error) { if err := c.ensureInitialized(ctx); err != nil { return nil, nil, err @@ -655,7 +660,7 @@ func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile permissionProfilesURL = baseURL.ResolveReference(permissionProfilesURL) - _, annos, err := c.doRequest(ctx, http.MethodGet, permissionProfilesURL, nil, &permissionProfilesResponse) + _, annos, err := c.doRequest(ctx, http.MethodGet, permissionProfilesURL, nil, &permissionProfilesResponse, uhttp.WithNoCache()) if err != nil { return nil, nil, err } @@ -791,6 +796,7 @@ func (c *Client) doRequest( url *url.URL, body any, response any, + extraOpts ...uhttp.RequestOption, ) (http.Header, annotations.Annotations, error) { token, err := c.tokenSource.Token() if err != nil { @@ -806,6 +812,7 @@ func (c *Client) doRequest( if body != nil { requestOptions = append(requestOptions, uhttp.WithJSONBody(body)) } + requestOptions = append(requestOptions, extraOpts...) request, err := c.wrapper.NewRequest(ctx, method, url, requestOptions...) if err != nil { From 4c7908207e06da57487160f0356d8de8c7be0713 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 25 Aug 2026 17:44:51 -0300 Subject: [PATCH 20/28] fix: force sqlite storage engine in CI, get-baton's baton CLI can't read Pebble MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same root cause already fixed on PR #63/#64: GitHub Actions tests this PR's merge-preview against main, and main is already on baton-sdk v0.25.0 (Pebble-by-default) even though this branch's own go.mod is still on v0.24.2 — the merge cleanly picks up main's newer line since this PR never touches it. The downloaded baton CLI (v0.4.5, built against SDK v0.8.24) can't read a Pebble-format file. --- .github/workflows/ci.yaml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index bc83b7d6..a62b166d 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -5,6 +5,19 @@ on: push: branches: - main +env: + # Forces the legacy v1/SQLite c1z format instead of baton-sdk v0.25.0's new default + # (Pebble/v3). GitHub Actions tests this PR's merge-preview (this PR's head merged + # onto main), and main is already on baton-sdk v0.25.0 — since this PR doesn't touch + # go.mod's baton-sdk line, that merge picks up v0.25.0 even though this branch's own + # tip is still on v0.24.2. The `baton` CLI these jobs download + # (ConductorOne/github-workflows' get-baton action, currently v0.4.5) is built against + # baton-sdk v0.8.24 — long before Pebble existed — and fails every read of a + # Pebble-format file with a bare "c1z: invalid file", no matter how the file was + # produced. Safe to drop once a `baton` CLI release built against a Pebble-aware + # baton-sdk ships and get-baton picks it up, or once this branch merges and its own + # go.mod matches main again. + BATON_STORAGE_ENGINE: sqlite jobs: test-groups: runs-on: ubuntu-latest From afb82e45bf54d137468c7fc3c6154ddb830c4502 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 25 Aug 2026 17:47:45 -0300 Subject: [PATCH 21/28] fix: split GetPermissionProfiles caching by caller, fix stale annotation-forwarding comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetPermissionProfiles unconditionally bypassing the shared HTTP GET cache saved userBuilder from cross-sync staleness, but List and Revoke share this same call and don't have that risk — sharing uhttp's cache between them when both fire in one sync used to cost 1 real request, unconditional WithNoCache() silently made it 2. Added GetPermissionProfilesFresh as the dedicated no-cache variant for userBuilder's memoization; GetPermissionProfiles (List/Revoke) stays cacheable. Also corrected tryFastPathGrant's doc comment: it still attributed "exactly one real call per sync" to uhttp's GET cache, which no longer applies to the fresh variant — getPermissionProfiles' own memoization is what guarantees that now. --- pkg/client/client.go | 32 ++++++++++++++---- pkg/client/client_test.go | 71 +++++++++++++++++++++++++++++++++++++++ pkg/connector/users.go | 14 ++++---- 3 files changed, 103 insertions(+), 14 deletions(-) diff --git a/pkg/client/client.go b/pkg/client/client.go index 8565b53e..b618bb4c 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -635,18 +635,32 @@ func (c *Client) GetUserByEmail(ctx context.Context, userEmail string) (*User, a return &user, annos, nil } -// GetPermissionProfiles fetches all permission profiles from the DocuSign account. +// GetPermissionProfiles fetches all permission profiles from the DocuSign account. The +// response may be served from the shared HTTP GET cache — fine for List/Revoke, neither +// of which memoizes across syncs the way userBuilder does (see GetPermissionProfilesFresh). // // Pagination: This endpoint does NOT support pagination. It returns all permission profiles in a single request. // Typically, DocuSign accounts have a limited number of permission profiles (< 50), so this is acceptable. // // Returns: all permission profiles, annotations, error. -// GetPermissionProfiles bypasses the shared HTTP GET cache: userBuilder calls this at -// most once per sync (see its memoization fields' doc in pkg/connector/users.go), so by -// the time this request fires, that caller has already decided a fresh call is needed — -// a cached response here could only ever serve a stale snapshot left over from a -// previous sync on the same long-lived connector process, never save a real call. func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile, annotations.Annotations, error) { + return c.getPermissionProfiles(ctx, false) +} + +// GetPermissionProfilesFresh is identical to GetPermissionProfiles but bypasses the +// shared HTTP GET cache. userBuilder calls this at most once per sync (see its +// memoization fields' doc in pkg/connector/users.go), so by the time this request +// fires, that caller has already decided a fresh call is needed — a cached response +// here could only ever serve a stale snapshot left over from a previous sync on the +// same long-lived connector process, never save a real call. List and Revoke don't +// share that risk (neither memoizes this call across syncs), and sharing uhttp's cache +// with them when both fire in the same sync saves a real network call — so only this +// path opts out of it, not GetPermissionProfiles itself. +func (c *Client) GetPermissionProfilesFresh(ctx context.Context) ([]PermissionProfile, annotations.Annotations, error) { + return c.getPermissionProfiles(ctx, true) +} + +func (c *Client) getPermissionProfiles(ctx context.Context, noCache bool) ([]PermissionProfile, annotations.Annotations, error) { if err := c.ensureInitialized(ctx); err != nil { return nil, nil, err } @@ -660,7 +674,11 @@ func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile permissionProfilesURL = baseURL.ResolveReference(permissionProfilesURL) - _, annos, err := c.doRequest(ctx, http.MethodGet, permissionProfilesURL, nil, &permissionProfilesResponse, uhttp.WithNoCache()) + var extraOpts []uhttp.RequestOption + if noCache { + extraOpts = append(extraOpts, uhttp.WithNoCache()) + } + _, annos, err := c.doRequest(ctx, http.MethodGet, permissionProfilesURL, nil, &permissionProfilesResponse, extraOpts...) if err != nil { return nil, nil, err } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 9ef328e4..d3e3ce60 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -273,3 +273,74 @@ func TestMultiAccountResourceIsolation(t *testing.T) { } } } + +// newCountingPermissionProfilesClient wires a Client to a mock server that counts real +// GET /permission_profiles hits, to distinguish a real network call from one served by +// uhttp's shared GET cache. +func newCountingPermissionProfilesClient(t *testing.T) (*Client, *int) { + t.Helper() + calls := 0 + + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + switch { + case r.URL.Path == "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case strings.HasSuffix(r.URL.Path, "/permission_profiles"): + calls++ + _ = json.NewEncoder(w).Encode(PermissionProfilesResponse{ + PermissionProfiles: []PermissionProfile{{PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}}, + }) + default: + http.NotFound(w, r) + } + }) + + mockServerURL, _ := url.Parse(mockServer.URL) + transport := &rewriteTransport{target: mockServerURL, base: http.DefaultTransport} + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: transport}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + return NewClient(context.Background(), false, tokenSource, "", "", wrapper), &calls +} + +// TestGetPermissionProfiles_CachingSplitByCaller is a regression test for a review +// finding: GetPermissionProfiles used to unconditionally bypass uhttp's GET cache for +// every caller, but List and Revoke (unlike userBuilder) don't memoize this call across +// syncs — sharing the cache between them when both fire in the same sync saves a real +// network call, and unconditional WithNoCache() silently turned that 1 call into 2. +// GetPermissionProfiles must still be cacheable; only GetPermissionProfilesFresh (the +// dedicated variant for userBuilder's cross-sync-safe memoization) bypasses the cache. +func TestGetPermissionProfiles_CachingSplitByCaller(t *testing.T) { + ctx := context.Background() + + t.Run("GetPermissionProfiles is cacheable: two calls, one real request", func(t *testing.T) { + c, calls := newCountingPermissionProfilesClient(t) + for i := 0; i < 2; i++ { + if _, _, err := c.GetPermissionProfiles(ctx); err != nil { + t.Fatalf("call %d: %v", i, err) + } + } + if *calls != 1 { + t.Errorf("expected 1 real request across 2 GetPermissionProfiles calls (cache should serve the second), got %d", *calls) + } + }) + + t.Run("GetPermissionProfilesFresh always issues a real request", func(t *testing.T) { + c, calls := newCountingPermissionProfilesClient(t) + for i := 0; i < 2; i++ { + if _, _, err := c.GetPermissionProfilesFresh(ctx); err != nil { + t.Fatalf("call %d: %v", i, err) + } + } + if *calls != 2 { + t.Errorf("expected 2 real requests across 2 GetPermissionProfilesFresh calls (no caching), got %d", *calls) + } + }) +} diff --git a/pkg/connector/users.go b/pkg/connector/users.go index f607febb..736185e3 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -148,7 +148,7 @@ func (b *userBuilder) getPermissionProfiles(ctx context.Context, syncID string) b.permissionProfilesTransientFails = 0 } - profiles, _, err := b.client.GetPermissionProfiles(ctx) + profiles, _, err := b.client.GetPermissionProfilesFresh(ctx) if err != nil && !isCacheablePermissionProfilesError(err) { if isReclassifiedRateLimitError(err) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return nil, err @@ -313,12 +313,12 @@ func newPermissionProfileGrant(permissionProfileID string, userID *v2.ResourceId // over budget — exactly the amplification this fix exists to reduce. handled=true with a // nil err means the grant was resolved. // -// Never forwards GetPermissionProfiles' annotations: after the first real call in a -// sync, repeat calls are served from uhttp's GET cache, which replays that first -// response's rate-limit snapshot verbatim — forwarding it on every subsequent active -// user would feed the SDK's self-throttling rate limiter a frozen, increasingly stale -// signal instead of the fresh per-request data GetUserDetails supplied before this fast -// path existed. +// Never forwards GetPermissionProfilesFresh's annotations: getPermissionProfiles' +// memoization (see its doc) already limits this builder to exactly one real call per +// sync, so its rate-limit snapshot is one sample from one point in the sync, not +// representative of per-user request pacing — forwarding it on every active user would +// feed the SDK's self-throttling rate limiter that single frozen signal instead of the +// fresh per-request data GetUserDetails supplied before this fast path existed. func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resource, userID *v2.ResourceId, syncID string) (*v2.Grant, annotations.Annotations, error, bool) { profile := rs.GetProfile(resource) From 84cd0ca2f598c8cb226408f4d0667de6a13aab19 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Tue, 25 Aug 2026 17:58:37 -0300 Subject: [PATCH 22/28] fix: add sleep to sync-test jobs, DocuSign doesn't apply writes instantly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit test-permission-profiles failed: the final grant succeeded, but the sync run immediately after still read the pre-grant state, so the verification query found no grants. sync-test@v3 already has a sleep input built for exactly this (DocuSign write-propagation delay) — no job here was using it. Added to all three jobs; only test-permission-profiles has actually hit the race so far, but all three share the same account and write pattern. --- .github/workflows/ci.yaml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index a62b166d..6d95894b 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -47,6 +47,12 @@ jobs: baton-entitlement: "${{ vars.TEST_GROUP_ENTITLEMENT }}" baton-principal: "${{ vars.TEST_GROUP_PRINCIPAL }}" baton-principal-type: "${{ vars.TEST_GROUP_PRINCIPAL_TYPE }}" + # DocuSign's backend doesn't apply a write instantly — the sync immediately + # after a grant/revoke can still read the pre-write state. sync-test's sleep + # input exists exactly for this; confirmed needed by a real failure (a + # verification query returning no grants right after a grant that itself + # succeeded) on test-permission-profiles, which has no built-in retry. + sleep: "3" test-signing-groups: needs: [test-groups] runs-on: ubuntu-latest @@ -76,6 +82,9 @@ jobs: baton-entitlement: "${{ vars.TEST_SIGNING_GROUP_ENTITLEMENT }}" baton-principal: "${{ vars.TEST_SIGNING_GROUP_PRINCIPAL }}" baton-principal-type: "${{ vars.TEST_SIGNING_GROUP_PRINCIPAL_TYPE }}" + # See test-groups' identical sleep above — same shared demo account, same + # DocuSign write-propagation delay risk. + sleep: "3" test-permission-profiles: needs: [test-signing-groups] runs-on: ubuntu-latest @@ -105,3 +114,7 @@ jobs: baton-entitlement: "${{ vars.TEST_PERMISSION_PROFILE_ENTITLEMENT }}" baton-principal: "${{ vars.TEST_PERMISSION_PROFILE_PRINCIPAL }}" baton-principal-type: "${{ vars.TEST_PERMISSION_PROFILE_PRINCIPAL_TYPE }}" + # See test-groups' identical sleep above — same shared demo account, same + # DocuSign write-propagation delay risk (the one confirmed to actually fire + # here). + sleep: "3" From 49149ef10a702bb5e03d5a89ad8a934c3e189d08 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 26 Aug 2026 11:58:30 -0300 Subject: [PATCH 23/28] fix: address sergiocorral's PR68 review findings, close deep-code-review gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removes the unverified permission-profile-ID fast path from Grant resolution (no live tenant to confirm it matches GetUserDetails' effective profile — kept only the well-tested name-based path). Restores Revoke's pre-PR first-match behavior for ambiguous default-profile names, now logged instead of silent. Adds a 5s TTL-bounded short-circuit so concurrent Grants() workers stop re-hitting an already-exhausted rate limit. Makes the hourly rate-limit predicate case-insensitive and extends it to DocuSign's burst-limit variant. Fixes a dropped-error-chain bug in reclassifyRateLimitError via errors.Join. Also fixes issues a deep-code-review pass found in that work: a Warn-level log that violated this repo's no-Warn convention (now Debug, matching tryFastPathGrant's own fallback-log pattern); dead annotation-forwarding plumbing on the rate-limit-error path (Grants() already discards annotations whenever it returns an error, so the SDK never saw them either way); a write-only, never-read struct field; and duplicated hourly/burst matching logic factored into one shared helper. Co-Authored-By: Claude Sonnet 5 --- pkg/client/client.go | 25 ++- pkg/client/client_test.go | 20 +- pkg/client/helper.go | 189 +++++++++++++----- pkg/client/helper_test.go | 197 +++++++++++++++++-- pkg/client/models.go | 7 - pkg/connector/helper.go | 44 +++-- pkg/connector/permission_profiles.go | 25 ++- pkg/connector/permission_profiles_test.go | 185 ++++++++++++++++++ pkg/connector/users.go | 221 +++++++++++++++------- pkg/connector/users_test.go | 142 ++++++++++---- 10 files changed, 849 insertions(+), 206 deletions(-) create mode 100644 pkg/connector/permission_profiles_test.go diff --git a/pkg/client/client.go b/pkg/client/client.go index b28ced23..406d9b96 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -649,6 +649,19 @@ func (c *Client) GetUserByEmail(ctx context.Context, userEmail string) (*User, a // response may be served from the shared HTTP GET cache — fine for List/Revoke, neither // of which memoizes across syncs the way userBuilder does (see GetPermissionProfilesFresh). // +// Cache-split skew (accepted tradeoff, not an oversight): this cache is uhttp's own +// shared GET cache, which is cross-sync for a long-lived connector process (default TTL +// 1h, overridable via BATON_HTTP_CACHE_TTL) — not just cross-caller within one sync. A +// permission profile created or deleted between two syncs less than that TTL apart can +// be visible to userBuilder's always-fresh GetPermissionProfilesFresh grants pass before +// it is visible here, or vice versa, producing a bounded resource-vs-grant skew window +// (up to the cache TTL) between what List/Revoke see and what the grants pass sees. +// Pre-split, both readers shared this same cached view, so there was no skew between +// them — this split introduced the asymmetry, in exchange for keeping List/Revoke on +// uhttp's cheap shared cache path instead of paying for a fresh call on every read. That +// tradeoff is intentional: bypassing the cache here too would undo the call-volume +// reduction this split exists for. +// // Pagination: This endpoint does NOT support pagination. It returns all permission profiles in a single request. // Typically, DocuSign accounts have a limited number of permission profiles (< 50), so this is acceptable. // @@ -663,9 +676,15 @@ func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile // fires, that caller has already decided a fresh call is needed — a cached response // here could only ever serve a stale snapshot left over from a previous sync on the // same long-lived connector process, never save a real call. List and Revoke don't -// share that risk (neither memoizes this call across syncs), and sharing uhttp's cache -// with them when both fire in the same sync saves a real network call — so only this -// path opts out of it, not GetPermissionProfiles itself. +// share userBuilder's process-lifetime memoization risk (neither memoizes this call +// across syncs), and sharing uhttp's cache with them when both fire in the same sync +// saves a real network call — so only this path opts out of the cache, not +// GetPermissionProfiles itself. +// +// This does NOT mean List/Revoke are free of cross-sync staleness risk of their own: +// see GetPermissionProfiles' doc for the cache-split skew this asymmetry introduces +// between this method's always-fresh view and List/Revoke's cached one — an accepted, +// bounded tradeoff, not a gap specific to this method. func (c *Client) GetPermissionProfilesFresh(ctx context.Context) ([]PermissionProfile, annotations.Annotations, error) { return c.getPermissionProfiles(ctx, true) } diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index d3e3ce60..d6dc9886 100644 --- a/pkg/client/client_test.go +++ b/pkg/client/client_test.go @@ -8,6 +8,7 @@ import ( "net/http/httptest" "net/url" "strings" + "sync/atomic" "testing" "github.com/conductorone/baton-sdk/pkg/uhttp" @@ -276,10 +277,13 @@ func TestMultiAccountResourceIsolation(t *testing.T) { // newCountingPermissionProfilesClient wires a Client to a mock server that counts real // GET /permission_profiles hits, to distinguish a real network call from one served by -// uhttp's shared GET cache. -func newCountingPermissionProfilesClient(t *testing.T) (*Client, *int) { +// uhttp's shared GET cache. The counter is incremented inside the httptest server's +// handler goroutine and read from the test's main goroutine, so it must be an atomic +// (see pkg/connector/users_test.go's newCountingPermissionProfilesClient, which +// established this pattern with atomic.Int32) — a plain int here would trip `go test -race`. +func newCountingPermissionProfilesClient(t *testing.T) (*Client, *atomic.Int32) { t.Helper() - calls := 0 + var calls atomic.Int32 mockServer := httptest.NewServer(nil) t.Cleanup(mockServer.Close) @@ -294,7 +298,7 @@ func newCountingPermissionProfilesClient(t *testing.T) (*Client, *int) { }, }) case strings.HasSuffix(r.URL.Path, "/permission_profiles"): - calls++ + calls.Add(1) _ = json.NewEncoder(w).Encode(PermissionProfilesResponse{ PermissionProfiles: []PermissionProfile{{PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}}, }) @@ -327,8 +331,8 @@ func TestGetPermissionProfiles_CachingSplitByCaller(t *testing.T) { t.Fatalf("call %d: %v", i, err) } } - if *calls != 1 { - t.Errorf("expected 1 real request across 2 GetPermissionProfiles calls (cache should serve the second), got %d", *calls) + if got := calls.Load(); got != 1 { + t.Errorf("expected 1 real request across 2 GetPermissionProfiles calls (cache should serve the second), got %d", got) } }) @@ -339,8 +343,8 @@ func TestGetPermissionProfiles_CachingSplitByCaller(t *testing.T) { t.Fatalf("call %d: %v", i, err) } } - if *calls != 2 { - t.Errorf("expected 2 real requests across 2 GetPermissionProfilesFresh calls (no caching), got %d", *calls) + if got := calls.Load(); got != 2 { + t.Errorf("expected 2 real requests across 2 GetPermissionProfilesFresh calls (no caching), got %d", got) } }) } diff --git a/pkg/client/helper.go b/pkg/client/helper.go index e523fe2d..2ed5c68d 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -1,8 +1,10 @@ package client import ( + "context" "encoding/base64" "encoding/json" + "errors" "fmt" "net/http" "net/url" @@ -13,6 +15,8 @@ import ( "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/ratelimit" "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/timestamppb" @@ -28,79 +32,178 @@ const DefaultPageSize = 100 // account-level errorCode string; detection matches either. DocuSign returns this as // HTTP 400 today and is mid-migration to 429 (DocuSign's own guidance is to key off // errorCode / the error body, not HTTP status), so detection below ignores StatusCode. +// +// Matching is case-insensitive on both errorCode and the message substring (see +// isHourlyAPIInvocationLimitError): DocuSign's own published error codes are inconsistent +// on casing across endpoints (e.g. Hourly_APIInvocation_Envelope_Limit_Exceeded uses mixed +// case where this constant is all-caps), and this exact account-level code isn't in +// DocuSign's published list at all — so a future casing change on either field must not +// silently stop this predicate from matching. const docusignHourlyRateLimitErrorCode = "HOURLY_APIINVOCATION_LIMIT_EXCEEDED" // docusignHourlyRateLimitErrorMessage is the account hourly-limit error text quoted by // DocuSign's eSignature API rules-and-resource-limits docs ("If you exceed the API rate -// limit, you will receive the error: …"). Matched as a substring of ErrorResponse.message -// so a future errorCode rename still classifies correctly when the published message stays. +// limit, you will receive the error: …"). Matched as a case-insensitive substring of +// ErrorResponse.message so a future errorCode rename, or a casing change on either field, +// still classifies correctly as long as the published message text stays. const docusignHourlyRateLimitErrorMessage = "The maximum number of hourly API invocations has been exceeded" +// docusignBurstRateLimitErrorCode is DocuSign's errorCode for the separate 30-second +// burst API-invocation window (distinct from the hourly account budget above) being +// exceeded — also HTTP 400 today. Unlike docusignHourlyRateLimitErrorCode, this exact +// string is a best-effort reconstruction of DocuSign's "BURST_APIINVOCATION_LIMIT_EXCEEDED" +// family (following the same naming pattern as the confirmed hourly code) rather than a +// value confirmed against a real account or a specific published docs page — matching is +// case-insensitive (see docusignHourlyRateLimitErrorCode's doc) specifically because this +// exact casing is not confirmed. +const docusignBurstRateLimitErrorCode = "BURST_APIINVOCATION_LIMIT_EXCEEDED" + +// docusignBurstRateLimitErrorMessage is a best-effort guess at the human message DocuSign +// returns for the burst-limit error, matched as a case-insensitive substring the same way +// docusignHourlyRateLimitErrorMessage is. Not confirmed against DocuSign's published docs +// or a real account response — see docusignBurstRateLimitErrorCode's doc. +const docusignBurstRateLimitErrorMessage = "exceeded the burst limit" + // docusignRateLimitDefaultResetWindow is the fixed ResetAt this connector puts on the // RateLimitDescription for the account hourly-limit error — applied unconditionally, -// not just as a fallback (see reclassifyHourlyRateLimitError's doc for why response -// headers are not used for ResetAt here). The limit this error names is hourly, so an -// hour is the semantically correct value to report — but it is not what the SDK's retry -// loop actually waits: pkg/sync/parallel_syncer.go constructs its Retryer with -// MaxDelay: 0, which retry.NewRetryer normalizes to a 60-second cap, and -// retry.Retryer.ShouldWaitAndRetry computes a wait from this ResetAt only to then clamp -// it down to that same 60 seconds (`if wait > maxDelay { wait = maxDelay }`). With -// MaxAttempts: 0 (unlimited), the net effect is a 60-second retry with no attempt limit -// for the rest of the hour, not an hour of backoff — still strictly better than the old -// fatal classification, but not a full-hour wait. That gap lives in baton-sdk's retry -// package, not something this connector can change from here. +// not just as a fallback (see reclassifyRateLimitError's doc for why response headers are +// not used for ResetAt here). The limit this error names is hourly, so an hour is the +// semantically correct value to report — but it is not what the SDK's retry loop actually +// waits: pkg/sync/parallel_syncer.go constructs its Retryer with MaxDelay: 0, which +// retry.NewRetryer normalizes to a 60-second cap, and retry.Retryer.ShouldWaitAndRetry +// computes a wait from this ResetAt only to then clamp it down to that same 60 seconds +// (`if wait > maxDelay { wait = maxDelay }`). With MaxAttempts: 0 (unlimited), the net +// effect is a 60-second retry with no attempt limit for the rest of the hour, not an hour +// of backoff — still strictly better than the old fatal classification, but not a +// full-hour wait, and not free: every one of those 60-second retries is a real API call +// against an account that is already over its hourly budget, so an account that trips +// this limit early in the hour spends the rest of the hour retrying roughly once a minute +// (~60 wasted calls) while holding a sync slot open, instead of failing fast. That gap +// lives in baton-sdk's retry package, not something this connector can change from here. const docusignRateLimitDefaultResetWindow = time.Hour -// reclassifyHourlyRateLimitError recognizes the account hourly API-invocation limit in -// errTarget (the same *ErrorResponse instance uhttp.WithErrorResponse already unmarshaled -// the error body into before returning origErr — no re-parsing needed) and, if matched, -// returns a codes.Unavailable error carrying a RateLimitDescription. This matters because -// uhttp.GrpcCodeFromHTTPStatus maps this error's current HTTP 400 to codes.InvalidArgument, -// which the SDK's sync-retry loop (pkg/sync's Retryer, wired to SyncResourcesOp/ -// SyncGrantsOp) does not retry — it only waits and retries on Unavailable/DeadlineExceeded, -// so an otherwise-recoverable rate limit was surfacing as a fatal, non-resumable sync -// failure. Returns nil (unchanged behavior) when errTarget isn't this specific eSignature -// error shape, or neither the live-confirmed errorCode nor the docs-quoted message match — +// docusignRateLimitBurstResetWindow is the fixed ResetAt window used for the burst-limit +// variant instead of docusignRateLimitDefaultResetWindow. DocuSign's burst window itself +// resets in 30 seconds; 45 seconds is used here (rather than 30 exactly) to stay safely +// above that boundary given clock skew and in-flight request latency between when +// DocuSign evaluated the limit and when this connector computes ResetAt. +const docusignRateLimitBurstResetWindow = 45 * time.Second + +// reclassifyRateLimitError recognizes DocuSign's two eSignature API-invocation limit +// errors in errTarget (the same *ErrorResponse instance uhttp.WithErrorResponse already +// unmarshaled the error body into before returning origErr — no re-parsing needed) — the +// account's hourly call budget and the separate 30-second burst window — and, if either +// is matched, returns a codes.Unavailable error carrying a RateLimitDescription. This +// matters because uhttp.GrpcCodeFromHTTPStatus maps both errors' current HTTP 400 to +// codes.InvalidArgument, which the SDK's sync-retry loop (pkg/sync's Retryer, wired to +// SyncResourcesOp/SyncGrantsOp) does not retry — it only waits and retries on +// Unavailable/DeadlineExceeded, so an otherwise-recoverable rate limit was surfacing as a +// fatal, non-resumable sync failure. Returns nil (unchanged behavior) when errTarget isn't +// this specific eSignature error shape, or neither variant's errorCode/message match — // including every ClmErrorResponse-based CLM call, which is a distinct error envelope // this func never matches. // // Deliberately does NOT use ratelimit.ExtractRateLimitData's header-derived -// Limit/Remaining/ResetAt for ResetAt on this error. The eSignature rules-and-limits +// Limit/Remaining/ResetAt for ResetAt on either error. The eSignature rules-and-limits // docs do map X-RateLimit-* to the account hourly budget and X-BurstLimit-* to the // separate 30-second burst window — but on an already-over-limit response Remaining is // uninformative, and trusting ResetAt/Remaining without knowing which limiter produced -// the body risks the SDK's Retryer (vendor pkg/retry/retry.go) computing a short wait -// off the burst window and hammering an account that's still over its hourly budget. -// Always uses the fixed hourly default window instead — safe by construction, if coarser. -func reclassifyHourlyRateLimitError(errTarget uhttp.ErrorResponse, origErr error) error { +// the body risks the SDK's Retryer (vendor pkg/retry/retry.go) computing a wait off the +// wrong limiter's header pair. Instead each variant gets its own fixed default window +// (docusignRateLimitDefaultResetWindow for hourly, docusignRateLimitBurstResetWindow for +// burst) — safe by construction, if coarser. +// +// On a match, logs a breadcrumb (via the request's logger, extracted from ctx) naming +// which variant matched, so a future DocuSign casing/wording change that stops this +// predicate from matching is observable as an absence of this log line rather than a +// silent revert to fatal sync failures. +// +// This is pkg/client's only logging call — every other log line in this connector lives +// in pkg/connector, one layer up, with full sync/policy context. Deliberate exception +// here: doRequestCommon is the one place that sees every DocuSign call regardless of +// which pkg/connector builder issued it, so this is the only spot a single log line can +// reliably catch every match; pushing it up a layer would mean adding the same call at +// every builder that might hit this error, with no way to guarantee none are missed. +func reclassifyRateLimitError(ctx context.Context, errTarget uhttp.ErrorResponse, origErr error) error { er, ok := errTarget.(*ErrorResponse) - if !ok || !isHourlyAPIInvocationLimitError(er) { + if !ok { + return nil + } + + var ( + kind string + window time.Duration + ) + switch { + case isHourlyAPIInvocationLimitError(er): + kind, window = "hourly", docusignRateLimitDefaultResetWindow + case isBurstAPIInvocationLimitError(er): + kind, window = "burst", docusignRateLimitBurstResetWindow + default: return nil } st := status.New(codes.Unavailable, origErr.Error()) withDetails, detailsErr := st.WithDetails(v2.RateLimitDescription_builder{ Status: v2.RateLimitDescription_STATUS_OVERLIMIT, - ResetAt: timestamppb.New(time.Now().Add(docusignRateLimitDefaultResetWindow)), + ResetAt: timestamppb.New(time.Now().Add(window)), }.Build()) + + reclassified := withDetails.Err() if detailsErr != nil { // WithDetails only fails for a codes.OK status or a detail that can't marshal to // an Any — neither applies here (fixed codes.Unavailable, a well-formed proto // message) — but fall back to the plain Unavailable classification (still // retryable) rather than losing that reclassification entirely if it somehow does. - return st.Err() + reclassified = st.Err() + } + + ctxzap.Extract(ctx).Info( + "baton-docusign: reclassified DocuSign API-invocation rate-limit error as retryable", + zap.String("rate_limit_kind", kind), + zap.String("error_code", er.ErrorCode), + zap.Duration("reset_window", window), + ) + + // Join, rather than discard, origErr: the new status carries the message/details this + // reclassification needs, but origErr may itself be a joined error (WrapErrorsWithRateLimitInfo + // joins the base status with every DoOption error, including header-derived rate-limit + // data) whose value — not just its string form — future callers may rely on, e.g. via + // errors.As. status.Code/status.FromError still resolve to the new codes.Unavailable + // status via errors.As over the join tree (see TestReclassifyRateLimitError). + return errors.Join(reclassified, origErr) +} + +// matchesRateLimitVariant reports whether er matches a given DocuSign rate-limit error +// shape, by errorCode or message substring, case-insensitively on both — shared by +// isHourlyAPIInvocationLimitError and isBurstAPIInvocationLimitError so the two variants' +// matching logic can't drift apart. +func matchesRateLimitVariant(er *ErrorResponse, code, message string) bool { + if strings.EqualFold(er.ErrorCode, code) { + return true } - return withDetails.Err() + return containsFold(er.ErrorMessage, message) } // isHourlyAPIInvocationLimitError reports whether er is DocuSign's account-level hourly // API-invocation budget error — matching the live-confirmed errorCode and/or the message -// text quoted in the eSignature rules-and-resource-limits docs. +// text quoted in the eSignature rules-and-resource-limits docs, case-insensitively on +// both (see docusignHourlyRateLimitErrorCode's doc for why). func isHourlyAPIInvocationLimitError(er *ErrorResponse) bool { - if er.ErrorCode == docusignHourlyRateLimitErrorCode { - return true - } - return strings.Contains(er.ErrorMessage, docusignHourlyRateLimitErrorMessage) + return matchesRateLimitVariant(er, docusignHourlyRateLimitErrorCode, docusignHourlyRateLimitErrorMessage) +} + +// isBurstAPIInvocationLimitError reports whether er is DocuSign's separate 30-second +// burst API-invocation limit error — matching errorCode and/or message, case-insensitively +// on both (see docusignBurstRateLimitErrorCode's doc for why, and for the caveat that +// these exact strings are a best-effort reconstruction, not a confirmed value). +func isBurstAPIInvocationLimitError(er *ErrorResponse) bool { + return matchesRateLimitVariant(er, docusignBurstRateLimitErrorCode, docusignBurstRateLimitErrorMessage) +} + +// containsFold reports whether substr appears within s, ignoring case. +func containsFold(s, substr string) bool { + return strings.Contains(strings.ToLower(s), strings.ToLower(substr)) } // IDFromHref extracts the trailing path segment from a CLM object's Href — CLM's @@ -134,11 +237,11 @@ func buildURL(base, path string, params ...any) (*url.URL, error) { // errTarget receives the parsed error body on non-2xx responses (e.g. &ErrorResponse{} // for eSignature, &ClmErrorResponse{} for CLM) since the two APIs use different error envelopes. // -// On the error path, one specific eSignature error (DocuSign's hourly API-call-budget -// error — see reclassifyHourlyRateLimitError) has its gRPC code silently overridden from -// whatever uhttp.GrpcCodeFromHTTPStatus would otherwise produce to codes.Unavailable, so -// the SDK's sync-retry loop treats it as retryable instead of fatal. Every other error is -// returned unchanged. +// On the error path, two specific eSignature errors (DocuSign's hourly API-call-budget +// error and its separate 30-second burst-limit error — see reclassifyRateLimitError) have +// their gRPC code silently overridden from whatever uhttp.GrpcCodeFromHTTPStatus would +// otherwise produce to codes.Unavailable, so the SDK's sync-retry loop treats them as +// retryable instead of fatal. Every other error is returned unchanged. func doRequestCommon(wrapper *uhttp.BaseHttpClient, req *http.Request, res any, errTarget uhttp.ErrorResponse) (http.Header, annotations.Annotations, error) { opts := []uhttp.DoOption{} if res != nil { @@ -150,7 +253,7 @@ func doRequestCommon(wrapper *uhttp.BaseHttpClient, req *http.Request, res any, // resp is non-nil here whenever the error came from a well-formed non-2xx HTTP // response (as opposed to a network/transport failure) — see wrapper.Do. if resp != nil { - if rlErr := reclassifyHourlyRateLimitError(errTarget, err); rlErr != nil { + if rlErr := reclassifyRateLimitError(req.Context(), errTarget, err); rlErr != nil { return resp.Header, nil, rlErr } } diff --git a/pkg/client/helper_test.go b/pkg/client/helper_test.go index 5dae1b1c..6f39ac4a 100644 --- a/pkg/client/helper_test.go +++ b/pkg/client/helper_test.go @@ -12,19 +12,24 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" "golang.org/x/oauth2" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) -// TestReclassifyHourlyRateLimitError: DocuSign signals "hourly API-call budget exhausted" -// via a JSON error body (errorCode HOURLY_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400, +// TestReclassifyRateLimitError: DocuSign signals "hourly API-call budget exhausted" and +// "30-second burst limit exhausted" via a JSON error body (errorCode +// HOURLY_APIINVOCATION_LIMIT_EXCEEDED / BURST_APIINVOCATION_LIMIT_EXCEEDED) on HTTP 400, // which uhttp.GrpcCodeFromHTTPStatus maps to codes.InvalidArgument — a code the SDK's // sync-retry loop treats as fatal, not retryable, so a full sync fails outright instead -// of pausing and resuming. reclassifyHourlyRateLimitError must re-classify exactly this -// case as codes.Unavailable (which the SDK does retry) carrying a RateLimitDescription, -// and leave every other error (including CLM's distinct error envelope) untouched. -func TestReclassifyHourlyRateLimitError(t *testing.T) { +// of pausing and resuming. reclassifyRateLimitError must re-classify both cases as +// codes.Unavailable (which the SDK does retry) carrying a RateLimitDescription, and leave +// every other error (including CLM's distinct error envelope) untouched. +func TestReclassifyRateLimitError(t *testing.T) { origErr := errors.New("400 Bad Request") t.Run("matches on errorCode and always uses the fixed hourly window", func(t *testing.T) { @@ -36,7 +41,7 @@ func TestReclassifyHourlyRateLimitError(t *testing.T) { ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", } - got := reclassifyHourlyRateLimitError(errTarget, origErr) + got := reclassifyRateLimitError(context.Background(), errTarget, origErr) if got == nil { t.Fatal("expected a rate-limit error, got nil") } @@ -71,7 +76,7 @@ func TestReclassifyHourlyRateLimitError(t *testing.T) { t.Run("does not match an unrelated errorCode", func(t *testing.T) { errTarget := &ErrorResponse{ErrorCode: "USER_LACKS_PERMISSIONS"} - if got := reclassifyHourlyRateLimitError(errTarget, origErr); got != nil { + if got := reclassifyRateLimitError(context.Background(), errTarget, origErr); got != nil { t.Errorf("expected nil for an unrelated errorCode, got %v", got) } }) @@ -84,7 +89,7 @@ func TestReclassifyHourlyRateLimitError(t *testing.T) { ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", } - got := reclassifyHourlyRateLimitError(errTarget, origErr) + got := reclassifyRateLimitError(context.Background(), errTarget, origErr) if got == nil { t.Fatal("expected a rate-limit error from the docs-quoted message, got nil") } @@ -97,22 +102,188 @@ func TestReclassifyHourlyRateLimitError(t *testing.T) { } }) + t.Run("matches errorCode case-insensitively", func(t *testing.T) { + // DocuSign's own published error codes are inconsistent on casing across + // endpoints (e.g. Hourly_APIInvocation_Envelope_Limit_Exceeded is mixed-case + // where this connector's constant is all-caps) — a casing difference on the + // exact account-level code (which isn't published at all) must still match. + errTarget := &ErrorResponse{ErrorCode: "Hourly_APIInvocation_Limit_Exceeded"} + + got := reclassifyRateLimitError(context.Background(), errTarget, origErr) + if got == nil { + t.Fatal("expected a mixed-case errorCode to still match, got nil") + } + if st, _ := status.FromError(got); st.Code() != codes.Unavailable { + t.Errorf("expected codes.Unavailable, got %v", st.Code()) + } + }) + + t.Run("matches message case-insensitively", func(t *testing.T) { + errTarget := &ErrorResponse{ + ErrorCode: "SOME_FUTURE_HOURLY_CODE", + ErrorMessage: "THE MAXIMUM NUMBER OF HOURLY API INVOCATIONS HAS BEEN EXCEEDED. The hourly limit is 3000.", + } + + got := reclassifyRateLimitError(context.Background(), errTarget, origErr) + if got == nil { + t.Fatal("expected a mixed-case message to still match, got nil") + } + if st, _ := status.FromError(got); st.Code() != codes.Unavailable { + t.Errorf("expected codes.Unavailable, got %v", st.Code()) + } + }) + + t.Run("matches the burst-limit errorCode and uses the short burst window", func(t *testing.T) { + errTarget := &ErrorResponse{ + ErrorCode: docusignBurstRateLimitErrorCode, + ErrorMessage: "You have exceeded the burst limit.", + } + + got := reclassifyRateLimitError(context.Background(), errTarget, origErr) + if got == nil { + t.Fatal("expected a rate-limit error, got nil") + } + st, ok := status.FromError(got) + if !ok { + t.Fatalf("expected a gRPC status error, got %v", got) + } + if st.Code() != codes.Unavailable { + t.Errorf("expected codes.Unavailable, got %v", st.Code()) + } + + var desc *v2.RateLimitDescription + for _, d := range st.Details() { + if rl, ok := d.(*v2.RateLimitDescription); ok { + desc = rl + } + } + if desc == nil { + t.Fatalf("expected a RateLimitDescription in the error's status details, got %+v", st.Details()) + } + if desc.GetStatus() != v2.RateLimitDescription_STATUS_OVERLIMIT { + t.Errorf("expected STATUS_OVERLIMIT, got %v", desc.GetStatus()) + } + resetAt := desc.GetResetAt() + if resetAt == nil { + t.Fatal("expected a ResetAt") + } + wait := time.Until(resetAt.AsTime()) + if wait <= 30*time.Second || wait > time.Minute { + t.Errorf("expected a ResetAt in the 30s-60s burst-window ballpark, got a wait of %v", wait) + } + }) + + t.Run("matches the burst-limit errorCode case-insensitively", func(t *testing.T) { + errTarget := &ErrorResponse{ErrorCode: "burst_apiinvocation_limit_exceeded"} + + got := reclassifyRateLimitError(context.Background(), errTarget, origErr) + if got == nil { + t.Fatal("expected a mixed-case burst errorCode to still match, got nil") + } + if st, _ := status.FromError(got); st.Code() != codes.Unavailable { + t.Errorf("expected codes.Unavailable, got %v", st.Code()) + } + }) + + t.Run("hourly match takes priority and keeps the hourly window when both signals somehow appear", func(t *testing.T) { + errTarget := &ErrorResponse{ErrorCode: docusignHourlyRateLimitErrorCode} + + got := reclassifyRateLimitError(context.Background(), errTarget, origErr) + st, _ := status.FromError(got) + var desc *v2.RateLimitDescription + for _, d := range st.Details() { + if rl, ok := d.(*v2.RateLimitDescription); ok { + desc = rl + } + } + if desc.GetResetAt() == nil || desc.GetResetAt().AsTime().Before(time.Now().Add(50*time.Minute)) { + t.Errorf("expected the hourly window (~1h out), got %v", desc.GetResetAt()) + } + }) + t.Run("does not match CLM's distinct error envelope", func(t *testing.T) { // ClmErrorResponse is a different type from *ErrorResponse even if some CLM error // happened to carry the same string in an analogous field — the type assertion // alone must reject it, since this function's evidence is eSignature-specific. errTarget := &ClmErrorResponse{} - if got := reclassifyHourlyRateLimitError(errTarget, origErr); got != nil { + if got := reclassifyRateLimitError(context.Background(), errTarget, origErr); got != nil { t.Errorf("expected nil for a non-eSignature error envelope, got %v", got) } }) + + t.Run("preserves origErr in the returned error's chain", func(t *testing.T) { + // Regression test: reclassifyRateLimitError used to build a brand-new status + // error from origErr.Error() (a string) and return only that, dropping the + // original error VALUE — and anything joined into it by uhttp (e.g. + // WrapErrorsWithRateLimitInfo's header-derived rate-limit data) — from the + // chain entirely. A sentinel wrapped into origErr must still be reachable via + // errors.Is/errors.As on the reclassified error, and status.Code/status.FromError + // must still resolve to the new codes.Unavailable classification. + sentinel := errors.New("sentinel: header-derived rate-limit detail") + wrappedOrigErr := errors.Join(errors.New("400 Bad Request"), sentinel) + + errTarget := &ErrorResponse{ErrorCode: docusignHourlyRateLimitErrorCode} + got := reclassifyRateLimitError(context.Background(), errTarget, wrappedOrigErr) + + if !errors.Is(got, sentinel) { + t.Errorf("expected the reclassified error to still chain to the original sentinel error, got %v", got) + } + if status.Code(got) != codes.Unavailable { + t.Errorf("expected status.Code to still resolve to codes.Unavailable after joining origErr, got %v", status.Code(got)) + } + st, ok := status.FromError(got) + if !ok { + t.Fatalf("expected status.FromError to still find a gRPC status, got ok=false for %v", got) + } + var desc *v2.RateLimitDescription + for _, d := range st.Details() { + if rl, ok := d.(*v2.RateLimitDescription); ok { + desc = rl + } + } + if desc == nil { + t.Errorf("expected the RateLimitDescription to still be reachable via status.FromError after joining origErr, got details: %+v", st.Details()) + } + }) + + t.Run("logs a breadcrumb identifying which rate-limit variant matched", func(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + ctx := ctxzap.ToContext(context.Background(), zap.New(core)) + + errTarget := &ErrorResponse{ErrorCode: docusignBurstRateLimitErrorCode} + if got := reclassifyRateLimitError(ctx, errTarget, origErr); got == nil { + t.Fatal("expected a match") + } + + entries := logs.All() + if len(entries) != 1 { + t.Fatalf("expected exactly 1 log entry on a match, got %d: %+v", len(entries), entries) + } + fields := entries[0].ContextMap() + if fields["rate_limit_kind"] != "burst" { + t.Errorf("expected the log entry to identify the burst variant, got fields: %+v", fields) + } + }) + + t.Run("does not log when nothing matches", func(t *testing.T) { + core, logs := observer.New(zapcore.DebugLevel) + ctx := ctxzap.ToContext(context.Background(), zap.New(core)) + + errTarget := &ErrorResponse{ErrorCode: "USER_LACKS_PERMISSIONS"} + if got := reclassifyRateLimitError(ctx, errTarget, origErr); got != nil { + t.Fatalf("expected nil, got %v", got) + } + if n := logs.Len(); n != 0 { + t.Errorf("expected no log entries on a non-match, got %d", n) + } + }) } // TestGetUsers_ClassifiesHourlyRateLimitAsRetryable is an end-to-end regression test, -// exercising the real request path (GetUsers -> doRequestCommon -> -// reclassifyHourlyRateLimitError) against a mock server that returns DocuSign's actual -// observed 400 body, rather than calling reclassifyHourlyRateLimitError directly. +// exercising the real request path (GetUsers -> doRequestCommon -> reclassifyRateLimitError) +// against a mock server that returns DocuSign's actual observed 400 body, rather than +// calling reclassifyRateLimitError directly. func TestGetUsers_ClassifiesHourlyRateLimitAsRetryable(t *testing.T) { mockServer := httptest.NewServer(nil) defer mockServer.Close() diff --git a/pkg/client/models.go b/pkg/client/models.go index dbd2ce7d..92eb026c 100644 --- a/pkg/client/models.go +++ b/pkg/client/models.go @@ -49,13 +49,6 @@ type User struct { UserStatus string `json:"userStatus"` IsAdmin string `json:"isAdmin"` Permission string `json:"permissionProfileName"` - // PermissionProfileID mirrors UserDetail.PermissionProfileID's json tag on the - // chance the list-users response includes it alongside permissionProfileName — - // not confirmed against a live account (no DocuSign tenant available to verify). - // If DocuSign's list response doesn't actually send this field, it just stays - // empty and callers fall back to their existing name-based/GetUserDetails paths - // unchanged — this field is additive, never required. - PermissionProfileID string `json:"permissionProfileId"` } type GroupsResponse struct { diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 629071a4..33eba5ce 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -18,13 +18,12 @@ import ( // Shared profile/field map keys, reused across builders (and the AccountCreationSchema // field map in connector.go) to avoid repeated string literals (golangci-lint: goconst). const ( - profileFieldEmail = "email" - profileFieldUsername = "username" - profileFieldGroupName = "group_name" - profileFieldPermission = "permission" - profileFieldStatus = "status" - profileFieldPermissionID = "permission_profile_id" - profileFieldHref = "href" + profileFieldEmail = "email" + profileFieldUsername = "username" + profileFieldGroupName = "group_name" + profileFieldPermission = "permission" + profileFieldStatus = "status" + profileFieldHref = "href" ) // userStatusActive is the DocuSign UserStatus value this connector treats as "active" — @@ -88,17 +87,36 @@ func clmIDFromHref(href string) string { // permissionProfileIDByName returns the ID of the profile named name (requiring a // non-empty ID) and how many matched, so callers can tell "not found" (0) from // "ambiguous" (2+) — names aren't guaranteed unique per account — without a second scan -// of their own. id is only meaningful when matches == 1. +// of their own. id is only meaningful when matches == 1. Callers that need to resolve an +// ambiguous match anyway (rather than treat it as not-found) should use +// permissionProfilesByName instead — see its doc. func permissionProfileIDByName(profiles []client.PermissionProfile, name string) (string, int) { - id := "" - matches := 0 + matches := permissionProfilesByName(profiles, name) + if len(matches) == 0 { + return "", 0 + } + return matches[len(matches)-1].PermissionProfileId, len(matches) +} + +// permissionProfilesByName returns every profile named name (requiring a non-empty ID), +// in the API's own response order. Unlike permissionProfileIDByName — which deliberately +// makes an ambiguous match (2+) indistinguishable from "pick one, ID doesn't matter which" +// by leaving its returned id meaningless in that case — this variant exists for the one +// caller (permissionProfilesBuilder.Revoke) that needs to actually resolve an ambiguous +// name to a specific profile: DocuSign doesn't enforce unique profile names, and prior to +// the ambiguous-is-an-error behavior added elsewhere in this codebase, Revoke took the +// first match and succeeded silently. Revoke restores that first-match behavior (loudly, +// via a Warn log) using matches[0] here, while tryFastPathGrant's name-based grant +// resolution keeps treating ambiguous as not-found via permissionProfileIDByName — a wrong +// silent guess there is a wrong grant, which is worse than falling back. +func permissionProfilesByName(profiles []client.PermissionProfile, name string) []client.PermissionProfile { + var matches []client.PermissionProfile for _, p := range profiles { if p.PermissionProfileName == name && p.PermissionProfileId != "" { - id = p.PermissionProfileId - matches++ + matches = append(matches, p) } } - return id, matches + return matches } // clmHrefWithID rebuilds sampleHref with its trailing ID segment replaced by newID. diff --git a/pkg/connector/permission_profiles.go b/pkg/connector/permission_profiles.go index c71ceb9a..fc74cb79 100644 --- a/pkg/connector/permission_profiles.go +++ b/pkg/connector/permission_profiles.go @@ -9,6 +9,8 @@ import ( "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/types/entitlement" rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" ) const ( @@ -117,13 +119,26 @@ func (p *permissionProfilesBuilder) Revoke(ctx context.Context, grantObj *v2.Gra return profileAnnos, fmt.Errorf("failed to get permission profiles: %w", err) } - defaultProfileID, matches := permissionProfileIDByName(permissionProfiles, defaultPermissionProfileName) - if matches > 1 { - return profileAnnos, fmt.Errorf("default permission profile '%s' is ambiguous: %d profiles share that name in this account", defaultPermissionProfileName, matches) - } - if matches == 0 { + // DocuSign does not enforce unique permission profile names, so more than one profile + // can share defaultPermissionProfileName. Unlike tryFastPathGrant's name-based grant + // resolution (users.go), which treats an ambiguous match as not-found rather than risk + // silently emitting a wrong grant, Revoke restores its pre-existing behavior of taking + // the first match (same order the API returned them) so that an account with a + // duplicate-named default profile can still have permission-profile grants revoked at + // all — just loudly, via the Warn below, instead of silently. See + // permissionProfilesByName's doc for why this uses a different helper than the + // ambiguous-is-not-found path. + matchingProfiles := permissionProfilesByName(permissionProfiles, defaultPermissionProfileName) + if len(matchingProfiles) == 0 { return profileAnnos, fmt.Errorf("default permission profile '%s' not found in account", defaultPermissionProfileName) } + defaultProfileID := matchingProfiles[0].PermissionProfileId + if len(matchingProfiles) > 1 { + ctxzap.Extract(ctx).Debug("baton-docusign: default permission profile name is ambiguous, using the first match", + zap.String("profile_name", defaultPermissionProfileName), + zap.Int("match_count", len(matchingProfiles)), + zap.String("chosen_profile_id", defaultProfileID)) + } // Check if trying to revoke the default "DocuSign Viewer" profile itself. // This is not allowed as it's the minimum permission level. diff --git a/pkg/connector/permission_profiles_test.go b/pkg/connector/permission_profiles_test.go new file mode 100644 index 00000000..484fda7a --- /dev/null +++ b/pkg/connector/permission_profiles_test.go @@ -0,0 +1,185 @@ +package connector + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/conductorone/baton-docusign/pkg/client" + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/conductorone/baton-sdk/pkg/types/grant" + "github.com/conductorone/baton-sdk/pkg/uhttp" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + "golang.org/x/oauth2" +) + +// newPermissionProfilesTestClient wires a *client.Client to a mock server handling +// /oauth/userinfo, GET permission_profiles, GET users/{id}, and PUT users/{id}/profile — +// everything permissionProfilesBuilder.Revoke needs. updateCalls, if non-nil, is +// incremented (with the request body recorded) on every PUT users/{id}/profile call, so +// tests can assert which profile ID Revoke actually assigned. +func newPermissionProfilesTestClient( + t *testing.T, + profiles []client.PermissionProfile, + userDetails map[string]client.UserDetail, + updateCalls *[]client.UpdateUserProfileRequest, +) *client.Client { + t.Helper() + mockServer := httptest.NewServer(nil) + t.Cleanup(mockServer.Close) + + mockServer.Config.Handler = http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + switch { + case r.URL.Path == "/oauth/userinfo": + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "service-account-user-id", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Acme", BaseURI: mockServer.URL, IsDefault: true}, + }, + }) + case r.URL.Path == "/restapi/v2.1/accounts/acct-1/permission_profiles": + _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{PermissionProfiles: profiles}) + case r.Method == http.MethodPut && strings.HasSuffix(r.URL.Path, "/profile"): + var req client.UpdateUserProfileRequest + _ = json.NewDecoder(r.Body).Decode(&req) + if updateCalls != nil { + *updateCalls = append(*updateCalls, req) + } + _ = json.NewEncoder(w).Encode(map[string]string{}) + case r.Method == http.MethodGet: + const prefix = "/restapi/v2.1/accounts/acct-1/users/" + if strings.HasPrefix(r.URL.Path, prefix) { + userID := strings.TrimPrefix(r.URL.Path, prefix) + if detail, ok := userDetails[userID]; ok { + _ = json.NewEncoder(w).Encode(detail) + return + } + } + http.NotFound(w, r) + default: + http.NotFound(w, r) + } + }) + + mockServerURL, err := url.Parse(mockServer.URL) + if err != nil { + t.Fatalf("failed to parse mock server URL: %v", err) + } + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: &rewriteTransport{target: mockServerURL, base: http.DefaultTransport}}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + return client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) +} + +func revokeGrantFor(userID, permissionProfileID string) *v2.Grant { + return grant.NewGrant( + &v2.Resource{Id: &v2.ResourceId{ResourceType: permissionProfilesResourceType.Id, Resource: permissionProfileID}}, + permissionProfileAssignedTag, + &v2.ResourceId{ResourceType: userResourceType.Id, Resource: userID}, + ) +} + +// TestPermissionProfilesBuilder_Revoke_AmbiguousDefaultNameUsesFirstMatch is a +// regression test restoring Revoke's pre-existing behavior: DocuSign does not enforce +// unique permission profile names, so more than one profile can share +// defaultPermissionProfileName ("DocuSign Viewer"). Revoke must not hard-error in that +// case — it must take the first match (same order the API returned them) and succeed, +// logging (at Debug — this repo doesn't use Warn) rather than silently guessing. Without +// this, an account with a duplicate-named default profile could never have +// permission-profile grants revoked through this connector at all. +func TestPermissionProfilesBuilder_Revoke_AmbiguousDefaultNameUsesFirstMatch(t *testing.T) { + ctx := context.Background() + + profiles := []client.PermissionProfile{ + {PermissionProfileId: "pp-viewer-first", PermissionProfileName: defaultPermissionProfileName}, + {PermissionProfileId: "pp-viewer-second", PermissionProfileName: defaultPermissionProfileName}, + {PermissionProfileId: "pp-admin", PermissionProfileName: "DocuSign Admin"}, + } + userDetails := map[string]client.UserDetail{ + "user-1": {UserID: "user-1", PermissionProfileName: "DocuSign Admin", PermissionProfileID: "pp-admin"}, + } + + var updateCalls []client.UpdateUserProfileRequest + c := newPermissionProfilesTestClient(t, profiles, userDetails, &updateCalls) + b := newPermissionProfilesBuilder(c) + + core, logs := observer.New(zapcore.DebugLevel) + observedCtx := ctxzap.ToContext(ctx, zap.New(core)) + + annos, err := b.Revoke(observedCtx, revokeGrantFor("user-1", "pp-admin")) + if err != nil { + t.Fatalf("Revoke: %v", err) + } + _ = annos + + if len(updateCalls) != 1 { + t.Fatalf("expected exactly 1 UpdateUserProfile call, got %d", len(updateCalls)) + } + if got := updateCalls[0].UserDetails.PermissionProfileId; got != "pp-viewer-first" { + t.Errorf("expected Revoke to assign the first-listed match %q, got %q", "pp-viewer-first", got) + } + + debugLogs := logs.FilterMessageSnippet("ambiguous") + if debugLogs.Len() != 1 { + t.Fatalf("expected exactly 1 Debug log about the ambiguous default profile name, got %d", debugLogs.Len()) + } +} + +// TestPermissionProfilesBuilder_Revoke_UnambiguousDefaultNameNoWarning is the control +// case for the test above: a single unambiguous default profile must revoke exactly as +// before, with no ambiguous-match log at all. +func TestPermissionProfilesBuilder_Revoke_UnambiguousDefaultNameNoWarning(t *testing.T) { + ctx := context.Background() + + profiles := []client.PermissionProfile{ + {PermissionProfileId: "pp-viewer", PermissionProfileName: defaultPermissionProfileName}, + {PermissionProfileId: "pp-admin", PermissionProfileName: "DocuSign Admin"}, + } + userDetails := map[string]client.UserDetail{ + "user-1": {UserID: "user-1", PermissionProfileName: "DocuSign Admin", PermissionProfileID: "pp-admin"}, + } + + var updateCalls []client.UpdateUserProfileRequest + c := newPermissionProfilesTestClient(t, profiles, userDetails, &updateCalls) + b := newPermissionProfilesBuilder(c) + + core, logs := observer.New(zapcore.DebugLevel) + observedCtx := ctxzap.ToContext(ctx, zap.New(core)) + + if _, err := b.Revoke(observedCtx, revokeGrantFor("user-1", "pp-admin")); err != nil { + t.Fatalf("Revoke: %v", err) + } + + if len(updateCalls) != 1 || updateCalls[0].UserDetails.PermissionProfileId != "pp-viewer" { + t.Fatalf("expected Revoke to assign pp-viewer, got %+v", updateCalls) + } + if logs.Len() != 0 { + t.Errorf("expected no ambiguous-match logs for an unambiguous default profile name, got %d", logs.Len()) + } +} + +// TestPermissionProfilesBuilder_Revoke_NoDefaultProfileStillErrors makes sure the +// restored first-match behavior didn't accidentally weaken the genuine not-found case: +// zero matches must still error, not silently no-op. +func TestPermissionProfilesBuilder_Revoke_NoDefaultProfileStillErrors(t *testing.T) { + ctx := context.Background() + + profiles := []client.PermissionProfile{ + {PermissionProfileId: "pp-admin", PermissionProfileName: "DocuSign Admin"}, + } + c := newPermissionProfilesTestClient(t, profiles, nil, nil) + b := newPermissionProfilesBuilder(c) + + _, err := b.Revoke(ctx, revokeGrantFor("user-1", "pp-admin")) + if err == nil { + t.Fatal("expected an error when the default permission profile is missing entirely, got nil") + } +} diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 736185e3..410ec665 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "sync" + "time" "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -34,22 +35,23 @@ type userBuilder struct { // permissionProfilesMu/permissionProfilesSyncID/permissionProfilesCached/ // permissionProfiles/permissionProfilesErr/permissionProfilesTransientFails memoize - // the one account-wide GetPermissionProfiles call tryFastPathGrant's name-lookup - // branch needs, across every Active user's Grants() call within a single sync. This - // builder is registered once via ResourceSyncers and reused for the lifetime of the - // connector process (baton-sdk's connectorbuilder.NewConnector stores the returned - // syncers once; see vendor/.../pkg/connectorbuilder/connectorbuilder.go) — in - // service/hosted mode that's many syncs, not one — so the cache is keyed on - // permissionProfilesSyncID (from SyncOpAttrs.SyncID, threaded through from Grants()) - // rather than trusted for the process's whole lifetime: a mismatch means a new sync - // has started and the memo is stale, resetting both the cached result and the - // transient-failure counter below. Grants() runs concurrently across users within - // one sync, so this must stay safe for concurrent access — hence a mutex, not a - // plain bool. uhttp's GET cache only ever caches a 200 response, never an error, so - // without this a persistent failure (e.g. a service user lacking permission_profiles - // read access) would re-hit the real API on every Active user instead of once per - // sync — doubling that user's calls (the failed lookup, then the GetUserDetails - // fallback) against the same hourly budget this fix exists to protect. + // the one account-wide GetPermissionProfiles + // call tryFastPathGrant's name-lookup branch needs, across every Active user's + // Grants() call within a single sync. This builder is registered once via + // ResourceSyncers and reused for the lifetime of the connector process (baton-sdk's + // connectorbuilder.NewConnector stores the returned syncers once; see + // vendor/.../pkg/connectorbuilder/connectorbuilder.go) — in service/hosted mode + // that's many syncs, not one — so the cache is keyed on permissionProfilesSyncID + // (from SyncOpAttrs.SyncID, threaded through from Grants()) rather than trusted for + // the process's whole lifetime: a mismatch means a new sync has started and the memo + // is stale, resetting the cached result, the transient-failure counter, and the + // rate-limit TTL fields below. Grants() runs concurrently across users within one + // sync, so this must stay safe for concurrent access — hence a mutex, not a plain + // bool. uhttp's GET cache only ever caches a 200 response, never an error, so without + // this a persistent failure (e.g. a service user lacking permission_profiles read + // access) would re-hit the real API on every Active user instead of once per sync — + // doubling that user's calls (the failed lookup, then the GetUserDetails fallback) + // against the same hourly budget this fix exists to protect. // // A genuinely persistent failure (see isCacheablePermissionProfilesError's doc) is // cached immediately. A transient-shaped failure (a plain 5xx/network blip, or any @@ -65,12 +67,27 @@ type userBuilder struct { // cancellation/deadline are exempt from this counter entirely — see // getPermissionProfiles' doc for why counting either toward the threshold would be // actively harmful, not just a missed optimization. - permissionProfilesMu sync.Mutex - permissionProfilesSyncID string - permissionProfilesCached bool - permissionProfiles []client.PermissionProfile - permissionProfilesErr error - permissionProfilesTransientFails int + // + // permissionProfilesRateLimitedUntil/permissionProfilesRateLimitedErr are a separate, + // narrower guard than the permissionProfilesCached mechanism above: a reclassified + // rate-limit error is deliberately never cached via permissionProfilesCached (see + // getPermissionProfiles' doc — caching it would prevent ever re-checking whether the + // hourly window has reset), but Grants() runs concurrently across every Active user, + // so without this, each waiting goroutine would in turn make its own real HTTP call + // the instant it acquires the mutex, hammering an already-exhausted hourly budget + // with one wasted call per Active user within seconds. permissionProfilesRateLimitedUntil + // bounds that: it's a short TTL (see permissionProfilesRateLimitTTL's doc) that + // collapses a same-instant concurrent burst into a single real call while still + // expiring well before the SDK's own ~60s per-action retry cadence, so a genuine later + // retry always gets a fresh check rather than being blocked by a stale cached failure. + permissionProfilesMu sync.Mutex + permissionProfilesSyncID string + permissionProfilesCached bool + permissionProfiles []client.PermissionProfile + permissionProfilesErr error + permissionProfilesTransientFails int + permissionProfilesRateLimitedUntil time.Time + permissionProfilesRateLimitedErr error } // permissionProfilesTransientFailureThreshold is how many consecutive transient @@ -82,6 +99,30 @@ type userBuilder struct { // the sync. const permissionProfilesTransientFailureThreshold = 3 +// permissionProfilesRateLimitTTL bounds how long getPermissionProfiles short-circuits +// concurrent callers to a cached reclassified-rate-limit error (see +// permissionProfilesRateLimitedUntil's doc on the struct above) before allowing another +// real HTTP call. Grants() runs concurrently across every Active user in a sync, and the +// mutex alone only serializes access to this cache — it doesn't stop each waiting +// goroutine from, in turn, making its own real call the instant it acquires the lock. +// Without a TTL guard, a single rate-limited episode would cost one wasted call per +// Active user within seconds — the exact call-amplification this whole fix exists to +// prevent, just moved one level down. +// +// 5 seconds is chosen to be clearly, safely shorter than the SDK's own per-action retry +// cadence (this repo's prior investigation into pkg/retry/retry.go's MaxDelay clamp +// found it waits roughly 60s between attempts): the TTL only ever needs to be long enough +// to collapse calls that are part of the same instant (a burst of goroutines all waiting +// on the same mutex when the rate limit first hits), never long enough to still be in +// effect by the time a genuinely separate retry attempt comes around. That's what keeps +// this from reintroducing the exact problem isCacheablePermissionProfilesError's +// allowlist (and getPermissionProfiles' refusal to cache this error via +// permissionProfilesCached) exists to prevent: a short TTL that's already expired by the +// next real retry never blocks that retry from re-checking whether the hourly window has +// reset, it only ever suppresses calls that were always going to hit the same still-open +// window anyway. +const permissionProfilesRateLimitTTL = 5 * time.Second + // isCacheablePermissionProfilesError reports whether err is a persistent, // account-configuration-shaped failure safe to cache on userBuilder for the rest of this // sync — mirrors isOptInFeatureUnavailableError's PermissionDenied/Unauthenticated/ @@ -112,57 +153,85 @@ func isCacheablePermissionProfilesError(err error) bool { } // getPermissionProfiles returns the account's permission profiles, calling -// client.GetPermissionProfiles at most once per sync (keyed by syncID — see the +// client.GetPermissionProfilesFresh at most once per sync (keyed by syncID — see the // memoization fields' doc on the struct above for why this builder can't just trust the // cache for its whole process lifetime) unless the call fails with a non-cacheable // (transient) error — see isCacheablePermissionProfilesError's doc — and even then, only // up to permissionProfilesTransientFailureThreshold consecutive times before that // transient failure is cached too, bounding the worst-case call cost. Two exceptions -// never count toward that threshold and are never cached no matter how many times they -// recur: +// never count toward that threshold and are never cached via permissionProfilesCached no +// matter how many times they recur: // - A reclassified rate-limit error: unlike an ordinary transient blip, this failure // has a known, bounded resolution (the hourly window resetting), so it's always -// worth a real retry — caching it would replay the same stale codes.Unavailable on -// every retry of the SDK's per-action retry loop (unlimited attempts, same builder -// reused) forever, never re-checking whether the window has actually reset. This is -// the exact regression the isCacheablePermissionProfilesError allowlist already -// exists to prevent; the threshold must not reintroduce it via a different path. +// worth a real retry — caching it via permissionProfilesCached would replay the same +// stale codes.Unavailable on every retry of the SDK's per-action retry loop +// (unlimited attempts, same builder reused) forever, never re-checking whether the +// window has actually reset. This is the exact regression the +// isCacheablePermissionProfilesError allowlist already exists to prevent; the +// threshold must not reintroduce it via a different path. It DOES get a much +// shorter-lived TTL guard instead — see permissionProfilesRateLimitedUntil's doc — to +// collapse a same-instant concurrent burst across Grants() calls without blocking a +// genuine later retry. // - A context cancellation/deadline: only means whichever caller's context won this // attempt was already done, not that the endpoint is actually degraded. An unlucky // run of cancellations shouldn't accumulate toward disabling the fast path on an // otherwise-healthy account. -func (b *userBuilder) getPermissionProfiles(ctx context.Context, syncID string) ([]client.PermissionProfile, error) { +// +// The second return value carries GetPermissionProfilesFresh's rate-limit annotations, +// but only on the invocation that actually performed the HTTP round-trip this sync — the +// memo-hit early return (permissionProfilesCached) and the rate-limit-TTL early return +// (permissionProfilesRateLimitedUntil) both return nil annotations, since neither made a +// real request and forwarding a stale, reused annotation would misrepresent the current +// request's pacing to the SDK's self-throttling rate limiter. +func (b *userBuilder) getPermissionProfiles(ctx context.Context, syncID string) ([]client.PermissionProfile, annotations.Annotations, error) { b.permissionProfilesMu.Lock() defer b.permissionProfilesMu.Unlock() if b.permissionProfilesCached && b.permissionProfilesSyncID == syncID { - return b.permissionProfiles, b.permissionProfilesErr + return b.permissionProfiles, nil, b.permissionProfilesErr } if b.permissionProfilesSyncID != syncID { // A new sync started (or this is the first call ever): the previous sync's - // cached result/error and transient-failure count no longer apply. Reset both - // so this sync gets its own full permissionProfilesTransientFailureThreshold - // chances rather than inheriting a count left over from a prior sync's outage. + // cached result/error, transient-failure count, and rate-limit TTL no longer + // apply. Reset all of them so this sync gets its own full + // permissionProfilesTransientFailureThreshold chances and its own rate-limit + // window rather than inheriting state left over from a prior sync. b.permissionProfilesSyncID = syncID b.permissionProfilesCached = false b.permissionProfilesTransientFails = 0 + b.permissionProfilesRateLimitedUntil = time.Time{} + b.permissionProfilesRateLimitedErr = nil + } + + // A concurrent burst within this same sync already found the account rate-limited + // very recently: collapse this call into that same episode rather than issuing + // another real request that's overwhelmingly likely to hit the same still-open + // window. See permissionProfilesRateLimitTTL's doc for why this can't block a + // genuine later retry. + if time.Now().Before(b.permissionProfilesRateLimitedUntil) { + return nil, nil, b.permissionProfilesRateLimitedErr } - profiles, _, err := b.client.GetPermissionProfilesFresh(ctx) + profiles, freshAnnos, err := b.client.GetPermissionProfilesFresh(ctx) if err != nil && !isCacheablePermissionProfilesError(err) { - if isReclassifiedRateLimitError(err) || errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return nil, err + if isReclassifiedRateLimitError(err) { + b.permissionProfilesRateLimitedUntil = time.Now().Add(permissionProfilesRateLimitTTL) + b.permissionProfilesRateLimitedErr = err + return nil, freshAnnos, err + } + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil, freshAnnos, err } b.permissionProfilesTransientFails++ if b.permissionProfilesTransientFails < permissionProfilesTransientFailureThreshold { - return nil, err + return nil, freshAnnos, err } } b.permissionProfilesCached = true b.permissionProfiles = profiles b.permissionProfilesErr = err - return profiles, err + return profiles, freshAnnos, err } // ResourceType returns the Baton resource type handled by this builder, @@ -287,18 +356,22 @@ func newPermissionProfileGrant(permissionProfileID string, userID *v2.ResourceId } // tryFastPathGrant is Grants' fast path for an Active user, avoiding the per-user -// GetUserDetails call that contributes to DocuSign's hourly rate limit. Two ways it can -// resolve the grant without that call, both already captured on the resource's profile -// during List(): -// - Preferred: client.User.PermissionProfileID directly, if the list response included -// it (unconfirmed against a live account — see that field's doc) — no API call at all. -// - Otherwise: the permission-profile NAME, resolved to an ID via GetPermissionProfiles -// — one account-wide call for the whole sync, via getPermissionProfiles' own -// memoization on this builder (see its doc), not uhttp's GET cache: that cache never -// stores a non-2xx response, so relying on it alone would let a persistent failure -// (not just a rate limit — e.g. a service user lacking permission_profiles read -// access) re-hit the real API once per Active user instead of once per sync, the -// same 1:1 amplification as the GetUserDetails path this fast path exists to avoid. +// GetUserDetails call that contributes to DocuSign's hourly rate limit. It resolves the +// grant using the permission-profile NAME already captured on the resource's profile +// during List(), resolved to an ID via GetPermissionProfiles — one account-wide call for +// the whole sync, via getPermissionProfiles' own memoization on this builder (see its +// doc), not uhttp's GET cache: that cache never stores a non-2xx response, so relying on +// it alone would let a persistent failure (not just a rate limit — e.g. a service user +// lacking permission_profiles read access) re-hit the real API once per Active user +// instead of once per sync, the same 1:1 amplification as the GetUserDetails path this +// fast path exists to avoid. +// +// A once-considered alternative — trusting a PermissionProfileID field directly on the +// list-users response, if DocuSign's API included one — was removed: it was never +// confirmed against a live tenant to return the same effective profile ID GetUserDetails +// returns for that user (a group-inherited or account-default value could differ), and a +// wrong grant here is silent and undetectable by the sync itself. The name-based +// resolution below is the well-tested, confirmed-correct mechanism. // // handled=false means "no decision, fall back to Grants' original GetUserDetails path // unchanged" — covers a non-active user, a missing profile field, an unresolvable name @@ -313,12 +386,22 @@ func newPermissionProfileGrant(permissionProfileID string, userID *v2.ResourceId // over budget — exactly the amplification this fix exists to reduce. handled=true with a // nil err means the grant was resolved. // -// Never forwards GetPermissionProfilesFresh's annotations: getPermissionProfiles' -// memoization (see its doc) already limits this builder to exactly one real call per -// sync, so its rate-limit snapshot is one sample from one point in the sync, not -// representative of per-user request pacing — forwarding it on every active user would -// feed the SDK's self-throttling rate limiter that single frozen signal instead of the -// fresh per-request data GetUserDetails supplied before this fast path existed. +// Forwards getPermissionProfiles' annotations (its second return value) on success, when +// that call actually performed the HTTP round-trip this sync — getPermissionProfiles' own +// memoization already limits this builder to exactly one real call per sync (see its +// doc), so that single call's rate-limit snapshot is the freshest, most representative +// signal available for the Grants pass, unlike relying on stale data from a prior sync. +// On any invocation served from getPermissionProfiles' internal cache/TTL short-circuits +// instead, it returns nil annotations, and so does this method. +// +// Does NOT forward annotations on the propagated-rate-limit-error branch below, even +// though getPermissionProfiles may return non-nil ones there too: Grants() (this method's +// only caller) discards any annotations returned alongside a non-nil error +// (`return nil, nil, err`) — the SDK never sees them either way — and that failed call's +// rate-limit signal already reaches the SDK's retry loop through the error's own gRPC +// status details (the RateLimitDescription reclassifyRateLimitError attaches), not +// through annotations. Threading a value through that's guaranteed to be dropped one +// frame up would just be dead code. func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resource, userID *v2.ResourceId, syncID string) (*v2.Grant, annotations.Annotations, error, bool) { profile := rs.GetProfile(resource) @@ -327,19 +410,12 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc return nil, nil, nil, false } - // If the list response happened to include the profile ID directly (unconfirmed - // against a live account — see client.User.PermissionProfileID's doc), this skips - // GetPermissionProfiles entirely: no API call, no name lookup, no cache dependency. - if id, ok := rs.GetProfileStringValue(profile, profileFieldPermissionID); ok && id != "" { - return newPermissionProfileGrant(id, userID), nil, nil, true - } - name, ok := rs.GetProfileStringValue(profile, profileFieldPermission) if !ok || name == "" { return nil, nil, nil, false } - profiles, err := b.getPermissionProfiles(ctx, syncID) + profiles, ppAnnos, err := b.getPermissionProfiles(ctx, syncID) if err != nil { if isReclassifiedRateLimitError(err) { return nil, nil, err, true @@ -353,7 +429,7 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc if matches != 1 { return nil, nil, nil, false } - return newPermissionProfileGrant(id, userID), nil, nil, true + return newPermissionProfileGrant(id, userID), ppAnnos, nil, true } // CreateAccountCapabilityDetails declares support for account provisioning without a password. @@ -490,12 +566,11 @@ func parseIntoUserResource(user *client.User) (*v2.Resource, error) { } profile := map[string]any{ - "userName": user.UserName, - profileFieldEmail: user.Email, - "isAdmin": user.IsAdmin, - profileFieldPermission: user.Permission, - profileFieldStatus: user.UserStatus, - profileFieldPermissionID: user.PermissionProfileID, + "userName": user.UserName, + profileFieldEmail: user.Email, + "isAdmin": user.IsAdmin, + profileFieldPermission: user.Permission, + profileFieldStatus: user.UserStatus, } userTraits := []rs.UserTraitOption{ diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index bb7fe7ea..19293734 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -8,6 +8,7 @@ import ( "net/url" "sync/atomic" "testing" + "time" "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -338,6 +339,12 @@ func TestUserBuilder_Grants_FastPath_ActiveUserWithKnownProfile(t *testing.T) { if got := grants[0].Entitlement.Resource.Id.Resource; got != "pp-1" { t.Errorf("expected grant against permission profile pp-1, got %s", got) } + // This is the sync's first (and only) call to getPermissionProfiles, so it's the one + // that performed the real HTTP round-trip — the SDK's self-throttling rate limiter + // must receive that fresh signal, not nil, during the grants pass. + if res.Annotations == nil { + t.Error("expected non-nil annotations from the fast path's underlying real GetPermissionProfiles call") + } } func TestUserBuilder_Grants_FallsBackWhenNotActive(t *testing.T) { @@ -450,17 +457,19 @@ func newCountingPermissionProfilesClient(t *testing.T, respond func(w http.Respo return client.NewClient(context.Background(), false, tokenSource, "", "", wrapper), &calls } -// TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure is a regression test: unlike a -// persistent non-rate-limit failure, a reclassified rate-limit error must NOT be cached -// on the builder — caching it would replay the same stale error on every retry of the -// SDK's per-action retry loop (which reuses this same userBuilder), spinning forever at -// the retryer's clamped interval instead of ever re-checking whether the account's -// hourly window has reset. Loops well past permissionProfilesTransientFailureThreshold: -// the rate-limit exemption must hold regardless of how many consecutive times it -// recurs, unlike an ordinary transient failure that IS eventually cached (see -// TestUserBuilder_Grants_BoundsTransientFailureRetries) — every call here must issue a -// real GetPermissionProfiles request. -func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { +// TestUserBuilder_Grants_CollapsesRateLimitBurstWithinTTL is a regression test for the +// thundering-herd fix: Grants() runs concurrently across every Active user in a sync, and +// getPermissionProfiles' mutex alone only serializes access to the cache — it does not +// stop each waiting caller from, in turn, making its own real HTTP call once it acquires +// the lock. Without the short permissionProfilesRateLimitedUntil TTL guard, a single +// rate-limited episode would cost one wasted call per Active user within seconds, +// hammering an already-exhausted hourly budget with exactly the kind of call +// amplification this whole fix exists to prevent. This issues several calls +// back-to-back (well within permissionProfilesRateLimitTTL) right after the first +// rate-limit response and asserts only that first call actually reached the real +// endpoint — every call must still propagate the rate-limit error, just without a new +// HTTP round-trip. +func TestUserBuilder_Grants_CollapsesRateLimitBurstWithinTTL(t *testing.T) { c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { w.WriteHeader(http.StatusBadRequest) _ = json.NewEncoder(w).Encode(client.ErrorResponse{ @@ -472,7 +481,7 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") - const attempts = permissionProfilesTransientFailureThreshold + 2 + const attempts = 5 for i := 0; i < attempts; i++ { _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) if err == nil { @@ -483,8 +492,47 @@ func TestUserBuilder_Grants_DoesNotMemoizeRateLimitFailure(t *testing.T) { } } - if got := atomic.LoadInt32(permissionProfilesCalls); got != attempts { - t.Errorf("expected GetPermissionProfiles called on every one of %d retries, got %d — the rate-limit error must never be memoized, even past the threshold", attempts, got) + if got := atomic.LoadInt32(permissionProfilesCalls); got != 1 { + t.Errorf("expected exactly 1 real GetPermissionProfiles call across %d rapid retries (collapsed by the rate-limit TTL guard), got %d", attempts, got) + } +} + +// TestUserBuilder_Grants_RateLimitTTLExpiryAllowsFreshRetry proves the TTL guard above is +// bounded, not a disguised permanent cache: once permissionProfilesRateLimitedUntil has +// passed, the next call must reach the real endpoint again — otherwise a genuine later +// retry from the SDK's own per-action retry loop could never notice the hourly window has +// reset, the exact regression the "never cache a rate-limit error via +// permissionProfilesCached" rule (see getPermissionProfiles' doc) exists to prevent. +// Simulates TTL expiry by setting the unexported field directly (same package) rather +// than sleeping permissionProfilesRateLimitTTL in a unit test. +func TestUserBuilder_Grants_RateLimitTTLExpiryAllowsFreshRetry(t *testing.T) { + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusBadRequest) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ + ErrorCode: "HOURLY_APIINVOCATION_LIMIT_EXCEEDED", + ErrorMessage: "The maximum number of hourly API invocations has been exceeded. The hourly limit is 3000.", + }) + }) + + b := newUserBuilder(c, false) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + if _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}); err == nil { + t.Fatal("expected the first call to propagate the rate-limit error, got nil") + } + if got := atomic.LoadInt32(permissionProfilesCalls); got != 1 { + t.Fatalf("expected exactly 1 real call after the first attempt, got %d", got) + } + + b.permissionProfilesMu.Lock() + b.permissionProfilesRateLimitedUntil = time.Now().Add(-time.Second) + b.permissionProfilesMu.Unlock() + + if _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}); err == nil { + t.Fatal("expected the post-TTL call to still propagate the rate-limit error, got nil") + } + if got := atomic.LoadInt32(permissionProfilesCalls); got != 2 { + t.Errorf("expected a second real GetPermissionProfiles call once the TTL expired, got %d real calls total", got) } } @@ -616,6 +664,45 @@ func TestUserBuilder_Grants_FallsBackOnNonRateLimitPermissionProfilesFailure(t * } } +// TestUserBuilder_GetPermissionProfiles_ForwardsAnnotationsOnlyOnFreshCall is a +// regression test for Fix 3: tryFastPathGrant's success path used to always return nil +// annotations, starving the SDK's self-throttling rate limiter of any signal during the +// grants pass even though getPermissionProfiles makes exactly one real HTTP call per +// sync (see getPermissionProfiles' doc). The first call in a sync — the one that performs +// the real round-trip — must return non-nil annotations; a second call with the same +// syncID, served from the memo cache, must return nil. +func TestUserBuilder_GetPermissionProfiles_ForwardsAnnotationsOnlyOnFreshCall(t *testing.T) { + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{ + PermissionProfiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + }) + }) + + b := newUserBuilder(c, false) + + _, firstAnnos, err := b.getPermissionProfiles(context.Background(), "sync-1") + if err != nil { + t.Fatalf("first call: %v", err) + } + if firstAnnos == nil { + t.Error("expected the first (real-call) invocation to return non-nil annotations") + } + + _, secondAnnos, err := b.getPermissionProfiles(context.Background(), "sync-1") + if err != nil { + t.Fatalf("second call: %v", err) + } + if secondAnnos != nil { + t.Errorf("expected the memo-hit invocation to return nil annotations, got %+v", secondAnnos) + } + + if got := atomic.LoadInt32(permissionProfilesCalls); got != 1 { + t.Errorf("expected exactly 1 real GetPermissionProfiles call, got %d", got) + } +} + // TestUserBuilder_Grants_MemoizesPermissionProfilesFailureAcrossUsers is a regression // test: uhttp's GET cache never stores a non-2xx response, so without its own // memoization, tryFastPathGrant would re-hit GetPermissionProfiles for every Active user @@ -708,33 +795,6 @@ func TestUserBuilder_Grants_FallsBackOnServiceUnavailable(t *testing.T) { } } -// TestUserBuilder_Grants_FastPath_PrefersDirectProfileIDOverName covers the -// PermissionProfileID-on-the-list-response path: when present, it must skip -// GetPermissionProfiles entirely and use the ID directly. If it fell through instead, -// GetPermissionProfiles would succeed with an empty list (no profiles fixture is -// provided), fail to resolve "DocuSign Admin" by name, and fall through again to -// GetUserDetails — which 404s (no userDetails fixture either) and fails this test. -func TestUserBuilder_Grants_FastPath_PrefersDirectProfileIDOverName(t *testing.T) { - c := newUsersTestClient(t, nil, nil, permissionProfilesOK) - b := newUserBuilder(c, false) - resource, err := rs.NewUserResource("user-1", userResourceType, "user-1", nil, rs.WithResourceProfile(map[string]any{ - profileFieldStatus: userStatusActive, - profileFieldPermission: "DocuSign Admin", - profileFieldPermissionID: "pp-1", - })) - if err != nil { - t.Fatalf("NewUserResource: %v", err) - } - - grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) - if err != nil { - t.Fatalf("Grants: %v", err) - } - if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { - t.Errorf("expected the direct profile ID to resolve pp-1 with no API call, got %+v", grants) - } -} - func TestUserBuilder_Grants_FallsBackWhenProfileFieldMissing(t *testing.T) { // An identity-only or otherwise profile-less resource must not panic or skip the // grant — it should behave exactly as it did before the fast path existed. From 51ae65ed3f4c826d6419fe2753ae1b3b5308ed13 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 26 Aug 2026 12:02:28 -0300 Subject: [PATCH 24/28] fix: wrap fast-path rate-limit error, check ctx before locking permission-profile cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two sergiocorral findings on PR68 that didn't make it into the prior fix batch. tryFastPathGrant's propagated rate-limit error was returned bare, unlike every other error return in this file — now wrapped with %w (verified this still preserves codes.Unavailable/RateLimitDescription through errors.As, same property reclassifyRateLimitError's errors.Join already relies on). getPermissionProfiles now checks ctx.Err() before taking permissionProfilesMu, since the lock is held across a real HTTP round-trip and an already-cancelled caller shouldn't queue behind it for nothing. Co-Authored-By: Claude Sonnet 5 --- pkg/connector/users.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 410ec665..58bbe303 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -184,6 +184,13 @@ func isCacheablePermissionProfilesError(err error) bool { // real request and forwarding a stale, reused annotation would misrepresent the current // request's pacing to the SDK's self-throttling rate limiter. func (b *userBuilder) getPermissionProfiles(ctx context.Context, syncID string) ([]client.PermissionProfile, annotations.Annotations, error) { + if err := ctx.Err(); err != nil { + // Fail fast on an already-done context instead of queuing behind + // permissionProfilesMu: the lock is held across the real HTTP round-trip below + // (see getPermissionProfiles' doc), so a goroutine whose context is already + // cancelled/expired would otherwise wait out that entire call for nothing. + return nil, nil, err + } b.permissionProfilesMu.Lock() defer b.permissionProfilesMu.Unlock() @@ -418,7 +425,12 @@ func (b *userBuilder) tryFastPathGrant(ctx context.Context, resource *v2.Resourc profiles, ppAnnos, err := b.getPermissionProfiles(ctx, syncID) if err != nil { if isReclassifiedRateLimitError(err) { - return nil, nil, err, true + // Wrapped (not returned bare) so a log line downstream can tell this + // originated in the fast-path grant resolution, not any other DocuSign call — + // %w preserves the gRPC status (codes.Unavailable + RateLimitDescription) + // through errors.As, which status.Code/status.FromError already rely on (see + // reclassifyRateLimitError's identical use of errors.Join for the same reason). + return nil, nil, fmt.Errorf("failed to resolve permission profile for user %s: %w", userID.Resource, err), true } ctxzap.Extract(ctx).Debug("baton-docusign: GetPermissionProfiles failed, falling back to per-user GetUserDetails for this Grants call", zap.String("user_id", userID.Resource), zap.Error(err)) From 54641d61e13fdb8c4cf447a20adfd2adc55b48d0 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 26 Aug 2026 12:06:38 -0300 Subject: [PATCH 25/28] fix: disable permission-profiles per-sync cache when SyncID is empty MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Last of the sergiocorral findings on PR68. attrs.SyncID is only populated when baton-sdk's own version check passes; when it isn't, every Grants() call arrives with syncID == "", which equals permissionProfilesSyncID's zero value — so the cache's mismatch check would never fire again after the first write, silently reverting to the exact process-lifetime memoization bug the SyncID keying exists to prevent. Now an empty syncID disables the cache entirely (every call is real, matching pre-fast-path cost) and logs once so the condition is observable instead of a silent regression. Updated three existing tests that were relying on the empty-SyncID zero value as a stand-in for "one consistent sync" to use a real SyncID instead, so they still exercise the caching behavior they're named for. Co-Authored-By: Claude Sonnet 5 --- pkg/connector/users.go | 35 ++++++++++++++++++++++------- pkg/connector/users_test.go | 45 ++++++++++++++++++++++++++++++++----- 2 files changed, 67 insertions(+), 13 deletions(-) diff --git a/pkg/connector/users.go b/pkg/connector/users.go index 58bbe303..c8a058ce 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -80,14 +80,15 @@ type userBuilder struct { // collapses a same-instant concurrent burst into a single real call while still // expiring well before the SDK's own ~60s per-action retry cadence, so a genuine later // retry always gets a fresh check rather than being blocked by a stale cached failure. - permissionProfilesMu sync.Mutex - permissionProfilesSyncID string - permissionProfilesCached bool - permissionProfiles []client.PermissionProfile - permissionProfilesErr error - permissionProfilesTransientFails int - permissionProfilesRateLimitedUntil time.Time - permissionProfilesRateLimitedErr error + permissionProfilesMu sync.Mutex + permissionProfilesSyncID string + permissionProfilesCached bool + permissionProfiles []client.PermissionProfile + permissionProfilesErr error + permissionProfilesTransientFails int + permissionProfilesRateLimitedUntil time.Time + permissionProfilesRateLimitedErr error + permissionProfilesEmptySyncIDLogged bool } // permissionProfilesTransientFailureThreshold is how many consecutive transient @@ -194,6 +195,24 @@ func (b *userBuilder) getPermissionProfiles(ctx context.Context, syncID string) b.permissionProfilesMu.Lock() defer b.permissionProfilesMu.Unlock() + if syncID == "" { + // baton-sdk only threads a real SyncID through when its own version check + // passes (see the struct doc above); when it doesn't, every call arrives with + // syncID == "" and permissionProfilesSyncID's zero value is also "" — so the + // mismatch check below would never fire again after the first cache write, + // silently reverting to the exact process-lifetime memoization bug this + // SyncID-keying was added to fix, just without ever saying so. Refuse to + // memoize at all in that case instead: every call is a real one (the + // pre-fast-path cost), which is correct if slower, and log it once per + // process so the condition is observable rather than a silent regression. + if !b.permissionProfilesEmptySyncIDLogged { + b.permissionProfilesEmptySyncIDLogged = true + ctxzap.Extract(ctx).Debug("baton-docusign: SyncOpAttrs.SyncID is empty, disabling the permission-profiles per-sync cache", + zap.String("effect", "falls back to one real call per Active user instead of one per sync")) + } + return b.client.GetPermissionProfilesFresh(ctx) + } + if b.permissionProfilesCached && b.permissionProfilesSyncID == syncID { return b.permissionProfiles, nil, b.permissionProfilesErr } diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index 19293734..f09c4fa7 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -483,7 +483,7 @@ func TestUserBuilder_Grants_CollapsesRateLimitBurstWithinTTL(t *testing.T) { const attempts = 5 for i := 0; i < attempts; i++ { - _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{SyncID: "sync-1"}) if err == nil { t.Fatalf("call %d: expected Grants to propagate the rate-limit error, got nil", i) } @@ -517,7 +517,7 @@ func TestUserBuilder_Grants_RateLimitTTLExpiryAllowsFreshRetry(t *testing.T) { b := newUserBuilder(c, false) resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") - if _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}); err == nil { + if _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{SyncID: "sync-1"}); err == nil { t.Fatal("expected the first call to propagate the rate-limit error, got nil") } if got := atomic.LoadInt32(permissionProfilesCalls); got != 1 { @@ -528,7 +528,7 @@ func TestUserBuilder_Grants_RateLimitTTLExpiryAllowsFreshRetry(t *testing.T) { b.permissionProfilesRateLimitedUntil = time.Now().Add(-time.Second) b.permissionProfilesMu.Unlock() - if _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}); err == nil { + if _, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{SyncID: "sync-1"}); err == nil { t.Fatal("expected the post-TTL call to still propagate the rate-limit error, got nil") } if got := atomic.LoadInt32(permissionProfilesCalls); got != 2 { @@ -567,6 +567,41 @@ func TestUserBuilder_Grants_DoesNotMemoizeServiceUnavailableFailure(t *testing.T } } +// TestUserBuilder_Grants_EmptySyncIDDisablesCache is a regression test: SyncOpAttrs.SyncID +// is only threaded through when baton-sdk's own version check passes (see the +// memoization fields' doc on userBuilder). If it doesn't, every Grants() call arrives with +// syncID == "", and permissionProfilesSyncID's zero value is also "" — so, without an +// explicit guard, the cache's own mismatch check would never distinguish sync boundaries +// again after the first write, silently reverting to the exact process-lifetime +// memoization bug this SyncID-keying exists to fix. Even with a normally-cacheable +// (successful) response, every call with syncID == "" must still reach the real endpoint. +func TestUserBuilder_Grants_EmptySyncIDDisablesCache(t *testing.T) { + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + _ = json.NewEncoder(w).Encode(client.PermissionProfilesResponse{ + PermissionProfiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + }) + }) + + b := newUserBuilder(c, false) + resource := userResourceWithProfile(t, "user-1", userStatusActive, "DocuSign Admin") + + for i := 0; i < 3; i++ { + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("call %d: Grants: %v", i, err) + } + if len(grants) != 1 || grants[0].Entitlement.Resource.Id.Resource != "pp-1" { + t.Errorf("call %d: expected the fast path to resolve pp-1, got %+v", i, grants) + } + } + + if got := atomic.LoadInt32(permissionProfilesCalls); got != 3 { + t.Errorf("expected GetPermissionProfiles to be called on every Grants call (3 calls) when SyncID is empty, got %d — the per-sync cache must be disabled, not silently keyed on \"\"", got) + } +} + // TestUserBuilder_Grants_BoundsTransientFailureRetries is a regression test for the // worst case of leaving transient failures uncached: without a cap, a *sustained* // outage (not just a blip) would cost every Active user in the sync two calls (the @@ -586,7 +621,7 @@ func TestUserBuilder_Grants_BoundsTransientFailureRetries(t *testing.T) { const totalUsers = permissionProfilesTransientFailureThreshold + 2 for i := 0; i < totalUsers; i++ { - grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{SyncID: "sync-1"}) if err != nil { t.Fatalf("call %d: Grants: %v", i, err) } @@ -723,7 +758,7 @@ func TestUserBuilder_Grants_MemoizesPermissionProfilesFailureAcrossUsers(t *test b := newUserBuilder(c, false) for _, userID := range []string{"user-1", "user-2"} { resource := userResourceWithProfile(t, userID, userStatusActive, "DocuSign Admin") - grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{}) + grants, _, err := b.Grants(context.Background(), resource, rs.SyncOpAttrs{SyncID: "sync-1"}) if err != nil { t.Fatalf("Grants(%s): %v", userID, err) } From 452ff3554fd2da45ddeb148c9683da6906be9cb0 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 26 Aug 2026 20:59:04 -0300 Subject: [PATCH 26/28] ci: retrigger checks after actions outage Empty commit to re-run stuck/cancelled GitHub Actions workflows on PR #68. Co-authored-by: Cursor From f56b9fac2c3c226e796296106d381b6eee170787 Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 26 Aug 2026 22:53:06 -0300 Subject: [PATCH 27/28] fix: address PR 68 bot review nits Fix SA5011 in users_test, race-safe updateCalls recording, align ambiguous-profile docs with Debug logging, use first-match in permissionProfileIDByName, and fix stale reclassifyRateLimitError doc. Co-authored-by: Cursor --- pkg/connector/helper.go | 20 +++++---- pkg/connector/permission_profiles.go | 2 +- pkg/connector/permission_profiles_test.go | 52 +++++++++++++++++------ pkg/connector/users_test.go | 1 + 4 files changed, 51 insertions(+), 24 deletions(-) diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 33eba5ce..5cb1912a 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -33,8 +33,8 @@ const ( const userStatusActive = "Active" // isReclassifiedRateLimitError reports whether err represents a genuine rate-limit -// overlimit — either DocuSign's hourly error (pkg/client/helper.go's -// reclassifyHourlyRateLimitError) or a plain HTTP 429 uhttp's own +// overlimit — either DocuSign's hourly/burst error (pkg/client/helper.go's +// reclassifyRateLimitError) or a plain HTTP 429 uhttp's own // WrapErrorsWithRateLimitInfo already classifies this way — identified by a // RateLimitDescription with Status == STATUS_OVERLIMIT specifically, not merely the // presence of a RateLimitDescription at all: uhttp's wrapper.go attaches one to every @@ -95,18 +95,20 @@ func permissionProfileIDByName(profiles []client.PermissionProfile, name string) if len(matches) == 0 { return "", 0 } - return matches[len(matches)-1].PermissionProfileId, len(matches) + // First match — same order as permissionProfilesByName / Revoke. Callers must still + // treat id as meaningful only when matches == 1. + return matches[0].PermissionProfileId, len(matches) } // permissionProfilesByName returns every profile named name (requiring a non-empty ID), // in the API's own response order. Unlike permissionProfileIDByName — which deliberately // makes an ambiguous match (2+) indistinguishable from "pick one, ID doesn't matter which" -// by leaving its returned id meaningless in that case — this variant exists for the one -// caller (permissionProfilesBuilder.Revoke) that needs to actually resolve an ambiguous -// name to a specific profile: DocuSign doesn't enforce unique profile names, and prior to -// the ambiguous-is-an-error behavior added elsewhere in this codebase, Revoke took the -// first match and succeeded silently. Revoke restores that first-match behavior (loudly, -// via a Warn log) using matches[0] here, while tryFastPathGrant's name-based grant +// by leaving its returned id meaningful only when matches == 1 — this variant exists for +// the one caller (permissionProfilesBuilder.Revoke) that needs to actually resolve an +// ambiguous name to a specific profile: DocuSign doesn't enforce unique profile names, and +// prior to the ambiguous-is-an-error behavior added elsewhere in this codebase, Revoke took +// the first match and succeeded silently. Revoke restores that first-match behavior +// (logged at Debug) using matches[0] here, while tryFastPathGrant's name-based grant // resolution keeps treating ambiguous as not-found via permissionProfileIDByName — a wrong // silent guess there is a wrong grant, which is worse than falling back. func permissionProfilesByName(profiles []client.PermissionProfile, name string) []client.PermissionProfile { diff --git a/pkg/connector/permission_profiles.go b/pkg/connector/permission_profiles.go index fc74cb79..73a40ddc 100644 --- a/pkg/connector/permission_profiles.go +++ b/pkg/connector/permission_profiles.go @@ -125,7 +125,7 @@ func (p *permissionProfilesBuilder) Revoke(ctx context.Context, grantObj *v2.Gra // silently emitting a wrong grant, Revoke restores its pre-existing behavior of taking // the first match (same order the API returned them) so that an account with a // duplicate-named default profile can still have permission-profile grants revoked at - // all — just loudly, via the Warn below, instead of silently. See + // all — logged at Debug rather than silently guessing with no trace. See // permissionProfilesByName's doc for why this uses a different helper than the // ambiguous-is-not-found path. matchingProfiles := permissionProfilesByName(permissionProfiles, defaultPermissionProfileName) diff --git a/pkg/connector/permission_profiles_test.go b/pkg/connector/permission_profiles_test.go index 484fda7a..1fc69f6f 100644 --- a/pkg/connector/permission_profiles_test.go +++ b/pkg/connector/permission_profiles_test.go @@ -7,6 +7,7 @@ import ( "net/http/httptest" "net/url" "strings" + "sync" "testing" "github.com/conductorone/baton-docusign/pkg/client" @@ -20,16 +21,37 @@ import ( "golang.org/x/oauth2" ) +// recordedUpdates collects UpdateUserProfileRequest bodies from the mock PUT handler. +// Appends happen on the httptest goroutine; Snapshot is called from the test goroutine — +// the mutex gives a happens-before edge so go test -race stays clean. +type recordedUpdates struct { + mu sync.Mutex + reqs []client.UpdateUserProfileRequest +} + +func (r *recordedUpdates) append(req client.UpdateUserProfileRequest) { + r.mu.Lock() + defer r.mu.Unlock() + r.reqs = append(r.reqs, req) +} + +func (r *recordedUpdates) Snapshot() []client.UpdateUserProfileRequest { + r.mu.Lock() + defer r.mu.Unlock() + out := make([]client.UpdateUserProfileRequest, len(r.reqs)) + copy(out, r.reqs) + return out +} + // newPermissionProfilesTestClient wires a *client.Client to a mock server handling // /oauth/userinfo, GET permission_profiles, GET users/{id}, and PUT users/{id}/profile — -// everything permissionProfilesBuilder.Revoke needs. updateCalls, if non-nil, is -// incremented (with the request body recorded) on every PUT users/{id}/profile call, so -// tests can assert which profile ID Revoke actually assigned. +// everything permissionProfilesBuilder.Revoke needs. updateCalls, if non-nil, records +// every PUT users/{id}/profile body so tests can assert which profile ID Revoke assigned. func newPermissionProfilesTestClient( t *testing.T, profiles []client.PermissionProfile, userDetails map[string]client.UserDetail, - updateCalls *[]client.UpdateUserProfileRequest, + updateCalls *recordedUpdates, ) *client.Client { t.Helper() mockServer := httptest.NewServer(nil) @@ -52,7 +74,7 @@ func newPermissionProfilesTestClient( var req client.UpdateUserProfileRequest _ = json.NewDecoder(r.Body).Decode(&req) if updateCalls != nil { - *updateCalls = append(*updateCalls, req) + updateCalls.append(req) } _ = json.NewEncoder(w).Encode(map[string]string{}) case r.Method == http.MethodGet: @@ -107,7 +129,7 @@ func TestPermissionProfilesBuilder_Revoke_AmbiguousDefaultNameUsesFirstMatch(t * "user-1": {UserID: "user-1", PermissionProfileName: "DocuSign Admin", PermissionProfileID: "pp-admin"}, } - var updateCalls []client.UpdateUserProfileRequest + var updateCalls recordedUpdates c := newPermissionProfilesTestClient(t, profiles, userDetails, &updateCalls) b := newPermissionProfilesBuilder(c) @@ -120,10 +142,11 @@ func TestPermissionProfilesBuilder_Revoke_AmbiguousDefaultNameUsesFirstMatch(t * } _ = annos - if len(updateCalls) != 1 { - t.Fatalf("expected exactly 1 UpdateUserProfile call, got %d", len(updateCalls)) + gotCalls := updateCalls.Snapshot() + if len(gotCalls) != 1 { + t.Fatalf("expected exactly 1 UpdateUserProfile call, got %d", len(gotCalls)) } - if got := updateCalls[0].UserDetails.PermissionProfileId; got != "pp-viewer-first" { + if got := gotCalls[0].UserDetails.PermissionProfileId; got != "pp-viewer-first" { t.Errorf("expected Revoke to assign the first-listed match %q, got %q", "pp-viewer-first", got) } @@ -147,7 +170,7 @@ func TestPermissionProfilesBuilder_Revoke_UnambiguousDefaultNameNoWarning(t *tes "user-1": {UserID: "user-1", PermissionProfileName: "DocuSign Admin", PermissionProfileID: "pp-admin"}, } - var updateCalls []client.UpdateUserProfileRequest + var updateCalls recordedUpdates c := newPermissionProfilesTestClient(t, profiles, userDetails, &updateCalls) b := newPermissionProfilesBuilder(c) @@ -158,11 +181,12 @@ func TestPermissionProfilesBuilder_Revoke_UnambiguousDefaultNameNoWarning(t *tes t.Fatalf("Revoke: %v", err) } - if len(updateCalls) != 1 || updateCalls[0].UserDetails.PermissionProfileId != "pp-viewer" { - t.Fatalf("expected Revoke to assign pp-viewer, got %+v", updateCalls) + gotCalls := updateCalls.Snapshot() + if len(gotCalls) != 1 || gotCalls[0].UserDetails.PermissionProfileId != "pp-viewer" { + t.Fatalf("expected Revoke to assign pp-viewer, got %+v", gotCalls) } - if logs.Len() != 0 { - t.Errorf("expected no ambiguous-match logs for an unambiguous default profile name, got %d", logs.Len()) + if logs.FilterMessageSnippet("ambiguous").Len() != 0 { + t.Errorf("expected no ambiguous-match logs for an unambiguous default profile name, got %d", logs.FilterMessageSnippet("ambiguous").Len()) } } diff --git a/pkg/connector/users_test.go b/pkg/connector/users_test.go index f09c4fa7..b434011d 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -332,6 +332,7 @@ func TestUserBuilder_Grants_FastPath_ActiveUserWithKnownProfile(t *testing.T) { } if res == nil { t.Fatal("expected non-nil SyncOpResults") + return } if len(grants) != 1 { t.Fatalf("expected exactly 1 grant, got %d: %+v", len(grants), grants) From 413cd996d586080c00ce2ad883da9db6eddc0eba Mon Sep 17 00:00:00 2001 From: Felipe Lucero Date: Wed, 26 Aug 2026 22:59:42 -0300 Subject: [PATCH 28/28] fix: rename revoke test to match Debug ambiguity assertion Rename UnambiguousDefaultNameNoWarning to NoAmbiguityLog so the test name reflects FilterMessageSnippet("ambiguous") rather than Warn level. Co-authored-by: Cursor --- pkg/connector/permission_profiles_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/connector/permission_profiles_test.go b/pkg/connector/permission_profiles_test.go index 1fc69f6f..dd4d4ef0 100644 --- a/pkg/connector/permission_profiles_test.go +++ b/pkg/connector/permission_profiles_test.go @@ -156,10 +156,10 @@ func TestPermissionProfilesBuilder_Revoke_AmbiguousDefaultNameUsesFirstMatch(t * } } -// TestPermissionProfilesBuilder_Revoke_UnambiguousDefaultNameNoWarning is the control -// case for the test above: a single unambiguous default profile must revoke exactly as -// before, with no ambiguous-match log at all. -func TestPermissionProfilesBuilder_Revoke_UnambiguousDefaultNameNoWarning(t *testing.T) { +// TestPermissionProfilesBuilder_Revoke_NoAmbiguityLog is the control case for the test +// above: a single unambiguous default profile must revoke exactly as before, with no +// ambiguous-match Debug log. +func TestPermissionProfilesBuilder_Revoke_NoAmbiguityLog(t *testing.T) { ctx := context.Background() profiles := []client.PermissionProfile{