[CXH-2366] - Add Zoom ownership transfer custom action - #33
Conversation
Adds a resource-scoped connector action that reassigns a user's meetings,
webinars, and cloud recordings to another Zoom user, then disassociates or
permanently deletes the user, via Zoom's DELETE /v2/users/{userId} transfer
query parameters.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if err != nil { | ||
| if isUserNotFound(err) { | ||
| return actions.NewReturnValues( | ||
| true, | ||
| actions.NewStringReturnField("message", fmt.Sprintf("user %s was already removed from the account", userID)), | ||
| ), nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: any 404 from DELETE /users/{userId} is reported as success: true with "already removed", but the target user is not the only thing this endpoint can fail to find — Zoom also 404s (code 1001, "User not exist") when transfer_email names a user that isn't in the account. In that case the user is not removed and nothing is transferred, yet the automation records success and moves on. Consider narrowing the short-circuit: inspect the Zoom error code/message in APIError.Body and only treat it as already-gone when it refers to userID, or only apply the short-circuit when no transfer fields were supplied.
There was a problem hiding this comment.
Went with a more conservative fix than string-matching the error body: transfer_email is now verified via a GetUser call before the destructive delete (Zoom documents that endpoint as accepting either a user ID or email). If that recipient doesn't exist, we fail fast with InvalidArgument before ever calling delete — so by the time the delete call runs, transfer_email is already known-good, and a 404 from that call can only mean the target user. No reliance on Zoom's undocumented error-message format. See f1f932f.
| // another user (TransferEmail) as part of removing them from the account. | ||
| type DeleteUserOptions struct { | ||
| // Action is Disassociate (unlink the user from the account) or Delete | ||
| // (permanently remove the user). Empty defers to Zoom's default (delete). |
There was a problem hiding this comment.
🟡 Suggestion: Zoom's documented default for the action query param on DELETE /v2/users/{userId} is disassociate, not delete. That makes this comment misleading for the DeleteUser path (CAPABILITY_RESOURCE_DELETE), which passes a zero DeleteUserOptions and therefore disassociates rather than permanently deletes.
| // (permanently remove the user). Empty defers to Zoom's default (delete). | |
| // (permanently remove the user). Empty defers to Zoom's default | |
| // (disassociate). |
| actions.NewStringReturnField("message", fmt.Sprintf("user %s was already removed from the account", userID)), | ||
| ), nil, nil | ||
| } | ||
| return nil, nil, fmt.Errorf("baton-zoom: transfer_and_delete_user: %s: %w", userID, err) |
There was a problem hiding this comment.
🟡 Suggestion: this path returns a bare wrapped error with no gRPC status code. pkg/zoom uses a raw *http.Client rather than uhttp.BaseHttpClient, so nothing upstream maps the Zoom status onto a code — a 403 or 429 reaches the SDK as Unknown. Since APIError now carries StatusCode, map it here (401→Unauthenticated, 403→PermissionDenied, 429→ResourceExhausted, 5xx→Internal) via uhttp.WrapErrors or status.Error so retry vs. surface is decided correctly.
There was a problem hiding this comment.
Added mapAPIError to map APIError.StatusCode onto gRPC codes (401/403/404/429/5xx) for this action's error paths. f1f932f
| return actions.NewReturnValues( | ||
| true, | ||
| actions.NewStringReturnField("message", fmt.Sprintf("user %s data transferred and %sd from the account", userID, deleteAction)), | ||
| ), nil, nil |
There was a problem hiding this comment.
🟡 Suggestion: all three transfer flags are optional, so the common "just remove the user" invocation returns "user X data transferred and deleted from the account" when nothing was transferred. Consider building the message conditionally on whether any transfer flag was set, so the automation record reflects what actually happened.
There was a problem hiding this comment.
Fixed — message now only claims a transfer happened when a transfer flag was actually set. f1f932f
Connector PR Review: [CXH-2366] - Add Zoom ownership transfer custom actionBlocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0 Review SummaryFull-diff scan for security and correctness across the new Also verified while reviewing: Security IssuesNone found. Correctness IssuesNone found. Suggestions
Reviewed head SHA Prompt for AI agents |
- Pre-validate transfer_email via GetUser before the destructive delete call, so a later 404 on delete is unambiguous (Zoom returns the same 404/1001 for a missing target user and a missing transfer_email recipient, with no structured field to tell them apart). - Correct the DeleteUserOptions.Action doc comment: Zoom's default when action is omitted is disassociate, not delete. - Map APIError.StatusCode onto gRPC codes (401/403/404/429/5xx) so retry-vs-permanent-failure is decided correctly upstream. - Only claim "data transferred" in the success message when a transfer flag was actually set. - Add unit tests for the action handler and DeleteUserWithTransfer's query construction.
|
Addressed all 5 review-bot suggestions in f1f932f:
|
…failures - mapAPIError now treats any unmapped 4xx (e.g. Zoom's 400 for transfer_email matching the deleted user's email) as InvalidArgument instead of falling through to an uncoded, retryable Unknown. - Replace t.Fatal/t.Fatalf calls inside httptest handler goroutines with t.Error/t.Errorf — Fatal's Goexit only unwinds the handler goroutine without writing a response, leaving the client to see a broken connection instead of the intended test failure.
…tions
- transferAndDeleteUserAction now rejects an empty user_id.Resource and a
user_id.ResourceType that doesn't match "user", before the delete call.
--invoke-action bypasses the platform's own AllowedResourceTypeIds
check, so a malformed reference could otherwise reach Zoom, 404, and
get reported as "already removed" success.
- GetUser now builds its URL with url.JoinPath instead of raw fmt.Sprint
concatenation, matching DeleteUser/CreateUser/PatchUserLicense. This
matters now because the transfer_email precheck is the first caller to
pass it untrusted, operator-supplied text — unescaped, a value
containing "?" could make the verification GET resolve a different
identifier than the literal value implies.
- TestTransferAndDeleteUserAction_SuccessMessages now asserts the actual
query string sent to the client, not just the success message, so a
field swapped in the zoom.DeleteUserOptions{} literal (verified via a
temporary local swap of TransferMeeting/TransferWebinar, which this
test catches) wouldn't silently pass.
| userID := userRef.GetResource() | ||
| if userID == "" { | ||
| return nil, nil, status.Error(codes.InvalidArgument, "baton-zoom: transfer_and_delete_user: user_id is required") | ||
| } | ||
| // RequireResourceIDArg doesn't enforce AllowedResourceTypeIds itself — | ||
| // the platform does that before invocation, but --invoke-action (local | ||
| // and CI testing) bypasses that check, so a wrong-type reference would | ||
| // otherwise reach the delete call below. | ||
| if resourceType := userRef.GetResourceType(); resourceType != "" && resourceType != resourceTypeUser.Id { | ||
| return nil, nil, status.Errorf(codes.InvalidArgument, "baton-zoom: transfer_and_delete_user: user_id must reference a %q resource, got %q", resourceTypeUser.Id, resourceType) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: url.JoinPath escapes ?/# (which the new TestGetUser_EscapesSpecialCharacters covers) but it does not escape / — it runs path.Join, which resolves ../ segments. A user_id resource of ../accounts/{id} therefore turns the delete into DELETE /v2/accounts/{id}?action=delete (Zoom's sub-account disassociate endpoint), and a transfer_email of ../accounts/me makes the verification GET succeed against an unrelated endpoint. Both values are caller-supplied and only checked for emptiness, so consider rejecting userID/transferEmail containing / (or strings.Contains(v, "..")) here before the client calls, plus a ../ case in the escaping test.
There was a problem hiding this comment.
Verified this empirically (url.JoinPath("https://api.zoom.us/v2", "users", "../accounts/me") really does resolve to ".../v2/accounts/me") — real bug. Fixed both layers: GetUser and DeleteUserWithTransfer now build the URL with raw concatenation + url.PathEscape instead of url.JoinPath, confining the id to one opaque path segment no matter its content; and the handler now rejects "/" in user_id/transfer_email outright as defense in depth. Added tests confirming ../ and "?" both land in the request path verbatim. See e521b5d.
…thTransfer url.JoinPath resolves ../ segments in its inputs (verified empirically), so a user_id or transfer_email of "../accounts/me" redirected the request to a different Zoom endpoint entirely, bypassing the transfer_email precheck this action relies on for its 404 disambiguation. - Switch GetUser and DeleteUserWithTransfer to raw concatenation with url.PathEscape instead of url.JoinPath, confining the caller-supplied id to a single opaque path segment regardless of its content. - Reject "/" in user_id and transfer_email in the handler as defense in depth, ahead of the client calls. - Add tests confirming ../ and "?" both land in the request path verbatim rather than being resolved or split into a query string.
| // leave "?" to split off a bogus query string; PathEscape avoids | ||
| // both. | ||
| assert.Empty(t, gotRawQuery) | ||
| assert.Equal(t, "/users/"+tt.id, gotPath) |
There was a problem hiding this comment.
🟡 Suggestion: gotPath captures r.URL.Path, which is the decoded path, so this assertion passes even for an implementation that sends the dot-segments unescaped — plain c.baseURL + "/users/" + userId yields EscapedPath() == "/users/../accounts/me" on the wire, and the server decodes it back to exactly this value. It does catch the url.JoinPath regression (which resolves to /accounts/me client-side), but it does not verify the property the change actually relies on: that ..%2F reaches Zoom escaped so an intermediary proxy can't normalize it. Capture r.URL.EscapedPath() (or r.RequestURI) and assert "/users/..%2Faccounts%2Fme". Same applies to line 84 in TestDeleteUserWithTransfer_EscapesDotSegments.
There was a problem hiding this comment.
Confirmed and fixed — verified empirically that r.URL.Path decodes to the same string whether the request was sent with the dot-segment escaped or raw, so the old assertion couldn't distinguish a correct implementation from a broken one. Both tests now assert r.URL.EscapedPath() against the literal encoded form. Verified the new assertion actually discriminates: temporarily reintroduced the naive-concatenation bug in GetUser, confirmed only the new wantEscapedPath checks failed (old Path/RawQuery checks still passed, as predicted), then reverted. Test-only change, no production code touched. See cfeb3d3.
r.URL.Path is decoded, so it reads identically for "../accounts/me" whether the request was sent with the dot-segment escaped or raw -- it can't distinguish a correct url.PathEscape implementation from a broken naive-concatenation one. r.URL.EscapedPath() is what actually reaches the wire, which is the property the fix in GetUser and DeleteUserWithTransfer is relied on for. Verified by temporarily reintroducing the naive-concatenation bug and confirming these assertions (and only these) fail, then reverting.
| transferEmail, _ := actions.GetStringArg(args, argTransferEmail) | ||
| transferMeeting, _ := actions.GetBoolArg(args, argTransferMeeting) | ||
| transferWebinar, _ := actions.GetBoolArg(args, argTransferWebinar) | ||
| transferRecording, _ := actions.GetBoolArg(args, argTransferRecording) | ||
| transferring := transferMeeting || transferWebinar || transferRecording | ||
|
|
||
| if transferring && transferEmail == "" { | ||
| return nil, nil, status.Error(codes.InvalidArgument, "baton-zoom: transfer_and_delete_user: transfer_email is required when transfer_meeting, transfer_webinar, or transfer_recording is set") | ||
| } | ||
| if strings.Contains(transferEmail, "/") { | ||
| return nil, nil, status.Error(codes.InvalidArgument, "baton-zoom: transfer_and_delete_user: transfer_email must not contain \"/\"") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: two ways a transfer can silently not happen before an irreversible delete. (1) The ok returns from GetStringArg/GetBoolArg are discarded, so an arg that is present but wrong-typed (e.g. "transfer_meeting": "true" as a string via --invoke-action, the same bypass path guarded against above for resource_type) reads as false and the user is deleted with no transfer, reporting success. (2) If the operator sets transfer_email but ticks none of the three checkboxes, transferring is false — the user is permanently deleted, nothing is transferred, and the message is a plain "deleted from the account" with no signal. Consider rejecting present-but-wrong-typed args, and requiring at least one transfer flag when transfer_email is set (which would also avoid the pre-flight GET /users/{email} and the transfer_email query param on a request that transfers nothing).
| func (c *Client) DeleteUserWithTransfer(ctx context.Context, userId string, opts DeleteUserOptions) error { | ||
| // See GetUser: PathEscape, not url.JoinPath, so a userId containing ../ | ||
| // can't redirect this request to a different Zoom endpoint. | ||
| requestURL := c.baseURL + "/users/" + url.PathEscape(userId) |
There was a problem hiding this comment.
🟡 Suggestion: this replaces url.JoinPath with raw concatenation, and NewClient does not normalize c.baseURL (it comes straight from the operator-settable --base-url). A base URL with a trailing slash now yields …/v2//users/{id}, where JoinPath previously collapsed it. Combined with the new 404-as-success idempotency in transferAndDeleteUserAction, a 404 from the malformed path would be reported as "user already removed from the account" rather than as a failure. Trimming a trailing / in NewClient (which would also fix the pre-existing fmt.Sprint call sites) keeps the PathEscape fix without the regression.
2 participants
INTENT
CXH-2366 has no acceptance-criteria section; the criteria below are derived from its description and the originating Slack request (ShopMy, via #C087N95G127), which asks for the same custom-action pattern across five connectors (Zoom, Notion, Figma, Loom, Miro):
baton-zoomthat transfers ownership of a user's calls/meetings/webinars/recordings to another user, invocable on demand (e.g. via C1's "Perform connector action" automation step) prior to deleting the account.Fixes CXH-2366
What / Why / How
What: Adds a resource-scoped
transfer_and_delete_useraction on the Zoom connector's user resource type. It calls Zoom'sDELETE /v2/users/{userId}with theaction,transfer_email,transfer_meeting,transfer_webinar, andtransfer_recordingquery params in a single call, then removes the user (disassociate or permanent delete, operator's choice).Why: Customer (ShopMy) wants an explicit, on-demand step to hand off a departing user's Zoom data to e.g. their manager before removing them, rather than losing recordings/meetings/webinars on deletion.
How tested:
go build ./...,go vet ./..., andgo test ./...all pass. Self-reviewed againstreview-actions-layer's 20 rules (all pass/N/A — see below). Manual end-to-end testing against a real Zoom sandbox tenant/user is still needed before merge (see Follow-ups) since this is a permanent, destructive operation not suited to running unattended in CI.Review notes
ResourceActionProvider(notGlobalActionProvider) sinceACTION_TYPE_RESOURCE_DELETEisn't a lifecycle-FSM type — mirrorsbaton-okta'sdelete_useraction.zoom.APIError(previously all client errors were untyped strings) so the handler can treat a 404 ("already deleted") as success on retry, per the idempotency rule.baton_capabilities.json/config_schema.jsonregenerated via./connector capabilities/./connector config;docs/connector.mdxupdated with the action table.Follow-ups (not in scope for this PR)
baton-zoomis not currently container-ready (fails theRunConnector/session-store/V2-interface audit). Out of scope here; should be tracked as its own ticket before this connector can run in the container/Lambda deployment path.Delete()deprovisioning flow (auto-derives the manager, transfers recordings/meetings/whiteboards automatically, no operator input). That's a different customer ask (automatic vs. this PR's on-demand, operator-controlled action) but touches the samepkg/zoom/client.goDeleteUsercode path, so there's a real chance of a merge conflict or duplicated logic depending on merge order. Worth a quick sync with whoever owns CXH-2024 (Jacob Aguon) on whether the two should be reconciled (e.g. this action's client method could eventually replace/absorb feat: add transfer parameters to user deletion #31's, or feat: add transfer parameters to user deletion #31 could addtransfer_whiteboardsupport to this action's schema instead of a separate code path).