Skip to content
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

Fix ilm navigation #81664

Merged
merged 19 commits into from Nov 10, 2020
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 43 additions & 6 deletions src/plugins/es_ui_shared/public/url/attempt_to_uri_decode.test.ts
Expand Up @@ -19,14 +19,51 @@

import { attemptToURIDecode } from './attempt_to_uri_decode';

// this function doesn't work for % with other special chars or sequence %25
// known issue https://github.com/elastic/kibana/issues/82440
test('decodes an encoded string', () => {
const encodedString = 'test%3F';
expect(attemptToURIDecode(encodedString)).toBe('test?');
const originalName = 'test;,/?:@&=+$#';
const encodedName = encodeURIComponent(originalName);
// react router v5 automatically decodes route match params
const reactRouterDecoded = decodeURI(encodedName);

expect(attemptToURIDecode(encodedName)).toBe(originalName);
expect(attemptToURIDecode(reactRouterDecoded)).toBe(originalName);
});

// react router partially decodes %25 sequence to % in match params
// https://github.com/elastic/kibana/pull/81664
test('ignores the error if a string is already decoded', () => {
const decodedString = 'test%';
expect(attemptToURIDecode(decodedString)).toBe(decodedString);
const originalName = 'test%';

const encodedName = encodeURIComponent(originalName);
// react router v5 automatically decodes route match params
const reactRouterDecoded = decodeURI(encodedName);

expect(attemptToURIDecode(encodedName)).toBe(originalName);
expect(attemptToURIDecode(reactRouterDecoded)).toBe(originalName);
});

test('returns wrong decoded value for %25 sequence', () => {
const originalName = 'test%25';

const encodedName = encodeURIComponent(originalName);
// react router v5 automatically decodes route match params
const reactRouterDecoded = decodeURI(encodedName);

expect(attemptToURIDecode(encodedName)).toBe(originalName);
expect(attemptToURIDecode(reactRouterDecoded)).not.toBe(originalName);
});

test('returns wrong decoded value for % with other escaped characters', () => {
const originalName = 'test%?#';

const encodedName = encodeURIComponent(originalName);
// react router v5 automatically decodes route match params
const reactRouterDecoded = decodeURI(encodedName);

expect(attemptToURIDecode(encodedName)).toBe(originalName);
expect(attemptToURIDecode(reactRouterDecoded)).not.toBe(originalName);
});

test("doesn't convert undefined to a string", () => {
expect(attemptToURIDecode(undefined)).toBeUndefined();
});
8 changes: 5 additions & 3 deletions src/plugins/es_ui_shared/public/url/attempt_to_uri_decode.ts
Expand Up @@ -19,12 +19,14 @@

