[EPD-2938] Support Employee Information attributes in account provisioning - #130
[EPD-2938] Support Employee Information attributes in account provisioning#130c1-squire-dev[bot] wants to merge 3 commits into
Conversation
…oning Account creation previously set only the primary email, given/family name, and password: the Employee Information attributes were writable only through the update_user_profile / update_user connector actions, so a joiner needed a second Automation step to apply department, manager, and friends. CreateAccount now reads department, job_title, cost_center, employee_type, employee_id, and manager_email from the ConductorOne account profile and applies them in the same users.insert call, shaping each field with the same builders the update path uses (organizations[0] for department/title/cost center/employee type, an externalIds entry of type organization for the employee ID, and a relations entry of type manager). The accepted profile keys and their aliases are now defined once, in employeeInfoJSONFields, and shared with profileFromJSON so the create and update paths cannot drift on which keys they understand. Empty values are dropped rather than sent - an empty string means "clear" on the update path, but a brand-new account has nothing to clear, and sending one anyway would create a phantom empty organization. A malformed manager_email or a wrong-typed value fails before the insert instead of being skipped: the update path can report a partial success through skipped_fields, CreateAccount has no such channel, and failing pre-insert leaves nothing half-configured. Also makes update_user (the ACCOUNT_UPDATE_PROFILE action that C1 push rules drive) accept its user_id as a plain string as well as a resource reference, matching every other user-scoped action in this connector. Google's userKey accepts a primary email or Google user ID, so an automation author holding either previously had the call rejected before it ever reached the Directory API - the misconfiguration this ticket calls out. Recovery email/phone and custom-schema attributes are deliberately left out of the creation path; they remain action-only. 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}
| func applyEmployeeInfoToNewUser(user *admin.User, patch userProfilePatch) error { | ||
| for _, dest := range []*(*string){ | ||
| &patch.department, &patch.jobTitle, &patch.costCenter, | ||
| &patch.employeeType, &patch.employeeID, &patch.managerEmail, | ||
| } { | ||
| if *dest != nil && **dest == "" { | ||
| *dest = nil | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this makes CreateAccount newly fail on profile keys it previously ignored entirely. An existing tenant whose account profile already maps department/title/manager_email/employee_id (the sync read path emits all of those, so mappings commonly carry them) will now hard-fail joiner provisioning on a malformed value — e.g. a whitespace-only manager_email survives the == "" check here and then fails mail.ParseAddress, and a numeric employee_id fails in stringFromJSON. The hard-fail choice is well argued in the PR description, but consider at least strings.TrimSpace before the emptiness check so blank-ish values are treated as absent rather than aborting the insert.
| // the synced resource ID, and an automation author who has one of those | ||
| // (rather than a ConductorOne-internal resource ID) previously had the call | ||
| // rejected here before it ever reached the Directory API. | ||
| userId, err := extractUserId(args, l, actionUpdateUser) |
There was a problem hiding this comment.
🟡 Suggestion: on the missing-user_id branch, extractUserId (pkg/connector/helpers.go:87) logs the whole args struct with zap.Any("args", args). For update_user that struct contains user_profile, which can carry recovery_email, recovery_phone, names, and custom-schema values — PII that this handler never logged before. Consider logging only the action name (or the arg keys) rather than the full args for this call site.
Connector PR Review: [EPD-2938] Support Employee Information attributes in account provisioningBlocking Issues: 0 | Suggestions: 8 | Threads Resolved: 0 Review SummaryThe new commit inverts the create-path strictness: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Three defects found reviewing the Employee Information provisioning change. extractUserId's plain-string fallback selects args.Fields directly, which panics on a nil *structpb.Struct. GetResourceIDArg nil-guards, so the previous inline check in update_user returned a clean InvalidArgument; routing through the shared helper made a no-argument invocation panic instead (recovered by the SDK as "panic in action handler"). Guarding in extractUserId covers all nine user-scoped actions, eight of which were already exposed. A whitespace-only value survived the empty-drop loop, so a profile carrying " " for manager_email - routine in HRIS- and CSV-sourced account profiles - reached mail.ParseAddress, failed, and aborted the whole create. Whitespace-only now counts as empty for all six attributes, matching the documented "empty values are dropped" rule. mail.ParseAddress also accepts the display-name form, but the raw input was stored rather than the parsed address, so "Jane Doe <jane@example.com>" produced a manager relation matching no Google user and reading back verbatim on the next sync. The bare address is now stored. Each fix has a regression test that fails against the prior source. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| return uhttp.WrapErrors(codes.InvalidArgument, | ||
| fmt.Sprintf("google-workspace: invalid manager_email: %s", *patch.managerEmail), err) | ||
| } | ||
| user.Relations = buildManagerRelations(nil, addr.Address) |
There was a problem hiding this comment.
🟡 Suggestion: The create path now stores addr.Address, but the update paths still store the raw string — buildManagerRelations(currentRelations, *patch.managerEmail) at line 1211 and buildManagerRelations(currentRelations, managerEmail) at line 838 both pass the caller's input through unnormalized. So a joiner created from an account profile carrying Jane Doe <jane@example.com> gets a resolvable manager relation, and the very next update_user/update_user_manager from that same profile overwrites it with the display-name form that (per this comment's own reasoning) matches no user. Normalizing inside buildManagerRelations instead would fix both paths and make the "create and update produce identical wire values" claim in this function's doc comment true again.
| // real value would persist blank padding onto the new account and - for | ||
| // manager_email, which is validated below - fail the entire create on a | ||
| // profile that simply carries no manager. | ||
| if *dest != nil && strings.TrimSpace(**dest) == "" { |
There was a problem hiding this comment.
🟡 Suggestion: TrimSpace is used only for the emptiness test, so the value that is stored keeps its padding — the same HRIS/CSV profiles that motivate this loop will write " Engineering " as the department, " E-1234 " as the employee ID, etc. Only manager_email gets a trimmed value written (via addr.Address below). Consider trimming in place (v := strings.TrimSpace(**dest); if v == "" { *dest = nil } else { *dest = &v }) so all six fields are normalized consistently.
The six Employee Information attributes were declared IsRequired: false, but a wrong-typed value or an unusable manager_email failed users.insert outright, so in practice a malformed one was required to be correct. An HRIS-sourced profile routinely carries employee_id or cost_center as a JSON number, and a manager who has not been provisioned yet, so the joiner lost their account over data that only enriches it. Account creation now drops any attribute it cannot use - empty, whitespace-only, wrong-typed, or an unparseable manager_email - and creates the account regardless, naming each dropped attribute and its reason in a Warn log line. Warn rather than Error: a misconfigured attribute mapping is a customer-side condition and the operation still succeeds. update_user applies the attributes once the profile is corrected. Alias resolution degrades the same way, so a numeric job_title no longer discards a usable title alongside it. The update path keeps its strict rejection: there the account already exists, so failing loudly costs nothing, and the action reports partial success through skipped_fields. profileFromJSON and its tests are unchanged. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
|
|
||
| Yes: | ||
| - **Create/Delete user accounts** — via Directory API `users.insert` and `users.delete`. New accounts are created with a generated random password. | ||
| - **Create/Delete user accounts** — via Directory API `users.insert` and `users.delete`. New accounts are created with a generated random password. Creation also applies any Employee Information attributes carried on the ConductorOne account profile in the same `users.insert` call — `department`, `job_title` (also `jobTitle`/`title`), `cost_center`, `employee_type`, `employee_id`, and `manager_email` (each also accepted in camelCase) — mapping to `organizations[0]` (`department`/`title`/`costCenter`/`description`), an `externalIds` entry of type `organization`, and a `relations` entry of type `manager`. All six are optional and never fail account creation: empty, whitespace-only, wrong-typed, and unusable-`manager_email` values are dropped (each named with a reason in a `Warn` log line) and the account is still created, with `update_user` available to apply them afterwards. Recovery email/phone and custom-schema attributes remain action-only. |
There was a problem hiding this comment.
🟡 Suggestion: this says empty and whitespace-only values are dropped "(each named with a reason in a Warn log line)", but applyEmployeeInfoToNewUser nils those out in the trimming loop (user_actions.go:1461) before dropped is built, so only wrong-typed values and an unusable manager_email ever reach dropped_fields. An operator reading this will look for a log line that never appears. Suggest scoping the "named with a reason" clause to wrong-typed/unusable values and keeping empty/whitespace-only as "simply not sent" (the wording README.md already uses).
| switch { | ||
| case err != nil: | ||
| dropped = append(dropped, | ||
| fmt.Sprintf("%s (not a valid email address: %q)", argManagerEmail, *patch.managerEmail)) |
There was a problem hiding this comment.
🟡 Suggestion: this drop reason embeds the raw manager_email value, and the string ends up in a Warn-level dropped_fields log at user.go:506. The wrong-typed messages deliberately name only the key and the JSON type — this one echoes what is often a real person's name or address ("Jane Doe", a typo'd address) into logs on a routine joiner flow. Consider naming the field and the failure without the value (e.g. manager_email (not a valid email address)), keeping the connector's "no PII in logs" posture consistent.
| require.NotNil(t, resp) | ||
| require.Equal(t, 1, state.insertCount) | ||
|
|
||
| require.Nil(t, extractRelations(state.lastInsertBody), "the unusable manager relation is dropped") |
There was a problem hiding this comment.
🟡 Suggestion: this pins that the relation is absent but never asserts the drop is reported. The dropped_fields log line is now the only signal an operator gets that the manager was skipped (the hard failure it replaced is gone), yet applyEmployeeInfoToNewUser's returned description for an unusable manager_email has no test — the wrong-typed equivalent does (TestEmployeeInfoFromProfile_WrongTypedValuesAreReported). Consider asserting on applyEmployeeInfoToNewUser's return for this case too.
Summary
Account provisioning previously created a bare Google Workspace account — primary email, given/family name, password — and the Employee Information attributes were writable only through the
update_user_profile/update_userconnector actions (shipped in v0.2.60/v0.2.61). A joiner therefore needed a second Automation step to set department, manager, and friends.1. Create.
CreateAccountnow readsdepartment,job_title,cost_center,employee_type,employee_id, andmanager_emailfrom the ConductorOne account profile and applies them in the sameusers.insertcall:departmentorganizations[0].departmentjob_titlejobTitle,titleorganizations[0].titlecost_centercostCenterorganizations[0].costCenteremployee_typeemployeeTypeorganizations[0].descriptionemployee_idemployeeIdexternalIds[]entry of typeorganizationmanager_emailmanagerEmailrelations[]entry of typemanagerEach field is shaped by the same builders the update path uses (
buildUpdatedOrganizations/buildUpdatedExternalIDs/buildManagerRelations), starting from an empty current state, so create and update produce identical wire values. The accepted keys and aliases now live in one place (employeeInfoJSONFields) shared withprofileFromJSON, so the two paths cannot drift — a key understood on create but dropped on update (or vice versa) would silently break a mover flow.Two behaviours worth reviewing explicitly:
employee_idas a JSON number), and an unparseablemanager_email(a manager not yet provisioned, or a display name where an address was expected). Each dropped attribute is named with a reason in aWarnlog line (dropped_fields), andupdate_userapplies it once the profile is corrected. The alternative — failing the insert — costs the joiner their entire account over data that only decorates it.skipped_fields.profileFromJSONand its tests are unchanged; only the create path degrades gracefully.manager_emailis normalized to the bare address before it is stored:mail.ParseAddressalso accepts the display-name form, but Google resolvesrelations[].valueonly as an email, so storing the raw string would produce a manager relation matching no user and reading back verbatim on the next sync.2. Update.
update_user— theACCOUNT_UPDATE_PROFILEaction C1 push rules drive, which already supports all six attributes — now accepts itsuser_idas a plain string as well as a resource reference, via the sameextractUserIdhelper every other user-scoped action in this connector already used. Google'suserKeyaccepts a primary email or Google user ID, so an automation author holding either previously had the call rejected before it ever reached the Directory API. That is the misconfiguration point the ticket calls out.That helper's plain-string fallback selects
args.Fieldsdirectly, which panics on a nil*structpb.Struct; the SDK passesrequest.GetArgs()straight through, so an action invoked with no arguments reached it as nil.extractUserIdnow guards, which fixesupdate_userand the eight other user-scoped actions that were already exposed.Out of scope per the ticket and left untouched: custom-schema attributes and recovery email/phone at creation (still action-only), clearing the manager relation, new OAuth scopes (
admin.directory.useralready covers all of this), and group/role provisioning.Testing
go build ./...,go test ./... -count=1, andgolangci-lint run(0 issues) all pass.Tests in
pkg/connector/user_create_account_test.gorunCreateAccountagainst a mockusers.insertserver and assert on the actual request body:organizations/externalIds/relations, and are present on the resourceCreateAccountreturns (so the joiner sees them without waiting for a sync)organizations/externalIds/relationskeys at all — asserted against the raw JSON, since those areinterface{}-typed fields Google's marshaller serializes whenever non-nil, even when emptyjob_title/jobTitle/titleand the camelCase aliases all resolvemanager_emailand a numericemployee_idare each dropped while the account is still created, with the valid attributes in the same profile still appliedjob_titledoes not discard a usabletitlealias alongside itmanager_emailin display-name form or with surrounding whitespace is stored as the bare addressemployeeInfoFromProfileignores recovery/custom-schema/name keys, so the create path cannot quietly widen, and reports each dropped attribute with the reasonCreateAccountlogsPlus
TestUpdateUserGlobal_PlainStringUserID, coveringupdate_userwith both a primary email and a Google user ID asuser_id, andTestUpdateUserGlobal_NilArgs/TestExtractUserId_NilAndEmptyArgs, covering the nil-argument guard.Not covered by automated tests: behaviour against a live tenant. The wire fields are the ones the v0.2.61 action handlers already exercise in production, and the repeated-field quirks documented in
applyUserProfilePatchdon't apply on create (nothing to merge or shrink).Follow-ups / caveats
CreateAccountremains non-idempotent: creating an account that already exists still surfaces Google's duplicate error rather than falling back to updating the existing user. Making create-on-existing apply the profile would silently overwrite a live user's org attributes, so it's deliberately left out — the mover path isupdate_user.update_user_manager).manager_emaildisplay-name normalization is applied on the create path only.applyUserProfilePatchandupdate_user_managerstill store the raw input and have the same latent issue; fixing them means touching actions this PR does not otherwise change, so it is left for a follow-up.