Migrate url shortener service#50896
Conversation
💔 Build Failed |
💔 Build Failed |
💔 Build Failed |
|
@elastic/kibana-security The last test failure (https://github.com/elastic/kibana/pull/50896/checks?check_run_id=331286604) looks a bit strange to me. Shouldn't the API return |
💔 Build Failed |
I agree |
💚 Build Succeeded |
| "id": "share", | ||
| "version": "kibana", | ||
| "server": false, | ||
| "server": true, |
There was a problem hiding this comment.
the plugin contains server part
There was a problem hiding this comment.
Yes, this means the platform should pick up the server directory for server side code. This PR moved the url shortener into the share plugin whereas before it was only a client side thing (handling the share registry)
💚 Build SucceededTo update your PR or re-run it, just comment with: |
kertal
left a comment
There was a problem hiding this comment.
Code LGTM 👍 tested locally with state:storeInSessionStorage on/off, works as expected
one question: when you assert a valid url , wouldn't it be enough to check the url to start with /app ?
|
Merging despite app-arch code ownership. This PR will transfer ownership to the kibana app team. |
Good question, I just moved the existing functionality. As I'm always unsure in security related cases - ping @elastic/kibana-security is something blocking us to just check whether it's a valid URL that starts with the string |
…le-sql-highlighting * 'master' of github.com:elastic/kibana: (56 commits) Migrate url shortener service (elastic#50896) Re-enable datemath in from/to canvas timelion args (elastic#52159) [Logs + Metrics UI] Remove eslint exceptions (elastic#50979) [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405) [ML] API integration tests - initial tests for bucket span estimator (elastic#52636) [Watcher] New Platform (NP) Migration (elastic#50908) Decouple Authorization subsystem from Legacy API. (elastic#52638) [APM] Fix some warnings logged in APM tests (elastic#52487) [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500) [ui/public/utils] Move items into ui/vis (elastic#52615) fix newlines in kbn-analytics build script Add top level examples folder and command to run, `--run-examples`. (elastic#52027) feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662) [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675) [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657) [SIEM][Detection Engine] Adds the default name space to the end of the signals index [Logs UI] Generalize ML module management (elastic#50662) Removing stateful saved object finder (elastic#52166) Shim oss telemetry (elastic#51168) [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344) ... # Conflicts: # src/legacy/core_plugins/console/public/legacy.ts # src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts # src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts # src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js
This PR migrates as much as possible of the url shortener service.
In case of session storage for URL state is enabled, the url shortener service renders the app with an injected var which isn't possible in the new platform currently.
Because of this this part still resides in the legacy platform and the new url shortener service will redirect the request if session storage is enabled.
Depends on #50137
Related: #50775