/*
* Use this function with any match params coming from react router to safely decode values.
* https://github.com/elastic/kibana/pull/81664
* After an update to react router v6, this functions should be deprecated.
* Known issue for navigation with special characters in paths
* https://github.com/elastic/kibana/issues/82440
*/
export const attemptToURIDecode = (value: string) => {
export const attemptToURIDecode = (value?: string): string | undefined => {
let result = value;
try {
result = decodeURIComponent(value);
result = value ? decodeURIComponent(value) : value;
} catch (e) {
// do nothing
}
Expand Down
@@ -0,0 +1,55 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { act } from 'react-dom/test-utils';
import { registerTestBed, TestBed, TestBedConfig } from '../../../../../test_utils';
import { App } from '../../../public/application/app';
import { TestSubjects } from '../helpers';

const getTestBedConfig = (initialEntries: string[]): TestBedConfig => ({
memoryRouter: {
initialEntries,
},
defaultProps: {
getUrlForApp: () => {},
navigateToApp: () => {},
},
});

const initTestBed = (initialEntries: string[]) =>
registerTestBed(App, getTestBedConfig(initialEntries))();

export interface AppTestBed extends TestBed<TestSubjects> {
actions: {
clickPolicyNameLink: () => void;
clickCreatePolicyButton: () => void;
};
}

export const setup = async (initialEntries: string[]): Promise<AppTestBed> => {
const testBed = await initTestBed(initialEntries);

const clickPolicyNameLink = async () => {
const { component, find } = testBed;
await act(async () => {
find('policyTablePolicyNameLink').simulate('click', { button: 0 });
});
component.update();
};

const clickCreatePolicyButton = async () => {
const { component, find } = testBed;
await act(async () => {
find('createPolicyButton').simulate('click', { button: 0 });
});
component.update();
};

return {
...testBed,
actions: { clickPolicyNameLink, clickCreatePolicyButton },
};
};
@@ -0,0 +1,251 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { AppTestBed, setup } from './app.helpers';
import { setupEnvironment } from '../helpers/setup_environment';
import { getDefaultHotPhasePolicy, POLICY_NAME } from '../edit_policy/constants';
import { act } from 'react-dom/test-utils';

const SPECIAL_CHARS_NAME = 'test?#$+=&@:';
const DOLLAR_SIGN_NAME = 'test%';
// navigation doesn't work for % with other special chars or sequence %25
// known issue https://github.com/elastic/kibana/issues/82440
const DOLLAR_SIGN_WITH_OTHER_CHARS_NAME = 'test%#';
const DOLLAR_SIGN_25_SEQUENCE = 'test%25';

window.scrollTo = jest.fn();

describe('<App />', () => {
let testBed: AppTestBed;
const { server, httpRequestsMockHelpers } = setupEnvironment();
afterAll(() => {
server.restore();
});

describe('new policy creation', () => {
test('when there are no policies', async () => {
httpRequestsMockHelpers.setLoadPolicies([]);
await act(async () => {
testBed = await setup(['/']);
});

const { component, actions } = testBed;
component.update();

await actions.clickCreatePolicyButton();
component.update();

expect(testBed.find('policyTitle').text()).toBe(`Create an index lifecycle policy`);
expect(testBed.find('policyNameField').props().value).toBe('');
});

test('when there are policies', async () => {
httpRequestsMockHelpers.setLoadPolicies([getDefaultHotPhasePolicy(POLICY_NAME)]);
await act(async () => {
testBed = await setup(['/']);
});

const { component, actions } = testBed;
component.update();

await actions.clickCreatePolicyButton();
component.update();

expect(testBed.find('policyTitle').text()).toBe(`Create an index lifecycle policy`);
expect(testBed.find('policyNameField').props().value).toBe('');
});
});

describe('navigation with special characters', () => {
beforeAll(async () => {
httpRequestsMockHelpers.setLoadPolicies([getDefaultHotPhasePolicy(SPECIAL_CHARS_NAME)]);
});

test('when clicked on policy name in table', async () => {
await act(async () => {
testBed = await setup(['/']);
});

const { component, actions } = testBed;
component.update();

await actions.clickPolicyNameLink();
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${SPECIAL_CHARS_NAME}`
);
});

test('when loading edit policy page url', async () => {
await act(async () => {
testBed = await setup([`/policies/edit/${encodeURIComponent(SPECIAL_CHARS_NAME)}`]);
});

const { component } = testBed;
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${SPECIAL_CHARS_NAME}`
);
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

]);
});

const { component } = testBed;
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${SPECIAL_CHARS_NAME}`
);
});
});

describe('navigation with dollar sign', () => {
beforeAll(async () => {
httpRequestsMockHelpers.setLoadPolicies([getDefaultHotPhasePolicy(DOLLAR_SIGN_NAME)]);
});

test('when loading edit policy page url', async () => {
await act(async () => {
testBed = await setup([`/policies/edit/${encodeURIComponent(DOLLAR_SIGN_NAME)}`]);
});

const { component } = testBed;
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_NAME}`
);
});

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

const { component } = testBed;
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_NAME}`
);
});
});

describe('navigation with dollar sign with other special characters', () => {
beforeAll(async () => {
httpRequestsMockHelpers.setLoadPolicies([
getDefaultHotPhasePolicy(DOLLAR_SIGN_WITH_OTHER_CHARS_NAME),
]);
});

test('when clicked on policy name in table', async () => {
await act(async () => {
testBed = await setup(['/']);
});

const { component, actions } = testBed;
component.update();

await actions.clickPolicyNameLink();
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_WITH_OTHER_CHARS_NAME}`
);
});

test('when loading edit policy page url', async () => {
await act(async () => {
testBed = await setup([
`/policies/edit/${encodeURIComponent(DOLLAR_SIGN_WITH_OTHER_CHARS_NAME)}`,
]);
});

const { component } = testBed;
component.update();

// known issue https://github.com/elastic/kibana/issues/82440
expect(testBed.find('policyTitle').text()).not.toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_WITH_OTHER_CHARS_NAME}`
);
});

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

const { component } = testBed;
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_WITH_OTHER_CHARS_NAME}`
);
});
});

describe('navigation with %25 sequence', () => {
beforeAll(async () => {
httpRequestsMockHelpers.setLoadPolicies([getDefaultHotPhasePolicy(DOLLAR_SIGN_25_SEQUENCE)]);
});

test('when clicked on policy name in table', async () => {
await act(async () => {
testBed = await setup(['/']);
});

const { component, actions } = testBed;
component.update();

await actions.clickPolicyNameLink();
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_25_SEQUENCE}`
);
});

test('when loading edit policy page url', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 () => {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

await act(async () => {
testBed = await setup([`/policies/edit/${encodeURIComponent(DOLLAR_SIGN_25_SEQUENCE)}`]);
});

const { component } = testBed;
component.update();

// known issue https://github.com/elastic/kibana/issues/82440
expect(testBed.find('policyTitle').text()).not.toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_25_SEQUENCE}`
);
});

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

const { component } = testBed;
component.update();

expect(testBed.find('policyTitle').text()).toBe(
`Edit index lifecycle policy ${DOLLAR_SIGN_25_SEQUENCE}`
);
});
});
});