diff --git a/README.md b/README.md index 5f0f75e4..0be5d98d 100644 --- a/README.md +++ b/README.md @@ -77,11 +77,50 @@ 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, 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. +- **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`. + ## Connector actions Connector actions are custom operations invoked on demand from C1 automations: @@ -90,7 +129,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 +147,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..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. + - **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`. @@ -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/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.go b/pkg/connector/user.go index b461d55c..feeacd61 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -484,6 +484,31 @@ 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. + // + // 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 { return nil, nil, nil, fmt.Errorf("credentialOptions cannot be nil") } diff --git a/pkg/connector/user_actions.go b/pkg/connector/user_actions.go index f29eb9ff..a8541a21 100644 --- a/pkg/connector/user_actions.go +++ b/pkg/connector/user_actions.go @@ -1427,6 +1427,78 @@ 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. +// +// 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, + } { + // 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 + } + } + + 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 + // 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)) + 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) + } + } + + // 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 dropped +} + const ( actionUpdateUser = "update_user" argUserProfile = "user_profile" @@ -1480,8 +1552,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 +1605,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 +1662,127 @@ 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 +} + +// 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, 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 + 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, dropped +} + +// 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..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" @@ -302,6 +305,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"}}, @@ -339,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 new file mode 100644 index 00000000..62ad18b0 --- /dev/null +++ b/pkg/connector/user_create_account_test.go @@ -0,0 +1,472 @@ +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) +} + +// 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() + + userRT := newTestUserResourceType(t, server) + + profile := baseCreateProfile() + profile["department"] = "Engineering" + profile["manager_email"] = "not-an-email" + + resp, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + 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") +} + +// 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) + + profile := baseCreateProfile() + profile["employee_id"] = 12345 + profile["cost_center"] = 4200 + profile["department"] = "Engineering" + + resp, _, _, err := userRT.CreateAccount(context.Background(), + createAccountProfile(t, profile), &v2.LocalCredentialOptions{}) + 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, 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.Empty(t, dropped) + 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.Empty(t, applyEmployeeInfoToNewUser(user, userProfilePatch{})) + require.Nil(t, user.Organizations) + 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) +}