Conversation
Iframe the Experience Governance MFE's shell-free embed entry and bridge it with a vanilla re-implementation of @assets/microfrontend's MessageRpc, so no Unified Shell dependency is needed. - resolve the qa/stage/prod CDN bundle from da-live's hostname, with an ?egov=local override for a local MFE dev server; the same env picks the MFE's `env` prop so bundle and backend API always match - deep-link via ?egovPath=, so a governance screen is shareable and survives reload. Writes always replace: adding history entries first needs a popstate listener feeding the path back into the MFE, so browser Back/Forward does not navigate within the tab yet - pin postMessage to the embed's origin and validate event.origin inbound, since the props payload carries a live IMS bearer token - unit-test the handshake, props and callback protocol plus the ?egovPath= round trip Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
An unrecognized or missing env resolved to stage in three places: the bridge's props default, the embed bundle URL, and the env prop handed to the MFE. That fails unsafe -- a user on da.live could silently get stage data in a production-looking UI. All three now fall back to prod, which also matches what resolveEgovEnv already does for unknown hostnames.
Test coverage for the two security-relevant branches: - Env resolution had none, and its fallbacks had just changed in 7c6975e under review. Adds cases for the ?egov= override (accepted and rejected), the localhost / *.aem.page stage split, and unknown host to prod. The `|| PROD` fallbacks themselves are unreachable, since resolveEgovEnv only ever returns an allow-listed value, so the tests cover the reachable route and say so rather than implying the defensive line is guarded. Also pins two ways the guard could be loosened later: ?egov=PROD stays rejected (the allow-list is lowercase-only) and lookalike hosts miss the suffix check. - The bridge's event.origin check is what keeps the IMS token in, and nothing guarded it. Adds a test that a message from another origin is dropped, verified by deleting the check and watching it fail. Code: - Stop advertising onToast, which reactCallback never handled. Claiming a callback the host ignores can make the MFE suppress its own in-frame UI for it, turning a dropped toast into silence. - Grant clipboard-write on the iframe, matching every sibling embed. The frame is cross-origin, where clipboard-write defaults to self, so any copy inside it fails silently without this. - Use TAB_CONTEXT/TAB_MEMORY instead of 8 raw tab strings; the file imported from constants.js but never pulled the tab ids in, so it could drift from renderers.js. - Name the egov/egovPath query params instead of repeating them inline. - Fix the TAB_CONTEXT comment, which said the label was "Context". - Document colorScheme/locale: 'light' matches the host, which skills.html pins to light until dark mode lands, so the pin and the prop have to be un-pinned together.
|
Thanks for taking the time on this one. It was a genuinely careful read, and it made the PR better. All eight are done in f0e33ac: Tests
Code
|
fmeschbe
left a comment
fmeschbe
left a comment
There was a problem hiding this comment.
I am supportive of this addition in principle to offer access to Enterprise Context from within Experience Workspace. This is a first step in to the right direction. This being the first step, actual integration may change as we evolve Experience Workspace.
I think the Experience Workspace extensibility model at this point is much too intrusive and requires to many overlapping changes as exhibited by this PR. However, this is not this PR's fault, but rather the problem of the current extensibility model (and yes, this is my actual human wording ;-) ).
Going forward, Experience Workspace extensibility should be constructed in such a way to make plugs much easier and straight forward. However, I could also imagine that there may be more to it than just making extensions more pluggable.
Keep in mind that this repository IS an extension being loaded into experience workspace. So everything this PR is modifying is part of the extension, not the core experience workspace code. Whether the skills mgmt UI stays an extension, moves into core EW or even moves into the CMA bridge code will be a separate question. When we discuss that question, we'll also have to decide how we make the skills mgmt UI extensible, so a PR like this can be less intrusive. |
|
@anfibiacreativa May I please ask you to have a second look at this PR to see if your feedback was addressed by Alejandro and we can proceed with merging? |
The egov MFE iframe only ever loads a single trusted governance origin, so allow="clipboard-write *" grants more than needed. Dropping the `*` defaults the allowlist to 'src', following least-privilege.
Per review, keep this PR focused on the Enterprise Context tab. The TAB_MEMORY constant swap is orthogonal cleanup and can land separately.
5 participants
What this does
Adds an Enterprise Context tab to the Skills Editor that embeds the Experience Governance micro-frontend (MFE from here on).
Why
Enterprise Context has only ever been reachable from Experience Manager, via
aem-home-ui. Anyone who wanted to manage it had to leave Experience Workspace and go somewhere else to do it. This brings that UI in here so it reads as part of the workspace instead of a separate destination.Embedding the micro-frontend rather than porting the UI keeps a second benefit that matters to both sides: the MFE stays on its own release cadence. Changes its maintainers ship appear in Experience Manager and here at the same time, with no follow-up work in this repo.
How
The MFE already ships a shell-free
embedentry that expects no Unified Shell, so this targets that. The host side is a vanilla re-implementation of the MessageRpc protocol from@assets/microfrontend(handshake,reactSetProps,reactCallback— seesrc/message/MessageRpc.ts), which avoids taking on Unified Shell as a dependency.?egov=localfor a local MFE dev server (same convention as?nx=/?da-admin=). The same env selects the MFE'senvprop so bundle and backend API match.?egovPath=, so a governance screen is shareable and survives reload.postMessageis pinned to the embed's origin andevent.originvalidated inbound, rather than'*'.How to test
URL for testing:
That page renders the editor and the new tab, but it has no IMS session (da-live's shell provides it), so the MFE itself can't authenticate there. For the working feature, run the stack locally:
Then open
http://localhost:3000/apps/skills?nx=local#/<org>/<site>and pick the Enterprise Context tab.?egovPath=/brands/<id>and reload — should land on that screen.?egov=qa|local.Known limitations
Scoped out deliberately to keep this PR to a reviewable size. Both are planned as follow-up PRs.
popstatelistener that feeds the path back into the MFE (it owns its own router and only sees the host URL through props). Without that, Back would change the URL while the iframe kept showing the same screen. This is the next PR; note the MFE's usual parent doesn't wire it up either.Checks
npm test— 228 unit tests pass, including new coverage for the bridge's handshake/props/callback protocol and the?egovPath=round trip.npm run lint— clean.Auth was checked with the
darkalleyIMS client against a single org. Other orgs and anonymous sessions haven't been exercised yet; the bridge omitsimsToken/imsOrgwhen it can't read them.