From 2351a47b71498ed4e64142db301bff9a2fea91e6 Mon Sep 17 00:00:00 2001 From: "c1-squire-dev[bot]" Date: Tue, 11 Aug 2026 16:03:09 +0000 Subject: [PATCH 1/3] [EPD-2938] Support Employee Information attributes in account provisioning 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] --- README.md | 36 ++- docs/docs-info.md | 4 +- pkg/connector/user.go | 21 +- pkg/connector/user_actions.go | 140 +++++++-- pkg/connector/user_actions_global_test.go | 41 +++ pkg/connector/user_create_account_test.go | 327 ++++++++++++++++++++++ 6 files changed, 544 insertions(+), 25 deletions(-) create mode 100644 pkg/connector/user_create_account_test.go diff --git a/README.md b/README.md index 5f0f75e4..dda48e37 100644 --- a/README.md +++ b/README.md @@ -77,11 +77,41 @@ baton resources | Operation | Description | | ----------------------------- | ------------------------------------------------------------------- | -| Create/Delete user | Directory API `users.insert` / `users.delete` | +| Create/Delete user | Directory API `users.insert` / `users.delete`. Account creation also applies any Employee Information attributes present on the account profile — see below | | Delete group | Directory API `groups.delete` (group creation is the `create_group` connector action, below) | | Grant/Revoke group membership | Directory API `members.insert` / `members.delete` | | Grant/Revoke role assignment | Directory API `roleAssignments.insert` / `roleAssignments.delete` | +### Account profile attributes at creation + +Account creation reads the following keys from the ConductorOne account profile +and applies them in the same `users.insert` call, so a joiner provisions a +fully-populated account in one step: + +| Profile key | Accepted aliases | Google Directory field | +| ----------- | ---------------- | ---------------------- | +| `email`, `given_name`, `family_name` | — | `primaryEmail`, `name.givenName`, `name.familyName` (required) | +| `department` | — | `organizations[0].department` | +| `job_title` | `jobTitle`, `title` | `organizations[0].title` | +| `cost_center` | `costCenter` | `organizations[0].costCenter` | +| `employee_type` | `employeeType` | `organizations[0].description` (the Admin console's "Employee type") | +| `employee_id` | `employeeId` | `externalIds[]` entry of type `organization` (the Admin console's "Employee ID") | +| `manager_email` | `managerEmail` | `relations[]` entry of type `manager` | + +The Employee Information keys are the same ones `update_user`'s `user_profile` +object accepts, so a single account profile drives both the joiner (create) and +subsequent mover (update) flows. Notes: + +- All six are optional; keys that are absent or empty are simply not sent (an + empty value means "clear" on the update path, but a brand-new account has + nothing to clear). +- A malformed `manager_email` or a wrong-typed value (e.g. `employee_id` sent as + a JSON number) fails the call **before** the account is created, rather than + being silently dropped — unlike the update path, account creation has no + `skipped_fields` channel to report a partial success on. +- Recovery email/phone and custom-schema attributes are **not** settable at + creation; they remain available through `update_user_profile` / `update_user`. + ## Connector actions Connector actions are custom operations invoked on demand from C1 automations: @@ -90,7 +120,7 @@ Connector actions are custom operations invoked on demand from C1 automations: | ------ | ------------- | ----------- | | `update_user_status` / `disable_user` / `enable_user` | `user_id` / `is_suspended` | Suspend or activate a user (idempotent) | | `update_user_profile` | `user_id`, plus any of `given_name`, `family_name`, `recovery_email`, `recovery_phone`, `department`, `job_title`, `cost_center`, `employee_type`, `employee_id`, `manager_email`, `custom_schemas` | Partial profile update (patch semantics); supports Employee Information attributes and custom-schema attribute values. Exception: an `employee_id` change that reduces the number of external IDs on the account (clearing it, or consolidating duplicate entries down to the new value) uses a full-object update instead, since Google does not reliably shrink a repeated field via patch. An empty or malformed `manager_email` does not fail the whole call — see the partial-success note below. | -| `update_user` | `user_id` (resource ID), `user_profile` (JSON string; same keys as `update_user_profile` above) | Profile update from a JSON object; consumed by C1 push rules for automated profile sync. Same partial-success behavior as `update_user_profile` for `manager_email`. | +| `update_user` | `user_id` (user resource reference, or the primary email / Google user ID as a plain string), `user_profile` (JSON string; same keys as `update_user_profile` above) | Profile update from a JSON object; consumed by C1 push rules for automated profile sync. Same partial-success behavior as `update_user_profile` for `manager_email`. | | `update_user_manager` | `user_id`, `manager_email` | Set the user's `manager` relation | | `make_admin` | `user_id`, `status` (bool) | Promote/demote a user to/from super administrator | | `change_user_org_unit` | `user_id`, `org_unit_path` | Move a user to a different organizational unit | @@ -108,6 +138,8 @@ Connector actions are custom operations invoked on demand from C1 automations: > **Job title round-trip:** the synced user profile exposes the job title under both `title` and `job_title` for backward compatibility. `update_user`'s `user_profile` JSON object accepts any of `job_title`, `jobTitle`, or `title` as the source key. `update_user_profile` has a fixed schema and only exposes `job_title` as an argument name — pass the value under that key. +> **Identifying the user:** the user-scoped actions accept `user_id` either as a user resource reference (what the UI's resource picker and C1 push rules send) or as a plain string. Google's `userKey` accepts a user's primary email or Google user ID as well as the synced resource ID, so any of the three works when the action is wired up by hand. + > **Partial success and `manager_email`:** `update_user_profile`/`update_user` never clear an assigned manager through this action (matching `update_user_manager`), so an empty or invalid `manager_email` is not applied — but unlike other invalid fields, it does not fail the whole call when at least one other field in the same payload is valid. The response's `success: true` only means the call completed; check the `skipped_fields` return field (a comma-separated list naming any provided field that wasn't applied, and why) to detect this — a caller that checks `success` alone will not be told that `manager_email` specifically was skipped. # Credentials Setup diff --git a/docs/docs-info.md b/docs/docs-info.md index 91d5864e..d83549ac 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -9,7 +9,7 @@ 2. Can the connector provision any resources? If so, which ones? 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 empty values are not sent; a malformed `manager_email` or a wrong-typed value fails before the account is created. Recovery email/phone and custom-schema attributes remain action-only. - **Grant/Revoke group membership** — via Directory API `members.insert` and `members.delete`. - **Grant/Revoke role assignment** — via Directory API `roleAssignments.insert` and `roleAssignments.delete`. - **Create/Delete groups** — creation via the `create_group` connector action (Directory API `groups.insert`); deletion via Directory API `groups.delete`. @@ -20,7 +20,7 @@ | --- | --- | --- | | `update_user_status` / `disable_user` / `enable_user` | `users.update` | Suspend / activate a user (idempotent) | | `update_user_profile` | `users.patch` (`users.update` when an `employee_id` change reduces the number of external IDs on the account) | Partial profile update: name, recovery details, Employee Information (department, job title, cost center, employee ID, employee type), manager relation, custom-schema values. An empty/invalid `manager_email` is skipped (not applied, reported via the `skipped_fields` return field) rather than failing the whole call, as long as another provided field is valid. | - | `update_user` | `users.patch` (`users.update` when an `employee_id` change reduces the number of external IDs on the account) | Profile update from a `user_profile` JSON object (same fields as `update_user_profile`); consumed by C1 push rules for automated profile sync. Same `manager_email` partial-success behavior as `update_user_profile`. | + | `update_user` | `users.patch` (`users.update` when an `employee_id` change reduces the number of external IDs on the account) | Profile update from a `user_profile` JSON object (same fields as `update_user_profile`); consumed by C1 push rules for automated profile sync. `user_id` accepts a user resource reference, or the user's primary email or Google user ID as a plain string. Same `manager_email` partial-success behavior as `update_user_profile`. | | `update_user_manager` | `users.update` | Set the user's `manager` relation | | `make_admin` | `users.makeAdmin` | Promote/demote a user to/from super administrator | | `change_user_org_unit` | `users.update` | Move a user to a different organizational unit | diff --git a/pkg/connector/user.go b/pkg/connector/user.go index b461d55c..05571ba1 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -14,9 +14,11 @@ import ( "github.com/conductorone/baton-sdk/pkg/crypto" "github.com/conductorone/baton-sdk/pkg/pagination" rs "github.com/conductorone/baton-sdk/pkg/types/resource" + "github.com/conductorone/baton-sdk/pkg/uhttp" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" admin "google.golang.org/api/admin/directory/v1" + "google.golang.org/grpc/codes" mapset "github.com/deckarep/golang-set/v2" @@ -484,6 +486,24 @@ func (o *userResourceType) CreateAccount(ctx context.Context, accountInfo *v2.Ac ChangePasswordAtNextLogin: changePasswordAtNextLogin, } + // Employee Information attributes (department, job title, cost center, + // employee type, employee ID, manager email) are optional; any the account + // profile carries are applied in the same users.insert call, so a joiner + // provisions a fully-populated account in one step instead of needing a + // follow-up update_user_profile action. The key aliases accepted here are + // the ones the update path accepts (employeeInfoJSONFields), so the same + // profile object drives both. Everything is parsed and validated before the + // insert below, so a malformed profile fails without leaving a + // half-configured account behind. + employeeInfo, err := employeeInfoFromProfile(pMap) + if err != nil { + return nil, nil, nil, uhttp.WrapErrors(codes.InvalidArgument, + "google-workspace: invalid account profile", err) + } + if err := applyEmployeeInfoToNewUser(user, employeeInfo); err != nil { + return nil, nil, nil, err + } + if credentialOptions == nil { return nil, nil, nil, fmt.Errorf("credentialOptions cannot be nil") } @@ -494,7 +514,6 @@ func (o *userResourceType) CreateAccount(ctx context.Context, accountInfo *v2.Ac var password string var plaintextData []*v2.PlaintextData - var err error if credentialOptions.GetRandomPassword() != nil || credentialOptions.GetPlaintextPassword() != nil { password, err = crypto.GeneratePassword(ctx, credentialOptions) diff --git a/pkg/connector/user_actions.go b/pkg/connector/user_actions.go index f29eb9ff..45f568c7 100644 --- a/pkg/connector/user_actions.go +++ b/pkg/connector/user_actions.go @@ -1427,6 +1427,60 @@ func buildManagerRelations(relations []*admin.UserRelation, managerEmail string) return updated } +// applyEmployeeInfoToNewUser writes the Employee Information attributes from an +// account profile onto a not-yet-created admin.User: department, job title and +// cost center onto a new primary Organizations entry, employee type onto that +// same entry's Description, employee ID as the "organization" ExternalIds entry, +// and manager email as the "manager" Relations entry. The same builders the +// update path uses shape each field, so create and update produce identical +// wire values - here they always start from an empty current state, since a +// brand-new account has nothing to merge with. +// +// Empty values are dropped rather than sent: on the update path an empty string +// means "clear this", but there is nothing on an account that does not exist yet +// to clear, and sending one anyway would create a phantom empty organization +// that reads back on the next sync. +// +// An invalid manager_email is rejected outright instead of being skipped the way +// the update path skips it. The update path can report a partial success through +// its skipped_fields return value; CreateAccount has no such channel, so +// skipping here would silently drop the manager on a joiner - and because this +// runs before the insert, failing costs nothing but the caller's retry. +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 + } + } + + if patch.managerEmail != nil { + if _, err := mail.ParseAddress(*patch.managerEmail); err != nil { + return uhttp.WrapErrors(codes.InvalidArgument, + fmt.Sprintf("google-workspace: invalid manager_email: %s", *patch.managerEmail), err) + } + user.Relations = buildManagerRelations(nil, *patch.managerEmail) + } + + // Both builders report whether they produced anything; assign only when they + // did, since Organizations and ExternalIds are interface{}-typed fields that + // Google's generated marshaller serializes whenever they are non-nil - even + // an empty slice - which would put "organizations":[] on the wire for every + // account created without these attributes. + if orgs, changed := buildUpdatedOrganizations(nil, patch); changed { + user.Organizations = orgs + } + if patch.employeeID != nil { + if ids, changed := buildUpdatedExternalIDs(nil, *patch.employeeID); changed { + user.ExternalIds = ids + } + } + + return nil +} + const ( actionUpdateUser = "update_user" argUserProfile = "user_profile" @@ -1480,8 +1534,9 @@ var updateUserGlobalActionSchema = &v2.BatonActionSchema{ { Name: argUserID, DisplayName: displayUser, - Description: "The user to update.", - IsRequired: true, + Description: "The user to update. Accepts a user resource reference, or - for callers that " + + "do not have one - the user's primary email or Google user ID as a plain string.", + IsRequired: true, Field: &config.Field_ResourceIdField{ ResourceIdField: &config.ResourceIdField{ Rules: &config.ResourceIDRules{ @@ -1532,11 +1587,16 @@ var updateUserGlobalActionSchema = &v2.BatonActionSchema{ func (c *GoogleWorkspace) updateUserActionHandler(ctx context.Context, args *structpb.Struct) (*structpb.Struct, annotations.Annotations, error) { l := ctxzap.Extract(ctx) - userRef, ok := actions.GetResourceIDArg(args, argUserID) - if !ok || userRef.GetResource() == "" { - return nil, nil, uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: update_user: user_id is required") + // Accepts either shape: the resource reference the UI's picker and C1 push + // rules send, or a plain string. The plain-string path matters because + // Google's userKey accepts a primary email or the Google user ID as well as + // 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) + if err != nil { + return nil, nil, err } - userId := userRef.GetResource() profileJSON, err := actions.RequireStringArg(args, argUserProfile) if err != nil { @@ -1584,33 +1644,73 @@ func (c *GoogleWorkspace) updateUserActionHandler(ctx context.Context, args *str return result, nil, nil } -// profileFromJSON maps a user_profile JSON object (snake_case or camelCase keys) -// to a userProfilePatch. Only keys present in the object are applied. -func profileFromJSON(profile map[string]any) (userProfilePatch, error) { - var patch userProfilePatch - for _, f := range []struct { - dest *(*string) - keys []string - }{ - {&patch.givenName, []string{argGivenName, "givenName"}}, - {&patch.familyName, []string{argFamilyName, "familyName"}}, - {&patch.recoveryEmail, []string{argRecoveryEmail, "recoveryEmail"}}, - {&patch.recoveryPhone, []string{argRecoveryPhone, "recoveryPhone"}}, +// profileFieldBinding binds one userProfilePatch field to the profile-object +// keys accepted for it (the canonical snake_case name first, then camelCase and +// legacy aliases). +type profileFieldBinding struct { + dest *(*string) + keys []string +} + +// employeeInfoJSONFields returns the bindings for the six Employee Information +// attributes. Shared by profileFromJSON (the update path) and the account +// -creation path in user.go so the two can never drift on which profile keys +// they accept: a joiner and a subsequent mover both read the same C1 account +// profile object, and a key that only one side understood would silently apply +// on create and be dropped on update (or vice versa). +func employeeInfoJSONFields(patch *userProfilePatch) []profileFieldBinding { + return []profileFieldBinding{ {&patch.department, []string{argDepartment}}, {&patch.jobTitle, []string{argJobTitle, "jobTitle", profileKeyTitle}}, {&patch.costCenter, []string{argCostCenter, "costCenter"}}, {&patch.employeeType, []string{argEmployeeType, "employeeType"}}, {&patch.employeeID, []string{argEmployeeID, "employeeId"}}, {&patch.managerEmail, []string{argManagerEmail, "managerEmail"}}, - } { + } +} + +// applyProfileFields reads each binding's keys out of profile, leaving the +// destination nil when none of them are present. +func applyProfileFields(profile map[string]any, fields []profileFieldBinding) error { + for _, f := range fields { v, ok, err := stringFromJSON(profile, f.keys...) if err != nil { - return patch, err + return err } if ok { *f.dest = &v } } + return nil +} + +// employeeInfoFromProfile maps a ConductorOne account profile to a patch holding +// only the six Employee Information attributes. Deliberately narrower than +// profileFromJSON: recovery email/phone and custom schemas stay action-only +// (out of scope for account provisioning), so reading them here would quietly +// widen what a create/update through the provisioning path can write. +func employeeInfoFromProfile(profile map[string]any) (userProfilePatch, error) { + var patch userProfilePatch + if err := applyProfileFields(profile, employeeInfoJSONFields(&patch)); err != nil { + return patch, err + } + return patch, nil +} + +// profileFromJSON maps a user_profile JSON object (snake_case or camelCase keys) +// to a userProfilePatch. Only keys present in the object are applied. +func profileFromJSON(profile map[string]any) (userProfilePatch, error) { + var patch userProfilePatch + fields := []profileFieldBinding{ + {&patch.givenName, []string{argGivenName, "givenName"}}, + {&patch.familyName, []string{argFamilyName, "familyName"}}, + {&patch.recoveryEmail, []string{argRecoveryEmail, "recoveryEmail"}}, + {&patch.recoveryPhone, []string{argRecoveryPhone, "recoveryPhone"}}, + } + fields = append(fields, employeeInfoJSONFields(&patch)...) + if err := applyProfileFields(profile, fields); err != nil { + return patch, err + } if raw, ok := profile[argCustomSchemas]; ok { m, ok := raw.(map[string]any) if !ok { diff --git a/pkg/connector/user_actions_global_test.go b/pkg/connector/user_actions_global_test.go index cd2b0b9b..2e0bb1e7 100644 --- a/pkg/connector/user_actions_global_test.go +++ b/pkg/connector/user_actions_global_test.go @@ -302,6 +302,47 @@ func TestUpdateUserGlobal_CustomSchemasViaProfile(t *testing.T) { require.Contains(t, string(raw), "emea") } +// TestUpdateUserGlobal_PlainStringUserID covers the identifiers an automation +// author actually has on hand. Google's userKey accepts the primary email and +// the Google user ID as well as the synced resource ID, so a plain string must +// reach the Directory API rather than being rejected up front for not being a +// ConductorOne resource reference. +func TestUpdateUserGlobal_PlainStringUserID(t *testing.T) { + for _, userKey := range []string{"t@example.com", "user123"} { + t.Run(userKey, func(t *testing.T) { + state := &testProfileServerState{ + users: map[string]*directoryAdmin.User{ + userKey: { + Id: "user123", + PrimaryEmail: "t@example.com", + Name: &directoryAdmin.UserName{GivenName: "Old", FamilyName: "Name"}, + }, + }, + } + server := newTestProfileServer(state) + defer server.Close() + + dir := newTestDirectoryService(t, server.URL, server.Client()) + c := newTestGlobalConnector(t, dir) + + args := &structpb.Struct{Fields: map[string]*structpb.Value{ + argUserID: strArg(userKey), + "user_profile": strArg(`{"department":"Engineering"}`), + }} + + resp, _, err := c.updateUserActionHandler(context.Background(), args) + require.NoError(t, err) + require.True(t, resp.GetFields()["success"].GetBoolValue()) + require.Equal(t, 1, state.patchCount) + + orgs, err := extractFromInterface[*directoryAdmin.UserOrganization](state.lastPatchBody.Organizations) + require.NoError(t, err) + require.Len(t, orgs, 1) + require.Equal(t, "Engineering", orgs[0].Department) + }) + } +} + func TestUpdateUserGlobal_MissingUserProfile(t *testing.T) { state := &testProfileServerState{ users: map[string]*directoryAdmin.User{"user123": {Id: "user123"}}, diff --git a/pkg/connector/user_create_account_test.go b/pkg/connector/user_create_account_test.go new file mode 100644 index 00000000..12fcdbd5 --- /dev/null +++ b/pkg/connector/user_create_account_test.go @@ -0,0 +1,327 @@ +package connector + +import ( + "context" + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "sync" + "testing" + + v2 "github.com/conductorone/baton-sdk/pb/c1/connector/v2" + "github.com/stretchr/testify/require" + directoryAdmin "google.golang.org/api/admin/directory/v1" + "google.golang.org/protobuf/types/known/structpb" +) + +// testInsertServerState backs a mock Directory API supporting the POST +// (users.insert) endpoint exercised by CreateAccount. +type testInsertServerState struct { + mtx sync.Mutex + insertCount int + // lastInsertRawBody is the raw JSON of the last insert, kept alongside its + // decoded form so tests can assert on wire-level details (e.g. that no + // empty "organizations" array was sent) that decoding into a Go struct + // would hide. + lastInsertRawBody []byte + lastInsertBody *directoryAdmin.User +} + +func newTestInsertServer(state *testInsertServerState) *httptest.Server { + mux := http.NewServeMux() + + mux.HandleFunc("/admin/directory/v1/users", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + http.Error(w, "method not allowed", http.StatusMethodNotAllowed) + return + } + + state.mtx.Lock() + defer state.mtx.Unlock() + + raw, _ := io.ReadAll(r.Body) + body := &directoryAdmin.User{} + _ = json.Unmarshal(raw, body) + state.insertCount++ + state.lastInsertRawBody = raw + state.lastInsertBody = body + + // Echo the created user back the way the real API does, with a + // server-assigned id. + if body.Name != nil && body.Name.FullName == "" { + body.Name.FullName = body.Name.GivenName + " " + body.Name.FamilyName + } + _ = json.NewEncoder(w).Encode(safeUserResponse{ + Id: "newuser123", + PrimaryEmail: body.PrimaryEmail, + Name: body.Name, + Organizations: extractOrganizations(body), + ExternalIDs: testExternalIDs(body), + Relations: extractRelations(body), + }) + }) + + return httptest.NewServer(mux) +} + +// createAccountProfile builds an AccountInfo from a profile map, mirroring what +// ConductorOne sends into CreateAccount. +func createAccountProfile(t *testing.T, profile map[string]any) *v2.AccountInfo { + t.Helper() + s, err := structpb.NewStruct(profile) + require.NoError(t, err) + return &v2.AccountInfo{Profile: s} +} + +// baseCreateProfile is the minimum CreateAccount has always required. +func baseCreateProfile() map[string]any { + return map[string]any{ + "email": "new.user@example.com", + "given_name": "New", + "family_name": "User", + } +} + +func TestCreateAccount_EmployeeInformation_SentOnInsert(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["department"] = "Engineering" + profile["job_title"] = "Staff Engineer" + profile["cost_center"] = "CC-42" + profile["employee_type"] = "Full-time" + profile["employee_id"] = "E-1234" + profile["manager_email"] = "manager@example.com" + + resp, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err) + require.NotNil(t, resp) + require.Equal(t, 1, state.insertCount) + + orgs := extractOrganizations(state.lastInsertBody) + require.Len(t, orgs, 1) + require.True(t, orgs[0].Primary, "the organization created for a brand-new account is the primary one") + require.Equal(t, "Engineering", orgs[0].Department) + require.Equal(t, "Staff Engineer", orgs[0].Title) + require.Equal(t, "CC-42", orgs[0].CostCenter) + require.Equal(t, "Full-time", orgs[0].Description, "employee_type maps to Organization.Description") + + ids := testExternalIDs(state.lastInsertBody) + require.Len(t, ids, 1) + require.Equal(t, externalIDTypeOrganization, ids[0].Type, "employee_id is the 'organization' external ID") + require.Equal(t, "E-1234", ids[0].Value) + + rels := extractRelations(state.lastInsertBody) + require.Len(t, rels, 1) + require.Equal(t, relTypeManager, rels[0].Type) + require.Equal(t, "manager@example.com", rels[0].Value) + + // The resource CreateAccount returns must already carry the attributes, so + // the joiner flow sees them without waiting for the next sync. + successResp, ok := resp.(*v2.CreateAccountResponse_SuccessResult) + require.True(t, ok) + require.Equal(t, "newuser123", successResp.Resource.GetId().GetResource()) + returned := successResp.Resource.GetProfile().GetFields() + require.Equal(t, "Engineering", returned["department"].GetStringValue()) + require.Equal(t, "Staff Engineer", returned[argJobTitle].GetStringValue()) + require.Equal(t, "CC-42", returned["cost_center"].GetStringValue()) + require.Equal(t, "Full-time", returned[argEmployeeType].GetStringValue()) + require.Equal(t, "E-1234", returned[argEmployeeID].GetStringValue()) + require.Equal(t, "manager@example.com", returned[argManagerEmail].GetStringValue()) +} + +func TestCreateAccount_NoEmployeeInformation_SendsNoEmptyArrays(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, baseCreateProfile()), &v2.LocalCredentialOptions{}) + require.NoError(t, err) + require.Equal(t, 1, state.insertCount) + + // Organizations/ExternalIds/Relations are interface{}-typed fields Google's + // marshaller serializes whenever they are non-nil - even when empty - so + // assert on the raw body, which is where a stray "organizations":[] shows up. + raw := string(state.lastInsertRawBody) + require.NotContains(t, raw, "organizations") + require.NotContains(t, raw, "externalIds") + require.NotContains(t, raw, "relations") +} + +func TestCreateAccount_EmptyEmployeeInformationValues_AreDropped(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + // Empty strings mean "clear" on the update path; a brand-new account has + // nothing to clear, so they must not create a phantom empty organization. + profile := baseCreateProfile() + profile["department"] = "" + profile["job_title"] = "" + profile["employee_id"] = "" + profile["manager_email"] = "" + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err, "an empty manager_email is simply absent on create, not an invalid address") + require.Equal(t, 1, state.insertCount) + + raw := string(state.lastInsertRawBody) + require.NotContains(t, raw, "organizations") + require.NotContains(t, raw, "externalIds") + require.NotContains(t, raw, "relations") +} + +func TestCreateAccount_PartialEmployeeInformation(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["department"] = "Support" + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err) + + orgs := extractOrganizations(state.lastInsertBody) + require.Len(t, orgs, 1) + require.Equal(t, "Support", orgs[0].Department) + require.Empty(t, orgs[0].Title) + + raw := string(state.lastInsertRawBody) + require.NotContains(t, raw, "externalIds", "an absent employee_id must not send an external ID entry") + require.NotContains(t, raw, "relations", "an absent manager_email must not send a relation entry") +} + +func TestCreateAccount_JobTitleAliases(t *testing.T) { + // The account profile and the update action's user_profile object must + // accept the same key aliases, so a joiner and a later mover agree on where + // the job title comes from. + for _, key := range []string{"job_title", "jobTitle", "title"} { + t.Run(key, func(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile[key] = "Analyst" + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err) + + orgs := extractOrganizations(state.lastInsertBody) + require.Len(t, orgs, 1) + require.Equal(t, "Analyst", orgs[0].Title) + }) + } +} + +func TestCreateAccount_CamelCaseAliases(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["costCenter"] = "CC-7" + profile["employeeType"] = "Contractor" + profile["employeeId"] = "E-77" + profile["managerEmail"] = "lead@example.com" + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err) + + orgs := extractOrganizations(state.lastInsertBody) + require.Len(t, orgs, 1) + require.Equal(t, "CC-7", orgs[0].CostCenter) + require.Equal(t, "Contractor", orgs[0].Description) + + ids := testExternalIDs(state.lastInsertBody) + require.Len(t, ids, 1) + require.Equal(t, "E-77", ids[0].Value) + + rels := extractRelations(state.lastInsertBody) + require.Len(t, rels, 1) + require.Equal(t, "lead@example.com", rels[0].Value) +} + +func TestCreateAccount_InvalidManagerEmail_FailsBeforeInsert(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["manager_email"] = "not-an-email" + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.Error(t, err, "CreateAccount has no skipped_fields channel, so an invalid manager must fail loudly") + require.Equal(t, 0, state.insertCount, "validation must happen before the account is created") +} + +func TestCreateAccount_WrongTypedAttribute_FailsBeforeInsert(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + // A numeric employee_id would otherwise be silently dropped; the update + // path rejects it, and so must this one. + profile := baseCreateProfile() + profile["employee_id"] = 12345 + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.Error(t, err) + require.Equal(t, 0, state.insertCount) +} + +func TestEmployeeInfoFromProfile_IgnoresOutOfScopeKeys(t *testing.T) { + // Recovery details and custom schemas stay action-only (out of scope for + // account provisioning); reading them here would quietly widen what the + // create path can write. + patch, err := employeeInfoFromProfile(map[string]any{ + "department": "Engineering", + "recovery_email": "recovery@example.com", + "recovery_phone": "+14155550100", + "custom_schemas": map[string]any{"MySchema": map[string]any{"region": "emea"}}, + "given_name": "New", + }) + require.NoError(t, err) + require.NotNil(t, patch.department) + require.Equal(t, "Engineering", *patch.department) + require.Nil(t, patch.recoveryEmail) + require.Nil(t, patch.recoveryPhone) + require.Nil(t, patch.customSchemas) + require.Nil(t, patch.givenName) +} + +func TestApplyEmployeeInfoToNewUser_EmptyPatchLeavesUserUntouched(t *testing.T) { + user := &directoryAdmin.User{PrimaryEmail: "a@example.com"} + require.NoError(t, applyEmployeeInfoToNewUser(user, userProfilePatch{})) + require.Nil(t, user.Organizations) + require.Nil(t, user.ExternalIds) + require.Nil(t, user.Relations) +} From e9840a2f34443b6c01936a440de8cb5adb92c180 Mon Sep 17 00:00:00 2001 From: Marcus Whitaker Date: Tue, 11 Aug 2026 16:38:52 +0000 Subject: [PATCH 2/3] [EPD-2938] Fix nil-args panic and manager_email normalization on create 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 " 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] --- README.md | 10 ++- pkg/connector/helpers.go | 9 +++ pkg/connector/user_actions.go | 17 ++++- pkg/connector/user_actions_global_test.go | 43 ++++++++++++ pkg/connector/user_create_account_test.go | 81 +++++++++++++++++++++++ 5 files changed, 154 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index dda48e37..88b7cf10 100644 --- a/README.md +++ b/README.md @@ -102,9 +102,13 @@ The Employee Information keys are the same ones `update_user`'s `user_profile` object accepts, so a single account profile drives both the joiner (create) and subsequent mover (update) flows. Notes: -- All six are optional; keys that are absent or empty are simply not sent (an - empty value means "clear" on the update path, but a brand-new account has - nothing to clear). +- All six are optional; keys that are absent, empty, or whitespace-only are + simply not sent (an empty value means "clear" on the update path, but a + brand-new account has nothing to clear). +- `manager_email` is normalized to the bare address before it is stored, so a + value that arrives padded or in display-name form + (`Jane Doe `) still produces a `manager` relation Google can + resolve. - A malformed `manager_email` or a wrong-typed value (e.g. `employee_id` sent as a JSON number) fails the call **before** the account is created, rather than being silently dropped — unlike the update path, account creation has no diff --git a/pkg/connector/helpers.go b/pkg/connector/helpers.go index de82175a..03077369 100644 --- a/pkg/connector/helpers.go +++ b/pkg/connector/helpers.go @@ -82,6 +82,15 @@ func extractUserId(args *structpb.Struct, l *zap.Logger, actionName string) (str if ref, ok := actions.GetResourceIDArg(args, argUserID); ok && ref.GetResource() != "" { return ref.GetResource(), nil } + // GetResourceIDArg nil-guards internally, but the plain-string fallback below + // selects args.Fields directly, which panics on a nil *structpb.Struct. The + // SDK passes request.GetArgs() straight through, so an action invoked with no + // arguments at all arrives here as nil - that has to read back as a clean + // InvalidArgument, not a recovered panic. + if args == nil || args.Fields == nil { + l.Debug("google-workspace: user action handler: missing arguments", zap.String("action", actionName)) + return "", uhttp.WrapErrors(codes.InvalidArgument, "google-workspace: missing user_id argument") + } userIdValue, ok := args.Fields[argUserID] if !ok || userIdValue == nil { l.Debug("google-workspace: user action handler: missing user_id argument", zap.String("action", actionName), zap.Any("args", args)) diff --git a/pkg/connector/user_actions.go b/pkg/connector/user_actions.go index 45f568c7..0df594d3 100644 --- a/pkg/connector/user_actions.go +++ b/pkg/connector/user_actions.go @@ -1451,17 +1451,28 @@ func applyEmployeeInfoToNewUser(user *admin.User, patch userProfilePatch) error &patch.department, &patch.jobTitle, &patch.costCenter, &patch.employeeType, &patch.employeeID, &patch.managerEmail, } { - if *dest != nil && **dest == "" { + // Whitespace-only counts as empty. HRIS- and CSV-sourced account profiles + // routinely carry " " for a field nobody filled in; treating that as a + // 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) == "" { *dest = nil } } if patch.managerEmail != nil { - if _, err := mail.ParseAddress(*patch.managerEmail); err != nil { + // Store the parsed address rather than the raw input: mail.ParseAddress + // also accepts the display-name form ("Jane Doe "), but + // Google resolves relations[].value only as a bare email, so the raw + // string would be accepted here, match no user, and read back verbatim on + // the next sync. + addr, err := mail.ParseAddress(strings.TrimSpace(*patch.managerEmail)) + if err != nil { return uhttp.WrapErrors(codes.InvalidArgument, fmt.Sprintf("google-workspace: invalid manager_email: %s", *patch.managerEmail), err) } - user.Relations = buildManagerRelations(nil, *patch.managerEmail) + user.Relations = buildManagerRelations(nil, addr.Address) } // Both builders report whether they produced anything; assign only when they diff --git a/pkg/connector/user_actions_global_test.go b/pkg/connector/user_actions_global_test.go index 2e0bb1e7..64b571c2 100644 --- a/pkg/connector/user_actions_global_test.go +++ b/pkg/connector/user_actions_global_test.go @@ -5,7 +5,10 @@ import ( "testing" "github.com/stretchr/testify/require" + "go.uber.org/zap" directoryAdmin "google.golang.org/api/admin/directory/v1" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" "google.golang.org/protobuf/types/known/structpb" gwclient "github.com/conductorone/baton-google-workspace/pkg/client" @@ -380,3 +383,43 @@ func TestUpdateUserGlobal_MissingUserID(t *testing.T) { require.Error(t, err) require.Equal(t, 0, state.patchCount) } + +// TestUpdateUserGlobal_NilArgs pins that an action invoked with no arguments at +// all returns a clean InvalidArgument rather than panicking. The SDK passes +// request.GetArgs() straight through, so nil is a reachable input, and +// extractUserId's plain-string fallback selects args.Fields directly. +func TestUpdateUserGlobal_NilArgs(t *testing.T) { + state := &testProfileServerState{ + users: map[string]*directoryAdmin.User{"user123": {Id: "user123"}}, + } + server := newTestProfileServer(state) + defer server.Close() + + dir := newTestDirectoryService(t, server.URL, server.Client()) + c := newTestGlobalConnector(t, dir) + + require.NotPanics(t, func() { + _, _, err := c.updateUserActionHandler(context.Background(), nil) + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + }) + require.Equal(t, 0, state.patchCount) +} + +// TestExtractUserId_NilAndEmptyArgs covers the same guard at the helper every +// user-scoped action shares, so the other eight handlers are pinned too. +func TestExtractUserId_NilAndEmptyArgs(t *testing.T) { + for name, args := range map[string]*structpb.Struct{ + "nil struct": nil, + "nil fields": {}, + "no user_id": {Fields: map[string]*structpb.Value{}}, + } { + t.Run(name, func(t *testing.T) { + require.NotPanics(t, func() { + _, err := extractUserId(args, zap.NewNop(), "update_user") + require.Error(t, err) + require.Equal(t, codes.InvalidArgument, status.Code(err)) + }) + }) + } +} diff --git a/pkg/connector/user_create_account_test.go b/pkg/connector/user_create_account_test.go index 12fcdbd5..72c828ea 100644 --- a/pkg/connector/user_create_account_test.go +++ b/pkg/connector/user_create_account_test.go @@ -325,3 +325,84 @@ func TestApplyEmployeeInfoToNewUser_EmptyPatchLeavesUserUntouched(t *testing.T) require.Nil(t, user.ExternalIds) require.Nil(t, user.Relations) } + +// TestCreateAccount_WhitespaceOnlyValues_AreDropped covers the same "empty means +// absent" rule as the empty-string case, for the padding HRIS- and CSV-sourced +// account profiles carry in practice. A whitespace-only manager_email is the one +// that matters most: before it was treated as empty it reached mail.ParseAddress +// and failed the entire create, so a profile carrying no manager at all produced +// no account. +func TestCreateAccount_WhitespaceOnlyValues_AreDropped(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["department"] = " " + profile["job_title"] = "\t" + profile["cost_center"] = " " + profile["employee_type"] = " " + profile["employee_id"] = " \n " + profile["manager_email"] = " " + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err, "a whitespace-only value must not fail account creation") + require.Equal(t, 1, state.insertCount) + + var raw map[string]any + require.NoError(t, json.Unmarshal(state.lastInsertRawBody, &raw)) + require.NotContains(t, raw, "organizations") + require.NotContains(t, raw, "externalIds") + require.NotContains(t, raw, "relations") +} + +// TestCreateAccount_ManagerEmailDisplayNameForm_StoresBareAddress pins that the +// manager relation carries the bare address. mail.ParseAddress accepts the +// display-name form, but Google resolves relations[].value only as an email, so +// storing the raw input would silently produce a manager relation matching no +// user. +func TestCreateAccount_ManagerEmailDisplayNameForm_StoresBareAddress(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["manager_email"] = "Jane Doe " + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err) + + rels := extractRelations(state.lastInsertBody) + require.Len(t, rels, 1) + require.Equal(t, relTypeManager, rels[0].Type) + require.Equal(t, "jane@example.com", rels[0].Value, + "the display name must be stripped; Google matches relations[].value as a bare email") +} + +// TestCreateAccount_ManagerEmailSurroundingWhitespace_IsTrimmed guards the same +// normalization for the far more common case of an otherwise-valid address that +// arrives padded. +func TestCreateAccount_ManagerEmailSurroundingWhitespace_IsTrimmed(t *testing.T) { + state := &testInsertServerState{} + server := newTestInsertServer(state) + defer server.Close() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["manager_email"] = " manager@example.com " + + _, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + require.NoError(t, err) + + rels := extractRelations(state.lastInsertBody) + require.Len(t, rels, 1) + require.Equal(t, "manager@example.com", rels[0].Value) +} From f096b38077ed97902d6d30f361a219a980986b34 Mon Sep 17 00:00:00 2001 From: Marcus Whitaker Date: Tue, 11 Aug 2026 16:45:11 +0000 Subject: [PATCH 3/3] [EPD-2938] Make Employee Information attributes non-blocking on create 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] --- README.md | 13 ++- docs/docs-info.md | 2 +- pkg/connector/user.go | 30 ++++--- pkg/connector/user_actions.go | 99 ++++++++++++++++++----- pkg/connector/user_create_account_test.go | 90 ++++++++++++++++++--- 5 files changed, 185 insertions(+), 49 deletions(-) diff --git a/README.md b/README.md index 88b7cf10..0be5d98d 100644 --- a/README.md +++ b/README.md @@ -109,10 +109,15 @@ subsequent mover (update) flows. Notes: value that arrives padded or in display-name form (`Jane Doe `) still produces a `manager` relation Google can resolve. -- A malformed `manager_email` or a wrong-typed value (e.g. `employee_id` sent as - a JSON number) fails the call **before** the account is created, rather than - being silently dropped — unlike the update path, account creation has no - `skipped_fields` channel to report a partial success on. +- **None of them can fail account creation.** They enrich an account rather than + define it, so a value that is wrong-typed (e.g. `employee_id` sent as a JSON + number) or an unusable `manager_email` is dropped and the account is still + created. Every dropped attribute is named with a reason in a `Warn` log line + (`dropped_fields`). Use `update_user` to apply them once the profile is + corrected. Note this is deliberately *less* strict than the update path, which + rejects a wrong-typed value outright — there the account already exists, so + failing loudly costs nothing, whereas here it would cost the joiner their + account. - Recovery email/phone and custom-schema attributes are **not** settable at creation; they remain available through `update_user_profile` / `update_user`. diff --git a/docs/docs-info.md b/docs/docs-info.md index d83549ac..72ad8304 100644 --- a/docs/docs-info.md +++ b/docs/docs-info.md @@ -9,7 +9,7 @@ 2. Can the connector provision any resources? If so, which ones? Yes: - - **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 empty values are not sent; a malformed `manager_email` or a wrong-typed value fails before the account is created. Recovery email/phone and custom-schema attributes remain action-only. + - **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. - **Grant/Revoke group membership** — via Directory API `members.insert` and `members.delete`. - **Grant/Revoke role assignment** — via Directory API `roleAssignments.insert` and `roleAssignments.delete`. - **Create/Delete groups** — creation via the `create_group` connector action (Directory API `groups.insert`); deletion via Directory API `groups.delete`. diff --git a/pkg/connector/user.go b/pkg/connector/user.go index 05571ba1..feeacd61 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -14,11 +14,9 @@ import ( "github.com/conductorone/baton-sdk/pkg/crypto" "github.com/conductorone/baton-sdk/pkg/pagination" rs "github.com/conductorone/baton-sdk/pkg/types/resource" - "github.com/conductorone/baton-sdk/pkg/uhttp" "github.com/grpc-ecosystem/go-grpc-middleware/logging/zap/ctxzap" "go.uber.org/zap" admin "google.golang.org/api/admin/directory/v1" - "google.golang.org/grpc/codes" mapset "github.com/deckarep/golang-set/v2" @@ -492,16 +490,23 @@ func (o *userResourceType) CreateAccount(ctx context.Context, accountInfo *v2.Ac // provisions a fully-populated account in one step instead of needing a // follow-up update_user_profile action. The key aliases accepted here are // the ones the update path accepts (employeeInfoJSONFields), so the same - // profile object drives both. Everything is parsed and validated before the - // insert below, so a malformed profile fails without leaving a - // half-configured account behind. - employeeInfo, err := employeeInfoFromProfile(pMap) - if err != nil { - return nil, nil, nil, uhttp.WrapErrors(codes.InvalidArgument, - "google-workspace: invalid account profile", err) - } - if err := applyEmployeeInfoToNewUser(user, employeeInfo); err != nil { - return nil, nil, nil, err + // profile object drives both. + // + // None of them can fail the create. They enrich an account rather than + // define it, and the joiner needs the account far more than it needs a + // department: an attribute that is empty, wrong-typed, or (for + // manager_email) unusable is dropped and reported below, and the account is + // still created. update_user fills in anything that was dropped once the + // profile is corrected. + employeeInfo, droppedFields := employeeInfoFromProfile(pMap) + droppedFields = append(droppedFields, applyEmployeeInfoToNewUser(user, employeeInfo)...) + if len(droppedFields) > 0 { + // Warn, not Error: this is a misconfigured attribute mapping on the + // customer side, not a connector bug, and the operation still succeeds. + ctxzap.Extract(ctx).Warn( + "google-workspace: dropping unusable Employee Information attributes while creating account", + zap.Strings("dropped_fields", droppedFields), + ) } if credentialOptions == nil { @@ -514,6 +519,7 @@ func (o *userResourceType) CreateAccount(ctx context.Context, accountInfo *v2.Ac var password string var plaintextData []*v2.PlaintextData + var err error if credentialOptions.GetRandomPassword() != nil || credentialOptions.GetPlaintextPassword() != nil { password, err = crypto.GeneratePassword(ctx, credentialOptions) diff --git a/pkg/connector/user_actions.go b/pkg/connector/user_actions.go index 0df594d3..a8541a21 100644 --- a/pkg/connector/user_actions.go +++ b/pkg/connector/user_actions.go @@ -1441,12 +1441,14 @@ func buildManagerRelations(relations []*admin.UserRelation, managerEmail string) // to clear, and sending one anyway would create a phantom empty organization // that reads back on the next sync. // -// An invalid manager_email is rejected outright instead of being skipped the way -// the update path skips it. The update path can report a partial success through -// its skipped_fields return value; CreateAccount has no such channel, so -// skipping here would silently drop the manager on a joiner - and because this -// runs before the insert, failing costs nothing but the caller's retry. -func applyEmployeeInfoToNewUser(user *admin.User, patch userProfilePatch) error { +// Every one of these attributes is optional enrichment, so none of them fails +// the create: an unusable manager_email is dropped and named in the returned +// slice for the caller to log, the same way a wrong-typed value is. Failing +// instead would mean an HRIS-sourced profile carrying a manager's display name, +// or a manager who has not been provisioned yet, blocks the joiner's account +// entirely - a far worse outcome than an account that lands without its manager +// relation, which the mover path (update_user) then fills in. +func applyEmployeeInfoToNewUser(user *admin.User, patch userProfilePatch) []string { for _, dest := range []*(*string){ &patch.department, &patch.jobTitle, &patch.costCenter, &patch.employeeType, &patch.employeeID, &patch.managerEmail, @@ -1461,6 +1463,8 @@ func applyEmployeeInfoToNewUser(user *admin.User, patch userProfilePatch) error } } + var dropped []string + if patch.managerEmail != nil { // Store the parsed address rather than the raw input: mail.ParseAddress // also accepts the display-name form ("Jane Doe "), but @@ -1468,11 +1472,14 @@ func applyEmployeeInfoToNewUser(user *admin.User, patch userProfilePatch) error // string would be accepted here, match no user, and read back verbatim on // the next sync. addr, err := mail.ParseAddress(strings.TrimSpace(*patch.managerEmail)) - if err != nil { - return uhttp.WrapErrors(codes.InvalidArgument, - fmt.Sprintf("google-workspace: invalid manager_email: %s", *patch.managerEmail), err) + switch { + case err != nil: + dropped = append(dropped, + fmt.Sprintf("%s (not a valid email address: %q)", argManagerEmail, *patch.managerEmail)) + patch.managerEmail = nil + default: + user.Relations = buildManagerRelations(nil, addr.Address) } - user.Relations = buildManagerRelations(nil, addr.Address) } // Both builders report whether they produced anything; assign only when they @@ -1489,7 +1496,7 @@ func applyEmployeeInfoToNewUser(user *admin.User, patch userProfilePatch) error } } - return nil + return dropped } const ( @@ -1695,17 +1702,71 @@ func applyProfileFields(profile map[string]any, fields []profileFieldBinding) er return nil } +// stringFromJSONLenient is stringFromJSON's skip-and-continue counterpart: a +// wrong-typed key is described in the returned slice instead of aborting, and +// the remaining aliases are still tried, so a numeric "job_title" does not mask +// a perfectly good "title". Nothing is reported once a value resolves - the +// alias did its job and there is nothing for an operator to act on. +func stringFromJSONLenient(profile map[string]any, keys ...string) (string, bool, []string) { + var dropped []string + for _, k := range keys { + v, ok := profile[k] + if !ok || v == nil { + continue + } + s, ok := v.(string) + if !ok { + dropped = append(dropped, fmt.Sprintf("%s (expected a JSON string, got %s)", k, jsonTypeName(v))) + continue + } + return s, true, nil + } + return "", false, dropped +} + +// jsonTypeName names a decoded JSON value's type the way the profile author +// wrote it, rather than the way Go decoded it - structpb decodes every number +// to float64, and telling someone their employee_id is a "float64" when they +// wrote 12345 is not actionable. +func jsonTypeName(v any) string { + switch v.(type) { + case float64, int, int64, json.Number: + return "a number" + case bool: + return "a boolean" + case map[string]any: + return "an object" + case []any: + return "an array" + default: + return fmt.Sprintf("%T", v) + } +} + // employeeInfoFromProfile maps a ConductorOne account profile to a patch holding -// only the six Employee Information attributes. Deliberately narrower than -// profileFromJSON: recovery email/phone and custom schemas stay action-only -// (out of scope for account provisioning), so reading them here would quietly -// widen what a create/update through the provisioning path can write. -func employeeInfoFromProfile(profile map[string]any) (userProfilePatch, error) { +// only the six Employee Information attributes, returning a description of any +// attribute it had to drop. Deliberately narrower than profileFromJSON: recovery +// email/phone and custom schemas stay action-only (out of scope for account +// provisioning), so reading them here would quietly widen what a create through +// the provisioning path can write. +// +// Unlike profileFromJSON, a wrong-typed value is dropped rather than rejected. +// All six attributes are optional enrichment on a brand-new account, so none of +// them may fail the insert: an HRIS-sourced profile routinely carries a numeric +// employee_id or cost_center, and rejecting one would leave the joiner with no +// account at all. The update path keeps the strict behaviour - there the account +// already exists, so failing loudly costs nothing. +func employeeInfoFromProfile(profile map[string]any) (userProfilePatch, []string) { var patch userProfilePatch - if err := applyProfileFields(profile, employeeInfoJSONFields(&patch)); err != nil { - return patch, err + var dropped []string + for _, f := range employeeInfoJSONFields(&patch) { + v, ok, skipped := stringFromJSONLenient(profile, f.keys...) + dropped = append(dropped, skipped...) + if ok { + *f.dest = &v + } } - return patch, nil + return patch, dropped } // profileFromJSON maps a user_profile JSON object (snake_case or camelCase keys) diff --git a/pkg/connector/user_create_account_test.go b/pkg/connector/user_create_account_test.go index 72c828ea..62ad18b0 100644 --- a/pkg/connector/user_create_account_test.go +++ b/pkg/connector/user_create_account_test.go @@ -264,7 +264,12 @@ func TestCreateAccount_CamelCaseAliases(t *testing.T) { require.Equal(t, "lead@example.com", rels[0].Value) } -func TestCreateAccount_InvalidManagerEmail_FailsBeforeInsert(t *testing.T) { +// TestCreateAccount_InvalidManagerEmail_StillCreatesAccount pins that an +// unusable manager does not cost the joiner their account. A manager who has not +// been provisioned yet, or a display name where an address was expected, is a +// routine state for an HRIS-sourced profile; the account is created without the +// relation and update_user fills it in later. +func TestCreateAccount_InvalidManagerEmail_StillCreatesAccount(t *testing.T) { state := &testInsertServerState{} server := newTestInsertServer(state) defer server.Close() @@ -272,44 +277,103 @@ func TestCreateAccount_InvalidManagerEmail_FailsBeforeInsert(t *testing.T) { userRT := newTestUserResourceType(t, server) profile := baseCreateProfile() + profile["department"] = "Engineering" profile["manager_email"] = "not-an-email" - _, _, _, err := userRT.CreateAccount(context.Background(), + resp, _, _, err := userRT.CreateAccount(context.Background(), createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) - require.Error(t, err, "CreateAccount has no skipped_fields channel, so an invalid manager must fail loudly") - require.Equal(t, 0, state.insertCount, "validation must happen before the account is created") + require.NoError(t, err, "an unusable manager_email must not block account creation") + require.NotNil(t, resp) + require.Equal(t, 1, state.insertCount) + + require.Nil(t, extractRelations(state.lastInsertBody), "the unusable manager relation is dropped") + + // The valid attributes in the same profile still land. + orgs := extractOrganizations(state.lastInsertBody) + require.Len(t, orgs, 1) + require.Equal(t, "Engineering", orgs[0].Department) + + var raw map[string]any + require.NoError(t, json.Unmarshal(state.lastInsertRawBody, &raw)) + require.NotContains(t, raw, "relations") } -func TestCreateAccount_WrongTypedAttribute_FailsBeforeInsert(t *testing.T) { +// TestCreateAccount_WrongTypedAttribute_StillCreatesAccount covers the numeric +// employee_id / cost_center an HRIS routinely sends. These attributes enrich an +// account rather than define it, so a type mismatch drops the field instead of +// failing the insert. The update path stays strict - there the account already +// exists, so failing loudly costs nothing. +func TestCreateAccount_WrongTypedAttribute_StillCreatesAccount(t *testing.T) { state := &testInsertServerState{} server := newTestInsertServer(state) defer server.Close() userRT := newTestUserResourceType(t, server) - // A numeric employee_id would otherwise be silently dropped; the update - // path rejects it, and so must this one. profile := baseCreateProfile() profile["employee_id"] = 12345 + profile["cost_center"] = 4200 + profile["department"] = "Engineering" - _, _, _, err := userRT.CreateAccount(context.Background(), + resp, _, _, err := userRT.CreateAccount(context.Background(), createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) - require.Error(t, err) - require.Equal(t, 0, state.insertCount) + require.NoError(t, err, "a wrong-typed enrichment attribute must not block account creation") + require.NotNil(t, resp) + require.Equal(t, 1, state.insertCount) + + // The wrong-typed fields are dropped; the well-formed one in the same + // profile still applies. + orgs := extractOrganizations(state.lastInsertBody) + require.Len(t, orgs, 1) + require.Equal(t, "Engineering", orgs[0].Department) + require.Empty(t, orgs[0].CostCenter) + require.Nil(t, testExternalIDs(state.lastInsertBody)) +} + +// TestEmployeeInfoFromProfile_WrongTypedValuesAreReported pins the descriptions +// CreateAccount logs, so an operator can tell which attribute was dropped and +// why rather than discovering a blank field on the next sync. +func TestEmployeeInfoFromProfile_WrongTypedValuesAreReported(t *testing.T) { + patch, dropped := employeeInfoFromProfile(map[string]any{ + "department": "Engineering", + "employee_id": float64(12345), + "cost_center": true, + }) + require.NotNil(t, patch.department) + require.Equal(t, "Engineering", *patch.department) + require.Nil(t, patch.employeeID) + require.Nil(t, patch.costCenter) + require.ElementsMatch(t, []string{ + "cost_center (expected a JSON string, got a boolean)", + "employee_id (expected a JSON string, got a number)", + }, dropped) +} + +// TestEmployeeInfoFromProfile_WrongTypedAliasDoesNotMaskValidOne covers the +// skip-and-continue behavior across aliases: job_title and title are the same +// attribute, so a wrong-typed job_title must not discard a usable title. +func TestEmployeeInfoFromProfile_WrongTypedAliasDoesNotMaskValidOne(t *testing.T) { + patch, dropped := employeeInfoFromProfile(map[string]any{ + "job_title": float64(7), + "title": "Staff Engineer", + }) + require.NotNil(t, patch.jobTitle) + require.Equal(t, "Staff Engineer", *patch.jobTitle) + require.Empty(t, dropped, "nothing to report once an alias resolves the attribute") } func TestEmployeeInfoFromProfile_IgnoresOutOfScopeKeys(t *testing.T) { // Recovery details and custom schemas stay action-only (out of scope for // account provisioning); reading them here would quietly widen what the // create path can write. - patch, err := employeeInfoFromProfile(map[string]any{ + patch, dropped := employeeInfoFromProfile(map[string]any{ "department": "Engineering", "recovery_email": "recovery@example.com", "recovery_phone": "+14155550100", "custom_schemas": map[string]any{"MySchema": map[string]any{"region": "emea"}}, "given_name": "New", }) - require.NoError(t, err) + require.Empty(t, dropped) require.NotNil(t, patch.department) require.Equal(t, "Engineering", *patch.department) require.Nil(t, patch.recoveryEmail) @@ -320,7 +384,7 @@ func TestEmployeeInfoFromProfile_IgnoresOutOfScopeKeys(t *testing.T) { func TestApplyEmployeeInfoToNewUser_EmptyPatchLeavesUserUntouched(t *testing.T) { user := &directoryAdmin.User{PrimaryEmail: "a@example.com"} - require.NoError(t, applyEmployeeInfoToNewUser(user, userProfilePatch{})) + require.Empty(t, applyEmployeeInfoToNewUser(user, userProfilePatch{})) require.Nil(t, user.Organizations) require.Nil(t, user.ExternalIds) require.Nil(t, user.Relations)