feat: add update_profile action for LDAP attribute push - #170
feat: add update_profile action for LDAP attribute push#170c1-squire-dev[bot] wants to merge 15 commits into
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_profile action for LDAP attribute pushBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryThe new commits drop the unused Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
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>
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>
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>
| DisplayName: "Resource ID", | ||
| Description: "The distinguished name (DN) of the user to update.", | ||
| IsRequired: true, | ||
| Field: &config_sdk.Field_StringField{StringField: &config_sdk.StringField{}}, |
There was a problem hiding this comment.
if this is push attr I think this field needs to be ResourceIdField. based on the demo it looks like it should also be defined as a User resource action instead of Global Action, although not sure that's entirely necessary.
…date actions Pulls the canonicalize -> scope-check -> fetch -> diff -> modify pipeline out of updateUserAttrs into a new applyUserAttrUpdate helper so the upcoming resource-scoped update_profile action can reuse it. The empty-mask early return stays in updateUserAttrs itself; applyUserAttrUpdate always runs the full pipeline so a caller without that early return (update_profile) still gets NotFound on a vanished/out-of-scope user. Also, while touching this code: - add "email" -> "mail" to profileAttrAliases (previously unmapped, so an "email" mask entry tried a nonexistent raw attribute) - fix buildUserAttrChanges' case-sensitive attrs[maskName] lookup to fall back to a case-insensitive match (exact match still tried first) - thread actionName through buildUserAttrChanges so its password/objectClass denylist errors don't hardcode "update_user_attrs" - add a Warn log when a multi-valued attribute is about to be collapsed to a single value (existing behavior, now visible; not changed) - fix requireOptionalStructArg/requireOptionalStringSliceArg-style handling for attrs/attrs_update_mask so a present-but-wrong-typed argument errors instead of silently behaving as if absent - clarify the mask-entry precedence rule in buildUserAttrChanges' doc comment All 12 existing TestUpdateUserAttrs subtests pass unmodified; added a 13th covering empty-mask + malformed resource_id to guard the early-return placement above. Extended TestBuildUserAttrChanges and TestResolveUpdateAttrName with cases for the email alias, case-fold fallback, multi-value pinning, and mask-entry precedence.
Adds a new update_profile action, scoped to the "user" resource type, that lets callers set first_name/last_name/display_name/email by name (each resolved through the existing profileAttrAliases table) plus arbitrary raw LDAP attributes via a custom_attributes map, reusing the applyUserAttrUpdate pipeline extracted in the prior commit. Key behaviors: - named fields are applied only when present and non-empty (they cannot be used to clear an attribute); a present-but-empty named field is dropped from the write but reported in the "skipped" return field rather than vanishing silently - custom_attributes entries are written whenever present, including empty (clears), consistent with the existing update_user_attrs action - a custom_attributes key that case-insensitively collides with one of the four named argument names is dropped (never merged into/overwriting the named field's slot) and reported once in "skipped" - mask order is deterministic: named fields in declared order, then custom_attributes keys sorted ascending -- needed both for stable first-entry-wins precedence and to keep output independent of Go's randomized map iteration - no empty-mask early return: unlike update_user_attrs, this handler always runs the full pipeline, so a vanished or out-of-scope user still surfaces as NotFound even when every field sent was empty - best-effort read-back after a successful write populates the returned updated_user field; a read-back or encoding failure never turns an already-successful write into a reported failure - updateProfileActionSchema() returns a fresh struct on every call (not a package-level var), since RegisterResourceAction stamps ResourceTypeId onto the schema in place at registration time Adds pkg/connector/user_actions_test.go: pure-function tests for buildProfileUpdate and updateProfileActionSchema (run and pass here) plus a testcontainer-based TestUpdateProfile handler suite that requires Docker and is compile-checked only in this environment (no container runtime available here).
buildUserAttrChanges previously replaced a multi-valued LDAP attribute with a single supplied value whenever it currently held more than one value, silently discarding every other existing value (success:true, no error, no visible signal to the caller). Confirmed against a real slapd server: a user with two mail addresses lost both originals when updated via update_profile to set a single new email. Now, setting a non-empty value on an attribute that currently holds more than one value aborts the whole batch with an InvalidArgument-class error naming the attribute and its current value count, matching the existing password/objectClass denylist pattern. Clearing (empty value) a multi-valued attribute is unaffected -- that remains an explicit, intentional "remove all values" operation and still succeeds with no error, since LDAP Replace-with-no-values correctly clears every value. Removes the now-redundant pre-emptive Warn log for this case (the hard error is the signal). Updates the TestBuildUserAttrChanges pinning test that documented the old collapse behavior to assert the new hard-error behavior instead, and adds a case confirming clearing still works. Re-verified live against slapd: both mail values on a real multi-valued user survive an update_profile/update_user_attrs attempt to overwrite with a single value (now InvalidArgument instead of data loss), and clearing via custom_attributes still removes all values successfully.
Add a section to README.md and docs/connector.mdx for the new resource-scoped update_profile action, mirroring the existing update_user_attrs documentation style: arguments/returns tables, the named-field alias mapping, custom_attributes clear semantics, the collision rule, the password/objectClass/RDN restrictions, and the new multi-valued-attribute hard-error behavior. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
johnallers
changed the title
| acc, err := getAccount(ctx, client, targetDN.String()) | ||
| if err != nil { | ||
| log.Debug(actionName+": user lookup failed", zap.String("dn", targetDN.String()), zap.Error(err)) | ||
| return nil, status.Errorf(codes.NotFound, "ldap-connector: %s: user not found", actionName) |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): every getAccount failure is collapsed into NotFound and logged at Debug, not just "entry absent". getAccount → LdapGetWithStringDN also returns connection/bind/permission/"multiple entries found" errors, so a transient LDAP outage during a profile push reports the user as missing rather than as an infrastructure failure — and the only trace is a Debug line. Consider distinguishing the client's codes.NotFound (keep the fail-closed NotFound) from other errors (return Internal/Unavailable and log at Warn).
| // value) a multi-valued attribute is unaffected and still succeeds -- | ||
| // that is an explicit, intentional "remove all values" operation, not | ||
| // data loss. | ||
| func buildUserAttrChanges(ctx context.Context, entry *ldap.Entry, targetDN *ldap3.DN, attrs map[string]string, mask []string, actionName string) ([]ldap3.Change, []string, error) { |
There was a problem hiding this comment.
🟡 Suggestion: the new ctx context.Context parameter is never used in buildUserAttrChanges — the function is pure and does no I/O or logging. Unused parameters are called out by the connector review criteria (R2); either drop it or use it (e.g. ctxzap.Extract(ctx).Debug(...) for the skip decisions).
| if !ok { | ||
| return nil, fmt.Errorf("%s must be a list of strings", key) | ||
| } | ||
| result := make([]string, 0, len(listValue.ListValue.Values)) |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): listValue.ListValue.Values dereferences the inner message directly. A Value whose kind is *structpb.Value_ListValue with a nil ListValue would panic here. Protobuf unmarshalling normally allocates it, so this is defensive, but the nil-safe listValue.ListValue.GetValues() costs nothing (same for len(...) on line 229).
There was a problem hiding this comment.
Resolved in 5b85c67 — requireOptionalStringSliceArg was removed.
| - **Scope:** only entries within the configured user search scope (`user-search-dn`, falling back to `base-dn`) can be modified; out-of-scope or non-user DNs are rejected. | ||
| - **Single-valued:** each attribute is set to a single value; multi-valued and binary attributes are not supported. | ||
| - **Not modifiable:** password attributes (`userPassword`, or any name containing `password` — use credential rotation instead) and `objectClass` are rejected; the user's RDN attribute (for example `cn` when the DN is `cn=jdoe,...`) is skipped, since renaming requires a different operation. | ||
| - **Profile keys:** C1's profile field names are mapped to their LDAP attributes (`first_name` → `givenName`, `last_name` → `sn`, `display_name` → `displayName`, `user_id` → `uid`); any other name is treated as a raw LDAP attribute name. |
There was a problem hiding this comment.
🟡 Suggestion: this bullet is now stale for update_user_attrs. The head commit added "email": mail to profileAttrAliases (pkg/connector/action.go:53), so the mapping list should include email → mail. The Single-valued bullet on line 60 is stale too: setting a non-empty value on an attribute that already holds >1 value is now a hard error (pkg/connector/action.go:586), not a silent single-value replace — same wording as the update_profile note on line 82. README.md:94-96 has both gaps as well.
There was a problem hiding this comment.
Resolved in 5b85c67 — the update_user_attrs section was removed.
| | `resource_id` | yes | The distinguished name (DN) of the user to update. | | ||
| | `attrs` | yes | A map of attribute name → value. An empty value clears the attribute. | | ||
| | `attrs_update_mask` | yes | The subset of attribute names in `attrs` to actually write. | | ||
| | `resource_type` | no | The resource type; always `user` for this action. | |
There was a problem hiding this comment.
🟡 Suggestion: updateUserAttrsActionSchema() declares only resource_id, attrs, and attrs_update_mask — there is no resource_type argument, and the handler never reads one. docs/connector.mdx:48 correctly omits it. Drop this row (or add the argument to the schema if it is actually expected).
There was a problem hiding this comment.
Resolved in 5b85c67 — the update_user_attrs section was removed.
| { | ||
| Name: argCustomAttributes, | ||
| DisplayName: "Custom Attributes", | ||
| Description: "Map of arbitrary raw LDAP attribute name to value, for attributes beyond the named fields " + |
There was a problem hiding this comment.
🟡 Suggestion: custom_attributes is documented here (and in README.md:118 / docs/connector.mdx:79) as "arbitrary raw LDAP attribute name", but each key still passes through resolveUpdateAttrName in buildUserAttrChanges. So custom_attributes: {"user_id": "x"} writes uid, and keys login / path are silently dropped into skipped rather than written as literal attributes. buildProfileUpdate's collision guard only reserves the four profileNamedArgs. Either extend the guard to the rest of profileAttrAliases/profileSyntheticSkip, or document that synthetic profile keys are reserved in custom_attributes too.
update_profile (added in 9843aa7) already covers every case update_user_attrs handled: custom_attributes accepts any raw LDAP attribute name with the same set/clear semantics, and it's equally invocable generically. Confirmed against a live slapd server via the CLI's generic --invoke-action mechanism that update_profile still works end-to-end and update_user_attrs is no longer registered. Removes the schema, handler, result helper, and action-exclusive constants/helpers (updateUserAttrsActionSchema, updateUserAttrs, updateUserAttrsResult, requireOptionalStringSliceArg, actionNameUpdateUserAttrs, argResourceID, argAttrs, argAttrsUpdateMask) from pkg/connector/action.go. Keeps every helper shared with update_profile (applyUserAttrUpdate, buildUserAttrChanges, resolveUpdateAttrName, assertDNInScope, rdnAttrTypes, profileAttrAliases, profileSyntheticSkip) untouched aside from stale comment references. Drops the corresponding handler test (TestUpdateUserAttrs) while keeping all pure-helper regression tests (adapted to use a local test-only action name instead of the removed constant). Updates README.md and docs/connector.mdx to drop the update_user_attrs section and rewrite update_profile's cross-references so they state its restrictions directly instead of contrasting against the removed action.
johnallers
changed the title
| map[string]string{"userPassword": "secret"}, []string{"userPassword"}, "update_profile") | ||
| require.Error(t, err) | ||
| require.Contains(t, err.Error(), "update_profile") | ||
| require.NotContains(t, err.Error(), testActionName) |
There was a problem hiding this comment.
🟡 Suggestion: this assertion is now vacuous. It previously guarded against buildUserAttrChanges hardcoding "update_user_attrs" — a name the implementation actually once used. Now it asserts the error doesn't contain "test_action", a name never passed into this call and that no plausible implementation would hardcode, so the R1 regression guard can no longer fail. Same at line 441. Consider calling twice with two different action names (e.g. "action_a" then "action_b") and asserting each error contains its own name and not the other's.
| // violations) are expected customer-config conditions, not connector bugs, | ||
| // so they shouldn't trip Error-level alerting. The error is still returned | ||
| // and surfaced as a FAILED action by the SDK. | ||
| log.Warn(actionName+": modify failed", fields...) |
There was a problem hiding this comment.
Team hard-ban on Warn in connector code — this modify failure already returns an error the SDK surfaces as FAILED, so the Warn only inflates OTEL/alerting noise.
Drop it, or downgrade to Debug.
| // so they shouldn't trip Error-level alerting. The error is still returned | ||
| // and surfaced as a FAILED action by the SDK. | ||
| log.Warn(actionName+": modify failed", fields...) | ||
| return nil, fmt.Errorf("ldap-connector: %s: failed to modify user %q: %w", actionName, acc.DN, err) |
There was a problem hiding this comment.
This modify failure returns a bare fmt.Errorf, so it lands as codes.Unknown and the SDK can't tell a retryable 429/5xx-style LDAP blip from a terminal schema/permission reject. Sibling paths in this helper already use status.Errorf.
Map the LDAP result (permission/schema → PermissionDenied/InvalidArgument, unavailable/timeout → Unavailable) and return status.Errorf (or uhttp.WrapErrors) so the classification survives. Same-file pattern: the getAccount / buildUserAttrChanges branches just above.
| acc, err := getAccount(ctx, client, targetDN.String()) | ||
| if err != nil { | ||
| log.Debug(actionName+": user lookup failed", zap.String("dn", targetDN.String()), zap.Error(err)) | ||
| return nil, status.Errorf(codes.NotFound, "ldap-connector: %s: user not found", actionName) |
There was a problem hiding this comment.
Every getAccount failure is collapsed into NotFound (and logged at Debug). getAccount also fails on bind/connection/permission/"multiple entries" — those are not "user missing", and folding them into terminal NotFound stops the SDK from retrying transients.
Reserve NotFound for absent/out-of-scope; propagate other lookup errors with their real gRPC code (Unavailable / PermissionDenied) via %w or status.Errorf.
| # Superpowers planning docs and agent scratch (local only, not part of the connector) | ||
| docs/superpowers/ | ||
| .superpowers/ | ||
| .task-worktrees/ |
There was a problem hiding this comment.
.task-worktrees/ is a local agent/workflow artifact, not something every clone of this connector needs to ignore. Putting it in the tracked .gitignore ships a personal-setup change on a feature PR.
Drop this line from the PR and add it to your clone-local exclude instead (.git/info/exclude) — same effect, never committed.
| // landed by this point, so a read-back or resource-encoding problem should | ||
| // not turn a successful modify into a reported failure. | ||
| if entry, rerr := getAccount(ctx, u.client, result.DN); rerr != nil { | ||
| log.Warn("update_profile: read-back failed", zap.String("dn", result.DN), zap.Error(rerr)) |
There was a problem hiding this comment.
Same Warn ban as the modify path — these three read-back / encode failures are best-effort by design (the write already landed), so Warn only adds noise.
Downgrade all three (read-back failed / encoding updated user / encoding updated_user return field) to Debug, or drop them.
| "key that collides with a named field above is dropped and reported in skipped, never merged with it. Password, " + | ||
| "objectClass, and RDN attributes cannot be modified through this action.", | ||
| ActionType: []v2.ActionType{ | ||
| v2.ActionType_ACTION_TYPE_ACCOUNT, |
There was a problem hiding this comment.
Suggestion: leave only ACTION_TYPE_ACCOUNT_UPDATE_PROFILE here and drop ACTION_TYPE_ACCOUNT.
For an action registered on a resource type, the SDK's duplicate-type check rejects the same ActionType appearing in more than one action registered for that resource type — so keeping ACCOUNT here reserves it for update_profile and would collide with any future user-scoped action that also declares it (e.g. enable/disable). baton-box documents exactly this in code: https://github.com/ConductorOne/baton-box/blob/6c292ab31949c9a3f397b6e2aa7a7745e792aafe/pkg/connector/actions.go#L27-L29 — and baton-zuora's resource-scoped update_profile uses the single type too: https://github.com/ConductorOne/baton-zuora/blob/2b17feeafa7d2e496c553f9922c2e15bc320380e/pkg/connector/users_actions.go#L30
|
|
||
| | Action name | Additional fields | Description | | ||
| | ----------- | ----------------- | ----------- | | ||
| | `update_profile` | `user_id` (resource ID, required), `first_name` (string), `last_name` (string), `display_name` (string), `email` (string), `custom_attributes` (string map) | Set core profile fields (first name, last name, display name, email) and/or arbitrary custom LDAP attributes on an existing user. This action is scoped to the `user` resource type, which is what makes it discoverable and usable from C1's attribute-push-rule feature. It also backs C1's [push profile](/product/admin/account-provisioning) flow for LDAP users. | |
There was a problem hiding this comment.
This sentence promises more than the PR ships. Registering update_profile on the user resource type is what the attribute-push rule needs, but C1's automated profile-sync flow resolves a connector-wide action that takes a user_profile argument — that's the one this PR removed, so the push-profile claim doesn't hold with only this action.
Suggestion: either register that connector-wide action too (baton-microsoft-entra ships both, sharing one helper: https://github.com/ConductorOne/baton-microsoft-entra/blob/e0995cd14bee7b82d26f92b8f4fdff3f31b52a9f/pkg/connector/actions.go#L120), or drop the "also backs C1's push profile flow" sentence so the docs only claim the attribute-push path.
update_profile is resource-scoped, but was also tagged with the broader ACTION_TYPE_ACCOUNT. The SDK rejects registering a second action on the same resource type that repeats any ActionType tag (vendor/.../pkg/actions/actions.go:268-284), so carrying ACCOUNT here needlessly blocks any future user-scoped action from using that tag. Keep only ACTION_TYPE_ACCOUNT_UPDATE_PROFILE.
.task-worktrees/ is a Squire/agent-workflow artifact, not something every clone of this connector needs to ignore (unlike docs/superpowers/ and .superpowers/, which are connector-adjacent scratch). Moved the local convenience ignore into .git/info/exclude instead.
…anges ctx was only ever used to fetch a logger via ctxzap.Extract for the multi-value Warn log path; that path was removed in cc08f06 when it became a hard error instead, leaving ctx dead in the function body. Drop it from the signature and update all call sites (action.go and the ~20 call sites in TestBuildUserAttrChanges), removing the now-unused local ctx declaration in that test too.
The two "names the calling action, not a hardcoded one" subtests only checked that testActionName was absent from an error produced by a call that never passed testActionName in the first place -- true regardless of whether the implementation hardcoded some other wrong string. Add a second call in each subtest using testActionName as the actionName argument and assert the error tracks that value instead of "update_profile", proving the message follows whichever actionName was actually passed rather than merely lacking one unrelated string.
Of update_profile's four named fields, only last_name (sn) is universal (a MUST attribute of bare person). first_name (givenName), display_name (displayName), and email (mail) are only defined by RFC 2798's inetOrgPerson object class, so writing one of them to a non-inetOrgPerson entry fails loudly with LDAP result code 65 (Object Class Violation) -- safe (atomic, no partial write) but previously undocumented.
| `update_profile` is intended for generic LDAP directories (Active Directory and FreeIPA have their own connectors). It applies the following rules: | ||
|
|
||
| - **Named fields:** `first_name` → `givenName`, `last_name` → `sn`, `display_name` → `displayName`, `email` → `mail`. A named field is applied only when present and non-empty; a present-but-empty named field cannot clear the attribute and is instead reported in `skipped`. | ||
| - **`inetOrgPerson` requirement:** only `last_name` (`sn`) is universal — it's a MUST attribute of the base `person` object class. `first_name` (`givenName`), `display_name` (`displayName`), and `email` (`mail`) are only defined by RFC 2798's `inetOrgPerson` object class, so writing one of them to an entry that doesn't carry `inetOrgPerson` fails loudly with LDAP result code 65 ("Object Class Violation") — an atomic, clearly-signaled failure with no partial write, not a silent no-op. |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): "only defined by RFC 2798's inetOrgPerson" is stronger than the schemas actually are — mail is a COSINE attribute (RFC 4524) permitted by e.g. pilotPerson, and AD's user class allows mail/givenName/displayName without inetOrgPerson. The practical warning is right, but an operator whose entries carry person + a COSINE class will be told these fields can't work when they can. Consider softening to "not part of the base person class — they require inetOrgPerson or another class that permits them, otherwise the write fails with result code 65."
| continue | ||
| } | ||
|
|
||
| if strings.Contains(strings.ToLower(attrName), "password") { |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): the password guardrail is a substring match on "password", so it misses Active Directory's unicodePwd/dBCSPwd — and userFilter does include (objectClass=user), so AD entries are reachable by this action even though the docs call AD out of scope. AD would reject a plaintext write to unicodePwd anyway, so impact is low, but adding those two names (and pwdHistory) to an explicit denylist alongside the substring check would make the "password attributes cannot be modified" promise hold across the object classes this connector actually matches.
3 participants
Summary
Implements CXP-806 — an action that lets ConductorOne push profile-attribute changes into an LDAP directory, requested by IDMWorks.
This PR ships a single resource-scoped action,
update_profile, exposing four common profile fields (first_name,last_name,display_name,email) plus acustom_attributesmap for any other LDAP attribute by raw name.Note on scope vs. the original approach: earlier revisions of this PR shipped
update_profilealongside a separate global action,update_user_attrs, with a generic attribute-map + update-mask interface. That global action has been removed: it was resource-unscoped, which means ConductorOne's attribute-push-rule feature could never discover or offer it as a target (that feature requires an action namedupdate_profile, scoped to theuserresource type, taggedACTION_TYPE_ACCOUNT_UPDATE_PROFILE) — so it never actually satisfied this ticket's goal. Onceupdate_profileexisted, itscustom_attributesfield turned out to be a strict superset of what the global action did (same arbitrary-attribute-name, same set/clear-via-empty-string semantics, and equally invocable directly/generically, not just through push rules) — confirmed directly against a real LDAP server, invoking both through the same generic action-invoke path. Keeping both would have meant two ways to do the same thing with no functional difference, so the redundant one was dropped. (Flagging since the ticket title itself referencesupdate_user_attrsby name — the delivered action isupdate_profile, which supersedes it.)Modeled on
baton-active-directory's equivalent action 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
user_id, passed as a resource reference), not an objectGUID.user-search-dn(orbase-dn) may be modified; out-of-scope / non-user DNs are reported asNotFound(no existence leak).applied: 0.LdapModifyStrictis a non-swallowing modify path, since the defaultLdapModifymasksUnwillingToPerform/NoSuchAttribute/ etc. tonil.first_name→givenName,last_name→sn,display_name→displayName,email→mail. Applied only when present and non-empty — they can't be used to clear an attribute (empty is treated as "not supplied" and reported inskipped).custom_attributes: arbitrary raw LDAP attribute name → value, matched case-insensitively. An empty value clears the attribute. A key that collides with one of the four named fields above is dropped (named field wins) and reported once inskipped.userPassword/ anything containingpassword) andobjectClassare rejected; the RDN attribute is skipped (renaming needs a ModifyDN).Returns
success,updated_user(the user resource re-fetched after the update, best-effort),applied(count modified), andskipped(fields/attributes not written).Changes
pkg/connector/user_actions.go—update_profileschema,buildProfileUpdate, handler, andResourceActionsregistration on the user resource type.pkg/connector/action.go— the sharedapplyUserAttrUpdatehelper andbuildUserAttrChanges(case-insensitive attribute matching, alias table includingemail→mail, hard-error on multi-value collapse, RDN/password/objectClass guardrails),assertDNInScope,resolveUpdateAttrName,rdnAttrTypes.pkg/ldap/client.go—LdapModifyStrict(non-swallowing modify).pkg/connector/action_test.go,pkg/connector/user_actions_test.go— container-free unit tests for the pure helpers, plus OpenLDAP-testcontainer integration tests for the handler.README.md,docs/connector.mdx— document the action, its arguments/returns, and behavioral notes.Testing
go build ./...,go vet ./...,gofmtare clean.TestAssertDNInScope,TestResolveUpdateAttrName,TestBuildUserAttrChanges,TestBuildProfileUpdate,TestUpdateProfileActionSchema).slapdinstance, driven through the compiledbaton-ldapCLI's generic action-invoke path) — round-trip correctness, thecustom_attributescollision rule, clear-vs-set semantics, the password/objectClass/RDN guardrails, scope/not-found handling, and the multi-valued-attribute hard-error, all confirmed with directldapsearchevidence before and after each write. Also confirmed--list-action-schemasshows onlyupdate_profile(scoped touser) andcreate_ou— the removed global action is gone.TestUpdateProfile) compiles and runs in CI; not runnable in the local dev sandbox used for this update (no Docker available there), consistent with the existingTestCreateOU/TestLdapGetRaw.🤖 Generated with Claude Code