maxmustermann/kibana
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

[App Search] Log retention role fix (#95227) (#95356)

Browse source 
* LogRetentionCallout - add ability check to API call

- on non-admin users, we're otherwise getting a forbidden error

- remove now-unnecessary canManageLogSettings wrapping check around the link CTA, since the entire callout is now essentially gated behind the check

* LogRententionTooltip - add ability check to API call

- on non-admin users, we're otherwise getting a forbidden error

- tests now require a ...values spread for myRole

* [MISC] Refactor previous isLogRetentionUpdating check in favor of a Kea breakpoint

- both dedupe calls for the commented use case, but breakpoint feels simpler & more Kea-y

* PR feedback: Increase breakpoint speed

Co-authored-by: Constance <constancecchen@users.noreply.github.com>
This commit is contained in:
Kibana Machine 2021-03-24 17:50:55 -04:00 committed by GitHub
parent a813ce59f7
commit 86a17a0b91
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
6 changed files with 71 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
x-pack/plugins/enterprise_search/public/applications/app_search/components/log_retention/components/log_retention_callout.test.tsx
View file

@ -63,15 +63,6 @@ describe('LogRetentionCallout', () => {
expect(wrapper.find('.euiCallOutHeader__title').text()).toEqual('API Logs have been disabled.');
});
it('does not render a settings link if the user cannot manage settings', () => {
setMockValues({ myRole: { canManageLogSettings: false }, logRetention: { api: DISABLED } });
const wrapper = mountWithIntl(<LogRetentionCallout type={LogRetentionOptions.API} />);
expect(wrapper.find(EuiCallOut)).toHaveLength(1);
expect(wrapper.find(EuiLink)).toHaveLength(0);
expect(wrapper.find('p')).toHaveLength(0);
});
it('does not render if log retention is enabled', () => {
setMockValues({ ...values, logRetention: { api: { enabled: true } } });
const wrapper = shallow(<LogRetentionCallout type={LogRetentionOptions.API} />);
@ -100,5 +91,12 @@ describe('LogRetentionCallout', () => {
expect(actions.fetchLogRetention).not.toHaveBeenCalled();
});
it('does not fetch log retention data if the user does not have access to log settings', () => {
setMockValues({ ...values, logRetention: null, myRole: { canManageLogSettings: false } });
shallow(<LogRetentionCallout type={LogRetentionOptions.API} />);
expect(actions.fetchLogRetention).not.toHaveBeenCalled();
});
});
});

36
x-pack/plugins/enterprise_search/public/applications/app_search/components/log_retention/components/log_retention_callout.tsx
View file

@ -40,7 +40,7 @@ export const LogRetentionCallout: React.FC<Props> = ({ type }) => {
const hasLogRetention = logRetention !== null;
useEffect(() => {
if (!hasLogRetention) fetchLogRetention();
if (!hasLogRetention && canManageLogSettings) fetchLogRetention();
}, []);
const logRetentionSettings = logRetention?.[type];
@ -72,24 +72,22 @@ export const LogRetentionCallout: React.FC<Props> = ({ type }) => {
)
}
>
{canManageLogSettings && (
<p>
<FormattedMessage
id="xpack.enterpriseSearch.appSearch.logRetention.callout.description.manageSettingsDetail"
defaultMessage="To manage analytics & logging, {visitSettingsLink}."
values={{
visitSettingsLink: (
<EuiLinkTo to={SETTINGS_PATH}>
{i18n.translate(
'xpack.enterpriseSearch.appSearch.logRetention.callout.description.manageSettingsLinkText',
{ defaultMessage: 'visit your settings' }
)}
</EuiLinkTo>
),
}}
/>
</p>
)}
<p>
<FormattedMessage
id="xpack.enterpriseSearch.appSearch.logRetention.callout.description.manageSettingsDetail"
defaultMessage="To manage analytics & logging, {visitSettingsLink}."
values={{
visitSettingsLink: (
<EuiLinkTo to={SETTINGS_PATH}>
{i18n.translate(
'xpack.enterpriseSearch.appSearch.logRetention.callout.description.manageSettingsLinkText',
{ defaultMessage: 'visit your settings' }
)}
</EuiLinkTo>
),
}}
/>
</p>
</EuiCallOut>
<EuiSpacer />
</>

