Capability resolver: minor optimizations (#152784)

## Summary

Related to https://github.com/elastic/kibana/issues/146881

I tried to fix the issue, but couldn't so I kept the tiny optimizations
I made on the way:
- optimize reduce blocks to re-use the memo instead of spreading into a
new object
- update most of the existing resolver to only return the list of
changes rather than the whole capability objects (which was how it was
supposed to work) - minor perf improvement when merging the result of
the resolvers
This commit is contained in:
Pierre Gayvallet 2023-03-09 08:37:42 +01:00 committed by GitHub
parent ef918afb79
commit e058c06d57
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 33 additions and 69 deletions

View file

@ -10,7 +10,7 @@ import { mergeWith } from 'lodash';
import type { Capabilities } from '@kbn/core-capabilities-common';
export const mergeCapabilities = (...sources: Array<Partial<Capabilities>>): Capabilities =>
mergeWith({}, ...sources, (a: any, b: any) => {
mergeWith({}, ...sources, (a: unknown, b: unknown) => {
if (
(typeof a === 'boolean' && typeof b === 'object') ||
(typeof a === 'object' && typeof b === 'boolean')

View file

@ -46,16 +46,14 @@ export const resolveCapabilities = async (
applications: string[],
useDefaultCapabilities: boolean
): Promise<Capabilities> => {
const mergedCaps = cloneDeep({
const mergedCaps: Capabilities = cloneDeep({
...capabilities,
navLinks: applications.reduce(
(acc, app) => ({
...acc,
[app]: true,
}),
capabilities.navLinks
),
navLinks: applications.reduce((acc, app) => {
acc[app] = true;
return acc;
}, capabilities.navLinks),
});
return switchers.reduce(async (caps, switcher) => {
const resolvedCaps = await caps;
const changes = await switcher(request, resolvedCaps, useDefaultCapabilities);
@ -79,11 +77,8 @@ function recursiveApplyChanges<
}
return [key, typeof orig === typeof changed ? changed : orig];
})
.reduce(
(acc, [key, value]) => ({
...acc,
[key]: value,
}),
{} as TDestination
);
.reduce((acc, [key, value]) => {
acc[key as keyof TDestination] = value;
return acc;
}, {} as TDestination);
}

View file

@ -44,16 +44,9 @@ describe('setupCapabilities', () => {
const request = httpServerMock.createKibanaRequest();
await expect(switcher(request, capabilities, false)).resolves.toMatchInlineSnapshot(`
Object {
"catalogue": Object {},
"fileUpload": Object {
"show": true,
},
"management": Object {},
"navLinks": Object {},
}
`);
await expect(switcher(request, capabilities, false)).resolves.toMatchInlineSnapshot(
`Object {}`
);
});
it('registers a capabilities switcher that returns unaltered capabilities when default capabilities are requested', async () => {
@ -81,16 +74,7 @@ describe('setupCapabilities', () => {
const request = httpServerMock.createKibanaRequest();
await expect(switcher(request, capabilities, true)).resolves.toMatchInlineSnapshot(`
Object {
"catalogue": Object {},
"fileUpload": Object {
"show": true,
},
"management": Object {},
"navLinks": Object {},
}
`);
await expect(switcher(request, capabilities, true)).resolves.toMatchInlineSnapshot(`Object {}`);
expect(security.authz.mode.useRbacForRequest).not.toHaveBeenCalled();
expect(security.authz.checkPrivilegesDynamicallyWithRequest).not.toHaveBeenCalled();
@ -166,16 +150,9 @@ describe('setupCapabilities', () => {
const request = httpServerMock.createKibanaRequest();
await expect(switcher(request, capabilities, false)).resolves.toMatchInlineSnapshot(`
Object {
"catalogue": Object {},
"fileUpload": Object {
"show": true,
},
"management": Object {},
"navLinks": Object {},
}
`);
await expect(switcher(request, capabilities, false)).resolves.toMatchInlineSnapshot(
`Object {}`
);
expect(security.authz.mode.useRbacForRequest).toHaveBeenCalledTimes(1);
expect(security.authz.mode.useRbacForRequest).toHaveBeenCalledWith(request);
@ -249,16 +226,9 @@ describe('setupCapabilities', () => {
const request = httpServerMock.createKibanaRequest();
await expect(switcher(request, capabilities, false)).resolves.toMatchInlineSnapshot(`
Object {
"catalogue": Object {},
"fileUpload": Object {
"show": true,
},
"management": Object {},
"navLinks": Object {},
}
`);
await expect(switcher(request, capabilities, false)).resolves.toMatchInlineSnapshot(
`Object {}`
);
expect(security.authz.mode.useRbacForRequest).toHaveBeenCalledTimes(1);
expect(security.authz.mode.useRbacForRequest).toHaveBeenCalledWith(request);

View file

@ -22,7 +22,7 @@ export const setupCapabilities = (
core.capabilities.registerSwitcher(async (request, capabilities, useDefaultCapabilities) => {
if (useDefaultCapabilities) {
return capabilities;
return {};
}
const [, { security }] = await core.getStartServices();
@ -42,6 +42,6 @@ export const setupCapabilities = (
};
}
return capabilities;
return {};
});
};

View file

@ -23,9 +23,8 @@ export const setupCapabilitiesSwitcher = (
function getSwitcher(license$: Observable<ILicense>, logger: Logger): CapabilitiesSwitcher {
return async (request, capabilities) => {
const isAnonymousRequest = !request.route.options.authRequired;
if (isAnonymousRequest) {
return capabilities;
return {};
}
try {
@ -34,11 +33,11 @@ function getSwitcher(license$: Observable<ILicense>, logger: Logger): Capabiliti
// full license, leave capabilities as they were
if (mlEnabled && isFullLicense(license)) {
return capabilities;
return {};
}
const mlCaps = capabilities.ml as MlCapabilities;
const originalCapabilities = cloneDeep(mlCaps);
const originalCapabilities = capabilities.ml as MlCapabilities;
const mlCaps = cloneDeep(originalCapabilities);
// not full licence, switch off all capabilities
Object.keys(mlCaps).forEach((k) => {
@ -50,10 +49,10 @@ function getSwitcher(license$: Observable<ILicense>, logger: Logger): Capabiliti
basicLicenseMlCapabilities.forEach((c) => (mlCaps[c] = originalCapabilities[c]));
}
return capabilities;
return { ml: mlCaps };
} catch (e) {
logger.debug(`Error updating capabilities for ML based on licensing: ${e}`);
return capabilities;
return {};
}
};
}

View file

@ -156,7 +156,7 @@ export class AuthorizationService {
// If we have a license which doesn't enable security, or we're a legacy user we shouldn't
// disable any ui capabilities
if (!mode.useRbacForRequest(request)) {
return uiCapabilities;
return {};
}
const disableUICapabilities = disableUICapabilitiesFactory(

View file

@ -173,7 +173,7 @@ describe('capabilitiesSwitcher', () => {
const result = await switcher(request, capabilities, false);
expect(result).toEqual(buildCapabilities());
expect(result).toEqual({});
expect(spacesService.getActiveSpace).not.toHaveBeenCalled();
});
@ -191,7 +191,7 @@ describe('capabilitiesSwitcher', () => {
const result = await switcher(request, capabilities, true);
expect(result).toEqual(buildCapabilities());
expect(result).toEqual({});
expect(spacesService.getActiveSpace).not.toHaveBeenCalled();
});

View file

@ -24,7 +24,7 @@ export function setupCapabilitiesSwitcher(
const shouldNotToggleCapabilities = isAuthRequiredOrOptional || useDefaultCapabilities;
if (shouldNotToggleCapabilities) {
return capabilities;
return {};
}
try {