fix: migrate Adobe Post Office auth to Bearer + x-api-key (LLMO-7021)

[JayKid]

What

Migrates the Adobe Post Office (APO) email integration to the new API contract ahead of APO's domain move.

Per the APO team, the contract change is auth header + endpoint:

• Authorization: IMS <token> → Authorization: Bearer <token>
• New required header x-api-key: <ims-client-id>

The endpoint URL itself (ADOBE_POSTOFFICE_ENDPOINT → https://apo-prod.adobe.io / https://apo-stage.adobe.io) is a Vault change, not code — it is not modified here.

Changes

• src/support/email-service.js — Bearer auth + x-api-key: env.LLMO_EMAIL_IMS_CLIENT_ID
• src/controllers/trial-users.js (trial/welcome email) — Bearer auth + x-api-key: env.EMAIL_IMS_CLIENT_ID
• test/support/email-service.test.js — updated header assertions + x-api-key coverage

Testing

npx mocha test/support/email-service.test.js test/controllers/trial-users.test.js — 59 passing.

Notes

• Coordinated with the APO team (reference: OneAdobe/aem-xwalk-trial#107); migration guide: PostOffice Migration wiki (FDT space).
• Companion change in spacecat-reporting-worker (weekly digest) is a separate PR.
• Deploy + Vault URL flip must land together per environment, else sends break.

Ref: LLMO-7021

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[MysticatBot]

Hey @JayKid,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - one operational concern with the new env vars.
Complexity: MEDIUM - small diff; API surface signal.
Changes: Migrates Adobe Post Office auth from IMS scheme to Bearer and adds x-api-key header at both email call sites (3 files).

Must fix before merge

1. [Important] Missing env var guard for new x-api-key variables - src/support/email-service.js:103 / src/controllers/trial-users.js:164 (details inline)

Non-blocking (1): minor issues and suggestions

• question: Two different env var names (LLMO_EMAIL_IMS_CLIENT_ID vs EMAIL_IMS_CLIENT_ID) - are these intentionally distinct IMS client registrations? See inline comment on src/controllers/trial-users.js:164.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 2m 58s | Cost: $2.76 | Commit: 9e707d8921b51d90fe0f8b4f1c5669b02dc69c5f
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

issue (blocking): The new x-api-key env var (LLMO_EMAIL_IMS_CLIENT_ID) is used without a presence check. This file already validates ADOBE_POSTOFFICE_ENDPOINT and throws when it is missing. If this var is unset, the header is sent as x-api-key: undefined, producing an opaque 401/403 from APO rather than a clear configuration error.

Add a guard alongside the existing endpoint check:

if (!env.LLMO_EMAIL_IMS_CLIENT_ID) {
  return { status: 500, body: { error: 'LLMO_EMAIL_IMS_CLIENT_ID is not configured' } };
}

Same pattern applies to EMAIL_IMS_CLIENT_ID in src/controllers/trial-users.js:164.

[MysticatBot]

question: This uses env.EMAIL_IMS_CLIENT_ID while email-service.js uses env.LLMO_EMAIL_IMS_CLIENT_ID. Are these intentionally different IMS client registrations (one for the LLMO email flow, one for trial/welcome emails)? If so, makes sense - just confirming since both target the same APO endpoint.

[github-actions]

This PR will trigger a patch release when merged.

[JayKid]

@MysticatBot thanks — addressed in be4ef1b.

Blocking (guard): Added a presence check for the x-api-key source env var before the request is built, in both call sites, matching the existing ADOBE_POSTOFFICE_ENDPOINT guard pattern:

• email-service.js → returns LLMO_EMAIL_IMS_CLIENT_ID is not configured (result error, no fetch)
• trial-users.js → returns internalServerError('EMAIL_IMS_CLIENT_ID is not configured') before token acquisition

Added unit tests for both guards (and updated two pre-existing trial-user tests whose env didn't set the client id).

Non-blocking (env var names): Yes, intentional and pre-existing — this PR didn't introduce the divergence. email-service.js already authed with the LLMO_EMAIL_* IMS credential set (LLMO email flow) and trial-users.js with the EMAIL_* set (trial/welcome flow). Each new x-api-key is deliberately paired with the same client id that mints that path's IMS token, so auth stays internally consistent per flow. Whether the two sets resolve to one or two IMS client registrations in Vault is a config question outside this diff — flagging it to confirm during the joint migration, but it doesn't affect this change's correctness.

Re-requesting review — thanks!

[MysticatBot]

Hey @JayKid,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - prior blocking finding addressed, no new issues.
Complexity: MEDIUM - small diff; API surface signal.
Changes: Migrates Adobe Post Office auth from IMS to Bearer scheme and adds x-api-key header, with env var guards at both call sites (4 files).
Note: CI checks are still pending - confirm they pass before merge.

Previously flagged, now resolved

• Env var guard for x-api-key source variables now present at both call sites, matching the existing endpoint guard pattern.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 42s | Cost: $4.63 | Commit: be4ef1b9cbede9abb331f57a55b1503fa911e739
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[calvarezg]

Second opinion on PR #3066 (MysticatBot already reviewed → Approved)

Verdict: Changes recommended — go further than MysticatBot. The code is fine; two things should be nailed down before merge, and neither is catchable by the unit tests (they only assert the header string the code sends).

What MysticatBot missed

1. [Important — the load-bearing one] The x-api-key = IMS client_id + Bearer contract is unverified. email-service.js:108, trial-users.js:167. The PR assumes Adobe Post Office (a) accepts Bearer in place of IMS, and (b) treats the IMS client_id as the gateway API key. That is the common Adobe I/O pattern and is likely correct, but nothing in the diff, the code, or the tests proves it — the tests only assert the literal string sent. If the assumption is wrong, this migration silently returns 401/403 in the real environment and no test catches it. Needs one out-of-band confirmation from whoever owns the Post Office API-key registration, not a code check.

2. [Important — but pre-existing, not introduced here] trial-users.js mutates shared context.env. trial-users.js:152-155 writes env.IMS_CLIENT_ID = env.EMAIL_IMS_CLIENT_ID (+ SECRET/CODE/SCOPE) onto the live context.env and never restores it. email-service.js:24-32 solves the identical problem the safe way — local copy { ...env, IMS_CLIENT_ID: ... }, with a comment "Does NOT mutate context.env." In a warm Lambda the context.env object is reused across invocations, so a later request expecting the default IMS_CLIENT_ID can read the email client's credentials. Confirmed, but these lines predate this PR — the diff only adds the header lines next to them. Worth a follow-up ticket, not a blocker for this diff.

3. [Minor] Guard duplicated, no shared helper. Both (and only these two, confirmed via git grep po-server) Post Office callers hand-roll the same guard + Bearer + x-api-key block. Fine today; a third caller could reintroduce the old IMS scheme. Follow-up only — a buildPostOfficeHeaders() helper would prevent drift.

On MysticatBot's blocking findings

• Its one blocker (missing env-var guard) is resolved by the guards now at both call sites — concur, addressed.
• Its open "two different env var names" question is a non-issue: the soft-fail-vs-500 split matches each file's existing local convention (sendEmail returns result.error for every missing-config case; the controller returns internalServerError for every failure), so each guard is consistent with its neighbors.

Discarded in verification

• [was Important] providedToken could mismatch x-api-key → 403 — false positive. Both real callers (opportunity-workspace-notifications.js, cdn-opt-in-notification.js) either omit the token or mint it via getEmailServiceToken, which uses LLMO_EMAIL_IMS_CLIENT_ID — the same client as the x-api-key. The foreign-client 403 is hypothetical.

---

Bottom line: MysticatBot's approve is defensible for code quality. The one risk it can't see from the diff is that the Bearer + x-api-key contract is an unverified external assumption — if it is wrong, the tests will not tell you. Worth confirming before merge.

_{Second opinion via char-multi-persona-review — complements, does not override, MysticatBot's review.}

[JayKid]

@calvarezg thanks for the second opinion — addressed in 16648dc:

#2 (shared context.env mutation) — fixed. trial-users.js now builds a local emailEnv copy and passes { ...context, env: emailEnv } to ImsClient.createFrom, instead of writing IMS_CLIENT_ID/SECRET/CODE/SCOPE onto the shared context.env. Mirrors the safe pattern in email-service.js, so the warm-Lambda credential-bleed risk is gone. Agreed it was pre-existing, but since the diff already sits on those exact lines it was cheaper to fix than to ticket.

Bonus (from your #684 review): empty-token guard. getEmailServiceToken now throws if IMS returns a 200 without access_token, so it fails legibly instead of sending Bearer undefined. Tests added for both.

#1 (Bearer + x-api-key contract) — confirmed as the real risk, verifying out-of-band. Can't be proven in the diff, agreed. Early dev signal: a live weekly-digest run in DEV hit the new endpoint and returned 403 ErrMissingAPIKey on the old deployed code — which confirms the new APO enforces x-api-key. Next: deploy this branch + one real send to confirm Bearer + key are accepted (200). Tracking the key-registration confirmation with the APO owner (Duy) on LLMO-7021.

#3 (shared buildPostOfficeHeaders()) — follow-up. Agreed, not for this diff; it'd span two repos. Noting it on LLMO-7021 for a later cleanup.

Re-requesting review.

[calvarezg]

Re-review (updated to 16648dc4)

Verdict: Approve — my earlier "changes recommended" items are resolved. One merge-gate remains, and it is not a code issue.

On my second-opinion findings:

1. [Important] context.env mutation — fixed correctly. trial-users.js now builds emailEnv = { ...env, IMS_CLIENT_ID: env.EMAIL_IMS_CLIENT_ID, ... } and passes ImsClient.createFrom({ ...context, env: emailEnv }). The direct writes onto shared context.env are gone — mirrors the email-service.js pattern, with a comment explaining the warm-Lambda reason. The x-api-key header reads env.EMAIL_IMS_CLIENT_ID from the un-mutated env, consistent with the token minted under that client. Traced the send path — nothing downstream depends on the old env.IMS_CLIENT_ID write. Clean.

2. [Important] Bearer + x-api-key contract — the real risk, verifying out-of-band. Correct call. The DEV signal (old code hitting the new APO returned 403 ErrMissingAPIKey) confirms the new endpoint enforces x-api-key, but does not yet prove Bearer + this key are accepted (a 200). That confirmation (branch deploy + one real send, tracked with the APO owner on LLMO-7021) is what should gate merge. Not a code blocker.

3. [Minor] Shared header helper — deferred to LLMO-7021. Fine; it spans two repos.

New in this update:

• Empty-token guard in getEmailServiceToken — throws if IMS returns 200 without access_token, avoiding Bearer undefined. Good, tested.
• [Minor, new] The empty-token guard is only on the email-service path, not trial-users. trial-users.js calls imsClient.getServiceAccessToken() directly and uses imsTokenPayload.access_token without the same null check, so a misbehaving IMS could still send Bearer undefined + x-api-key there. Low probability. Worth one line if you touch it again; not blocking.

Bottom line: code is good to go. The only thing between here and merge is the out-of-band 200 confirmation on the Post Office contract, which you own and are tracking.

_{Second opinion via char-multi-persona-review — complements, does not override, MysticatBot's review.}