Skip to content

Commit

Permalink
Deleting no longer used privileges (#24873) (#26190)
Browse files Browse the repository at this point in the history 
* We can now delete old privileges

* Logging message when error deleting specific privilege
  • Loading branch information
@kobelb
kobelb authored Nov 26, 2018
1 parent a01114e commit 6de1f6e
Show file tree
Hide file tree
Showing 3 changed files with 132 additions and 29 deletions.
26 changes: 16 additions & 10 deletions x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
Expand Down
118 changes: 99 additions & 19 deletions x-pack/plugins/security/server/lib/authorization/register_privileges_with_cluster.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ const registerPrivilegesWithClusterTest = (description, {
savedObjectTypes,
privilegeMap,
existingPrivileges,
throwErrorWhenDeletingPrivileges,
errorDeletingPrivilegeName,
throwErrorWhenGettingPrivileges,
throwErrorWhenPuttingPrivileges,
assert
Expand Down Expand Up @@ -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,
});
Expand All @@ -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}`
Expand Down Expand Up @@ -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}`
Expand All @@ -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);
}
}
});

Expand Down Expand Up @@ -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'],
Expand Down Expand Up @@ -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' ]);
}
});

Expand Down Expand Up @@ -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: {
Expand Down
17 changes: 17 additions & 0 deletions x-pack/server/lib/esjs_shield_plugin.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 6de1f6e

Please sign in to comment.