diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index b3ecae4a..f08de8f8 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -69,6 +69,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 @@ -98,6 +104,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 @@ -127,3 +136,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" diff --git a/pkg/client/client.go b/pkg/client/client.go index d027bc7f..406d9b96 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -645,13 +645,51 @@ 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). +// +// 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. // // Returns: all permission profiles, annotations, error. 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 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) +} + +func (c *Client) getPermissionProfiles(ctx context.Context, noCache bool) ([]PermissionProfile, annotations.Annotations, error) { if err := c.ensureInitialized(ctx); err != nil { return nil, nil, err } @@ -665,7 +703,11 @@ func (c *Client) GetPermissionProfiles(ctx context.Context) ([]PermissionProfile permissionProfilesURL = baseURL.ResolveReference(permissionProfilesURL) - _, annos, err := c.doRequest(ctx, http.MethodGet, permissionProfilesURL, nil, &permissionProfilesResponse) + 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 } @@ -801,6 +843,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 { @@ -816,6 +859,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 { diff --git a/pkg/client/client_test.go b/pkg/client/client_test.go index 9ef328e4..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" @@ -273,3 +274,77 @@ 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. 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() + var calls atomic.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 { + 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.Add(1) + _ = 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 got := calls.Load(); got != 1 { + t.Errorf("expected 1 real request across 2 GetPermissionProfiles calls (cache should serve the second), got %d", got) + } + }) + + 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 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 4e24a45b..2ed5c68d 100644 --- a/pkg/client/helper.go +++ b/pkg/client/helper.go @@ -1,20 +1,211 @@ package client import ( + "context" "encoding/base64" "encoding/json" + "errors" "fmt" "net/http" "net/url" "strings" + "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" + "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" ) const DefaultPageSize = 100 +// docusignHourlyRateLimitErrorCode is the eSignature API's JSON error-body errorCode for +// 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. +// +// 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 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 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 + +// 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 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 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 { + 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(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. + 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 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, case-insensitively on +// both (see docusignHourlyRateLimitErrorCode's doc for why). +func isHourlyAPIInvocationLimitError(er *ErrorResponse) bool { + 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 // 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. @@ -45,6 +236,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, 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 { @@ -53,6 +250,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 := reclassifyRateLimitError(req.Context(), 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..6f39ac4a --- /dev/null +++ b/pkg/client/helper_test.go @@ -0,0 +1,336 @@ +package client + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "testing" + "time" + + 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" +) + +// 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. 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) { + // 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.", + } + + 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()) + } + if desc.GetRemaining() != 0 { + t.Errorf("expected Remaining to be 0 (matches OVERLIMIT, no header data is ever read), got %d", desc.GetRemaining()) + } + 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) { + errTarget := &ErrorResponse{ErrorCode: "USER_LACKS_PERMISSIONS"} + + if got := reclassifyRateLimitError(context.Background(), errTarget, origErr); got != nil { + t.Errorf("expected nil for an unrelated errorCode, got %v", got) + } + }) + + 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 := reclassifyRateLimitError(context.Background(), 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("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 := 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 -> 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() + + 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()) + } +} diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index bbc4ea02..5cb1912a 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -18,12 +18,47 @@ 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" - 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" — +// 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" + +// isReclassifiedRateLimitError reports whether err represents a genuine rate-limit +// 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 +// 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{} @@ -49,6 +84,43 @@ func clmIDFromHref(href string) string { return client.IDFromHref(href) } +// 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. 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) { + matches := permissionProfilesByName(profiles, name) + if len(matches) == 0 { + return "", 0 + } + // 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 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 { + var matches []client.PermissionProfile + for _, p := range profiles { + if p.PermissionProfileName == name && p.PermissionProfileId != "" { + matches = append(matches, p) + } + } + return matches +} + // clmHrefWithID rebuilds sampleHref with its trailing ID segment replaced by newID. // Assumes same-collection Hrefs share path shape (only the ID differs) — not verified // against a live tenant. Callers must only pass samples from a single collection; newID diff --git a/pkg/connector/helper_test.go b/pkg/connector/helper_test.go index 49612925..a1065683 100644 --- a/pkg/connector/helper_test.go +++ b/pkg/connector/helper_test.go @@ -4,6 +4,7 @@ import ( "context" "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" ) @@ -60,6 +61,71 @@ func TestClmHrefWithID(t *testing.T) { } } +func TestPermissionProfileIDByName(t *testing.T) { + tests := []struct { + 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", + wantMatches: 1, + }, + { + name: "no match", + profiles: []client.PermissionProfile{ + {PermissionProfileId: "pp-1", PermissionProfileName: "DocuSign Admin"}, + }, + lookup: "Nonexistent", + wantMatches: 0, + }, + { + name: "match with no usable ID doesn't count", + profiles: []client.PermissionProfile{ + {PermissionProfileId: "", PermissionProfileName: "DocuSign Admin"}, + }, + lookup: "DocuSign Admin", + wantMatches: 0, + }, + { + 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", + 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) { + 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) + } + }) + } +} + func TestClmPreferredHref(t *testing.T) { ctx := context.Background() fallbackCalled := false diff --git a/pkg/connector/permission_profiles.go b/pkg/connector/permission_profiles.go index ca8c8184..73a40ddc 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,17 +119,26 @@ 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 == "" { + // 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 — 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) + 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..dd4d4ef0 --- /dev/null +++ b/pkg/connector/permission_profiles_test.go @@ -0,0 +1,209 @@ +package connector + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "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" +) + +// 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, 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 *recordedUpdates, +) *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(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 recordedUpdates + 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 + + gotCalls := updateCalls.Snapshot() + if len(gotCalls) != 1 { + t.Fatalf("expected exactly 1 UpdateUserProfile call, got %d", len(gotCalls)) + } + 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) + } + + 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_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{ + {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 recordedUpdates + 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) + } + + 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.FilterMessageSnippet("ambiguous").Len() != 0 { + t.Errorf("expected no ambiguous-match logs for an unambiguous default profile name, got %d", logs.FilterMessageSnippet("ambiguous").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 fe21ba90..c8a058ce 100644 --- a/pkg/connector/users.go +++ b/pkg/connector/users.go @@ -2,7 +2,10 @@ package connector import ( "context" + "errors" "fmt" + "sync" + "time" "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" @@ -10,6 +13,10 @@ 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" + "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" ) @@ -25,6 +32,232 @@ 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 + + // 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 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 + // 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 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. + // + // 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 + permissionProfilesEmptySyncIDLogged bool +} + +// 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 + +// 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/ +// 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.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 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 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. +// +// 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) { + 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() + + 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 + } + if b.permissionProfilesSyncID != syncID { + // A new sync started (or this is the first call ever): the previous sync's + // 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, freshAnnos, err := b.client.GetPermissionProfilesFresh(ctx) + if err != nil && !isCacheablePermissionProfilesError(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, freshAnnos, err + } + } + + b.permissionProfilesCached = true + b.permissionProfiles = profiles + b.permissionProfilesErr = err + return profiles, freshAnnos, err } // ResourceType returns the Baton resource type handled by this builder, @@ -94,24 +327,34 @@ func (b *userBuilder) Entitlements(_ context.Context, _ *v2.Resource, _ rs.SyncO // Grants assigns permissions to users based on their DocuSign settings. // -// This method exists solely to emit the cross-type permission_profile grant -// as a sync optimization (the user detail API call already returns the -// user's permission profile ID, so permission_profiles.go doesn't need a -// second round trip per user). When the customer's sync filter excludes -// permission_profile, the SDK's sync engine skips calling Grants() entirely -// for user resources based on the SkipEntitlementsAndGrants annotation -// ResourceType() attaches in that case, so this method itself no longer -// needs to guard against that case. -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 +// This method exists solely to emit the cross-type permission_profile grant. When the +// customer's sync filter excludes permission_profile, the SDK's sync engine skips +// calling Grants() entirely for user resources based on the SkipEntitlementsAndGrants +// annotation ResourceType() attaches in that case, so this method itself no longer needs +// to guard against that case. +// +// Tries tryFastPathGrant first (an Active user's permission-profile ID or NAME, already +// captured on the resource's profile during List(), resolved without 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 tryFastPathGrant's own doc). +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, attrs.SyncID); 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) 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) } @@ -122,17 +365,102 @@ 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{ + 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. 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 +// (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 — 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. +// +// 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) + + userStatus, ok := rs.GetProfileStringValue(profile, profileFieldStatus) + if !ok || userStatus != userStatusActive { + return nil, nil, nil, false } - newGrant := grant.NewGrant(permissionProfileResource, permissionProfileAssignedTag, userID) - grants = append(grants, newGrant) + name, ok := rs.GetProfileStringValue(profile, profileFieldPermission) + if !ok || name == "" { + return nil, nil, nil, false + } + + profiles, ppAnnos, err := b.getPermissionProfiles(ctx, syncID) + if err != nil { + if isReclassifiedRateLimitError(err) { + // 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)) + return nil, nil, nil, false + } - return grants, &rs.SyncOpResults{Annotations: annos}, nil + id, matches := permissionProfileIDByName(profiles, name) + if matches != 1 { + return nil, nil, nil, false + } + return newPermissionProfileGrant(id, userID), ppAnnos, nil, true } // CreateAccountCapabilityDetails declares support for account provisioning without a password. @@ -159,12 +487,12 @@ func (b *userBuilder) CreateAccount( pMap := accountInfo.Profile.AsMap() annos := annotations.Annotations{} - email, ok := pMap["email"].(string) + email, ok := pMap[profileFieldEmail].(string) if !ok || email == "" { return nil, nil, nil, fmt.Errorf("email is required") } - username, ok := pMap["username"].(string) + username, ok := pMap[profileFieldUsername].(string) if !ok || username == "" { return nil, nil, nil, fmt.Errorf("username is required") } @@ -258,7 +586,7 @@ func newUserBuilder(client *client.Client, skipPermissionProfileResourceType boo 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 @@ -269,11 +597,11 @@ func parseIntoUserResource(user *client.User) (*v2.Resource, error) { } profile := map[string]any{ - "userName": user.UserName, - "email": 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 index e2de4719..b434011d 100644 --- a/pkg/connector/users_test.go +++ b/pkg/connector/users_test.go @@ -8,19 +8,23 @@ import ( "net/url" "sync/atomic" "testing" + "time" "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "github.com/conductorone/baton-sdk/pkg/annotations" - "github.com/conductorone/baton-sdk/pkg/types/resource" + 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, // mirroring the helper in pkg/client/client_test.go so requests issued against // the real DocuSign hosts (oauth userinfo, account base URI) land on the mock -// server instead. +// server instead. base is injectable (rather than hardcoding +// http.DefaultTransport) so callers can wrap/observe the underlying transport. type rewriteTransport struct { target *url.URL base http.RoundTripper @@ -113,7 +117,7 @@ func TestUserBuilder_Grants_SyncPermissionProfilesEnabled(t *testing.T) { b := newUserBuilder(c, false) // permission_profile in scope - grants, _, err := b.Grants(ctx, testUserResource(), resource.SyncOpAttrs{}) + grants, _, err := b.Grants(ctx, testUserResource(), rs.SyncOpAttrs{}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -161,7 +165,7 @@ func TestUserBuilder_Grants_UnconditionalRegardlessOfSyncPermissionProfiles(t *t // the grant, proving the old in-Grants() guard is gone. b := newUserBuilder(c, true) - grants, _, err := b.Grants(ctx, testUserResource(), resource.SyncOpAttrs{}) + grants, _, err := b.Grants(ctx, testUserResource(), rs.SyncOpAttrs{}) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -230,3 +234,622 @@ func TestUserBuilder_ResourceType_SyncPermissionProfilesDisabled(t *testing.T) { len(userResourceType.Annotations)) } } + +// 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 +) + +// newUsersTestClient 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. 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) + + 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": + 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: + // 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, base: http.DefaultTransport}}) + 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, permissionProfilesOK) // no user-details fixtures — a fallback call here would 404 and fail the test + b := newUserBuilder(c, false) + 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") + return + } + 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) + } + // 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) { + // A non-active user must go through GetUserDetails, exactly like before this fast + // 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{ + "user-1": {UserID: "user-1", PermissionProfileID: ""}, // matches "non-active users have no PP" + }, permissionProfilesOK) + b := newUserBuilder(c, false) + 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"}, + }, permissionProfilesOK) + b := newUserBuilder(c, false) + 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) + } +} + +// 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 + // 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"}, + }, permissionProfilesRateLimit) + b := newUserBuilder(c, false) + 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) + } +} + +// 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) + 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(&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) + } + }) + + 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), &calls +} + +// 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{ + 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") + + const attempts = 5 + for i := 0; i < attempts; i++ { + _, _, 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) + } + 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 != 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{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 { + 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{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 { + t.Errorf("expected a second real GetPermissionProfiles call once the TTL expired, got %d real calls total", got) + } +} + +// 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) { + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) + }) + + 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_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 +// 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) { + c, permissionProfilesCalls := newCountingPermissionProfilesClient(t, func(w http.ResponseWriter) { + w.WriteHeader(http.StatusServiceUnavailable) + _ = json.NewEncoder(w).Encode(client.ErrorResponse{ErrorMessage: "service unavailable"}) + }) + + 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{SyncID: "sync-1"}) + 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 +// 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) { + 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") + + // 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 — 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) + } + 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 +// 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, false) + 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_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 +// 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) { + 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.", + }) + }) + + 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{SyncID: "sync-1"}) + 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_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 +// 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, false) + 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) + } +} + +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"}, + }, permissionProfilesOK) + b := newUserBuilder(c, false) + 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) + } +}