fix: resolve rollout plugin iframe auth and org/site resolution - #779
markdaugherty wants to merge 8 commits into
Conversation
- Add setAccessToken to nx2/utils/api.js so daFetch can use a live token getter instead of loadIms()'s origin-dependent getNx() lookup - Rollout plugin (nx and nx2) now imports mergeCopy/overwriteCopy via relative path instead of a hardcoded da.live production URL, and primes daFetch with a live token read from nx1's daFetch.js (kept current by DA_SDK's ongoing postMessage refresh) - regionalDiff now accepts explicit org/site instead of always deriving them from location.hash, fixing undefined org/site when mergeCopy/rolloutCopy run outside loc's own app (e.g. MSM plugin) - Add test coverage for setAccessToken's override/live-getter behavior and regionalDiff's explicit org/site path Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CONFIG_CACHE was a single flat value, so a second call for a different org/site within one page's lifetime silently reused the first site's cached translate config. Now relevant since mergeCopy/ rolloutCopy derive org/site per-call rather than once per page. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| // Lets an iframe-hosted caller (e.g. a DA_SDK plugin) hand daFetch a live | ||
| // token instead of falling through to loadIms(), whose dynamic getNx() | ||
| // lookup resolves against window.location.origin and can diverge | ||
| // from the token the plugin's host frame actually issued. | ||
| let overrideAccessToken; | ||
| export function setAccessToken(getAccessToken) { | ||
| overrideAccessToken = getAccessToken; | ||
| } | ||
|
|
||
| export const daFetch = async ({ url, opts = { method: 'GET' }, redirect = false }) => { | ||
| const { accessToken } = await loadIms(); | ||
| const accessToken = (await overrideAccessToken?.()) || (await loadIms()).accessToken; |
There was a problem hiding this comment.
I think the iframe auth fix makes sense, but I’m a little nervous about introducing a module-global token override here. My concern isn’t that this literally overwrites the user’s IMS session, but that it creates shared mutable auth state in api.js. Today that’s probably okay because this plugin runs in its own iframe/module realm, but it feels brittle if this module is ever reused by multiple callers in one page context.
Would it be possible to make the token source explicit in a backward-compatible way by adding an optional getAccessToken field to daFetch({ ... }) instead?
daFetch({ url, opts, redirect, getAccessToken })with something like:
const accessToken = (await getAccessToken?.()) || (await loadIms()).accessToken;That keeps existing callers working unchanged (they still fall back to loadIms()), while iframe/plugin callers can inject their own token getter without introducing sticky global auth state. It also makes the auth dependency more local and easier to reason about in tests.
There was a problem hiding this comment.
Thanks for the recommendation. This makes sense to me as a more elegant solution, I'll work on refactoring the changes here.
| // loc's own app (e.g. mergeCopy invoked from another app's plugin) can | ||
| // call this for different sites within one page's lifetime, and a flat | ||
| // cache would silently serve the first site's config to every other one. | ||
| const cacheKey = `${org}/${site}`; |
There was a problem hiding this comment.
const cacheKey = `${org}/${site}`;If this is ever called with missing values, it’ll cache under "undefined/undefined". That’s probably harmless and not a regression, but it’s a little magic-string-y.
There was a problem hiding this comment.
Good catch — added an early guard so fetchConfig returns the same "Options not available" error for missing org/site without ever writing to the cache, instead of caching under an undefined/undefined key. See 6e4fa2d.
Addresses review feedback: avoid caching under an 'undefined/undefined' magic-string key when org or site is missing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Removes the module-global overrideAccessToken/setAccessToken mutable state from daFetch in favor of an optional getAccessToken callback passed per-call. Threads getAccessToken through loc project save/copy flows and both rollout plugins, which now resolve their own token via initIms() instead of mutating shared auth state. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…DaApiPath mergeCopy/rolloutCopy pass getAccessToken to regionalDiff, which now forwards it to fetchConfig's daFetch call instead of always falling back to loadIms(). source.save (used by saveHtml) resolves its URL via getDaApiPath, which calls isHlx6's internal upgrade-status ping. That ping ignored getAccessToken and always fell through to loadIms(), risking an unwanted sign-in prompt in contexts holding only a getAccessToken callback. isHlx6 and getDaApiPath now accept and forward it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This branch was successfully deployed
2 participants
The Rollout plugin in DA/EW is currently failing with a
Missing IMS Client IDerror when attempting a merge on Rollout.regionalDiffalso fails to correctly fetch the site config within an app/plugin due togetPathDetailsrelying onwindow.locationto parse these values.Changes implemented here:
getAccessTokenparam todaFetch(nx2/utils/api.js): callers can pass a live token getter instead of relying onloadIms()'s origin-dependentgetNx()lookup. Threaded throughmergeCopy/overwriteCopy/rolloutCopy,regionalDiff→fetchConfig, andsource.save/_saveHlx6/_saveDA→isHlx6/getDaApiPath(the hlx6 upgrade-status ping was previously always falling back toloadIms()regardless of the caller's token getter)mergeCopy/overwriteCopyvia relative path instead of a hardcodedda.liveproduction URL; primesdaFetchwith a live token read from nx1'sdaFetch.jsregionalDiffaccepts explicitorg/siteinstead of always deriving them fromlocation.hash, fixingundefinedorg/site whenmergeCopy/rolloutCopyrun outside loc's own app (e.g. the MSM plugin)fetchConfig's cache keyed by org/site instead of a single flat value, with a guard against caching when org/site are missinggetAccessToken's propagation (including throughregionalDiff/fetchConfigandisHlx6's upgrade-status ping) andregionalDiff's org/site resolution