-
Notifications
You must be signed in to change notification settings - Fork 32
Media lib Quality Improvements #581
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
kmurugulla
wants to merge
11
commits into
main
Choose a base branch
from
medialib-quality
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
b0f5c10
fix: use daFetch.initIms for plugin mode preview auth
kmurugulla 5537a7d
fix: clarify author role requirement for medialog access
kmurugulla e1abe64
feat: add user-friendly validation error messages
kmurugulla 51870cb
refactor: centralize hardcoded user messages
kmurugulla c41165d
fix: ensure consistent dark mode backgrounds
kmurugulla eb78245
refactor: improve code quality with constant reuse and error consistency
kmurugulla 5bbc395
refactor: use HTTP status code numbers for consistency with codebase
kmurugulla 3c3f042
refactor: rename t() to getMessage() for clarity
kmurugulla 5c247e6
refactor: replace custom CSS variables with inline light-dark() for c…
kmurugulla 9a1b48c
refactor: remove misleading CSS comment about variable inheritance
kmurugulla a361f06
refactor: optimize full index memory usage with field projection and …
kmurugulla File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.jsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're using nx here instead of nx2 because nx provides the
setImsDetails(token)function indaFetch.jswhich 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
postMessagewhen initializing the SDK (seenx/utils/sdk.js:70-71)setImsDetails(token)stores this externally-provided tokenCode flow:
nx2 Limitation
nx2/utils/ims.jsalways callsloadIms()which: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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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