-
Notifications
You must be signed in to change notification settings - Fork 16
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
IBX-7275: Exposed base admin UI configuration in REST API #1027
IBX-7275: Exposed base admin UI configuration in REST API #1027
Conversation
de08ce0
to
4ab0e0f
Compare
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.
Foremost I'm missing information what kind of response it forms, because there's no test coverage. Could you include in the description responses for XML and JSON formats?
265451b
to
94705df
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Very well formed XML 💪 JSON probably too :)
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.
@ciastektk Have you reviewed all data that could be returned for security implications, since this is not authenticated?
Is it public because that's easier, or because it strictly has to be?
Does it dump the entire config, or only what is strictly required by DAM?
Could you consider making it configurable, so those who don't need this for DAM can block it?
Some clients have reacted in the past about making content type info public, but that's already done in dedicated endpoints anyway, I think. So they'd have to disable rest entirely in the frontend to stop it.
If this dumps the entire config, another risk is if we later add some new config setting that is sensitive - and we forget that this endpoint is exposing all of it. A good reason to only expose what is specifically required. |
@ciastektk, @mnocon I have checked (the output in the description) and not found anything that is dangerous by itself. Some "known unknowns" exist:
|
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.
I've also looked into the data returned - there are only two differences:
- Data for Anonymous does not contain information about sections
- Data for Anonymous does not contain information about the current user
Both make sense to me
In case we're ok (for now) with this technical design - QA Approved!
We need to thing about next steps in terms of Providers, what kind of data should be exposed and of course how to secure current solution. This endpoint is very important because most of the data are used by UDW and frontend part changes of Image picker needs to be adjusted. For now, we decided to merge it as is. |
Endpoint
[GET] /api/ibexa/v2/application-config
This PR adds dedicated endpoint to get base admin ui config in REST API. The same configuration is stored in
window.ibexa.adminUIConfig
.Response
JSON
XML
Customize configuration option
Config Providers are extendable and allows to add extra data to configuration, so custom Generators with abstraction layer have been also added. That allows to customize configuration parameter in REST for specific namespace. Namespace is a key used in service tag e.g.:
multiFileUpload
(ref: https://github.com/ibexa/admin-ui/blob/main/src/bundle/Resources/config/services/ui_config/common.yaml#L21).Checklist:
$ composer fix-cs
)