18
x-pack/plugins/enterprise_search/public/applications/app_search/components/log_retention/components/log_retention_tooltip.test.tsx
View file

@ -19,7 +19,10 @@ import { LogRetentionOptions, LogRetentionMessage } from '../';
import { LogRetentionTooltip } from './';
describe('LogRetentionTooltip', () => {
const values = { logRetention: {} };
const values = {
logRetention: {},
myRole: { canManageLogSettings: true },
};
const actions = { fetchLogRetention: jest.fn() };
beforeEach(() => {
@ -53,7 +56,7 @@ describe('LogRetentionTooltip', () => {
});
it('does not render if log retention is not available', () => {
setMockValues({ logRetention: null });
setMockValues({ ...values, logRetention: null });
const wrapper = mount(<LogRetentionTooltip type={LogRetentionOptions.API} />);
expect(wrapper.isEmptyRender()).toBe(true);
@ -61,14 +64,21 @@ describe('LogRetentionTooltip', () => {
describe('on mount', () => {
it('fetches log retention data when not already loaded', () => {
setMockValues({ logRetention: null });
setMockValues({ ...values, logRetention: null });
shallow(<LogRetentionTooltip type={LogRetentionOptions.Analytics} />);
expect(actions.fetchLogRetention).toHaveBeenCalled();
});
it('does not fetch log retention data if it has already been loaded', () => {
setMockValues({ logRetention: {} });
setMockValues({ ...values, logRetention: {} });
shallow(<LogRetentionTooltip type={LogRetentionOptions.Analytics} />);
expect(actions.fetchLogRetention).not.toHaveBeenCalled();
});
it('does not fetch log retention data if the user does not have access to log settings', () => {
setMockValues({ ...values, logRetention: null, myRole: { canManageLogSettings: false } });
shallow(<LogRetentionTooltip type={LogRetentionOptions.Analytics} />);
expect(actions.fetchLogRetention).not.toHaveBeenCalled();

9
x-pack/plugins/enterprise_search/public/applications/app_search/components/log_retention/components/log_retention_tooltip.tsx
View file

@ -12,7 +12,9 @@ import { useValues, useActions } from 'kea';
import { EuiIconTip } from '@elastic/eui';
import { i18n } from '@kbn/i18n';
import { LogRetentionLogic, LogRetentionMessage, LogRetentionOptions } from '../';
import { AppLogic } from '../../../app_logic';
import { LogRetentionLogic, LogRetentionMessage, LogRetentionOptions } from '../index';
interface Props {
type: LogRetentionOptions;
@ -21,11 +23,14 @@ interface Props {
export const LogRetentionTooltip: React.FC<Props> = ({ type, position = 'bottom' }) => {
const { fetchLogRetention } = useActions(LogRetentionLogic);
const { logRetention } = useValues(LogRetentionLogic);
const {
myRole: { canManageLogSettings },
} = useValues(AppLogic);
const hasLogRetention = logRetention !== null;
useEffect(() => {
if (!hasLogRetention) fetchLogRetention();
if (!hasLogRetention && canManageLogSettings) fetchLogRetention();
}, []);
return hasLogRetention ? (

49
x-pack/plugins/enterprise_search/public/applications/app_search/components/log_retention/log_retention_logic.test.ts
View file

@ -107,21 +107,6 @@ describe('LogRetentionLogic', () => {
});
});
describe('setLogRetentionUpdating', () => {
describe('isLogRetentionUpdating', () => {
it('sets isLogRetentionUpdating to true', () => {
mount();
LogRetentionLogic.actions.setLogRetentionUpdating();
expect(LogRetentionLogic.values).toEqual({
...DEFAULT_VALUES,
isLogRetentionUpdating: true,
});
});
});
});
describe('clearLogRetentionUpdating', () => {
describe('isLogRetentionUpdating', () => {
it('resets isLogRetentionUpdating to false', () => {
@ -300,8 +285,27 @@ describe('LogRetentionLogic', () => {
});
describe('fetchLogRetention', () => {
beforeAll(() => jest.useFakeTimers());
afterAll(() => jest.useRealTimers());
describe('isLogRetentionUpdating', () => {
it('sets isLogRetentionUpdating to true', () => {
mount({
isLogRetentionUpdating: false,
});
LogRetentionLogic.actions.fetchLogRetention();
expect(LogRetentionLogic.values).toEqual({
...DEFAULT_VALUES,
isLogRetentionUpdating: true,
});
});
});
it('will call an API endpoint and update log retention', async () => {
mount();
jest.spyOn(LogRetentionLogic.actions, 'clearLogRetentionUpdating');
jest
.spyOn(LogRetentionLogic.actions, 'updateLogRetention')
.mockImplementationOnce(() => {});
@ -309,14 +313,14 @@ describe('LogRetentionLogic', () => {
http.get.mockReturnValue(Promise.resolve(TYPICAL_SERVER_LOG_RETENTION));
LogRetentionLogic.actions.fetchLogRetention();
expect(LogRetentionLogic.values.isLogRetentionUpdating).toBe(true);
jest.runAllTimers();
await nextTick();
expect(http.get).toHaveBeenCalledWith('/api/app_search/log_settings');
await nextTick();
expect(LogRetentionLogic.actions.updateLogRetention).toHaveBeenCalledWith(
TYPICAL_CLIENT_LOG_RETENTION
);
expect(LogRetentionLogic.values.isLogRetentionUpdating).toBe(false);
expect(LogRetentionLogic.actions.clearLogRetentionUpdating).toHaveBeenCalled();
});
it('handles errors', async () => {
@ -325,19 +329,12 @@ describe('LogRetentionLogic', () => {
http.get.mockReturnValue(Promise.reject('An error occured'));
LogRetentionLogic.actions.fetchLogRetention();
jest.runAllTimers();
await nextTick();
expect(flashAPIErrors).toHaveBeenCalledWith('An error occured');
expect(LogRetentionLogic.actions.clearLogRetentionUpdating).toHaveBeenCalled();
});
it('does not run if isLogRetentionUpdating is true, preventing duplicate fetches', async () => {
mount({ isLogRetentionUpdating: true });
LogRetentionLogic.actions.fetchLogRetention();
expect(http.get).not.toHaveBeenCalled();
});
});
});
});

10
x-pack/plugins/enterprise_search/public/applications/app_search/components/log_retention/log_retention_logic.ts
View file

@ -14,7 +14,6 @@ import { LogRetentionOptions, LogRetention, LogRetentionServer } from './types';
import { convertLogRetentionFromServerToClient } from './utils/convert_log_retention';
interface LogRetentionActions {
setLogRetentionUpdating(): void;
clearLogRetentionUpdating(): void;
closeModals(): void;
fetchLogRetention(): void;
@ -36,7 +35,6 @@ interface LogRetentionValues {
export const LogRetentionLogic = kea<MakeLogicType<LogRetentionValues, LogRetentionActions>>({
path: ['enterprise_search', 'app_search', 'log_retention_logic'],
actions: () => ({
setLogRetentionUpdating: true,
clearLogRetentionUpdating: true,
closeModals: true,
fetchLogRetention: true,
@ -57,7 +55,7 @@ export const LogRetentionLogic = kea<MakeLogicType<LogRetentionValues, LogRetent
{
clearLogRetentionUpdating: () => false,
closeModals: () => false,
setLogRetentionUpdating: () => true,
fetchLogRetention: () => true,
toggleLogRetention: () => true,
},
],
@ -71,12 +69,10 @@ export const LogRetentionLogic = kea<MakeLogicType<LogRetentionValues, LogRetent
],
}),
listeners: ({ actions, values }) => ({
fetchLogRetention: async () => {
if (values.isLogRetentionUpdating) return; // Prevent duplicate calls to the API
fetchLogRetention: async (_, breakpoint) => {
await breakpoint(100); // Prevents duplicate calls to the API (e.g., when a tooltip & callout are on the same page)
try {
actions.setLogRetentionUpdating();
const { http } = HttpLogic.values;
const response = await http.get('/api/app_search/log_settings');