Fix ilm navigation #81664

yuliacech · 2020-10-26T19:03:54Z

Summary

Fixes #81394.
This PR adds a decoding attempt of policy name on edit policy page.

Examples with different special characters

Example for policy name `test?`:

navigation is done from policy table via react router
- the route is encoded twice, since there is a decodeURI call in history.push
```
encodeURI(`/policies/edit/${encodeURIComponent(policyName)}`)
reactRouterNavigate('/policies/edit/test%253F')
```
- url in the address bar is already decoded once: test%3F
- match param provided by react router is already decoded once test%3F
- component decodes match param to get the correct policy name test?
when the user reloads the page, it works similar to router navigation 1.
when the user copies link to the edit page from the policy table or opens it in a new tab, the url in the address bar is different
- url in address bar: test%253F (because of the double encoding at the beginning)
- match param provided by react router: test%3F
  - sequence %25 is already decoded by react router, so that match param is different from value in the address bar
- component decodes match param to get the correct policy name test?
reloading the page with double encoded url in the address bar works similar to 3.

Percent sign in the policy name `test%`

navigation via react router
```
reactRouterNavigate('/policies/edit/test%2525')
```
- url in the address bar is decoded once: test%25
- match param is decoded once: test%25
- component decodes the match param to get the correct name test%
if the user reloads the page, it works differently to navigation 1.
- url in the address bar is test%25
- match param is already completely decoded: test%
  - as in the first example, sequence %25 is being automatically decoded by the react router and is now different from the value in the address bar
- component tries to decode the match param, ignores URI malformed error and uses initial test% value
user opens the link from policy table in a new tab
- address bar test%2525
- match param test%25
- component decodes to test%
reloading the page works similar to 3.

Known issues:

Since sequence %25 is being automatically decoded by react router in match param, percent sign can't be used with other special characters and the sequence itself can't be used in policy names.

Example percent sign with other characters `test%?`

router navigation reactRouterNavigate('test%25%3F')
- address bar test%25%3F
- match param test%25%3F
- component decodes to test%?
user reloads the page doesn't work
- address bar test%25%3F
- match param test%%3F partially decoded
- component tries to decode the match param, ignores the error and uses test%%3F wrong policy name
user opens the link from the table in a new tab
- address bar test%2525%253F
- match param test%25%3F (%25 decoded to %)
- component decodes to test%?
user reloads the page opened in a new tab, works similar to 3.

Example `%25` sequence `test%25`

router navigation reactRouterNavigate('test%252525')
- address bar test%2525
- match param test%2525
- component decodes to test%25
user reloads the page doesn't work
- address bar test%2525
- match param test%25 (%25 decoded to %)
- component decodes the match param to test% wrong policy name
user opens the link from the table in a new tab
- address bar test%252525
- match param test%2525 (%25 decoded to %)
- component decodes to test%25
user reloads the page opened in a new tab, works similar to 3.

TL;DR Edit page navigation will not work in some cases for policies with names that contain either percent sign with other special characters or %25 sequence.

# Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/constants.ts # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.test.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx

yuliacech · 2020-10-27T13:46:50Z

@elasticmachine merge upstream

elastic#81664

elasticmachine · 2020-10-27T15:29:44Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

