[CXH-2209] fix: gate clm_role on CLM availability, narrow opt-in error tolerance, log skipped folder-security entries #64
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -101,9 +101,9 @@ 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 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: | ||
|
|
||
|
|
@@ -115,11 +115,26 @@ Requirements: | |
| 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, 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. | ||
|
Comment on lines
+118
to
+126
Contributor
There was a problem hiding this comment. 🟡 Suggestion: this new fail-loud contract is described purely in terms of C1's opt-in toggle, but
Contributor
Author
There was a problem hiding this comment. Good catch, and it lines up with what we found the hard way earlier in this PR (the CI jobs needed BATON_SYNC_RESOURCE_TYPES added to keep passing, for exactly this reason). Added a paragraph in 21793c9 pointing self-hosted/CLI users at --sync-resource-types/BATON_SYNC_RESOURCE_TYPES to exclude clm_* on an eSignature-only account. |
||
|
|
||
| `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 | ||
|
Contributor
There was a problem hiding this comment. 🟡 Suggestion: same parsing caveat as the CI workflow —
Contributor
Author
There was a problem hiding this comment. Same mechanism as 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
| 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 | ||
|
Contributor
There was a problem hiding this comment. [Docs / nit] Suggested trim: // logSkippedFolderSecurityEntry Debug-logs an unmapped folder-security AccessType.
// Benign values (NoAccess / Inherit) return without logging; Custom logs as an
// unrepresentable active grant. kind is "group"|"role"|"user". |
||
| // 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)) | ||
|
FeliLucero1 marked this conversation as resolved.
|
||
| 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 | ||
|
Comment on lines
+214
to
+218
Contributor
There was a problem hiding this comment. 🟡 Suggestion: the comments argue
Contributor
Author
There was a problem hiding this comment. Same finding as the reply at #64 (comment) (
Comment on lines
+214
to
+218
Contributor
There was a problem hiding this comment. 🟡 Suggestion: This branch's own doc says a
Contributor
Author
There was a problem hiding this comment. Same standing decision — see #64 (comment) just above and #64 (comment): Warn/Info were already considered and explicitly rejected earlier in this PR for this per-entry logging. |
||
| } | ||
|
FeliLucero1 marked this conversation as resolved.
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. [Docs / nit] Same over-explaining pattern: Revoke/Inherit history and Custom contrast belong in Suggested trim: // clmIsBenignUnmappedAccessType is true for NoAccess and InheritFromParentFolder —
// inert AccessTypes Grants() skips without logging. Custom is not benign. |
||
| // 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 | ||
| } | ||
| } | ||
|
FeliLucero1 marked this conversation as resolved.
|
||
|
|
||
| // 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟠 Bug: a comma-separated value doesn't reach the SDK as a 4-element list. The SDK reads this via
v.GetStringSlice("sync-resource-types")(vendor/.../pkg/cli/commands.go:464), and for an env-sourced value viper returns the raw string (viper.go:1211— thereadAsCSVbranch at:1179only applies when the pflag changed), socast.ToStringSliceEhits itscase string: return strings.Fields(v)(vendor/.../spf13/cast/caste.go:1311) and yields the single element"user,group,permission_profile,signing_group". Two consequences: the sync aborts withinvalid resource type '...' in filter(vendor/.../pkg/sync/syncer.go:1284), andSyncFilterIsExplicit()becoming true with no matching ID also flipsskipPermissionProfileResourceTypeto true inpkg/connector/connector.go:226. Whitespace-separating the value works withstrings.Fields; passing--sync-resource-types=user,group,...as a flag also works via the CSV branch.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Traced this further — the readAsCSV branch does apply here, just not through viper's
Get()directly.DefineConfigurationV2(pkg/config/config.go:199) callscli.VisitFlags(mainCMD, v)right after registering pflags, beforecmd.Execute()ever runs. For an env-sourcedstringSlicefield,VisitFlags's default branch (pkg/cli/cli.go:176-179) doescmd.Flags().Set(f.Name, v.GetString(f.Name))— feeding the raw comma-separated env string into pflag's ownstringSliceValue.Set(), which CSV-splits viaencoding/csv(vendor/github.com/spf13/pflag/string_slice.go:22-29,42-54) and marks the flagChanged=trueas a side effect. By the time application code callsv.GetStringSlice("sync-resource-types")(commands.go:464etc.), viper's PFLAG-override branch fires (flag.HasChanged()is now true) and returns the pflag's already-correctly-split value — the raw env string never reachescast.ToStringSliceE's whitespace-splitting path at all.\n\nConfirmed empirically too: the passingtest-groupsCI run for this SHA scheduleslist-resourcesfor exactly the 4 intended types, with zero "invalid resource type" errors anywhere in the log.\n\nSoBATON_SYNC_RESOURCE_TYPESas a comma-separated env var does work as documented —VisitFlagsexists specifically to bridge env-sourced values through pflag's CSV parsing before anyGetStringSlice()call. No code change needed here.