From a686f6e5dcbae6a84fce367941b1b9660c26cd66 Mon Sep 17 00:00:00 2001 From: Brandon Kobel Date: Mon, 26 Nov 2018 12:35:08 -0800 Subject: [PATCH] Deleting no longer used privileges (#24873) (#26191) * We can now delete old privileges * Logging message when error deleting specific privilege --- .../register_privileges_with_cluster.js | 26 ++-- .../register_privileges_with_cluster.test.js | 118 +++++++++++++++--- x-pack/server/lib/esjs_shield_plugin.js | 17 +++ 3 files changed, 132 insertions(+), 29 deletions(-) diff --git a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js index 6845dd7590e2de..618d6e625fe638 100644 --- a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js +++ b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js @@ -51,12 +51,12 @@ export async function registerPrivilegesWithCluster(server) { }); }; - const shouldRemovePrivileges = (existingPrivileges, expectedPrivileges) => { + const getPrivilegesToDelete = (existingPrivileges, expectedPrivileges) => { if (isEmpty(existingPrivileges)) { - return false; + return []; } - return difference(Object.keys(existingPrivileges[application]), Object.keys(expectedPrivileges[application])).length > 0; + return difference(Object.keys(existingPrivileges[application]), Object.keys(expectedPrivileges[application])); }; const privilegeMap = buildPrivilegeMap(savedObjectTypes, actions); @@ -75,18 +75,24 @@ export async function registerPrivilegesWithCluster(server) { return; } - // The ES privileges POST endpoint only allows us to add new privileges, or update specified privileges; it doesn't - // remove unspecified privileges. We don't currently have a need to remove privileges, as this would be a - // backwards compatibility issue, and we'd have to figure out how to migrate roles, so we're throwing an Error if we - // unintentionally get ourselves in this position. - if (shouldRemovePrivileges(existingPrivileges, expectedPrivileges)) { - throw new Error(`Privileges are missing and can't be removed, currently.`); + const privilegesToDelete = getPrivilegesToDelete(existingPrivileges, expectedPrivileges); + for (const privilegeToDelete of privilegesToDelete) { + server.log(['security', 'debug'], `Deleting Kibana Privilege ${privilegeToDelete} from Elasticearch for ${application}`); + try { + await callCluster('shield.deletePrivilege', { + application, + privilege: privilegeToDelete + }); + } catch (err) { + server.log(['security', 'error'], `Error deleting Kibana Privilege ${privilegeToDelete}`); + throw err; + } } - server.log(['security', 'debug'], `Updated Kibana Privileges with Elasticearch for ${application}`); await callCluster('shield.postPrivileges', { body: expectedPrivileges }); + server.log(['security', 'debug'], `Updated Kibana Privileges with Elasticearch for ${application}`); } catch (err) { server.log(['security', 'error'], `Error registering Kibana Privileges with Elasticsearch for ${application}: ${err.message}`); throw err; diff --git a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js index b2a391aa495734..50a14566cf4e16 100644 --- a/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js +++ b/x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js @@ -21,6 +21,8 @@ const registerPrivilegesWithClusterTest = (description, { savedObjectTypes, privilegeMap, existingPrivileges, + throwErrorWhenDeletingPrivileges, + errorDeletingPrivilegeName, throwErrorWhenGettingPrivileges, throwErrorWhenPuttingPrivileges, assert @@ -67,9 +69,9 @@ const registerPrivilegesWithClusterTest = (description, { }; const createExpectUpdatedPrivileges = (mockServer, mockCallWithInternalUser, error) => { - return (postPrivilegesBody) => { + return (postPrivilegesBody, deletedPrivileges = []) => { expect(error).toBeUndefined(); - expect(mockCallWithInternalUser).toHaveBeenCalledTimes(2); + expect(mockCallWithInternalUser).toHaveBeenCalledTimes(2 + deletedPrivileges.length); expect(mockCallWithInternalUser).toHaveBeenCalledWith('shield.getPrivilege', { privilege: application, }); @@ -79,7 +81,19 @@ const registerPrivilegesWithClusterTest = (description, { body: postPrivilegesBody, } ); - + for (const deletedPrivilege of deletedPrivileges) { + expect(mockServer.log).toHaveBeenCalledWith( + ['security', 'debug'], + `Deleting Kibana Privilege ${deletedPrivilege} from Elasticearch for ${application}` + ); + expect(mockCallWithInternalUser).toHaveBeenCalledWith( + 'shield.deletePrivilege', + { + application, + privilege: deletedPrivilege + } + ); + } expect(mockServer.log).toHaveBeenCalledWith( ['security', 'debug'], `Registering Kibana Privileges with Elasticsearch for ${application}` @@ -116,6 +130,13 @@ const registerPrivilegesWithClusterTest = (description, { expect(actualError).toBeInstanceOf(Error); expect(actualError.message).toEqual(expectedErrorMessage); + if (throwErrorWhenDeletingPrivileges) { + expect(mockServer.log).toHaveBeenCalledWith( + ['security', 'error'], + `Error deleting Kibana Privilege ${errorDeletingPrivilegeName}` + ); + } + expect(mockServer.log).toHaveBeenCalledWith( ['security', 'error'], `Error registering Kibana Privileges with Elasticsearch for ${application}: ${expectedErrorMessage}` @@ -126,21 +147,37 @@ const registerPrivilegesWithClusterTest = (description, { test(description, async () => { const mockServer = createMockServer(); const mockCallWithInternalUser = registerMockCallWithInternalUser() - .mockImplementationOnce(async () => { - if (throwErrorWhenGettingPrivileges) { - throw throwErrorWhenGettingPrivileges; - } + .mockImplementation((api) => { + switch(api) { + case 'shield.getPrivilege': { + if (throwErrorWhenGettingPrivileges) { + throw throwErrorWhenGettingPrivileges; + } - // ES returns an empty object if we don't have any privileges - if (!existingPrivileges) { - return {}; - } + // ES returns an empty object if we don't have any privileges + if (!existingPrivileges) { + return {}; + } - return existingPrivileges; - }) - .mockImplementationOnce(async () => { - if (throwErrorWhenPuttingPrivileges) { - throw throwErrorWhenPuttingPrivileges; + return existingPrivileges; + } + case 'shield.deletePrivilege': { + if (throwErrorWhenDeletingPrivileges) { + throw throwErrorWhenDeletingPrivileges; + } + + break; + } + case 'shield.postPrivileges': { + if (throwErrorWhenPuttingPrivileges) { + throw throwErrorWhenPuttingPrivileges; + } + + return; + } + default: { + expect(true).toBe(false); + } } }); @@ -208,7 +245,7 @@ registerPrivilegesWithClusterTest(`inserts privileges when we don't have any exi } }); -registerPrivilegesWithClusterTest(`throws error when we should be removing privilege`, { +registerPrivilegesWithClusterTest(`deletes no-longer specified privileges`, { privilegeMap: { global: { foo: ['action:foo'], @@ -236,11 +273,32 @@ registerPrivilegesWithClusterTest(`throws error when we should be removing privi name: 'space_bar', actions: ['action:not-bar'], metadata: {}, + }, + space_baz: { + application, + name: 'space_baz', + actions: ['action:not-baz'], + metadata: {}, } } }, - assert: ({ expectErrorThrown }) => { - expectErrorThrown(`Privileges are missing and can't be removed, currently.`); + assert: ({ expectUpdatedPrivileges }) => { + expectUpdatedPrivileges({ + [application]: { + foo: { + application, + name: 'foo', + actions: ['action:foo'], + metadata: {}, + }, + space_bar: { + application, + name: 'space_bar', + actions: ['action:bar'], + metadata: {}, + } + } + }, [ 'quz', 'space_baz' ]); } }); @@ -434,6 +492,28 @@ registerPrivilegesWithClusterTest(`throws and logs error when errors getting pri } }); +registerPrivilegesWithClusterTest(`throws and logs error when errors deleting privileges`, { + privilegeMap: { + global: {}, + space: {} + }, + existingPrivileges: { + [application]: { + foo: { + application, + name: 'foo', + actions: ['action:not-foo'], + metadata: {}, + } + } + }, + throwErrorWhenDeletingPrivileges: new Error('Error deleting privileges'), + errorDeletingPrivilegeName: 'foo', + assert: ({ expectErrorThrown }) => { + expectErrorThrown('Error deleting privileges'); + } +}); + registerPrivilegesWithClusterTest(`throws and logs error when errors putting privileges`, { privilegeMap: { global: { diff --git a/x-pack/server/lib/esjs_shield_plugin.js b/x-pack/server/lib/esjs_shield_plugin.js index bcab31b554c9d4..fd1552e38bb5c3 100644 --- a/x-pack/server/lib/esjs_shield_plugin.js +++ b/x-pack/server/lib/esjs_shield_plugin.js @@ -390,6 +390,23 @@ }] }); + shield.deletePrivilege = ca({ + method: 'DELETE', + urls: [{ + fmt: '/_xpack/security/privilege/<%=application%>/<%=privilege%>', + req: { + application: { + type: 'string', + required: true + }, + privilege: { + type: 'string', + required: true + } + } + }] + }); + shield.postPrivileges = ca({ method: 'POST', needBody: true,