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

[Upgrade Assistant] Add permissions check to logs step #112420

Merged
merged 16 commits into from
Sep 23, 2021
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { useRequest } from '../../../public/request';

import { Privileges, Error as CustomError } from '../types';

interface Authorization {
export interface Authorization {
isLoading: boolean;
apiError: CustomError | null;
privileges: Privileges;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ export {
AuthorizationProvider,
AuthorizationContext,
useAuthorizationContext,
Authorization,
} from './authorization_provider';

export { WithPrivileges } from './with_privileges';
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { convertPrivilegesToArray } from './with_privileges';

describe('convertPrivilegesToArray', () => {
test('extracts section and privilege', () => {
expect(convertPrivilegesToArray('index.index_name')).toEqual([['index', 'index_name']]);
expect(convertPrivilegesToArray(['index.index_name', 'cluster.management'])).toEqual([
['index', 'index_name'],
['cluster', 'management'],
]);
expect(convertPrivilegesToArray('index.index_name.with-many.dots')).toEqual([
['index', 'index_name.with-many.dots'],
]);
});

test('throws when it cannot extract section and privilege', () => {
expect(() => {
convertPrivilegesToArray('bad_privilege_string');
}).toThrow('Required privilege must have the format "section.privilege"');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,14 @@ import { MissingPrivileges } from '../types';

import { useAuthorizationContext } from './authorization_provider';

type Privileges = string | string[];
interface Props {
/**
* Each required privilege must have the format "section.privilege".
* To indicate that *all* privileges from a section are required, we can use the asterix
* e.g. "index.*"
*/
privileges: string | string[];
privileges: Privileges;
children: (childrenProps: {
isLoading: boolean;
hasPrivileges: boolean;
Expand All @@ -26,24 +27,30 @@ interface Props {

type Privilege = [string, string];

const toArray = (value: string | string[]): string[] =>
const toArray = (value: Privileges): string[] =>
Array.isArray(value) ? (value as string[]) : ([value] as string[]);

export const WithPrivileges = ({ privileges: requiredPrivileges, children }: Props) => {
const { isLoading, privileges } = useAuthorizationContext();

const privilegesToArray: Privilege[] = toArray(requiredPrivileges).map((p) => {
const [section, privilege] = p.split('.');
if (!privilege) {
// Oh! we forgot to use the dot "." notation.
export const convertPrivilegesToArray = (privileges: Privileges): Privilege[] => {
Copy link
Member Author

@sabarasaba sabarasaba Sep 17, 2021

Choose a reason for hiding this comment

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

I patched this block of code to also accept privileges that might contain dots in its name. I extracted this part into its own function and wrote a few tests for it, I decided not to write tests for the HOC itself because it seemed a bit outside the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!!

return toArray(privileges).map((p) => {
// Since an privilege can contain a dot in its name:
// * `section` needs to be extracted from the beginning of the string until the first dot
// * `privilege` should be everything after the dot
const indexOfFirstPeriod = p.indexOf('.');
if (indexOfFirstPeriod === -1) {
throw new Error('Required privilege must have the format "section.privilege"');
}
return [section, privilege];

return [p.slice(0, indexOfFirstPeriod), p.slice(indexOfFirstPeriod + 1)];
});
};

export const WithPrivileges = ({ privileges: requiredPrivileges, children }: Props) => {
const { isLoading, privileges } = useAuthorizationContext();
const privilegesArray = convertPrivilegesToArray(requiredPrivileges);

const hasPrivileges = isLoading
? false
: privilegesToArray.every((privilege) => {
: privilegesArray.every((privilege) => {
const [section, requiredPrivilege] = privilege;
if (!privileges.missingPrivileges[section]) {
// if the section does not exist in our missingPriviledges, everything is OK
Expand All @@ -61,7 +68,7 @@ export const WithPrivileges = ({ privileges: requiredPrivileges, children }: Pro
return !privileges.missingPrivileges[section]!.includes(requiredPrivilege);
});

const privilegesMissing = privilegesToArray.reduce((acc, [section, privilege]) => {
const privilegesMissing = privilegesArray.reduce((acc, [section, privilege]) => {
if (privilege === '*') {
acc[section] = privileges.missingPrivileges[section] || [];
} else if (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export {
SectionError,
PageError,
useAuthorizationContext,
Authorization,
} from './components';

export { Privileges, MissingPrivileges, Error } from './types';
2 changes: 1 addition & 1 deletion src/plugins/es_ui_shared/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
* Side Public License, v 1.
*/

export { Privileges, MissingPrivileges } from '../__packages_do_not_import__/authorization';
export { Privileges, MissingPrivileges } from '../__packages_do_not_import__/authorization/types';
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to change these types to be exported directly from the types file, otherwise it doesn't tree-shake the rest of the things exported from the authorization file and it causes errors if you try to use it server side (certain dependencies needed by the components exported require window object to exist).

1 change: 1 addition & 0 deletions src/plugins/es_ui_shared/public/authorization/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,5 @@ export {
PageError,
useAuthorizationContext,
WithPrivileges,
Authorization,
} from '../../__packages_do_not_import__/authorization';
1 change: 1 addition & 0 deletions src/plugins/es_ui_shared/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export {
PageError,
Error,
useAuthorizationContext,
Authorization,
} from './authorization';

export { Forms, ace, GlobalFlyout, XJson };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import axiosXhrAdapter from 'axios/lib/adapters/xhr';

import { HttpSetup } from 'src/core/public';

import { AuthorizationContext, Authorization, Privileges } from '../../../public/shared_imports';
import { AppContextProvider } from '../../../public/application/app_context';
import { apiService } from '../../../public/application/lib/api';
import { breadcrumbService } from '../../../public/application/lib/breadcrumbs';
Expand All @@ -25,20 +26,31 @@ const { GlobalFlyoutProvider } = GlobalFlyout;

const mockHttpClient = axios.create({ adapter: axiosXhrAdapter });

const createAuthorizationContextValue = (privileges: Privileges) => {
return {
isLoading: false,
privileges: privileges ?? { hasAllPrivileges: false, missingPrivileges: {} },
} as Authorization;
};

export const WithAppDependencies =
(Comp: any, overrides: Record<string, unknown> = {}) =>
(Comp: any, { privileges, ...overrides }: Record<string, unknown> = {}) =>
(props: Record<string, unknown>) => {
apiService.setup(mockHttpClient as unknown as HttpSetup);
breadcrumbService.setup(() => '');

const appContextMock = getAppContextMock() as unknown as AppDependencies;

return (
<AppContextProvider value={merge(appContextMock, overrides)}>
<GlobalFlyoutProvider>
<Comp {...props} />
</GlobalFlyoutProvider>
</AppContextProvider>
<AuthorizationContext.Provider
value={createAuthorizationContextValue(privileges as Privileges)}
>
<AppContextProvider value={merge(appContextMock, overrides)}>
<GlobalFlyoutProvider>
<Comp {...props} />
</GlobalFlyoutProvider>
</AppContextProvider>
</AuthorizationContext.Provider>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,13 @@ jest.mock('../../../../public/application/lib/logs_checkpoint', () => {
});

import { DeprecationLoggingStatus } from '../../../../common/types';
import { DEPRECATION_LOGS_SOURCE_ID } from '../../../../common/constants';
import { OverviewTestBed, setupOverviewPage } from '../overview.helpers';
import { setupEnvironment, advanceTime } from '../../helpers';
import { DEPRECATION_LOGS_COUNT_POLL_INTERVAL_MS } from '../../../../common/constants';
import {
DEPRECATION_LOGS_INDEX,
DEPRECATION_LOGS_SOURCE_ID,
DEPRECATION_LOGS_COUNT_POLL_INTERVAL_MS,
} from '../../../../common/constants';

const getLoggingResponse = (toggle: boolean): DeprecationLoggingStatus => ({
isDeprecationLogIndexingEnabled: toggle,
Expand Down Expand Up @@ -389,4 +392,39 @@ describe('Overview - Fix deprecation logs step', () => {
expect(exists('apiCompatibilityNoteTitle')).toBe(true);
});
});

describe('Privileges check', () => {
test(`permissions warning callout is hidden if user has the right privileges`, async () => {
const { exists } = testBed;

// Index privileges warning callout should not be shown
expect(exists('noIndexPermissionsCallout')).toBe(false);
// Analyze logs and Resolve logs sections should be shown
expect(exists('externalLinksTitle')).toBe(true);
expect(exists('deprecationsCountTitle')).toBe(true);
});

test(`doesn't show analyze and resolve logs if it doesn't have the right privileges`, async () => {
await act(async () => {
testBed = await setupOverviewPage({
privileges: {
hasAllPrivileges: false,
missingPrivileges: {
index: [DEPRECATION_LOGS_INDEX],
},
},
});
});

const { exists, component } = testBed;

component.update();

// No index privileges warning callout should be shown
expect(exists('noIndexPermissionsCallout')).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any tests that assert this isn't shown when the user has the required privileges?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice call, I've just added one that checks for that also

// Analyze logs and Resolve logs sections should be hidden
expect(exists('externalLinksTitle')).toBe(false);
expect(exists('deprecationsCountTitle')).toBe(false);
});
});
});
21 changes: 12 additions & 9 deletions x-pack/plugins/upgrade_assistant/public/application/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@ import { Router, Switch, Route, Redirect } from 'react-router-dom';
import { ScopedHistory } from 'src/core/public';

import { RedirectAppLinks } from '../../../../../src/plugins/kibana_react/public';
import { APP_WRAPPER_CLASS, GlobalFlyout } from '../shared_imports';
import { APP_WRAPPER_CLASS, GlobalFlyout, AuthorizationProvider } from '../shared_imports';
import { AppDependencies } from '../types';
import { API_BASE_PATH } from '../../common/constants';
import { AppContextProvider, useAppContext } from './app_context';
import { EsDeprecations, ComingSoonPrompt, KibanaDeprecations, Overview } from './components';

Expand Down Expand Up @@ -46,18 +47,20 @@ export const AppWithRouter = ({ history }: { history: ScopedHistory }) => {
export const RootComponent = (dependencies: AppDependencies) => {
const {
history,
core: { i18n, application },
core: { i18n, application, http },
} = dependencies.services;

return (
<RedirectAppLinks application={application} className={APP_WRAPPER_CLASS}>
<i18n.Context>
<AppContextProvider value={dependencies}>
<GlobalFlyoutProvider>
<AppWithRouter history={history} />
</GlobalFlyoutProvider>
</AppContextProvider>
</i18n.Context>
<AuthorizationProvider httpClient={http} privilegesEndpoint={`${API_BASE_PATH}/privileges`}>
<i18n.Context>
<AppContextProvider value={dependencies}>
<GlobalFlyoutProvider>
<AppWithRouter history={history} />
</GlobalFlyoutProvider>
</AppContextProvider>
</i18n.Context>
</AuthorizationProvider>
</RedirectAppLinks>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ const i18nTexts = {
/>
),
calloutBody: i18n.translate('xpack.upgradeAssistant.overview.verifyChanges.calloutBody', {
defaultMessage:
'Reset the counter after making changes and continue monitoring to verify that you are no longer using deprecated APIs.',
defaultMessage: `After making changes, reset the counter and continue monitoring to verify you're no longer using deprecated features.`,
}),
loadingError: i18n.translate('xpack.upgradeAssistant.overview.verifyChanges.loadingError', {
defaultMessage: 'An error occurred while retrieving the count of deprecation logs',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React, { FunctionComponent, useState, useEffect } from 'react';

import { i18n } from '@kbn/i18n';
import { FormattedMessage } from '@kbn/i18n/react';
import { EuiText, EuiSpacer, EuiPanel, EuiLink, EuiCallOut } from '@elastic/eui';
import { EuiText, EuiSpacer, EuiPanel, EuiLink, EuiCallOut, EuiCode } from '@elastic/eui';
import type { EuiStepProps } from '@elastic/eui/src/components/steps/step';

import { useAppContext } from '../../../app_context';
Expand All @@ -19,6 +18,8 @@ import { useDeprecationLogging } from './use_deprecation_logging';
import { DeprecationLoggingToggle } from './deprecation_logging_toggle';
import { loadLogsCheckpoint, saveLogsCheckpoint } from '../../../lib/logs_checkpoint';
import type { OverviewStepProps } from '../../types';
import { DEPRECATION_LOGS_INDEX } from '../../../../../common/constants';
import { WithPrivileges, MissingPrivileges } from '../../../../shared_imports';

const i18nTexts = {
identifyStepTitle: i18n.translate('xpack.upgradeAssistant.overview.identifyStepTitle', {
Expand Down Expand Up @@ -71,13 +72,40 @@ const i18nTexts = {
'Go to your logs directory to view the deprecation logs or enable log collecting to see them in the UI.',
}
),
deniedPrivilegeTitle: i18n.translate(
'xpack.upgradeAssistant.overview.deprecationLogs.deniedPrivilegeTitle',
{
defaultMessage: `You're missing index privileges`,
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
}
),
deniedPrivilegeDescription: (privilegesMissing: MissingPrivileges) => (
// NOTE: hardcoding the missing privilege because the WithPrivileges HOC
sabarasaba marked this conversation as resolved.
Show resolved Hide resolved
// doesnt provide a way to retrieve which specific privileges an index
// is missing.
<FormattedMessage
id="xpack.upgradeAssistant.overview.deprecationLogs.deniedPrivilegeDescription"
defaultMessage="To analyze deprecation logs, you need the read index {privilegesCount, plural, one {privilege} other {privileges}} for: {missingPrivileges}"
values={{
missingPrivileges: (
<EuiCode transparentBackground={true}>{privilegesMissing?.index?.join(', ')}</EuiCode>
),
privilegesCount: privilegesMissing?.index?.length,
}}
/>
),
};

interface Props {
setIsComplete: OverviewStepProps['setIsComplete'];
hasPrivileges: boolean;
privilegesMissing: MissingPrivileges;
}

const FixLogsStep: FunctionComponent<Props> = ({ setIsComplete }) => {
const FixLogsStep: FunctionComponent<Props> = ({
setIsComplete,
hasPrivileges,
privilegesMissing,
}) => {
const state = useDeprecationLogging();
const {
services: {
Expand Down Expand Up @@ -123,7 +151,21 @@ const FixLogsStep: FunctionComponent<Props> = ({ setIsComplete }) => {
</>
)}

{state.isDeprecationLogIndexingEnabled && (
{!hasPrivileges && state.isDeprecationLogIndexingEnabled && (
<>
<EuiSpacer size="m" />
<EuiCallOut
iconType="help"
color="warning"
title={i18nTexts.deniedPrivilegeTitle}
data-test-subj="noIndexPermissionsCallout"
>
<p>{i18nTexts.deniedPrivilegeDescription(privilegesMissing)}</p>
</EuiCallOut>
</>
)}

{hasPrivileges && state.isDeprecationLogIndexingEnabled && (
<>
<EuiSpacer size="xl" />
<EuiText data-test-subj="externalLinksTitle">
Expand Down Expand Up @@ -168,6 +210,16 @@ export const getFixLogsStep = ({ isComplete, setIsComplete }: OverviewStepProps)
status,
title: i18nTexts.identifyStepTitle,
'data-test-subj': `fixLogsStep-${status}`,
children: <FixLogsStep setIsComplete={setIsComplete} />,
children: (
<WithPrivileges privileges={`index.${DEPRECATION_LOGS_INDEX}`}>
{({ hasPrivileges, privilegesMissing, isLoading }) => (
<FixLogsStep
setIsComplete={setIsComplete}
hasPrivileges={!isLoading && hasPrivileges}
privilegesMissing={privilegesMissing}
/>
)}
</WithPrivileges>
),
};
};
Loading