diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 32a2f22f..c6180fe1 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -10,11 +10,23 @@ on: # real state (one run's mid-cycle Grant/Revoke can make another run's "should be zero # grants after Revoke" assertion fail). A per-job concurrency block only protects a # *running* job from cancellation — GitHub Actions still cancels a *pending* job in the -# same group when a newer one queues. Declaring it once here makes the whole +# same group when a newer one queues, so two overlapping workflow runs could cancel each +# other's pending jobs mid-chain. Declaring it once here instead makes the whole # needs-chained run (all three jobs) queue/cancel as one unit against the shared group. concurrency: group: docusign-demo-account cancel-in-progress: false +env: + # This CI account has no CLM subscription, and (unlike a C1-hosted sync) nothing here + # filters resource types by OptInRequired — every registered resource type is attempted + # by default. CLM builders now fail the whole sync rather than skip gracefully when CLM + # isn't available (see pkg/connector/clm_roles.go), so the 5 clm_* types must be + # excluded here explicitly to test the ones the three test-* jobs below care about. + # This is an allowlist, not a CLM-only exclusion: if you register a new non-CLM + # resource type in pkg/connector/connector.go, add it here too — otherwise it silently + # gets zero CI sync-test coverage. Declared once at the workflow level (all three jobs + # inherit it) so there's no per-job copy to keep in sync. + BATON_SYNC_RESOURCE_TYPES: user,group,permission_profile,signing_group jobs: test-groups: runs-on: ubuntu-latest diff --git a/README.md b/README.md index 196ab938..52b857af 100644 --- a/README.md +++ b/README.md @@ -101,25 +101,43 @@ Copy the `code` parameter value and paste it when prompted. Save the refresh tok DocuSign CLM (Contract Lifecycle Management) is a separate DocuSign product from eSignature, with its own API and a separate production subscription. CLM members, roles, -groups, folders, folder security, and permission sets sync alongside the standard -eSignature resources, with no config flag to enable — accounts that don't have CLM simply -sync no CLM resources. +groups, folders, folder security, and permission sets are opt-in: they don't sync by +default, and a customer must explicitly enable each CLM resource type in C1's sync +configuration. Requirements: - Your DocuSign account must have a CLM production subscription. - **Demo environment or self-hosted with your own DocuSign app**: no extra setup — the - connector requests the additional CLM OAuth scopes (`spring_read`/`spring_write`) - automatically. + connector requests the additional CLM OAuth scopes (`impersonation`/`spring_read`/ + `spring_write`) automatically. - **Cloud-hosted production (ConductorOne's managed OAuth app)**: the managed app must also be granted the CLM API scopes on ConductorOne's platform side before any CLM data will sync. Contact ConductorOne if no CLM data appears in this mode. - -The 5 CLM resource types are always registered and visible to C1 — this avoids a C1 sync -engine treating CLM resources as deleted if they stop appearing (see -[CHANGE_TYPES.md](CHANGE_TYPES.md) if you're touching this). Without the CLM OAuth scopes -(or without a CLM subscription on the account), each CLM resource type's sync is skipped -gracefully rather than erroring the whole sync. +- **Already-connected install**: an existing OAuth connection keeps its old consent on + refresh, so re-run `--configure` (re-consent) once to pick up the new `impersonation` + scope. + +The 5 CLM resource types are always registered and visible to C1, but each carries +`OptInRequired` — C1 excludes them from a customer's sync by default, and they only run +once a customer explicitly opts in (see [CHANGE_TYPES.md](CHANGE_TYPES.md) if you're +touching this). C1's opt-in toggle does not validate the underlying DocuSign account +first, so a customer can enable CLM sync without actually having the subscription or +scopes above. If that happens, the sync fails loudly rather than silently succeeding +with zero CLM resources — an account that opted in but can't reach CLM is treated as a +misconfiguration to fix (disable the resource type, or activate the CLM feature), not an +expected state to tolerate. + +`OptInRequired` is enforced by ConductorOne's platform, not by the connector or baton-sdk +itself — a self-hosted connector running in service mode still receives the platform's +resource-type filter, but running `baton-docusign` directly as a one-shot CLI sync (the +quickstarts below, with no service/task involved at all) attempts all 5 CLM resource +types by default, with no opt-in gate at all. If that account doesn't have a CLM +subscription, the sync now fails instead of skipping CLM gracefully. Pass +`--sync-resource-types` (or `BATON_SYNC_RESOURCE_TYPES`, comma-separated) with the +resource type IDs you actually want (e.g. `user,group,permission_profile`) to exclude +`clm_member,clm_role,clm_group,clm_permission_set,clm_folder` on an eSignature-only +account run this way. CLM permission sets sync for visibility only — DocuSign's CLM API has no endpoint to assign or unassign a permission set, so they cannot be granted or revoked through this diff --git a/docs/connector.mdx b/docs/connector.mdx index f9568456..563186a5 100644 --- a/docs/connector.mdx +++ b/docs/connector.mdx @@ -26,9 +26,9 @@ The Docusign connector supports [automatic account provisioning and deprovisioni Every Docusign account must be assigned at least one permission profile. If all other permission profiles are revoked, the account will be automatically assigned the **DocuSign Viewer** profile, which cannot be revoked. -*By default, signing groups are not synced. Enable the **Include Signing Groups** setting to sync signing groups, if your account has the feature enabled. +*By default, signing groups are not synced. Enable the **Include Signing Groups** setting to sync signing groups. Once enabled, your account must actually have the signing groups feature — ConductorOne doesn't validate this before letting you turn the setting on, so enabling it without the feature will fail the sync rather than silently sync no signing groups. -**DocuSign CLM (Contract Lifecycle Management) is a separate, separately-licensed DocuSign product. CLM resources sync automatically if the account has a DocuSign CLM production subscription and the credential has been granted the OAuth scopes CLM needs; accounts without CLM simply sync no CLM resources. CLM permission sets sync for visibility only; DocuSign's CLM API has no endpoint to assign or unassign one. +**DocuSign CLM (Contract Lifecycle Management) is a separate, separately-licensed DocuSign product. CLM resources are opt-in — enable each CLM resource type in your sync configuration to turn them on. Once enabled, your DocuSign account must have a CLM production subscription and the credential must have been granted the OAuth scopes CLM needs; enabling a CLM resource type without them will fail the sync rather than silently sync no data, since ConductorOne doesn't validate the underlying subscription before letting you opt in. CLM permission sets sync for visibility only; DocuSign's CLM API has no endpoint to assign or unassign one. If you use **OAuth Authentication** (the default, managed method), syncing CLM data requires ConductorOne's managed OAuth app to be granted the CLM API scopes on the platform side. If CLM data doesn't appear after setup, contact ConductorOne. This doesn't apply to **Custom App (Demo Environment)**, where the connector requests the CLM scopes directly using your own DocuSign app credentials. diff --git a/docs/doc-info.md b/docs/doc-info.md index d9048da8..8683499f 100644 --- a/docs/doc-info.md +++ b/docs/doc-info.md @@ -45,7 +45,7 @@ **Important Note about CLM:** - - CLM (Contract Lifecycle Management) is a separate, separately-licensed DocuSign product with its own API. There is no config flag to enable it: CLM resources sync whenever the account and credential can reach the CLM API, and accounts without CLM sync no CLM resources. + - CLM (Contract Lifecycle Management) is a separate, separately-licensed DocuSign product with its own API. The 5 CLM resource types carry `OptInRequired` and don't sync until a customer explicitly enables them in C1's sync configuration; C1's opt-in toggle doesn't validate the underlying subscription/scopes first, so an account that opts in but can't reach CLM fails the sync loudly rather than silently syncing zero CLM resources. - Requires a DocuSign CLM production subscription. - When using ConductorOne's managed OAuth app (the default cloud-hosted authentication method), CLM also requires that managed app to be granted the CLM API scope on ConductorOne's platform side — this is outside the connector's own configuration. Self-hosted or demo-environment setups using a customer-supplied DocuSign app do not have this extra requirement. - CLM permission sets sync for visibility only; DocuSign's CLM API has no endpoint to assign or unassign one, so they cannot be granted or revoked. @@ -170,8 +170,9 @@ DocuSign Signing Groups are an optional feature. To sync signing groups: DocuSign CLM is a separate, separately-licensed DocuSign product. To sync CLM data: 1. Confirm your DocuSign account has a CLM production subscription. -2. Confirm the credential has been granted the CLM OAuth scopes (`spring_read`/`spring_write`). -3. The connector then syncs CLM Members, Roles, Groups, Folders, Folder Security, and Permission Sets automatically — there is no flag to set. +2. Confirm the credential has been granted the CLM OAuth scopes (`impersonation`/`spring_read`/`spring_write`). +3. The connector then syncs CLM Members, Roles, Groups, Folders, Folder Security, and Permission Sets once a customer explicitly enables each CLM resource type in C1's sync configuration (see the CLM note above — these types carry `OptInRequired`). +4. An already-connected credential keeps its old consent on refresh (refresh tokens don't resend scopes) — re-run with `--configure` once to re-consent and pick up the new `impersonation` scope. If running against ConductorOne's managed OAuth app (the default cloud-hosted production authentication method), the managed app also needs the CLM API scopes diff --git a/pkg/client/client.go b/pkg/client/client.go index ed033d7d..d027bc7f 100644 --- a/pkg/client/client.go +++ b/pkg/client/client.go @@ -313,6 +313,16 @@ func (c *Client) ensureInitialized(ctx context.Context) error { return nil } +// EnsureReady exposes the base eSignature-credential check every other client method +// runs internally before its real request, for callers with no eSignature endpoint of +// their own that still need to detect whether the base connection/credentials are +// valid — namely Connector.Validate() (see pkg/connector/connector.go), which runs +// this once, up front, before any resource type's List() executes. Memoized after the +// first successful call, same as every other method — see ensureInitialized. +func (c *Client) EnsureReady(ctx context.Context) error { + return c.ensureInitialized(ctx) +} + // buildClientURL safely reads baseURI and accountId to build a URL. func (c *Client) buildClientURL(path string, params ...any) (*url.URL, error) { c.mutex.RLock() diff --git a/pkg/client/clm_client.go b/pkg/client/clm_client.go index f1f94ea3..ca7fa846 100644 --- a/pkg/client/clm_client.go +++ b/pkg/client/clm_client.go @@ -184,12 +184,6 @@ func (c *Client) ensureClmInitialized(ctx context.Context) error { for k := range raw { keys = append(keys, k) } - // codes.FailedPrecondition (not a bare error, which status.Code() would read as - // codes.Unknown): a non-CLM account's discovery response plausibly has a - // different shape entirely (e.g. a bare account object with none of the - // candidate fields), so isOptInFeatureUnavailableError needs a recognizable - // code to tolerate this specific failure the same way it tolerates 401/403 — - // see that function's doc in helper.go. return status.Errorf(codes.FailedPrecondition, "baton-docusign: CLM account discovery response at %s did not contain a recognized "+ "base-URL field (checked %v); response contained these fields instead: %v", discoveryURL, clmBaseURLCandidateFields, keys) } @@ -199,6 +193,16 @@ func (c *Client) ensureClmInitialized(ctx context.Context) error { return nil } +// EnsureClmReady exposes the CLM-readiness check every other CLM client method runs +// internally before its real request, for callers with no CLM endpoint of their own +// that still need to detect CLM availability — namely Connector.Validate() (see +// pkg/connector/connector.go), which runs this once, up front, before any CLM +// builder's List() executes. Memoized after the first successful call, same as every +// other CLM method — see ensureClmInitialized. +func (c *Client) EnsureClmReady(ctx context.Context) error { + return c.ensureClmReady(ctx) +} + // clmExtractBaseURLField scans a CLM account discovery response for the first // recognized base-URL field, in clmBaseURLCandidateFields priority order. Split out // from ensureClmInitialized so this defensive-fallback logic can be unit tested diff --git a/pkg/connector/clm_folders.go b/pkg/connector/clm_folders.go index 513ba142..850f0dd1 100644 --- a/pkg/connector/clm_folders.go +++ b/pkg/connector/clm_folders.go @@ -18,6 +18,14 @@ import ( var _ connectorbuilder.StaticEntitlementSyncerV2 = (*clmFolderBuilder)(nil) +// The three folder-security principal kinds, as passed to logSkippedFolderSecurityEntry +// and (via the principal_kind field) queryable in logs. +const ( + clmFolderPrincipalKindGroup = "group" + clmFolderPrincipalKindRole = "role" + clmFolderPrincipalKindUser = "user" +) + // The 5 grantable Baton entitlement slugs for CLM folder security, in ascending order // of access. const ( @@ -74,10 +82,6 @@ func (f *clmFolderBuilder) List(ctx context.Context, _ *v2.ResourceId, attr rs.S PageToken: pageToken, }) if err != nil { - if attr.PageToken.Token == "" && isOptInFeatureUnavailableError(err) { - ctxzap.Extract(ctx).Info("baton-docusign: CLM is not available for this account or token, skipping clm_folder sync", zap.Error(err)) - return nil, &rs.SyncOpResults{}, nil - } return nil, nil, err } @@ -141,6 +145,8 @@ func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resour for _, entry := range folder.Security.Groups.Items { slug, ok := clmSlugForAccessType(entry.AccessType) if !ok { + logSkippedFolderSecurityEntry(ctx, clmFolderPrincipalKindGroup, entry.AccessType, + zap.String("folder_id", folderResource.Id.Resource), zap.String("group_href", entry.Href)) continue } principalID := &v2.ResourceId{ResourceType: clmGroupResourceType.Id, Resource: clmIDFromHref(entry.Href)} @@ -156,12 +162,17 @@ func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resour for _, entry := range folder.Security.Roles.Items { slug, ok := clmSlugForAccessType(entry.AccessType) if !ok { + logSkippedFolderSecurityEntry(ctx, clmFolderPrincipalKindRole, entry.AccessType, + zap.String("folder_id", folderResource.Id.Resource), zap.String("role", entry.Item)) continue } if !clmIsKnownRole(entry.Item) { // clm_role is a fixed, hardcoded 5-role list (clmRoleBuilder.List) — a role // name outside that set has no synced principal to grant against. Skip // rather than emit a grant to a dangling/unsynced resource. + ctxzap.Extract(ctx).Debug("baton-docusign: skipping CLM folder role-security entry for an unrecognized role", + zap.String("folder_id", folderResource.Id.Resource), zap.String("role", entry.Item), zap.String("access_type", entry.AccessType), + zap.String("principal_kind", clmFolderPrincipalKindRole)) continue } principalID := &v2.ResourceId{ResourceType: clmRoleResourceType.Id, Resource: entry.Item} @@ -171,6 +182,8 @@ func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resour for _, entry := range folder.Security.Users.Items { slug, ok := clmSlugForAccessType(entry.AccessType) if !ok { + logSkippedFolderSecurityEntry(ctx, clmFolderPrincipalKindUser, entry.AccessType, + zap.String("folder_id", folderResource.Id.Resource), zap.String("member_href", entry.Href)) continue } principalID := &v2.ResourceId{ResourceType: clmMemberResourceType.Id, Resource: clmIDFromHref(entry.Href)} @@ -180,6 +193,35 @@ func (f *clmFolderBuilder) Grants(ctx context.Context, folderResource *v2.Resour return grants, &rs.SyncOpResults{Annotations: annos}, nil } +// logSkippedFolderSecurityEntry logs the one Debug line for a folder-security entry +// whose AccessType didn't map to a grantable tier — shared by the Groups/Roles/Users +// branches of Grants, which differ only in kind ("group"/"role"/"user", carried as a +// field rather than interpolated into the message, so both messages stay constant +// strings — no per-call fmt.Sprintf) and the caller-supplied fields identifying the +// entry. Custom gets its own message, since unlike NoAccess/InheritFromParentFolder +// (clmIsBenignUnmappedAccessType) it's a real, active grant this connector can't +// represent — fully silencing it would hide an actual access-visibility gap. Both +// branches carry access_type so either case is findable by the same structured-log +// query as every other skip line in this file. +func logSkippedFolderSecurityEntry(ctx context.Context, kind, accessType string, fields ...zap.Field) { + if clmIsBenignUnmappedAccessType(accessType) { + // The common steady-state case (NoAccess/InheritFromParentFolder, on every + // folder of every sync) — return before this function's own append/log call. + // The caller's fields are already built by this point regardless. + return + } + fields = append(fields, zap.String("principal_kind", kind), zap.String("access_type", accessType)) + if accessType == client.ClmAccessTypeCustom { + ctxzap.Extract(ctx).Debug( + "baton-docusign: skipping CLM folder security entry with an unrepresentable Custom AccessType — a real, active grant C1 won't see", + fields...) + return + } + ctxzap.Extract(ctx).Debug( + "baton-docusign: skipping CLM folder security entry with an unmapped AccessType", + fields...) +} + // Grant sets a folder-security entry for the principal at the entitlement's tier. // Read-before-write: fetches the folder's current complete security state, modifies // only the one entry belonging to this principal (in whichever of Groups/Roles/Users @@ -433,6 +475,28 @@ func clmSlugForAccessType(accessType string) (string, bool) { return "", false } +// clmIsBenignUnmappedAccessType reports whether accessType is one of the two documented +// non-grantable-but-truly-inert values every folder-security entry can legitimately +// carry — NoAccess (this connector's own Revoke leaves entries in place at this value, +// so it appears on every subsequent sync of a revoked entry) and InheritFromParentFolder +// (an absence-of-override marker — see clmFolderEntitlement's doc). Grants() skips these +// the same way it skips Custom, but stays fully silent for them, unlike Custom: neither +// represents an access grant C1 is failing to show, so logging them would only add +// per-sync noise for two expected states large accounts can produce on every sync. +// +// Custom is deliberately NOT in this set — see its own Debug log in +// logSkippedFolderSecurityEntry: it's a real, active grant this connector can't +// round-trip to a single tier (an arbitrary flag combination), so silencing it the same +// way would hide an actual access-visibility gap, not just an expected inert state. +func clmIsBenignUnmappedAccessType(accessType string) bool { + switch accessType { + case client.ClmAccessTypeNoAccess, client.ClmAccessTypeInherit: + return true + default: + return false + } +} + // clmIsKnownRole reports whether name is one of the 5 fixed CLM account-level roles // (client.ClmRoles) — the same fixed set clmRoleBuilder.List syncs as clm_role // resources. Used to reject a folder-security Roles entry referencing a role outside diff --git a/pkg/connector/clm_folders_test.go b/pkg/connector/clm_folders_test.go index b3b1d73a..a397b035 100644 --- a/pkg/connector/clm_folders_test.go +++ b/pkg/connector/clm_folders_test.go @@ -3,6 +3,7 @@ package connector import ( "context" "fmt" + "strings" "testing" "github.com/conductorone/baton-docusign/pkg/client" @@ -11,6 +12,10 @@ import ( "github.com/conductorone/baton-sdk/pkg/annotations" "github.com/conductorone/baton-sdk/pkg/pagination" rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "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" ) // --- Pure-function tests: clmSlugForAccessType / clmAccessTypeForSlug --- @@ -58,22 +63,19 @@ func TestClmAccessTypeForSlug_RoundTrips(t *testing.T) { // --- Integration tests against the clmtest mock server --- -func TestClmFolderBuilder_List_SkipsGracefullyWhenClmUnavailable(t *testing.T) { +func TestClmFolderBuilder_List_FailsWhenClmUnavailable(t *testing.T) { // See clm_members_test.go's identical test for the full rationale. s, _ := clmtest.NewServer(t) badClient := s.NewClientWithToken("wrong-token") b := newClmFolderBuilder(badClient) ctx := context.Background() - resources, res, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) - if err != nil { - t.Fatalf("expected List to tolerate an unavailable CLM account and skip gracefully, got error: %v", err) + resources, _, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) + if err == nil { + t.Fatal("expected List to fail when CLM is unavailable, got nil error") } if len(resources) != 0 { - t.Errorf("expected zero resources when CLM is unavailable, got %d", len(resources)) - } - if res == nil || res.NextPageToken != "" { - t.Errorf("expected an empty (non-paginating) result, got %+v", res) + t.Errorf("expected zero resources on a hard failure, got %d", len(resources)) } } @@ -207,6 +209,179 @@ func TestClmFolderBuilder_Grants_SkipsUnknownRoleName(t *testing.T) { } } +func TestClmIsBenignUnmappedAccessType(t *testing.T) { + tests := []struct { + accessType string + want bool + }{ + {client.ClmAccessTypeNoAccess, true}, + // Custom is deliberately excluded — it's a real, active grant this connector + // can't round-trip, so logSkippedFolderSecurityEntry gives it its own distinct + // Debug log instead of silencing it like the truly-inert values here. + {client.ClmAccessTypeCustom, false}, + {client.ClmAccessTypeInherit, true}, + {client.ClmAccessTypeView, false}, + {"SomethingUnrecognized", false}, + {"", false}, + } + for _, tt := range tests { + if got := clmIsBenignUnmappedAccessType(tt.accessType); got != tt.want { + t.Errorf("clmIsBenignUnmappedAccessType(%q) = %v, want %v", tt.accessType, got, tt.want) + } + } +} + +// TestClmFolderBuilder_Grants_LogsOnlyForGenuinelyUnrecognizedAccessType tests the +// observable behavior Grants() ships, not just the clmIsBenignUnmappedAccessType +// predicate in isolation: a genuinely unrecognized AccessType must still log (at +// Debug), while a benign one (NoAccess here) must stay silent even at Debug. +func TestClmFolderBuilder_Grants_LogsOnlyForGenuinelyUnrecognizedAccessType(t *testing.T) { + _, c := clmtest.NewServer(t) + ctx := context.Background() + + // Seeds all three principal-type collections, not just Groups: that's what makes the + // principal_kind-to-distinguishing-field binding assertion below meaningful, rather + // than only ever exercising the Groups branch. + if _, err := c.PatchFolderSecurity(ctx, "folder-templates", client.ClmFolderSecurityWrite{ + Groups: []client.ClmGroupSecurityEntry{ + {AccessType: "SomethingUnrecognized", Href: "https://example.com/groups/group-x"}, + {AccessType: client.ClmAccessTypeNoAccess, Href: "https://example.com/groups/group-y"}, + }, + Roles: []client.ClmRoleSecurityEntry{ + {AccessType: "SomethingUnrecognized", Item: "FullSubscriber"}, + {AccessType: client.ClmAccessTypeNoAccess, Item: "Guest"}, + }, + Users: []client.ClmUserSecurityEntry{ + {AccessType: "SomethingUnrecognized", Href: "https://example.com/members/member-x"}, + {AccessType: client.ClmAccessTypeNoAccess, Href: "https://example.com/members/member-y"}, + }, + }); err != nil { + t.Fatalf("PatchFolderSecurity (seed): %v", err) + } + + core, logs := observer.New(zapcore.DebugLevel) + observedCtx := ctxzap.ToContext(ctx, zap.New(core)) + + b := newClmFolderBuilder(c) + folderResource, err := rs.NewResource("Templates", clmFolderResourceType, "folder-templates") + if err != nil { + t.Fatalf("NewResource: %v", err) + } + + grants, _, err := b.Grants(observedCtx, folderResource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 0 { + t.Fatalf("expected all 6 entries to be skipped (none map to a grantable tier), got %d grants: %+v", len(grants), grants) + } + + // Scoped to the access_type field so this only counts the three skip-log lines + // Grants() emits, not any unrelated log traffic from the HTTP/cache layer + // underneath GetFolder. + entries := logs.FilterFieldKey("access_type").All() + if len(entries) != 3 { + t.Fatalf("expected exactly 3 log entries (one per Groups/Roles/Users branch, for the genuinely unrecognized AccessType only), got %d: %+v", len(entries), entries) + } + // The three branches share one constant message (no per-kind fmt.Sprintf — see + // logSkippedFolderSecurityEntry's doc) and instead distinguish themselves via a + // principal_kind field. Binding principal_kind to its distinguishing field (not just + // checking each field appears SOMEWHERE across the 3 entries) catches a copy-paste + // slip that swaps them between branches, e.g. the Users loop emitting group_href + // under principal_kind "user". + const wantMessage = "baton-docusign: skipping CLM folder security entry with an unmapped AccessType" + wantFieldForKind := map[string]string{ + clmFolderPrincipalKindGroup: "group_href", + clmFolderPrincipalKindRole: "role", + clmFolderPrincipalKindUser: "member_href", + } + seenKind := map[string]bool{} + for _, e := range entries { + if e.Level != zapcore.DebugLevel { + t.Errorf("expected the unmapped-AccessType log to be at Debug, got %v", e.Level) + } + if e.Message != wantMessage { + t.Errorf("expected message %q, got %q", wantMessage, e.Message) + } + // Pins WHICH entry logged, not just how many: an inverted clmIsBenignUnmappedAccessType + // check (silencing SomethingUnrecognized and logging NoAccess instead) would still + // produce exactly 3 Debug entries, passing the assertions above on the exact bug this + // test exists to catch. + fields := e.ContextMap() + if got := fields["access_type"]; got != "SomethingUnrecognized" { + t.Errorf("expected the logged entry's access_type to be %q, got %q", "SomethingUnrecognized", got) + } + kind, _ := fields["principal_kind"].(string) + wantField, ok := wantFieldForKind[kind] + if !ok { + t.Errorf("unexpected principal_kind %q", kind) + continue + } + seenKind[kind] = true + if _, ok := fields[wantField]; !ok { + t.Errorf("expected principal_kind %q to carry the %q field, got fields %v", kind, wantField, fields) + } + } + for kind := range wantFieldForKind { + if !seenKind[kind] { + t.Errorf("expected one log entry with principal_kind %q (the branch that never fired)", kind) + } + } +} + +// TestClmFolderBuilder_Grants_LogsDistinctlyForCustomAccessType confirms Custom gets its +// own distinct Debug line, not silence like NoAccess/InheritFromParentFolder: unlike +// those two, Custom represents a real, active grant this connector can't round-trip to +// a single tier, so silencing it the same way would hide an actual access-visibility +// gap rather than just an expected inert state. +func TestClmFolderBuilder_Grants_LogsDistinctlyForCustomAccessType(t *testing.T) { + _, c := clmtest.NewServer(t) + ctx := context.Background() + + if _, err := c.PatchFolderSecurity(ctx, "folder-templates", client.ClmFolderSecurityWrite{ + Groups: []client.ClmGroupSecurityEntry{ + {AccessType: client.ClmAccessTypeCustom, Href: "https://example.com/groups/group-x"}, + {AccessType: client.ClmAccessTypeNoAccess, Href: "https://example.com/groups/group-y"}, + }, + }); err != nil { + t.Fatalf("PatchFolderSecurity (seed): %v", err) + } + + core, logs := observer.New(zapcore.DebugLevel) + observedCtx := ctxzap.ToContext(ctx, zap.New(core)) + + b := newClmFolderBuilder(c) + folderResource, err := rs.NewResource("Templates", clmFolderResourceType, "folder-templates") + if err != nil { + t.Fatalf("NewResource: %v", err) + } + + grants, _, err := b.Grants(observedCtx, folderResource, rs.SyncOpAttrs{}) + if err != nil { + t.Fatalf("Grants: %v", err) + } + if len(grants) != 0 { + t.Fatalf("expected both entries to be skipped, got %d grants: %+v", len(grants), grants) + } + + // Scoped to access_type-carrying entries only, like the sibling test above, so + // unrelated log traffic from the HTTP/cache layer underneath GetFolder can't leak + // into the count. + entries := logs.FilterFieldKey("access_type").All() + if len(entries) != 1 { + t.Fatalf("expected exactly 1 log entry (Custom; NoAccess should stay silent), got %d: %+v", len(entries), entries) + } + if entries[0].Level != zapcore.DebugLevel { + t.Errorf("expected the Custom-AccessType log to be at Debug, got %v", entries[0].Level) + } + if !strings.Contains(entries[0].Message, "Custom") { + t.Errorf("expected the log message to distinctly mention Custom, got %q", entries[0].Message) + } + if got := entries[0].ContextMap()["access_type"]; got != client.ClmAccessTypeCustom { + t.Errorf("expected access_type field to be %q, got %q", client.ClmAccessTypeCustom, got) + } +} + func TestClmFolderBuilder_GrantAndRevoke_Idempotent(t *testing.T) { srv, c := clmtest.NewServer(t) b := newClmFolderBuilder(c) diff --git a/pkg/connector/clm_groups.go b/pkg/connector/clm_groups.go index 318e0302..1bf6c087 100644 --- a/pkg/connector/clm_groups.go +++ b/pkg/connector/clm_groups.go @@ -10,8 +10,6 @@ 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" ) @@ -53,10 +51,6 @@ func (g *clmGroupBuilder) List(ctx context.Context, _ *v2.ResourceId, attr rs.Sy PageToken: pageToken, }) if err != nil { - if attr.PageToken.Token == "" && isOptInFeatureUnavailableError(err) { - ctxzap.Extract(ctx).Info("baton-docusign: CLM is not available for this account or token, skipping clm_group sync", zap.Error(err)) - return nil, &rs.SyncOpResults{}, nil - } return nil, nil, err } diff --git a/pkg/connector/clm_groups_test.go b/pkg/connector/clm_groups_test.go index 34c6630f..423f444e 100644 --- a/pkg/connector/clm_groups_test.go +++ b/pkg/connector/clm_groups_test.go @@ -37,22 +37,19 @@ func TestClmGroupBuilder_List(t *testing.T) { } } -func TestClmGroupBuilder_List_SkipsGracefullyWhenClmUnavailable(t *testing.T) { +func TestClmGroupBuilder_List_FailsWhenClmUnavailable(t *testing.T) { // See clm_members_test.go's identical test for the full rationale. s, _ := clmtest.NewServer(t) badClient := s.NewClientWithToken("wrong-token") b := newClmGroupBuilder(badClient) ctx := context.Background() - resources, res, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) - if err != nil { - t.Fatalf("expected List to tolerate an unavailable CLM account and skip gracefully, got error: %v", err) + resources, _, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) + if err == nil { + t.Fatal("expected List to fail when CLM is unavailable, got nil error") } if len(resources) != 0 { - t.Errorf("expected zero resources when CLM is unavailable, got %d", len(resources)) - } - if res == nil || res.NextPageToken != "" { - t.Errorf("expected an empty (non-paginating) result, got %+v", res) + t.Errorf("expected zero resources on a hard failure, got %d", len(resources)) } } diff --git a/pkg/connector/clm_members.go b/pkg/connector/clm_members.go index 74352724..1899219f 100644 --- a/pkg/connector/clm_members.go +++ b/pkg/connector/clm_members.go @@ -6,8 +6,6 @@ import ( "github.com/conductorone/baton-docusign/pkg/client" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" rs "github.com/conductorone/baton-sdk/pkg/types/resource" - "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" - "go.uber.org/zap" ) // clmMemberBuilder syncs CLM Members — CLM's own principal object. Synced as its own @@ -35,10 +33,6 @@ func (b *clmMemberBuilder) List(ctx context.Context, _ *v2.ResourceId, attr rs.S PageToken: pageToken, }) if err != nil { - if attr.PageToken.Token == "" && isOptInFeatureUnavailableError(err) { - ctxzap.Extract(ctx).Info("baton-docusign: CLM is not available for this account or token, skipping clm_member sync", zap.Error(err)) - return nil, &rs.SyncOpResults{}, nil - } return nil, nil, err } diff --git a/pkg/connector/clm_members_test.go b/pkg/connector/clm_members_test.go index ff578cce..0db9e2c4 100644 --- a/pkg/connector/clm_members_test.go +++ b/pkg/connector/clm_members_test.go @@ -37,26 +37,23 @@ func TestClmMemberBuilder_List_Pagination(t *testing.T) { } } -func TestClmMemberBuilder_List_SkipsGracefullyWhenClmUnavailable(t *testing.T) { - // Regression test for the wipe-risk fix: clm_member (and the other CLM/signing_group - // builders) is now registered unconditionally in ResourceSyncers() rather than - // gated by a config flag, so an account/token that genuinely can't use CLM must - // have its List() tolerate the resulting auth error and skip gracefully instead of - // failing the whole sync — see isOptInFeatureUnavailableError in helper.go. +func TestClmMemberBuilder_List_FailsWhenClmUnavailable(t *testing.T) { + // clm_member (like every OptInRequired CLM/signing_group resource type) only ever + // syncs once a customer has explicitly opted it in, and C1's opt-in toggle has no + // upstream check against DocuSign — so an account/token that can't use CLM at that + // point is a real misconfiguration, not an expected state. List() must fail loudly + // rather than silently succeed with zero resources — see clm_roles.go's doc comment. s, _ := clmtest.NewServer(t) badClient := s.NewClientWithToken("wrong-token") b := newClmMemberBuilder(badClient) ctx := context.Background() - resources, res, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) - if err != nil { - t.Fatalf("expected List to tolerate an unavailable CLM account and skip gracefully, got error: %v", err) + resources, _, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) + if err == nil { + t.Fatal("expected List to fail when CLM is unavailable, got nil error") } if len(resources) != 0 { - t.Errorf("expected zero resources when CLM is unavailable, got %d", len(resources)) - } - if res == nil || res.NextPageToken != "" { - t.Errorf("expected an empty (non-paginating) result, got %+v", res) + t.Errorf("expected zero resources on a hard failure, got %d", len(resources)) } } diff --git a/pkg/connector/clm_permission_sets.go b/pkg/connector/clm_permission_sets.go index 04b81b66..1c997ed5 100644 --- a/pkg/connector/clm_permission_sets.go +++ b/pkg/connector/clm_permission_sets.go @@ -7,8 +7,6 @@ import ( v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" "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" ) // clmPermissionSetAssignedTag mirrors permissionProfileAssignedTag's pattern for the @@ -42,10 +40,6 @@ func (b *clmPermissionSetBuilder) List(ctx context.Context, _ *v2.ResourceId, at PageToken: pageToken, }) if err != nil { - if attr.PageToken.Token == "" && isOptInFeatureUnavailableError(err) { - ctxzap.Extract(ctx).Info("baton-docusign: CLM is not available for this account or token, skipping clm_permission_set sync", zap.Error(err)) - return nil, &rs.SyncOpResults{}, nil - } return nil, nil, err } diff --git a/pkg/connector/clm_permission_sets_test.go b/pkg/connector/clm_permission_sets_test.go index 09e0fb91..aa484488 100644 --- a/pkg/connector/clm_permission_sets_test.go +++ b/pkg/connector/clm_permission_sets_test.go @@ -10,22 +10,19 @@ import ( rs "github.com/conductorone/baton-sdk/pkg/types/resource" ) -func TestClmPermissionSetBuilder_List_SkipsGracefullyWhenClmUnavailable(t *testing.T) { +func TestClmPermissionSetBuilder_List_FailsWhenClmUnavailable(t *testing.T) { // See clm_members_test.go's identical test for the full rationale. s, _ := clmtest.NewServer(t) badClient := s.NewClientWithToken("wrong-token") b := newClmPermissionSetBuilder(badClient) ctx := context.Background() - resources, res, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) - if err != nil { - t.Fatalf("expected List to tolerate an unavailable CLM account and skip gracefully, got error: %v", err) + resources, _, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) + if err == nil { + t.Fatal("expected List to fail when CLM is unavailable, got nil error") } if len(resources) != 0 { - t.Errorf("expected zero resources when CLM is unavailable, got %d", len(resources)) - } - if res == nil || res.NextPageToken != "" { - t.Errorf("expected an empty (non-paginating) result, got %+v", res) + t.Errorf("expected zero resources on a hard failure, got %d", len(resources)) } } diff --git a/pkg/connector/clm_roles.go b/pkg/connector/clm_roles.go index 83d7e560..7bba47e7 100644 --- a/pkg/connector/clm_roles.go +++ b/pkg/connector/clm_roles.go @@ -10,6 +10,9 @@ import ( // clmRoleBuilder syncs the 5 fixed CLM account-level roles (client.ClmRoles). Not // backed by an API call — see resource_types.go for why this resource type exists. +// CLM availability is checked once, up front, by Connector.Validate() rather than here +// — see that method's doc for why centralizing it there is better than every opted-in +// CLM builder repeating the same check on its own first page. type clmRoleBuilder struct { resourceType *v2.ResourceType client *client.Client @@ -19,8 +22,10 @@ func (b *clmRoleBuilder) ResourceType(_ context.Context) *v2.ResourceType { return clmRoleResourceType } -// List returns the fixed set of CLM roles. No pagination needed — the set is small -// and hardcoded, not fetched from the API. +// List returns the fixed set of CLM roles. No pagination needed — the set is small and +// hardcoded, not fetched from the API. CLM availability was already confirmed once, up +// front, by Connector.Validate() before any builder's List() runs — see that method's +// doc — so there's no error path here beyond rs.NewRoleResource construction failing. func (b *clmRoleBuilder) List(_ context.Context, _ *v2.ResourceId, _ rs.SyncOpAttrs) ([]*v2.Resource, *rs.SyncOpResults, error) { var resources []*v2.Resource for _, role := range client.ClmRoles { diff --git a/pkg/connector/clm_roles_test.go b/pkg/connector/clm_roles_test.go index 5a9fce24..0f051dff 100644 --- a/pkg/connector/clm_roles_test.go +++ b/pkg/connector/clm_roles_test.go @@ -5,13 +5,16 @@ import ( "testing" "github.com/conductorone/baton-docusign/pkg/client" + "github.com/conductorone/baton-docusign/pkg/client/clmtest" rs "github.com/conductorone/baton-sdk/pkg/types/resource" ) func TestClmRoleBuilder_List(t *testing.T) { - // Not backed by an API call — the mock server isn't even needed here, unlike every - // other CLM builder's List test. - b := newClmRoleBuilder(nil) + // The role set isn't backed by an API call at all — CLM availability is checked + // once, up front, by Connector.Validate() (see connector_test.go), not here — so + // this only needs a client to satisfy the builder's field, never calls it. + _, c := clmtest.NewServer(t) + b := newClmRoleBuilder(c) ctx := context.Background() resources, res, err := b.List(ctx, nil, rs.SyncOpAttrs{}) diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index fe190e8d..315e6057 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -24,6 +24,14 @@ type Connector struct { // at all (see ResourceSyncers). Unlike the CLM types, which are always registered, // this means ListResourceTypes() advertises a different set depending on the flag. includeSigningGroups bool + // includeClm reports whether this sync will touch any CLM resource type — the same + // opts.WillSyncResourceType(...) signal that already determines whether any CLM + // builder's List() gets invoked this run (see New()). Gates Validate()'s upfront CLM + // readiness check only: it does NOT gate resource-type registration. ResourceSyncers + // always registers all 5 CLM builders unconditionally, because toggling registration + // itself would make ListResourceTypes() advertise a different set between syncs and + // C1 would read previously-synced CLM resources/grants as deleted. + includeClm bool // skipPermissionProfileResourceType reports whether permission_profile is // excluded from the sync filter. skipPermissionProfileResourceType bool @@ -130,7 +138,27 @@ func (d *Connector) Metadata(_ context.Context) (*v2.ConnectorMetadata, error) { }, nil } -func (d *Connector) Validate(_ context.Context) (annotations.Annotations, error) { +// Validate runs once, before any resource type's List() (see baton-sdk's +// pkg/sync/syncer.go Sync()), so it's the right place to check readiness a single time +// upfront rather than discovering a bad account mid-sync at whichever builder's List() +// happens to run first. EnsureReady (base eSignature credentials) runs unconditionally +// — every sync needs those regardless of CLM — while EnsureClmReady is gated on +// includeClm: an account that never opted into any CLM resource type has no reason to +// pay for, or fail on, a CLM discovery call it doesn't need. This gate is separate from +// resource-type registration (see this file's includeClm field doc) and replaces each +// CLM builder's own List() checking readiness independently. +func (d *Connector) Validate(ctx context.Context) (annotations.Annotations, error) { + if err := d.client.EnsureReady(ctx); err != nil { + return nil, fmt.Errorf("baton-docusign: eSignature credential check failed: %w", err) + } + if !d.includeClm { + return nil, nil + } + if err := d.client.EnsureClmReady(ctx); err != nil { + return nil, fmt.Errorf("baton-docusign: CLM readiness check failed — clm_* resource types "+ + "are enabled for this sync but this account/credential cannot reach the CLM API; "+ + "disable those resource types or enable CLM on the account: %w", err) + } return nil, nil } @@ -153,24 +181,18 @@ func NewWithRefreshToken( return &Connector{ client: docusignClient, includeSigningGroups: includeSigningGroups, + includeClm: includeClm, skipPermissionProfileResourceType: skipPermissionProfileResourceType, }, nil } -func NewWithClient(client *client.Client, includeSigningGroups bool, skipPermissionProfileResourceType bool) (*Connector, error) { - return &Connector{ - client: client, - includeSigningGroups: includeSigningGroups, - skipPermissionProfileResourceType: skipPermissionProfileResourceType, - }, nil -} - -// NewWithTokenSource takes no includeClm: the token source is minted by ConductorOne's -// OAuth flow, so this path can't influence which scopes were granted, and the CLM -// builders no longer gate their List() bodies on it. +// NewWithTokenSource's token source is minted by ConductorOne's OAuth flow, so this +// path can't influence which scopes were granted (unlike NewWithRefreshToken, where +// includeClm also drives buildScopes) — but it still needs includeClm to gate +// Validate()'s CLM readiness check, so it's threaded through for that purpose alone. func NewWithTokenSource( ctx context.Context, isDemo bool, tokenSource oauth2.TokenSource, accountId string, - includeSigningGroups bool, clmBaseURLOverride string, + includeSigningGroups, includeClm bool, clmBaseURLOverride string, skipPermissionProfileResourceType bool, ) (*Connector, error) { docusignClient := client.NewClient(ctx, isDemo, tokenSource, accountId, clmBaseURLOverride) @@ -178,6 +200,7 @@ func NewWithTokenSource( return &Connector{ client: docusignClient, includeSigningGroups: includeSigningGroups, + includeClm: includeClm, skipPermissionProfileResourceType: skipPermissionProfileResourceType, }, nil } @@ -205,7 +228,7 @@ func New(ctx context.Context, docusignCfg *cfg.Docusign, opts *cli.ConnectorOpts if opts.TokenSource != nil { cbWithTokenSource, err := NewWithTokenSource( ctx, isDemo, opts.TokenSource, docusignCfg.AccountId, - docusignCfg.IncludeSigningGroups, docusignCfg.ClmBaseUrl, + docusignCfg.IncludeSigningGroups, includeClm, docusignCfg.ClmBaseUrl, skipPermissionProfileResourceType, ) if err != nil { diff --git a/pkg/connector/connector_test.go b/pkg/connector/connector_test.go index 6abf82b3..9043a989 100644 --- a/pkg/connector/connector_test.go +++ b/pkg/connector/connector_test.go @@ -2,9 +2,11 @@ package connector import ( "context" + "net/http" "testing" "github.com/conductorone/baton-docusign/pkg/client/clmtest" + "golang.org/x/oauth2" ) // alwaysRegisteredTypeIDs are the resource types ResourceSyncers registers on every @@ -12,8 +14,9 @@ import ( // registering conditionally would make ListResourceTypes() advertise fewer types than a // prior sync did, and C1 can then bucket every previously-synced resource and grant of a // vanished type as deleted. Gating happens via &v2.OptInRequired{} (resource_types.go) -// and each opt-in builder's List() tolerating an unavailable-feature error (helper.go), -// not by omitting the builder. +// alone, not by omitting the builder — Connector.Validate() fails the sync loudly, up +// front, rather than tolerating an unavailable-feature error, when a customer opts in +// without a reachable CLM subscription (see connector.go's Validate doc comment). var alwaysRegisteredTypeIDs = []string{ "user", "group", @@ -81,3 +84,95 @@ func TestResourceSyncers_SigningGroupRegistrationFollowsFlag(t *testing.T) { } } } + +// TestConnectorValidate covers both readiness checks Validate() centralizes (see +// connector.go's doc comment): EnsureReady (base eSignature credentials) runs +// unconditionally, EnsureClmReady only when includeClm is set. The includeClm=false +// subtests are the ones that matter most: one proves the CLM-specific check is +// genuinely skipped rather than coincidentally passing, the other proves Validate() now +// catches a misconfigured account upfront even when CLM was never opted into, instead +// of leaving that to whichever builder's List() happens to run first. +func TestConnectorValidate(t *testing.T) { + s, c := clmtest.NewServer(t) + badClmClient := s.NewClientWithToken("wrong-token") + ctx := context.Background() + + t.Run("includeClm=true, base and CLM both reachable: succeeds", func(t *testing.T) { + d := &Connector{client: c, includeClm: true} + if _, err := d.Validate(ctx); err != nil { + t.Errorf("expected Validate to succeed, got %v", err) + } + }) + + t.Run("includeClm=true, CLM unreachable: fails loudly", func(t *testing.T) { + d := &Connector{client: badClmClient, includeClm: true} + if _, err := d.Validate(ctx); err == nil { + t.Error("expected Validate to fail when CLM is unreachable, got nil error") + } + }) + + t.Run("includeClm=false, CLM unreachable but base fine: still succeeds", func(t *testing.T) { + // badClmClient's bad token only fails clmtest's requireAuth-gated CLM routes — + // its /oauth/userinfo (the base check) succeeds regardless of token (see + // clmtest/server.go's handleUserInfo) — so a nil error here proves the + // CLM-specific check was genuinely skipped, not just coincidentally passing. + d := &Connector{client: badClmClient, includeClm: false} + if _, err := d.Validate(ctx); err != nil { + t.Errorf("expected Validate to skip the CLM check (nil error) when includeClm is false, got %v", err) + } + }) + + t.Run("includeClm=false, base credentials bad: fails", func(t *testing.T) { + badBaseClient := newSigningGroupsTestClient(t, http.StatusUnauthorized) + d := &Connector{client: badBaseClient, includeClm: false} + if _, err := d.Validate(ctx); err == nil { + t.Error("expected Validate to fail on bad base credentials even when includeClm is false, got nil error") + } + }) +} + +// TestNewWithRefreshToken_StoresIncludeClm is a regression test for a real bug caught +// during review: NewWithRefreshToken already received includeClm as a parameter (used +// for OAuth scope selection via client.New) but silently dropped it instead of storing +// it on the returned Connector, so Validate() would never have run its CLM check for +// any connector built this way. A non-empty baseURLOverride makes client.New build a +// StaticTokenSource with zero network I/O (see client.go), so this needs no mock server. +func TestNewWithRefreshToken_StoresIncludeClm(t *testing.T) { + ctx := context.Background() + + for _, includeClm := range []bool{true, false} { + cb, err := NewWithRefreshToken( + ctx, false, "client-id", "client-secret", "https://redirect.example.com", + "refresh-token", "account-1", false, includeClm, + "https://clm.example.com", "https://api.example.com", false, + ) + if err != nil { + t.Fatalf("includeClm=%v: NewWithRefreshToken: %v", includeClm, err) + } + if cb.includeClm != includeClm { + t.Errorf("includeClm=%v: expected Connector.includeClm=%v, got %v", includeClm, includeClm, cb.includeClm) + } + } +} + +// TestNewWithTokenSource_StoresIncludeClm is a regression test for the more serious of +// the two constructor gaps: NewWithTokenSource — the ConductorOne-hosted path, i.e. the +// common production case — had no includeClm parameter at all, so Validate() would have +// silently never checked CLM readiness for the majority deployment path. client.NewClient +// makes zero network I/O at construction (see client.go), so this needs no mock server. +func TestNewWithTokenSource_StoresIncludeClm(t *testing.T) { + ctx := context.Background() + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "tok"}) + + for _, includeClm := range []bool{true, false} { + cb, err := NewWithTokenSource( + ctx, false, tokenSource, "account-1", false, includeClm, "https://clm.example.com", false, + ) + if err != nil { + t.Fatalf("includeClm=%v: NewWithTokenSource: %v", includeClm, err) + } + if cb.includeClm != includeClm { + t.Errorf("includeClm=%v: expected Connector.includeClm=%v, got %v", includeClm, includeClm, cb.includeClm) + } + } +} diff --git a/pkg/connector/helper.go b/pkg/connector/helper.go index 1795e533..bbc4ea02 100644 --- a/pkg/connector/helper.go +++ b/pkg/connector/helper.go @@ -41,49 +41,6 @@ func parsePageToken(i string, resourceID *v2.ResourceId) (*pagination.Bag, strin return b, b.PageToken(), nil } -// isOptInFeatureUnavailableError reports whether err indicates this account/token -// simply can't use an optional DocuSign feature — no subscription (CLM), the feature -// isn't enabled on the account (signing groups), or the OAuth token lacks the scopes it -// needs (CLM's spring_read/spring_write — see oauth.go) — rather than an unexpected -// failure. -// -// The 5 CLM resource types (and signing_group's List() has the same shape of check) -// are registered unconditionally in ResourceSyncers() and their List() bodies always -// run, with no config flag gating them, specifically so that a resource type never -// disappears from a later sync and gets treated as fully deleted. Tolerating this error -// on the first page of List() (see call sites) is what makes unconditional registration -// safe: the sync skips that one resource type gracefully instead of failing outright. -// -// Covers four codes, each tied to a specific confirmed failure mode of -// ensureClmInitialized's CLM base-URL discovery call (clm_client.go) — the first thing -// every CLM builder's List() does, now unconditionally: -// - PermissionDenied/Unauthenticated: the account/token lacks the CLM subscription -// or OAuth scope — the expected case for most eSignature-only accounts. -// - NotFound: the discovery endpoint 404s for an account that was never provisioned -// in the legacy SpringCM system. CLM's Object API returns 404 both for "doesn't -// exist" and "exists but no access", so treat it as a plausible no-access signal, -// not proof the account lacks CLM. -// - FailedPrecondition: ensureClmInitialized wraps its "response didn't contain a -// recognized base-URL field" error with this code specifically — a non-CLM -// account's discovery response plausibly has a different shape entirely (no CLM -// fields at all), which would otherwise surface as an unrecognized codes.Unknown -// and fail the whole sync. -// -// Deliberately still doesn't cover codes.Unknown itself (an un-coded, unwrapped error) -// or 5xx/transport failures (codes.Unavailable/DeadlineExceeded/etc.) — those stay -// loud, since they're as likely to indicate a real outage or bug as a no-CLM account, -// and swallowing them broadly would hide genuine failures. Every other resource type -// (user, group, permission_profile) is always attempted and does not tolerate this -// error at all, so a truly broken token still fails the sync via those. -func isOptInFeatureUnavailableError(err error) bool { - switch status.Code(err) { - case codes.PermissionDenied, codes.Unauthenticated, codes.NotFound, codes.FailedPrecondition: - return true - default: - return false - } -} - // clmIDFromHref extracts the trailing path segment from a CLM object's Href — see // client.IDFromHref's doc. pkg/client/clmtest can't import pkg/connector, so the single // definition lives in pkg/client and both packages delegate to it instead of diff --git a/pkg/connector/helper_test.go b/pkg/connector/helper_test.go index b6ac2361..49612925 100644 --- a/pkg/connector/helper_test.go +++ b/pkg/connector/helper_test.go @@ -2,40 +2,12 @@ package connector import ( "context" - "errors" "testing" v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" rs "github.com/conductorone/baton-sdk/pkg/types/resource" - "google.golang.org/grpc/codes" - "google.golang.org/grpc/status" ) -func TestIsOptInFeatureUnavailableError(t *testing.T) { - tests := []struct { - name string - err error - want bool - }{ - {"nil error", nil, false}, - {"permission denied", status.Error(codes.PermissionDenied, "no CLM subscription"), true}, - {"unauthenticated", status.Error(codes.Unauthenticated, "insufficient scope"), true}, - {"not found (e.g. account never provisioned in SpringCM)", status.Error(codes.NotFound, "no such account"), true}, - {"failed precondition (e.g. discovery response missing a recognized base-URL field)", status.Error(codes.FailedPrecondition, "no recognized field"), true}, - {"unavailable (rate limit/5xx)", status.Error(codes.Unavailable, "rate limited"), false}, - {"internal", status.Error(codes.Internal, "boom"), false}, - {"unknown (bare unwrapped error)", status.Error(codes.Unknown, "boom"), false}, - {"plain non-gRPC error", errors.New("some transport error"), false}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if got := isOptInFeatureUnavailableError(tt.err); got != tt.want { - t.Errorf("isOptInFeatureUnavailableError(%v) = %v, want %v", tt.err, got, tt.want) - } - }) - } -} - func TestClmHrefWithID(t *testing.T) { got, err := clmHrefWithID("https://clm.example.com/v2/acct-1/groups/group-old", "group-new") if err != nil { diff --git a/pkg/connector/resource_types.go b/pkg/connector/resource_types.go index 31e0640f..c716832a 100644 --- a/pkg/connector/resource_types.go +++ b/pkg/connector/resource_types.go @@ -29,9 +29,12 @@ var ( DisplayName: "Permission Profile", } - // signingGroupResourceType is registered unconditionally (see connector.go's - // ResourceSyncers) — OptInRequired is the gate, not a config flag, matching the - // CLM types below. + // signingGroupResourceType is registered only when includeSigningGroups is set (see + // connector.go's ResourceSyncers) — unlike the CLM types below, which are always + // registered and rely on OptInRequired alone. That means ListResourceTypes() + // advertises a different set depending on the flag; see + // TestResourceSyncers_SigningGroupRegistrationFollowsFlag for the tradeoff this + // carries. signingGroupResourceType = &v2.ResourceType{ Id: "signing_group", DisplayName: "Signing Group", @@ -40,11 +43,10 @@ var ( } // CLM (Contract Lifecycle Management) resource types. CLM is a separate DocuSign - // product/API surface from eSignature above. Registered unconditionally and no - // longer gated by any config flag (see connector.go's ResourceSyncers): each CLM - // builder's List() always runs, and &v2.OptInRequired{} plus - // isOptInFeatureUnavailableError (helper.go) are what keep an account without a CLM - // subscription from failing the sync. + // product/API surface from eSignature above. &v2.OptInRequired{} keeps these out of + // a customer's sync until explicitly enabled; once enabled, Connector.Validate() + // fails the sync loudly, up front, if the account can't actually reach CLM — see + // that method's doc comment in connector.go. // clmMemberResourceType is CLM's own principal object. Deliberately NOT reusing // userResourceType's id ("user") — the CLM Members API is a distinct upstream diff --git a/pkg/connector/singing_groups.go b/pkg/connector/singing_groups.go index e437ac45..65838bd5 100644 --- a/pkg/connector/singing_groups.go +++ b/pkg/connector/singing_groups.go @@ -38,10 +38,6 @@ func (g *signingGroupBuilder) List(ctx context.Context, _ *v2.ResourceId, attr r PageToken: pageToken, }) if err != nil { - if attr.PageToken.Token == "" && isOptInFeatureUnavailableError(err) { - ctxzap.Extract(ctx).Info("baton-docusign: signing groups are not available for this account, skipping signing_group sync", zap.Error(err)) - return nil, &rs.SyncOpResults{}, nil - } return nil, nil, err } diff --git a/pkg/connector/singing_groups_test.go b/pkg/connector/singing_groups_test.go new file mode 100644 index 00000000..6f1af89f --- /dev/null +++ b/pkg/connector/singing_groups_test.go @@ -0,0 +1,127 @@ +package connector + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/conductorone/baton-docusign/pkg/client" + "github.com/conductorone/baton-sdk/pkg/pagination" + rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/uhttp" + "golang.org/x/oauth2" +) + +// rewriteTransport is already declared in users_test.go (same package) — reused here +// rather than duplicated. + +// testSigningGroupID and testSigningGroupName are the one signing group +// newSigningGroupsTestClient's mock seeds in its /signing_groups response. +const ( + testSigningGroupID = "sg-1" + testSigningGroupName = "Test Signing Group" +) + +// newSigningGroupsTestClient builds a *client.Client wired to a mock server serving +// /oauth/userinfo plus a minimal /signing_groups response (one seeded group). +// signingGroupBuilder.List()'s only failure path that matters here is ensureInitialized +// (called by GetSigningGroups before it ever reaches the signing-groups endpoint), so a +// full eSignature REST API mock isn't needed to exercise it — matching how the CLM +// builders' equivalent tests fail at CLM account discovery. +func newSigningGroupsTestClient(t *testing.T, userInfoStatus int) *client.Client { + t.Helper() + var mockServer *httptest.Server + mockServer = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/oauth/userinfo" { + if userInfoStatus != http.StatusOK { + w.WriteHeader(userInfoStatus) + return + } + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(client.UserInfoResponse{ + Sub: "test-user", + Name: "Test User", + Accounts: []client.AccountInfo{ + {AccountId: "acct-1", AccountName: "Test Account", IsDefault: true, BaseURI: mockServer.URL}, + }, + }) + return + } + if strings.HasSuffix(r.URL.Path, "/signing_groups") { + // One seeded group is enough to exercise both the happy path and + // parseIntoSigningGroupResource, without a full pagination fixture (no next + // page: the zero-valued embedded Page makes getNextToken's + // EndPosition+1 < TotalSetSize read 0+1 < 0, false). + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(client.SigningGroupResponse{ + SigningGroups: []client.SigningGroup{ + {SigningGroupId: testSigningGroupID, GroupName: testSigningGroupName}, + }, + }) + return + } + http.NotFound(w, r) + })) + t.Cleanup(mockServer.Close) + + mockServerURL, err := url.Parse(mockServer.URL) + if err != nil { + t.Fatalf("parsing mock server URL: %v", err) + } + transport := &rewriteTransport{target: mockServerURL, base: http.DefaultTransport} + wrapper := uhttp.NewBaseHttpClient(&http.Client{Transport: transport}) + tokenSource := oauth2.StaticTokenSource(&oauth2.Token{AccessToken: "test-token"}) + + return client.NewClient(context.Background(), false, tokenSource, "", "", wrapper) +} + +// TestSigningGroupBuilder_List_FailsWhenUnavailable is a regression test: signing_group +// is gated behind the --include-signing-groups flag (connector.go), but that flag +// doesn't validate the account actually has the feature before letting an operator turn +// it on. List() must propagate any error (here, a 401 from eSignature account discovery) +// instead of tolerating it and silently syncing zero signing groups. +func TestSigningGroupBuilder_List_FailsWhenUnavailable(t *testing.T) { + c := newSigningGroupsTestClient(t, http.StatusUnauthorized) + b := newSigningGroupBuilder(c) + ctx := context.Background() + + resources, _, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) + if err == nil { + t.Fatal("expected List to fail when signing groups are unavailable, got nil error") + } + if len(resources) != 0 { + t.Errorf("expected zero resources on a hard failure, got %d", len(resources)) + } +} + +// TestSigningGroupBuilder_List_Succeeds is a sanity check for +// newSigningGroupsTestClient itself: confirms the happy path (account discovery +// succeeds) reaches List()'s normal return and correctly parses the one seeded signing +// group via parseIntoSigningGroupResource, distinguishing a correctly-wired mock from +// the fail-loud test above passing only because everything errors regardless. +func TestSigningGroupBuilder_List_Succeeds(t *testing.T) { + c := newSigningGroupsTestClient(t, http.StatusOK) + b := newSigningGroupBuilder(c) + ctx := context.Background() + + resources, res, err := b.List(ctx, nil, rs.SyncOpAttrs{PageToken: pagination.Token{Size: 10}}) + if err != nil { + t.Fatalf("List: %v", err) + } + if res == nil { + t.Fatal("expected a non-nil SyncOpResults") + } + if len(resources) != 1 { + t.Fatalf("expected the one seeded signing group, got %d: %+v", len(resources), resources) + } + if got := resources[0].Id.Resource; got != testSigningGroupID { + t.Errorf("expected resource ID %q, got %q", testSigningGroupID, got) + } + if got := resources[0].DisplayName; got != testSigningGroupName { + t.Errorf("expected display name %q, got %q", testSigningGroupName, got) + } +} diff --git a/vendor/go.uber.org/zap/zaptest/observer/logged_entry.go b/vendor/go.uber.org/zap/zaptest/observer/logged_entry.go new file mode 100644 index 00000000..ef89e25c --- /dev/null +++ b/vendor/go.uber.org/zap/zaptest/observer/logged_entry.go @@ -0,0 +1,39 @@ +// Copyright (c) 2017 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +package observer + +import "go.uber.org/zap/zapcore" + +// A LoggedEntry is an encoding-agnostic representation of a log message. +// Field availability is context dependent. +type LoggedEntry struct { + zapcore.Entry + Context []zapcore.Field +} + +// ContextMap returns a map for all fields in Context. +func (e LoggedEntry) ContextMap() map[string]interface{} { + encoder := zapcore.NewMapObjectEncoder() + for _, f := range e.Context { + f.AddTo(encoder) + } + return encoder.Fields +} diff --git a/vendor/go.uber.org/zap/zaptest/observer/observer.go b/vendor/go.uber.org/zap/zaptest/observer/observer.go new file mode 100644 index 00000000..4f7ce0ec --- /dev/null +++ b/vendor/go.uber.org/zap/zaptest/observer/observer.go @@ -0,0 +1,203 @@ +// Copyright (c) 2016-2022 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +// Package observer provides a zapcore.Core that keeps an in-memory, +// encoding-agnostic representation of log entries. It's useful for +// applications that want to unit test their log output without tying their +// tests to a particular output encoding. +package observer // import "go.uber.org/zap/zaptest/observer" + +import ( + "strings" + "sync" + "time" + + "go.uber.org/zap/internal" + "go.uber.org/zap/zapcore" +) + +// ObservedLogs is a concurrency-safe, ordered collection of observed logs. +type ObservedLogs struct { + mu sync.RWMutex + logs []LoggedEntry +} + +// Len returns the number of items in the collection. +func (o *ObservedLogs) Len() int { + o.mu.RLock() + n := len(o.logs) + o.mu.RUnlock() + return n +} + +// All returns a copy of all the observed logs. +func (o *ObservedLogs) All() []LoggedEntry { + o.mu.RLock() + ret := make([]LoggedEntry, len(o.logs)) + copy(ret, o.logs) + o.mu.RUnlock() + return ret +} + +// TakeAll returns a copy of all the observed logs, and truncates the observed +// slice. +func (o *ObservedLogs) TakeAll() []LoggedEntry { + o.mu.Lock() + ret := o.logs + o.logs = nil + o.mu.Unlock() + return ret +} + +// AllUntimed returns a copy of all the observed logs, but overwrites the +// observed timestamps with time.Time's zero value. This is useful when making +// assertions in tests. +func (o *ObservedLogs) AllUntimed() []LoggedEntry { + ret := o.All() + for i := range ret { + ret[i].Time = time.Time{} + } + return ret +} + +// FilterLevelExact filters entries to those logged at exactly the given level. +func (o *ObservedLogs) FilterLevelExact(level zapcore.Level) *ObservedLogs { + return o.Filter(func(e LoggedEntry) bool { + return e.Level == level + }) +} + +// FilterMessage filters entries to those that have the specified message. +func (o *ObservedLogs) FilterMessage(msg string) *ObservedLogs { + return o.Filter(func(e LoggedEntry) bool { + return e.Message == msg + }) +} + +// FilterLoggerName filters entries to those logged through logger with the specified logger name. +func (o *ObservedLogs) FilterLoggerName(name string) *ObservedLogs { + return o.Filter(func(e LoggedEntry) bool { + return e.LoggerName == name + }) +} + +// FilterMessageSnippet filters entries to those that have a message containing the specified snippet. +func (o *ObservedLogs) FilterMessageSnippet(snippet string) *ObservedLogs { + return o.Filter(func(e LoggedEntry) bool { + return strings.Contains(e.Message, snippet) + }) +} + +// FilterField filters entries to those that have the specified field. +func (o *ObservedLogs) FilterField(field zapcore.Field) *ObservedLogs { + return o.Filter(func(e LoggedEntry) bool { + for _, ctxField := range e.Context { + if ctxField.Equals(field) { + return true + } + } + return false + }) +} + +// FilterFieldKey filters entries to those that have the specified key. +func (o *ObservedLogs) FilterFieldKey(key string) *ObservedLogs { + return o.Filter(func(e LoggedEntry) bool { + for _, ctxField := range e.Context { + if ctxField.Key == key { + return true + } + } + return false + }) +} + +// Filter returns a copy of this ObservedLogs containing only those entries +// for which the provided function returns true. +func (o *ObservedLogs) Filter(keep func(LoggedEntry) bool) *ObservedLogs { + o.mu.RLock() + defer o.mu.RUnlock() + + var filtered []LoggedEntry + for _, entry := range o.logs { + if keep(entry) { + filtered = append(filtered, entry) + } + } + return &ObservedLogs{logs: filtered} +} + +func (o *ObservedLogs) add(log LoggedEntry) { + o.mu.Lock() + o.logs = append(o.logs, log) + o.mu.Unlock() +} + +// New creates a new Core that buffers logs in memory (without any encoding). +// It's particularly useful in tests. +func New(enab zapcore.LevelEnabler) (zapcore.Core, *ObservedLogs) { + ol := &ObservedLogs{} + return &contextObserver{ + LevelEnabler: enab, + logs: ol, + }, ol +} + +type contextObserver struct { + zapcore.LevelEnabler + logs *ObservedLogs + context []zapcore.Field +} + +var ( + _ zapcore.Core = (*contextObserver)(nil) + _ internal.LeveledEnabler = (*contextObserver)(nil) +) + +func (co *contextObserver) Level() zapcore.Level { + return zapcore.LevelOf(co.LevelEnabler) +} + +func (co *contextObserver) Check(ent zapcore.Entry, ce *zapcore.CheckedEntry) *zapcore.CheckedEntry { + if co.Enabled(ent.Level) { + return ce.AddCore(ent, co) + } + return ce +} + +func (co *contextObserver) With(fields []zapcore.Field) zapcore.Core { + return &contextObserver{ + LevelEnabler: co.LevelEnabler, + logs: co.logs, + context: append(co.context[:len(co.context):len(co.context)], fields...), + } +} + +func (co *contextObserver) Write(ent zapcore.Entry, fields []zapcore.Field) error { + all := make([]zapcore.Field, 0, len(fields)+len(co.context)) + all = append(all, co.context...) + all = append(all, fields...) + co.logs.add(LoggedEntry{ent, all}) + return nil +} + +func (co *contextObserver) Sync() error { + return nil +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 1dc3ac8c..94e93bba 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -738,6 +738,7 @@ go.uber.org/zap/internal/exit go.uber.org/zap/internal/pool go.uber.org/zap/internal/stacktrace go.uber.org/zap/zapcore +go.uber.org/zap/zaptest/observer # golang.org/x/crypto v0.54.0 ## explicit; go 1.25.0 golang.org/x/crypto/blowfish