Media lib Quality Improvements#581
Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch.
|
607e2b6 to
8b069dd
Compare
| } | ||
|
|
||
| if (resp.status === 401 || resp.status === 403) { | ||
| if (resp.status === HttpStatus.UNAUTHORIZED || resp.status === HttpStatus.FORBIDDEN) { |
There was a problem hiding this comment.
Not sure if this is really needed? The status numbers are pretty common, don't know if we should have a constant for this.
There was a problem hiding this comment.
✅ Resolved - Reverted to using magic numbers for consistency with the rest of the codebase. Other blocks (outside media-library) also use magic numbers for HTTP status checks.
Commit: bcdd9b0 - refactor: use HTTP status code numbers for consistency with codebase
| --ml-bg: light-dark(rgb(249 249 249), rgb(28 28 28)); | ||
| --ml-surface: light-dark(rgb(255 255 255), rgb(26 26 26)); | ||
| --ml-topbar-bg: light-dark(rgb(248 248 248), rgb(27 27 27)); |
There was a problem hiding this comment.
IHMO we should stick with the spectrum styles. With NX2 you will also get light/dark mode
There was a problem hiding this comment.
✅ Resolved - Switched to nx2 Spectrum styles with inline light-dark() pattern for consistent dark mode support across all components.
Changed from custom CSS variables (--ml-bg, --ml-surface, etc.) to inline light-dark(rgb(...), rgb(...)) values, matching the pattern used by sidebar and ensuring proper dark mode in shadow DOM components.
Commits:
There was a problem hiding this comment.
t is pretty short. I think is this on other places as well.
There was a problem hiding this comment.
✅ Resolved - Renamed t() to getMessage() across all 13 files for better clarity since this is not i18n.
Commit: d128547 - refactor: rename t() to getMessage() for clarity
| try { | ||
| const { loadIms } = await import('../../../../nx2/utils/ims.js'); | ||
| const { accessToken } = await loadIms() || {}; | ||
| const { initIms } = await import('../../../utils/daFetch.js'); |
There was a problem hiding this comment.
While this might work we should use NX2, so we can remove NX.
Also there is const { loadIms } = await import('../../utils/ims.js');util which is maybe used better here https://github.com/adobe/da-nx/blob/main/nx/utils/ims.js
There was a problem hiding this comment.
We're using nx here instead of nx2 because nx provides the setImsDetails(token) function in daFetch.js which enables token injection in plugin mode. This capability is critical for the SDK integration where tokens need to be injected externally.
Currently, nx2 doesn't have this token injection capability. Should we enhance nx2 to support this use case so we can eventually migrate away from nx?
There was a problem hiding this comment.
Not sure I fully understand the flow and usage of setImsDetails(...) are you doing a different IMS login?
Also would like to get a thought from @andreituicu on this from security pov.
There was a problem hiding this comment.
Good question! Here's the flow:
Current Implementation (using nx)
The media library supports plugin mode where it runs embedded in another application (like Adobe Express or other Adobe tools). In this mode:
- Host app handles authentication: The parent application already has an authenticated IMS session
- Token is passed via MessagePort: The parent sends the IMS token through
postMessagewhen initializing the SDK (seenx/utils/sdk.js:70-71) - Token is injected:
setImsDetails(token)stores this externally-provided token - No duplicate auth: The media library reuses the host's token instead of triggering its own IMS login
Code flow:
// In host app
mediaLibraryIframe.contentWindow.postMessage({ ready: true, token }, '*', [messagePort])
// In media library (nx/utils/sdk.js:70-71)
if (e.data.token) {
setImsDetails(e.data.token); // Inject external token
}
// Later in daFetch (nx/utils/daFetch.js:22-24)
if (imsDetails) { // Use injected token
const { accessToken } = await initIms();
opts.headers.Authorization = `Bearer ${accessToken.token}`;
}nx2 Limitation
nx2/utils/ims.js always calls loadIms() which:
- Loads the Adobe IMS library directly
- Performs its own authentication flow
- Has no mechanism to accept externally-provided tokens
Security Note
This is not a "different IMS login" - it's reusing an already-authenticated token from a trusted parent application. The host app has already completed Adobe IMS authentication; we're just accepting that token rather than forcing a second auth.
@andreituicu - Would appreciate your security review on this approach. The alternative would be to add setImsDetails() capability to nx2.
There was a problem hiding this comment.
🤔 For the use case of the embedding in non-DA properties I think this should either live somewhere else or in some Media Library only code, not in a shared one.
if we need special handling for IMS here for this use case this not be part of NX or DA core.
There was a problem hiding this comment.
in this specific use case , the media library code from da-nx is being loaded via da.live in the plugin mode. Plugin being used in layout / da editor mode. Even though plugin code is part of non-da.live related repo, as the plugin is loaded on da.live , i thought i would reuse as much as possible from da-nx.
I am open to move the specific IMS functionalities that will enable me to reuse code between app and plugin into media library , however it will sort of creates duplication of code. And i don't know whether there are any other apps that needed this model of reuse app and plugin from same code base.
Plugin reference - https://github.com/adobe-rnd/aem-apps/blob/main/tools/plugins/media-library/media-library.html
@mhaack Since I have couple of other changes added into this PR , if it is agreeable , i can take up the IMS related functionality ( moving into media library) in another PR
fd71c87 to
5bc18fa
Compare
- Replace hardcoded HTTP status codes (401, 403, 500) with HttpStatus constants across 12 files - Centralize CSS background colors into custom properties (--ml-bg, --ml-surface, --ml-topbar-bg) - Fix error propagation in worker to preserve status codes without string parsing - Add error hints to DA_READ_DENIED for consistent user guidance - Fix import path for HttpStatus in worker/fetch.js 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…onsistent dark mode
…remove redundant auditlog fetch
5bc18fa to
a361f06
Compare
Please always provide the GitHub issue(s) your PR is for, as well as test URLs where your change can be observed (before and after):
Fix #
Test URLs:
Notes