feat: add update_user_attrs action for arbitrary LDAP attributes#170
Conversation
Adds a global update_user_attrs action that sets or clears arbitrary LDAP attributes on an existing user, backing ConductorOne's profile-push flow (ACTION_TYPE_ACCOUNT_UPDATE_PROFILE) and also invocable directly. Modeled on baton-active-directory's action but adapted for generic LDAP: - Identity is the user DN (resource_id), not an objectGUID. - Fail-closed scope check against user-search-dn / base-dn. - Reads the entry first and drops no-op changes so re-runs are idempotent, avoiding brittle value read-back across directories that normalize values. - Uses a new non-swallowing LdapModifyStrict so genuine schema/permission rejections surface (the default LdapModify masks UnwillingToPerform et al.). - Aliases baton-ldap's synthetic profile keys (first_name->givenName, etc.); denies password* and objectClass; skips the RDN attribute; single-valued. CXP-806 Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
| } | ||
| log.Error("update_user_attrs: modify failed", fields...) | ||
| return nil, nil, fmt.Errorf("ldap-connector: update_user_attrs: failed to modify user %q: %w", acc.DN, err) |
There was a problem hiding this comment.
🟡 Suggestion: This modify failure returns a bare fmt.Errorf, unlike the other error paths in this handler which use status.Errorf. Since LdapModifyStrict surfaces real server rejections (permission denied, schema violation), the returned error lacks a gRPC status code, so the SDK maps it to Unknown and may retry/surface it incorrectly. Consider wrapping with an appropriate codes.Code (e.g. PermissionDenied/Internal). Low confidence — this mirrors the existing createOU pattern. (confidence: low)
Connector PR Review: feat: add update_user_attrs action for arbitrary LDAP attributesBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commit drops the optional, ignored Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
Add an integration test asserting a schema-violating modify (clearing the MUST attribute sn) surfaces as an error and does not partially apply, guarding the handler's non-swallowing modify path. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| } | ||
| } | ||
| log.Error("update_user_attrs: modify failed", fields...) | ||
| return nil, nil, fmt.Errorf("ldap-connector: update_user_attrs: failed to modify user %q: %w", acc.DN, err) |
There was a problem hiding this comment.
🟡 Suggestion: This modify failure returns a bare fmt.Errorf, unlike the other error paths in this handler which use status.Errorf. Since LdapModifyStrict surfaces real server rejections (permission denied, schema violation), the returned error lacks a gRPC status code, so the SDK maps it to Unknown and may retry/surface it incorrectly. Consider wrapping with an appropriate codes.Code (e.g. PermissionDenied/Internal). Low confidence — this mirrors the existing createOU pattern. (confidence: low)
There was a problem hiding this comment.
Evaluated this and I'm respectfully declining — the premise doesn't hold for the baton-sdk action framework (v0.19.1).
The gRPC code on a handler error is discarded, not mapped to Unknown and retried. In pkg/actions/actions.go, invokeGlobalAction runs the handler and, on error, calls oa.SetError(err) which stores only err.Error() (the string) in oa.Rv.Fields["error"] and sets status FAILED. GetActionStatus explicitly returns oa.Status/oa.Rv and not oa.Err (its comment: "Don't return oa.Err here … oa.Rv contains any error"), and the synchronous InvokeAction returns a nil RPC error alongside the FAILED status. The FAILED status is derived from oaErr != nil, independent of any codes.Code. So there's no code-based retry or surfacing to change here.
It's also already consistent with the codebase convention. createOU uses the same split: arg/validation failures → status.Errorf(codes.…) (action.go:219, 228), and the actual LDAP-operation failure → bare fmt.Errorf(…%w) (action.go:242, 251). updateUserAttrs mirrors that exactly; line 338 is the operation-failure case.
Switching to status.Errorf here would add no functional behavior, inject a noisier rpc error: code = … desc = prefix into the operator-facing error string, and drop the %w chain that in-process logging benefits from. Leaving as-is.
Add the update_user_attrs action to the Connector actions section: its arguments, return values, and the scope/single-value/denylist/RDN rules, plus the synthetic profile-key mapping. Matches the existing create_ou entry and the baton-okta docs style. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| fields = append(fields, zap.String("attr:"+ch.Modification.Type, fmt.Sprintf("len=%d", len(ch.Modification.Vals[0])))) | ||
| } | ||
| } | ||
| log.Error("update_user_attrs: modify failed", fields...) |
There was a problem hiding this comment.
🟡 Suggestion: A single modify rejection is now logged at Error three times — here, plus getConnection (client.go:101) and LdapModifyStrict (client.go:393) — and the SDK logs the returned error a fourth time. Many LDAP modify failures (e.g. insufficientAccessRights, schema violations) are customer-config conditions that are expected rather than connector bugs, so Error level here generates alert noise and retained error spans. Consider Warn for this handler-level log (which adds the useful attr-name/value-length detail), or dropping it since the lower layers already log the raw error. (confidence: medium, non-blocking)
There was a problem hiding this comment.
Good catch — addressed in 14a000c. This is valid: a modify rejection was logged at Error in three places (getConnection client.go:101, LdapModifyStrict client.go:393, and this handler), and most of these rejections (insufficientAccessRights, schema violations) are expected customer-config conditions rather than connector bugs.
Changes:
- Handler log
Error→Warn, keeping the attr-name/value-length detail. The error is still returned and surfaced as aFAILEDaction by the SDK. LdapModifyStrictlogError→Debug, since its sole caller logs a contextual error andgetConnectionalready logs the raw one.
Left getConnection (client.go:101) at Error — it's shared by every LDAP operation and predates this PR, so changing it is out of scope here.
Modify rejections in update_user_attrs (permission denied, schema violations) are expected customer-config conditions, not connector bugs, and were logged at Error up to three times (getConnection, LdapModifyStrict, handler). Downgrade the handler log to Warn (keeping the attr-name/value- length detail) and LdapModifyStrict's log to Debug (its sole caller logs a contextual error and getConnection already logs the raw one), reducing Error-level alert noise. Behavior is unchanged: the action still fails and surfaces the error. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Re: latest review suggestion (
|
The action always targets users (the entry is resolved under userFilter), so the resource_type argument was redundant and made the manual "Perform connector action" setup in the C1 UI confusing. Remove it from the schema. The handler never read resource_type, and the SDK invoke path validates only constraints (not that provided args match the declared schema), so the C1 push-profile pipeline sending a resource_type is still tolerated. A test asserts that. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
1 participant
Summary
Implements CXP-806 — an Update User action that sets or clears arbitrary LDAP attributes on an existing user, requested by IDMWorks.
The new global action
update_user_attrsbacks ConductorOne's profile-push flow (ACTION_TYPE_ACCOUNT+ACTION_TYPE_ACCOUNT_UPDATE_PROFILE) and is also invocable directly. It's modeled onbaton-active-directory's action of the same name but adapted for generic LDAP directories (OpenLDAP, 389-DS, OpenDJ, etc.). Active Directory and FreeIPA keep their own connectors and are out of scope.Behavior
resource_id), not an objectGUID — that's how baton-ldap identifies users.user-search-dn(orbase-dn) may be modified; out-of-scope / non-user DNs are reported asNotFound(no existence leak).applied: 0. This avoids brittle value-equality read-back across directories that normalize stored values (caseIgnoreMatchlowercasing, whitespace stripping, etc.).LdapModifyStrict, a non-swallowing modify path, because the defaultLdapModifymasksUnwillingToPerform/NoSuchAttribute/ etc. tonil(great for grant idempotency, bad for detecting a rejected attribute write).first_name→givenName,last_name→sn,display_name→displayName,user_id→uid;login/pathare skipped; everything else passes through as a raw LDAP attribute name (case-insensitive).userPassword/ anything containingpassword) andobjectClassare rejected; the RDN attribute is skipped (renaming needs a ModifyDN). Single-valued only (per theStringMapcontract); empty value clears the attribute; all changes go in one atomicModifyRequest.Returns
success,applied(count modified), andskipped(mask entries not written).Changes
pkg/connector/action.go— schema, registration inGlobalActions, handler, and pure helpers (buildUserAttrChanges,assertDNInScope,resolveUpdateAttrName,rdnAttrTypes).pkg/ldap/client.go—LdapModifyStrict(non-swallowing modify).pkg/connector/action_test.go— container-free unit tests for the pure helpers plus OpenLDAP-testcontainer integration tests for the handler.README.md— documents the action, its limitations, and the--provisioningrequirement.Testing
go build ./...,go vet ./...,gofmt, andgolangci-lint runare clean.TestAssertDNInScope,TestResolveUpdateAttrName,TestBuildUserAttrChanges). The Docker/OpenLDAP integration tests (TestUpdateUserAttrs) run in CI — Docker isn't available in the dev sandbox, same as the existingTestCreateOU/TestLdapGetRaw.🤖 Generated with Claude Code