Skip to content

Commit

Permalink
Review#3: remove unnecessary ts-ignore directives and show a toast wh…
Browse files Browse the repository at this point in the history
…en index patterns are not available.
  • Loading branch information
azasypkin committed Jan 17, 2020
1 parent e783bb2 commit e9cb504
Show file tree
Hide file tree
Showing 15 changed files with 86 additions and 92 deletions.
19 changes: 0 additions & 19 deletions src/core/public/saved_objects/saved_objects_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -448,23 +448,4 @@ describe('SavedObjectsClient', () => {
`);
});
});

it('maintains backwards compatibility by transforming http.fetch errors to be compatible with kfetch errors', () => {
const err = {
response: { ok: false, redirected: false, status: 409, statusText: 'Conflict' },
body: 'response body',
};
http.fetch.mockRejectedValue(err);
return expect(savedObjectsClient.get(doc.type, doc.id)).rejects.toMatchInlineSnapshot(`
Object {
"body": "response body",
"res": Object {
"ok": false,
"redirected": false,
"status": 409,
"statusText": "Conflict",
},
}
`);
});
});
6 changes: 1 addition & 5 deletions src/core/public/saved_objects/saved_objects_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -465,11 +465,7 @@ export class SavedObjectsClient {
* uses `{response: {status: number}}`.
*/
private savedObjectsFetch(path: string, { method, query, body }: HttpFetchOptions) {
return this.http.fetch(path, { method, query, body }).catch(err => {
const kfetchError = Object.assign(err, { res: err.response });
delete kfetchError.response;
return Promise.reject(kfetchError);
});
return this.http.fetch(path, { method, query, body });
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import React, { Component } from 'react';
import {
// @ts-ignore
EuiDescribedFormGroup,
} from '@elastic/eui';
import { EuiDescribedFormGroup } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import { NotificationsSetup } from 'src/core/public';
import { AuthenticatedUser, canUserChangePassword } from '../../../common/model';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/
import React from 'react';
import {
// @ts-ignore
EuiDescribedFormGroup,
EuiFormRow,
EuiText,
} from '@elastic/eui';
import { EuiDescribedFormGroup, EuiFormRow, EuiText } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import { AuthenticatedUser } from '../../../common/model';

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import {
EuiTitle,
EuiText,
EuiSpacer,
// @ts-ignore
EuiDescribedFormGroup,
EuiFormRow,
EuiFieldText,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,7 @@ import React, { useState, Fragment } from 'react';

import 'brace/mode/json';
import 'brace/theme/github';
import {
// @ts-ignore
EuiCodeEditor,
EuiFormRow,
EuiButton,
EuiSpacer,
EuiLink,
EuiText,
} from '@elastic/eui';
import { EuiCodeEditor, EuiFormRow, EuiButton, EuiSpacer, EuiLink, EuiText } from '@elastic/eui';
import { FormattedMessage } from '@kbn/i18n/react';
import { i18n } from '@kbn/i18n';
import { DocumentationLinksService } from '../../documentation_links';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ export class CollapsiblePanel extends Component<Props, State> {

public getTitle = () => {
return (
// @ts-ignore
<EuiFlexGroup alignItems={'baseline'} gutterSize="s" responsive={false}>
<EuiFlexItem grow={false}>
<EuiTitle>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ function getProps({
spacesEnabled = true,
}: {
action: 'edit' | 'clone';
role: Role;
role?: Role;
canManageSpaces?: boolean;
spacesEnabled?: boolean;
}) {
Expand Down Expand Up @@ -167,7 +167,7 @@ function getProps({

return {
action,
roleName: role.name,
roleName: role?.name,
license,
http,
indexPatterns,
Expand Down Expand Up @@ -238,18 +238,7 @@ describe('<EditRolePage />', () => {
});

it('can render when creating a new role', async () => {
const wrapper = mountWithIntl(
<EditRolePage
{...getProps({
action: 'edit',
role: {
metadata: {},
elasticsearch: { cluster: [], indices: [], run_as: [] },
kibana: [],
} as any,
})}
/>
);
const wrapper = mountWithIntl(<EditRolePage {...getProps({ action: 'edit' })} />);

await act(async () => {
await nextTick();
Expand Down Expand Up @@ -411,17 +400,7 @@ describe('<EditRolePage />', () => {

it('can render when creating a new role', async () => {
const wrapper = mountWithIntl(
<EditRolePage
{...getProps({
action: 'edit',
spacesEnabled: false,
role: {
metadata: {},
elasticsearch: { cluster: [], indices: [], run_as: [] },
kibana: [],
} as any,
})}
/>
<EditRolePage {...getProps({ action: 'edit', spacesEnabled: false })} />
);

await act(async () => {
Expand Down Expand Up @@ -527,4 +506,47 @@ describe('<EditRolePage />', () => {
expectReadOnlyFormButtons(wrapper);
});
});

it('can render if features are not available', async () => {
const { http } = coreMock.createStart();
http.get.mockImplementation(async path => {
if (path === '/api/features') {
const error = { response: { status: 404 } };
throw error;
}

if (path === '/api/spaces/space') {
return buildSpaces();
}
});

const wrapper = mountWithIntl(<EditRolePage {...{ ...getProps({ action: 'edit' }), http }} />);

await act(async () => {
await nextTick();
wrapper.update();
});

expect(wrapper.find(SpaceAwarePrivilegeSection)).toHaveLength(1);
expect(wrapper.find('[data-test-subj="userCannotManageSpacesCallout"]')).toHaveLength(0);
expectSaveFormButtons(wrapper);
});

it('can render if index patterns are not available', async () => {
const indexPatterns = dataPluginMock.createStartContract().indexPatterns;
indexPatterns.getTitles = jest.fn().mockRejectedValue({ response: { status: 403 } });

const wrapper = mountWithIntl(
<EditRolePage {...{ ...getProps({ action: 'edit' }), indexPatterns }} />
);

await act(async () => {
await nextTick();
wrapper.update();
});

expect(wrapper.find(SpaceAwarePrivilegeSection)).toHaveLength(1);
expect(wrapper.find('[data-test-subj="userCannotManageSpacesCallout"]')).toHaveLength(0);
expectSaveFormButtons(wrapper);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -96,12 +96,30 @@ function useRunAsUsers(

function useIndexPatternsTitles(
indexPatterns: IndexPatternsContract,
fatalErrors: FatalErrorsSetup
fatalErrors: FatalErrorsSetup,
notifications: NotificationsStart
) {
const [indexPatternsTitles, setIndexPatternsTitles] = useState<string[] | null>(null);
useEffect(() => {
indexPatterns.getTitles().then(setIndexPatternsTitles, err => fatalErrors.add(err));
}, [fatalErrors, indexPatterns]);
indexPatterns
.getTitles()
.catch((err: IHttpFetchError) => {
// If user doesn't have access to the index patterns they still should be able to create new
// or edit existing role.
if (err.response?.status === 403) {
notifications.toasts.addDanger({
title: i18n.translate('xpack.security.management.roles.noIndexPatternsPermission', {
defaultMessage: 'You need permission to access the list of available index patterns.',
}),
});
return [];
}

fatalErrors.add(err);
throw err;
})
.then(setIndexPatternsTitles);
}, [fatalErrors, indexPatterns, notifications]);

return indexPatternsTitles;
}
Expand Down Expand Up @@ -213,18 +231,25 @@ function useSpaces(http: HttpStart, fatalErrors: FatalErrorsSetup, spacesEnabled
function useFeatures(http: HttpStart, fatalErrors: FatalErrorsSetup) {
const [features, setFeatures] = useState<Feature[] | null>(null);
useEffect(() => {
http.get('/api/features').then(
fetchedFeatures => setFeatures(fetchedFeatures),
(err: IHttpFetchError) => {
// TODO: This check can be removed once all of these `resolve` entries are moved out of Angular and into the React app.
http
.get('/api/features')
.catch((err: IHttpFetchError) => {
// Currently, the `/api/features` endpoint effectively requires the "Global All" kibana privilege (e.g., what
// the `kibana_user` grants), because it returns information about all registered features (#35841). It's
// possible that a user with `manage_security` will attempt to visit the role management page without the
// correct Kibana privileges. If that's the case, then they receive a partial view of the role, and the UI does
// not allow them to make changes to that role's kibana privileges. When this user visits the edit role page,
// this API endpoint will throw a 404, which causes view to fail completely. So we instead attempt to detect the
// 404 here, and respond in a way that still allows the UI to render itself.
const unauthorizedForFeatures = err.response?.status === 404;
if (unauthorizedForFeatures) {
return [];
}

fatalErrors.add(err);
}
);
throw err;
})
.then(setFeatures);
}, [http, fatalErrors]);

return features;
Expand Down Expand Up @@ -256,7 +281,7 @@ export const EditRolePage: FunctionComponent<Props> = ({

const [formError, setFormError] = useState<RoleValidationResult | null>(null);
const runAsUsers = useRunAsUsers(userAPIClient, fatalErrors);
const indexPatternsTitles = useIndexPatternsTitles(indexPatterns, fatalErrors);
const indexPatternsTitles = useIndexPatternsTitles(indexPatterns, fatalErrors, notifications);
const privileges = usePrivileges(privilegesAPIClient, fatalErrors);
const spaces = useSpaces(http, fatalErrors, spacesEnabled);
const features = useFeatures(http, fatalErrors);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import {
EuiButton,
EuiComboBox,
// @ts-ignore
EuiDescribedFormGroup,
EuiFormRow,
EuiHorizontalRule,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ export class IndexPrivilegeForm extends Component<Props, State> {
{!isReadOnlyRole && (
<EuiFlexItem>
{
// @ts-ignore missing "compressed" prop definition
<EuiSwitch
data-test-subj={`restrictFieldsQuery${this.props.formIndex}`}
label={
Expand Down Expand Up @@ -256,12 +255,10 @@ export class IndexPrivilegeForm extends Component<Props, State> {
}

return (
// @ts-ignore
<EuiFlexGroup direction="column">
{!this.props.isReadOnlyRole && (
<EuiFlexItem>
{
// @ts-ignore missing "compressed" proptype
<EuiSwitch
data-test-subj={`restrictDocumentsQuery${this.props.formIndex}`}
label={
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,7 @@ export class FeatureTable extends Component<Props, {}> {
const availablePrivileges = Object.values(rankedFeaturePrivileges)[0];

return (
// @ts-ignore missing responsive from typedef
<EuiInMemoryTable
// @ts-ignore missing rowProps from typedef
responsive={false}
columns={this.getColumns(availablePrivileges)}
items={items}
Expand Down Expand Up @@ -237,9 +235,7 @@ export class FeatureTable extends Component<Props, {}> {
});

return (
// @ts-ignore missing name from typedef
<EuiButtonGroup
// @ts-ignore missing rowProps from typedef
name={`featurePrivilege_${featureId}`}
options={options}
idSelected={`${featureId}_${actualPrivilegeValue || NO_PRIVILEGE_VALUE}`}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import {
EuiComboBox,
// @ts-ignore
EuiDescribedFormGroup,
EuiFormRow,
EuiSuperSelect,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,9 @@ export class PrivilegeMatrix extends Component<Props, State> {
];

return (
// @ts-ignore missing rowProps from typedef
<EuiInMemoryTable
columns={columns}
items={rows}
// @ts-ignore missing rowProps from typedef
rowProps={(item: TableRow) => {
return {
className: item.feature.isBase ? 'secPrivilegeMatrix__row--isBasePrivilege' : '',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ export class SpacesPopoverList extends Component<Props, State> {
return (
<div key="manageSpacesSearchField" className="spcMenu__searchFieldWrapper">
{
// @ts-ignore onSearch isn't defined on the type
<EuiFieldSearch
placeholder={intl.formatMessage({
id: 'xpack.security.management.editRole.spacesPopoverList.findSpacePlaceholder',
Expand Down

0 comments on commit e9cb504

Please sign in to comment.