… characters (#80835) * Add uri decode to es_ui_shared and fix data stream issue with % name * Add a test for data streams tab opened for name with a dollar sign * Import uri decode function from es_ui_shared and fix navigation issues for filters * Add tests for data streams with special characters in name * Revert react router navigate changes (is done in a separate PR) * Reverting changes to dataManagement es client and get data stream api route * Fix data stream name filter when activated from a url parameter * Clean up for better consistency and fixes after #81664 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

… characters (elastic#80835) * Add uri decode to es_ui_shared and fix data stream issue with % name * Add a test for data streams tab opened for name with a dollar sign * Import uri decode function from es_ui_shared and fix navigation issues for filters * Add tests for data streams with special characters in name * Revert react router navigate changes (is done in a separate PR) * Reverting changes to dataManagement es client and get data stream api route * Fix data stream name filter when activated from a url parameter * Clean up for better consistency and fixes after elastic#81664 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

… characters (#80835) (#81897) * Add uri decode to es_ui_shared and fix data stream issue with % name * Add a test for data streams tab opened for name with a dollar sign * Import uri decode function from es_ui_shared and fix navigation issues for filters * Add tests for data streams with special characters in name * Revert react router navigate changes (is done in a separate PR) * Reverting changes to dataManagement es client and get data stream api route * Fix data stream name filter when activated from a url parameter * Clean up for better consistency and fixes after #81664 Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

cjcenizal

Tested locally, code LGTM! Had a couple comments and a concern that I think is worth blocking on.

Edit page navigation will not work in some cases for policies with names that contain either dollar sign with other special characters or %25 sequence.

Is it possible to add a test case to define this expected behavior? If it's not possible to add a client integration test (which seems likely, since the problematic behavior only occurs on browser reload if I understand correctly), then I think it's worth extracting the decoding logic in edit_policy.container.tsx into a utility file that can be unit tested, and add the additional test cases there. If a bug is ever reported about this behavior, these tests and accompanying comments will make it crystal clear that the bug is expected. WDYT?

cjcenizal · 2020-10-28T22:29:21Z

x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/constants.ts

@@ -7,6 +7,9 @@
 import { PolicyFromES } from '../../../common/types';

 export const POLICY_NAME = 'my_policy';
+// navigation doesn't work for % with other special chars or sequence %25
+// https://github.com/elastic/kibana/pull/81664
+export const SPECIAL_CHARS_NAME = 'test?#';


This file is only consumed in app.test.ts, and I find it's a bit easier to understand the tests if this value is colocated with the code that consumes it. Can we define it there until it needs to be used in multiple files, at which point we can extract it to a common location like this file?

Also, what do you think of using the full set of reserved characters that the API allows?

export const SPECIAL_CHARS_NAME = 'test?#$+=&@:';

cjcenizal · 2020-10-28T22:35:27Z

...index_lifecycle_management/public/application/sections/edit_policy/edit_policy.container.tsx

@@ -76,12 +76,22 @@ export const EditPolicy: React.FunctionComponent<Props & RouteComponentProps<Rou
    );
  }

+  let decodedPolicyName = policyName;


Can we add a comment here that this code can be removed if we upgrade to react-router 6 and history 5?

yuliacech · 2020-11-02T14:08:23Z

@elasticmachine merge upstream

kibanamachine · 2020-11-02T14:08:25Z

merge conflict between base and head

yuliacech · 2020-11-03T15:17:56Z

Thank you for the review @cjcenizal ! I added more tests to demonstrate known issues with navigation and linked to a new issue that tracks this. Also there is a comment in es_ui_shared.attempToURIDecode that we should deprecate the function after update to react-router v6.
Could you please have another look?

cjcenizal

Thanks for making these changes! Tested locally, code LGTM. Had a couple suggestions to make it easier for people to understand the tests.

cjcenizal · 2020-11-03T17:25:48Z

x-pack/plugins/index_lifecycle_management/__jest__/client_integration/app/app.test.ts

+    test('when loading edit policy page url with double encoding', async () => {
+      await act(async () => {
+        testBed = await setup([
+          encodeURI(`/policies/edit/${encodeURIComponent(SPECIAL_CHARS_NAME)}`),


As a reader of this file, I'd benefit from some explanation about why we test double-encoding. I also noticed that we have three tests for double-encoding. I think we can simplify the code and help the reader understand why we're testing double-encoding by creating a helper function at the top of this file or inside of the helpers file, consuming it everywhere we test double-encoding, and adding a comment inside it. For example:

const doubleEncodePolicyPath = (policyName) => { // The UI double-encodes links to this path to anticipate the `decodeURI` call made by `history.push()`. return encodeURI(`/policies/edit/${encodeURIComponent(policyName)}`); } /* snip */ testBed = await setup([doubleEncodePolicyPath(DOLLAR_SIGN_WITH_OTHER_CHARS_NAME)];

WDYT?

cjcenizal · 2020-11-03T17:35:02Z

x-pack/plugins/index_lifecycle_management/__jest__/client_integration/app/app.test.ts

+      );
+    });
+
+    test('when loading edit policy page url', async () => {


Can we describe the assertion we're making, too? This will help me scan the tests and identify which ones verify things work and which ones verify things don't work. For example, if the prior test description was this:

test('when clicked on policy name in table decodes correctly', async () => {

I'd expect this one to be:

test('when loading edit policy page url decodes incorrectly', async () => {

I agree, it was not very clear which tests were confirming the navigation bug, so I changed the test titles similar to what you suggested.

…ded paths and better description in test names

yuliacech · 2020-11-04T11:27:17Z

@elasticmachine merge upstream

yuliacech · 2020-11-05T14:38:00Z

@elasticmachine merge upstream

yuliacech · 2020-11-05T16:00:48Z

@elasticmachine merge upstream

yuliacech · 2020-11-06T07:48:15Z

@elasticmachine merge upstream

This reverts commit 5a735df

kibanamachine · 2020-11-09T18:44:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ab1c1d9

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`indexLifecycleManagement`	210.6KB	211.0KB	+381.0B

Distributable file count

id	before	after	diff
`default`	42766	42767	+1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`esUiShared`	277.9KB	277.9KB	+12.0B

History

💔 Build #86823 failed a0a4aaf
💔 Build #86763 failed 5a735df
💔 Build #86759 failed 42adcb2
💔 Build #86323 failed cc29641
💔 Build #86153 failed 0baccad

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

* Fix edit policy page navigation * Fix edit policy page navigation * Add links to PR for explanation * Added more tests and linked to a github issue about navigation issues * Fix decoding function for undefined values * Fix type check issues * Renamed dollar sign to percent sign, added a method for (double) encoded paths and better description in test names * Deleted Index Management from required bundles in ILM * Fixed merge conflicts * Revert "Deleted Index Management from required bundles in ILM" This reverts commit 5a735df Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

* master: (39 commits) Fix ilm navigation (elastic#81664) [Lens] Distinct icons for XY and pie chart value labels toolbar (elastic#82927) [data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. (elastic#81509) Index patterns api - load field list on server (elastic#82629) New events resolver (elastic#82170) [App Search] Misc naming tech debt (elastic#82770) load empty_kibana in test to have clean starting point (elastic#82772) Remove data <--> expressions circular dependencies. (elastic#82685) Update 8.0 breaking change template to gather information on how to programmatically detect it. (elastic#82905) Add alerting as codeowners to related documentation folder (elastic#82777) Add captions to user and space grid pages (elastic#82713) add alternate path for x-pack/Cloud test for Lens (elastic#82634) Uses asCurrentUser in getClusterUuid (elastic#82908) [Alerting][Connectors] Add new executor subaction to get 3rd party case fields (elastic#82519) Fix test import objects (elastic#82767) [ML] Add option for anomaly charts for metric detector should plot min, mean or max as appropriate (elastic#81662) Update alert type selection layout to rows instead of grid (elastic#73665) Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. (elastic#82817) Update grunt and related packages (elastic#79327) Allow the repository to search across all namespaces (elastic#82863) ...

* Fix edit policy page navigation * Fix edit policy page navigation * Add links to PR for explanation * Added more tests and linked to a github issue about navigation issues * Fix decoding function for undefined values * Fix type check issues * Renamed dollar sign to percent sign, added a method for (double) encoded paths and better description in test names * Deleted Index Management from required bundles in ILM * Fixed merge conflicts * Revert "Deleted Index Management from required bundles in ILM" This reverts commit 5a735df Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

…kibana into bootstrap-node-details-overlay * 'bootstrap-node-details-overlay' of github.com:phillipb/kibana: (49 commits) [Security Solution] Fix DNS Network table query (elastic#82778) [Workplace Search] Consolidate groups routes (elastic#83015) Adds cloud links to user menu (elastic#82803) [Security Solution][Detections] - follow up cleanup on auto refresh rules (elastic#83023) [App Search] Added the log retention panel to the Settings page (elastic#82982) [Maps] show icon when layer is filtered by time and allow layers to ignore global time range (elastic#83006) [DOCS] Consolidates drilldown pages (elastic#82081) [Maps] add on-prem EMS config (elastic#82525) migrate i18n mixin to KP (elastic#81799) [bundle optimization] fix imports of react-use lib (elastic#82847) [Discover] Add metric on adding filter (elastic#82961) [Lens] Performance refactoring for indexpattern fast lookup and Operation support matrix computation (elastic#82829) skip flaky suite (elastic#82804) Fix SO query for searching across spaces (elastic#83025) renaming built-in alerts to Stack Alerts (elastic#82873) [TSVB] Disable using top_hits in pipeline aggregations (elastic#82278) [Visualizations] Remove kui usage (elastic#82810) [Visualizations] Make the icon buttons labels more descriptive (elastic#82585) [Lens] Do not reset formatting when switching between custom ranges and auto histogram (elastic#82694) Fix ilm navigation (elastic#81664) ...

yuliacech added 4 commits October 26, 2020 16:50

Fix edit policy page navigation

85ae4c3

Fix edit policy page navigation

0577f4a

Add links to PR for explanation

36349b9

yuliacech added Feature:ILM release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.11.0 v8.0.0 labels Oct 27, 2020

Merge branch 'master' into fix_ilm_navigation

2d557aa

yuliacech added a commit to yuliacech/kibana that referenced this pull request Oct 27, 2020

Clean up for better consistency and fixes after

f4fa3f5

elastic#81664

yuliacech marked this pull request as ready for review October 27, 2020 15:29

yuliacech requested a review from a team as a code owner October 27, 2020 15:29

yuliacech requested a review from cjcenizal October 27, 2020 15:29

cjcenizal suggested changes Oct 28, 2020

View reviewed changes

yuliacech and others added 3 commits November 2, 2020 15:11

Merge branch 'master' into fix_ilm_navigation

2798750

Added more tests and linked to a github issue about navigation issues

0e98681

Fix decoding function for undefined values

f4015fc

Fix type check issues

9acc687

cjcenizal approved these changes Nov 3, 2020

View reviewed changes

Renamed dollar sign to percent sign, added a method for (double) enco…

05459f2

…ded paths and better description in test names

kibanamachine and others added 2 commits November 4, 2020 06:27

Merge branch 'master' into fix_ilm_navigation

c51af34

Merge branch 'master' into fix_ilm_navigation

21338cf

Merge branch 'master' into fix_ilm_navigation

0c16d97

Merge branch 'master' into fix_ilm_navigation

0baccad

kibanamachine and others added 5 commits November 6, 2020 02:48

Merge branch 'master' into fix_ilm_navigation

cc29641

Merge branch 'master' into fix_ilm_navigation

42adcb2

Deleted Index Management from required bundles in ILM

5a735df

Fixed merge conflicts

a0a4aaf

Revert "Deleted Index Management from required bundles in ILM"

ab1c1d9

This reverts commit 5a735df

yuliacech merged commit 1de3a02 into elastic:master Nov 10, 2020

yuliacech mentioned this pull request Nov 10, 2020

[7.x] Fix ilm navigation (#81664) #83035

Merged

yuliacech deleted the fix_ilm_navigation branch December 3, 2020 10:52

yuliacech mentioned this pull request Apr 21, 2021

404: Not Found error when trying to delete an Index Lifecycle Policy that contains a period, forward slash, or back slash #88253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ilm navigation #81664

Fix ilm navigation #81664

yuliacech commented Oct 26, 2020 •

edited

Loading

yuliacech commented Oct 27, 2020

elasticmachine commented Oct 27, 2020

cjcenizal left a comment

cjcenizal Oct 28, 2020

cjcenizal Oct 28, 2020

cjcenizal Oct 28, 2020

yuliacech commented Nov 2, 2020

kibanamachine commented Nov 2, 2020

yuliacech commented Nov 3, 2020

cjcenizal left a comment

cjcenizal Nov 3, 2020

cjcenizal Nov 3, 2020

yuliacech Nov 4, 2020

yuliacech commented Nov 4, 2020

yuliacech commented Nov 5, 2020

yuliacech commented Nov 5, 2020

yuliacech commented Nov 6, 2020

kibanamachine commented Nov 9, 2020

Fix ilm navigation #81664

Fix ilm navigation #81664

Conversation

yuliacech commented Oct 26, 2020 • edited Loading

Summary

Example for policy name test?:

Percent sign in the policy name test%

Known issues:

Example percent sign with other characters test%?

Example %25 sequence test%25

yuliacech commented Oct 27, 2020

elasticmachine commented Oct 27, 2020

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Oct 28, 2020

Choose a reason for hiding this comment

cjcenizal Oct 28, 2020

Choose a reason for hiding this comment

cjcenizal Oct 28, 2020

Choose a reason for hiding this comment

yuliacech commented Nov 2, 2020

kibanamachine commented Nov 2, 2020

yuliacech commented Nov 3, 2020

cjcenizal left a comment

Choose a reason for hiding this comment

cjcenizal Nov 3, 2020

Choose a reason for hiding this comment

cjcenizal Nov 3, 2020

Choose a reason for hiding this comment

yuliacech Nov 4, 2020

Choose a reason for hiding this comment

yuliacech commented Nov 4, 2020

yuliacech commented Nov 5, 2020

yuliacech commented Nov 5, 2020

yuliacech commented Nov 6, 2020

kibanamachine commented Nov 9, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History

yuliacech commented Oct 26, 2020 •

edited

Loading

Example for policy name `test?`:

Percent sign in the policy name `test%`

Example percent sign with other characters `test%?`

Example `%25` sequence `test%25`