docs: correct the update_profile action contract - #171
Conversation
Seven statements about update_profile in docs/connector.mdx and README.md described behaviour the connector does not have, or omitted a constraint a caller hits. Corrected against v0.5.0. - updated_user is reduced by the SDK to resource identity, displayName and the user trait; the profile carrying first_name, last_name and every non-trait custom_attributes key is dropped before the value reaches the caller. Both files described it as "the user resource after the update" with only a caveat about the field being absent. - The undefined-attribute result code is directory-dependent: OpenLDAP 2.6.6 returns 17, ApacheDS returns 16. Both files stated 17 unconditionally. - user_id takes the C1 account identifier, not the LDAP DN. A DN is rejected inside C1 before dispatch, so the failure never appears in connector logs. - An attribute push rule must map from a single-valued attribute, because the connector's user profile admits only single-valued attributes. - custom_attributes is a string map, so binary and option-tagged attributes cannot be set through the action. Previously unmentioned. - givenName is defined in RFC 4519 and mail in RFC 4524; only displayName is RFC 2798. The behaviour the sentence describes was already correct. - The "push profile" link pointed at a page that does not mention the feature; it now points at the attribute push rules page.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - **Collisions:** a `custom_attributes` key is dropped (never merged with, or overwriting, a named field's slot) and reported once in `skipped` when it case-insensitively matches either one of the four named field names, or the LDAP attribute a supplied named field is writing (`givenName`, `sn`, `displayName`, `mail`). The latter only applies when that named field was actually supplied and non-empty; otherwise `{"givenName": "Jane"}` is an ordinary raw write. | ||
| - **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. | ||
| - **Multi-valued attributes:** setting (not clearing) a value on an attribute that currently holds more than one value returns an error instead of silently discarding the extra values; clearing (an empty value) still removes all values. | ||
| - **Value types:** `custom_attributes` carries one string per attribute, so binary attributes (`jpegPhoto`, `userCertificate;binary`) and option-tagged attributes (`;lang-xx`) cannot be set through this action. |
There was a problem hiding this comment.
🟡 Suggestion: The option-tagged half of this claim doesn't hold. custom_attributes keys are passed through verbatim as the LDAP attribute type (pkg/connector/action.go:527 and :546 set PartialAttribute{Type: attrName}), and the read-back comparison uses GetEqualFoldAttributeValues(attrName), which fold-matches the full attribute description including the option. So a text-valued option-tagged attribute — description;lang-en, cn;lang-fr — is settable through this action today. The genuine constraint is the value type, not the option tag: a string map can't express binary content (jpegPhoto, userCertificate;binary). Consider narrowing to binary values only.
| Two requirements come from the C1 side rather than from LDAP: | ||
|
|
||
| - **`user_id` takes the C1 account identifier, not the LDAP DN.** C1 resolves the account to the connector's resource before dispatching the action. A DN fails inside C1 with `resource <dn> with type user was not found` and never reaches the connector, so it produces no connector log line and leaves the directory untouched. | ||
| - **An attribute push rule must map from a single-valued attribute.** The connector's user profile carries only attributes that hold exactly one value on the entry, so a multi-valued source resolves to nothing: the rule saves and enables, and each push reports zero attributes applied. |
There was a problem hiding this comment.
🟡 Suggestion: "holds exactly one value" is necessary but not sufficient. userResource admits an attribute into the profile only when len(v.Values) == 1 && !containsBinaryData(v.Values[0]) (pkg/connector/user.go:195), and containsBinaryData (user.go:160-167) rejects any rune outside ASCII 32-126. A single-valued sn: Müller or any non-Latin value is therefore also absent from the profile and produces the identical "rule enables, every push applies zero attributes" symptom this bullet attributes solely to multi-valuedness — the case most likely to be hit in a real directory. Worth naming alongside the multi-valued rule.
| The `update_profile` action returns `success` (bool), `updated_user` (the user resource after the update, best-effort re-fetched; absent if the read-back failed, though the write itself still succeeded), `applied` (the number of attributes changed), and `skipped` (named fields or `custom_attributes` entries that were not written). | ||
| The `update_profile` action returns `success` (bool), `updated_user` (the modified user resource, re-fetched after the write; absent if the read-back failed, though the write itself still succeeded), `applied` (the number of attributes changed), and `skipped` (named fields or `custom_attributes` entries that were not written). | ||
|
|
||
| `updated_user` carries the resource identity, `displayName`, and the user trait — not the entry's full attribute set. A value the action just wrote appears there only when it also feeds one of those: `display_name` through `displayName`, `email` through the trait's email list, and a `custom_attributes` key only when it maps to a trait field (`mail`, `displayName`, `sAMAccountName`, `userPrincipalName`, a non-RDN `uid` or `cn`, `lastLogonTimestamp`, `authTimestamp`). `first_name`, `last_name`, and every other `custom_attributes` key reach the directory but do not appear in `updated_user`. Use `applied` to confirm those. |
There was a problem hiding this comment.
🟡 Suggestion: displayName is listed inside the "maps to a trait field" parenthetical, but it isn't a trait field — it feeds Resource.DisplayName (pkg/connector/user.go:263), which the sentence already accounts for separately as one of the three carriers. Also, parseUserLogin (user.go:126) sources the login/alias set from objectGUID too, so the trait-field list is one short. Both are small, but this paragraph is the deliverable of the change.
Connector PR Review: docs: correct the update_profile action contractBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commit replaces the README claim that Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
README stated that provisioning must be enabled for actions to run. It is not required: with --provisioning absent and BATON_PROVISIONING unset, a full sync completes normally and update_profile still performs the LDAP modify, returning applied: 1 and gRPC OK. The flag gates five services -- grant management, resource management, resource delete, account provisioning and credential rotation -- and the SDK registers the action service outside that gate. No no-op stub exists for the action path, so no FailedPrecondition is reachable there. Actions are routed through a separate enablement option in the SDK CLI, and reported capabilities never consult the flag. The bind account still needs modify rights on the target entry; that half of the sentence was correct and is kept.
| Two requirements come from the C1 side rather than from LDAP: | ||
|
|
||
| - **`user_id` takes the C1 account identifier, not the LDAP DN.** C1 resolves the account to the connector's resource before dispatching the action. A DN fails inside C1 with `resource <dn> with type user was not found` and never reaches the connector, so it produces no connector log line and leaves the directory untouched. | ||
| - **An attribute push rule must map from a single-valued attribute.** The connector's user profile carries only attributes that hold exactly one value on the entry, so a multi-valued source resolves to nothing: the rule saves and enables, and each push reports zero attributes applied. |
There was a problem hiding this comment.
🟡 Suggestion: The README's new third C1-side note — that --provisioning / BATON_PROVISIONING does not gate actions (verified: RegisterActionServiceServer is called outside the opts.ProvisioningEnabled branch in the vendored SDK, and --invoke-action sets WithActionsEnabled() rather than provisioning) — has no counterpart here, even though every other clarification in this PR was mirrored in both files. Consider adding it to this "requirements from the C1 side" list, since the customer-facing doc is where readers are most likely to assume the flag is required. Confidence: medium; non-blocking.
2 participants
Summary
update_profileshipped with documentation in bothdocs/connector.mdxandREADME.md.End-to-end validation of the action against two directory implementations (ApacheDS 2.x and
OpenLDAP 2.6.6) and against a live C1 tenant found eight statements that describe behaviour
the connector does not have, or omit a constraint a caller hits. This corrects all eight.
The action's argument surface was already documented accurately — the four named-field
mappings, the collision rule, the clear/skip asymmetry, the guardrails and the multi-valued
refusal all match the shipped schema. Nothing in that part of the text changed.
Findings addressed
1.
updated_usercannot carry the attributes the action writes — verified in code,measured in the run.
pkg/connector/user.go:237puts every written value onResource.Profile, andresourceToBasicResource(vendor/.../baton-sdk/pkg/actions/args.go:274) copies into afive-field message where
profile,statusandcreated_athave no destination(
vendor/.../pb/c1/config/v1/config.pb.go:1238). Observed: a call writinggivenName,snand
titlereturnedresourceId,displayNameand theUserTraitannotation only. Thereduction is in the type, not in the helper, so no connector-side change alters it — the fix
is to document what the field carries and name the observable subset.
2. The undefined-attribute result code is directory-dependent — measured.
Same input, same build: OpenLDAP 2.6.6 returns 17 and the action fails
InvalidArgument;ApacheDS returns 16 and it fails
Unknown. Both files stated 17 without qualification.3.
user_idtakes the C1 account identifier, not the LDAP DN — measured.This connector's
userresource ID is the entry DN, so "resource ID" reads as "pass theDN". Doing that is rejected inside C1 before dispatch —
resource <dn> with type user was not found— which means no task reaches the connector, nothing appears in connector logs, andthe directory is untouched. The likeliest way for a customer to lose an afternoon.
4. A push rule must map from a single-valued attribute — measured.
userResource(pkg/connector/user.go:182-198) admits only single-valued attributes intothe profile. A rule sourcing a multi-valued attribute saves and enables normally, shows a
dash in the Data preview, and reports zero attributes applied on every push. Remapping the
same rule to a single-valued attribute resolved and wrote.
5. Binary and option-tagged attributes cannot be set — verified.
custom_attributesis a string map, sojpegPhoto,userCertificate;binaryand;lang-xxare inexpressible. Previously unmentioned in either file.
6.
givenNameandmailwere attributed to the wrong RFC — verified.RFC 4519 §2.12 defines
givenNameand RFC 4524 §2.16 definesmail; onlydisplayNamecomes from RFC 2798. The behavioural claim the sentence supports — result code 65 when the
entry lacks
inetOrgPerson— is correct and was confirmed on both directories, so only thecitation changed.
7. The "push profile" link pointed at the wrong page — verified.
/product/admin/account-provisioningcontains no occurrence of "push profile", "push rule"or "attribute push". Repointed to
/product/admin/push-rules, which documents the featurethe sentence names.
8. The README said provisioning gates the actions surface — measured, plus verified in the
pinned SDK.
README.mdstated that provisioning must be enabled for actions to run. With--provisioningabsent and
BATON_PROVISIONINGunset, a full sync completed normally andupdate_profilestillperformed the modify, returning
applied: 1and gRPCOK; the attribute was verified on theentry. In the SDK version this release pins,
internal/connector/connector.go:503registers theaction service unconditionally, immediately above the
if opts.ProvisioningEnabledblock thatgates grant management, resource management, resource delete, account provisioning and credential
rotation. No no-op stub exists for the action path, so no
FailedPreconditionis reachable there,and reported capabilities never consult the flag. The second half of the original sentence — the
bind account needs modify rights — was correct and is kept.
Why it mattered
Five of the seven are silent: the caller gets a success, or a green rule, or a failure that
leaves no log line, and the documentation gives no way to predict it. Findings 1, 3 and 4 in
particular each produce an outcome that looks like a connector bug and is not. Finding 8 runs the
other way: it told operators a write path was closed when it is open.
The published page has not picked up
update_profileyet, so correcting the in-repo texthere is what keeps the overstatement from reaching it.
Reviewer notes
they are the two claims that rest on an external spec rather than on this repo's code.
has since added a gate on the action path, say so and this bullet should change; the
measured behaviour of this release is unaffected either way.
AUTO-GENERATEDmarkers were added: no generator is wired for these sections.🤖 Generated with Claude Code