Skip to content

Commit

Permalink
use create to overwrite actions on update in action
Browse files Browse the repository at this point in the history
  • Loading branch information
gmmorris committed Jul 29, 2020
1 parent 5ea2870 commit d0c0249
Show file tree
Hide file tree
Showing 10 changed files with 249 additions and 23 deletions.
26 changes: 16 additions & 10 deletions x-pack/plugins/actions/server/actions_client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -893,7 +893,7 @@ describe('update()', () => {
},
references: [],
});
unsecuredSavedObjectsClient.update.mockResolvedValueOnce({
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: 'my-action',
type: 'action',
attributes: {
Expand Down Expand Up @@ -946,7 +946,7 @@ describe('update()', () => {
},
references: [],
});
unsecuredSavedObjectsClient.update.mockResolvedValueOnce({
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: 'my-action',
type: 'action',
attributes: {
Expand All @@ -972,17 +972,20 @@ describe('update()', () => {
name: 'my name',
config: {},
});
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(`
expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"action",
"my-action",
Object {
"actionTypeId": "my-action-type",
"config": Object {},
"name": "my name",
"secrets": Object {},
},
Object {
"id": "my-action",
"overwrite": true,
},
]
`);
expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -1043,7 +1046,7 @@ describe('update()', () => {
},
references: [],
});
unsecuredSavedObjectsClient.update.mockResolvedValueOnce({
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: 'my-action',
type: 'action',
attributes: {
Expand Down Expand Up @@ -1081,11 +1084,10 @@ describe('update()', () => {
c: true,
},
});
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.update.mock.calls[0]).toMatchInlineSnapshot(`
expect(unsecuredSavedObjectsClient.create).toHaveBeenCalledTimes(1);
expect(unsecuredSavedObjectsClient.create.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"action",
"my-action",
Object {
"actionTypeId": "my-action-type",
"config": Object {
Expand All @@ -1096,6 +1098,10 @@ describe('update()', () => {
"name": "my name",
"secrets": Object {},
},
Object {
"id": "my-action",
"overwrite": true,
},
]
`);
});
Expand All @@ -1118,7 +1124,7 @@ describe('update()', () => {
},
references: [],
});
unsecuredSavedObjectsClient.update.mockResolvedValueOnce({
unsecuredSavedObjectsClient.create.mockResolvedValueOnce({
id: 'my-action',
type: 'action',
attributes: {
Expand Down
26 changes: 17 additions & 9 deletions x-pack/plugins/actions/server/actions_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,18 +159,26 @@ export class ActionsClient {

this.actionTypeRegistry.ensureActionTypeEnabled(actionTypeId);

const result = await this.unsecuredSavedObjectsClient.update<RawAction>('action', id, {
actionTypeId,
name,
config: validatedActionTypeConfig as SavedObjectAttributes,
secrets: validatedActionTypeSecrets as SavedObjectAttributes,
});
// SOs client does partial updates - we want to overwrite the whole object, so we'll use create with overwrite
const result = await this.unsecuredSavedObjectsClient.create<RawAction>(
'action',
{
actionTypeId,
name,
config: validatedActionTypeConfig as SavedObjectAttributes,
secrets: validatedActionTypeSecrets as SavedObjectAttributes,
},
{
id,
overwrite: true,
}
);

return {
id,
actionTypeId: result.attributes.actionTypeId as string,
name: result.attributes.name as string,
config: result.attributes.config as Record<string, unknown>,
actionTypeId: result.attributes.actionTypeId,
name: result.attributes.name,
config: result.attributes.config,
isPreconfigured: false,
};
}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/actions/server/saved_objects/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ export function setupSavedObjects(
type: ACTION_SAVED_OBJECT_TYPE,
attributesToEncrypt: new Set(['secrets']),
attributesToExcludeFromAAD: new Set(['name']),
allowsExplicitIDs: true,
});

savedObjects.registerType({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { EncryptedSavedObjectTypeRegistration } from './encrypted_saved_objects_
*/
export class EncryptedSavedObjectAttributesDefinition {
public readonly attributesToEncrypt: ReadonlySet<string>;
public readonly allowsExplicitIDs: Readonly<boolean>;
private readonly attributesToExcludeFromAAD: ReadonlySet<string> | undefined;
private readonly attributesToStrip: ReadonlySet<string>;

Expand All @@ -34,6 +35,7 @@ export class EncryptedSavedObjectAttributesDefinition {
this.attributesToEncrypt = attributesToEncrypt;
this.attributesToStrip = attributesToStrip;
this.attributesToExcludeFromAAD = typeRegistration.attributesToExcludeFromAAD;
this.allowsExplicitIDs = typeRegistration.allowsExplicitIDs ?? false;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
function createEncryptedSavedObjectsServiceMock() {
return ({
isRegistered: jest.fn(),
allowsExplicitIDs: jest.fn(),
stripOrDecryptAttributes: jest.fn(),
encryptAttributes: jest.fn(),
decryptAttributes: jest.fn(),
Expand Down Expand Up @@ -49,6 +50,9 @@ export const encryptedSavedObjectsServiceMock = {
return clonedAttrs;
}

mock.allowsExplicitIDs.mockImplementation(
(type) => registrations.find((r) => r.type === type)?.allowsExplicitIDs ?? false
);
mock.isRegistered.mockImplementation(
(type) => registrations.findIndex((r) => r.type === type) >= 0
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,21 @@ describe('#isRegistered', () => {
});
});

describe('#allowsExplicitIDs', () => {
it('correctly returns allowsExplicitIDs when the specified type is registered', () => {
service.registerType({
type: 'allowsExplicitIDs-type-1',
attributesToEncrypt: new Set(['attr-1']),
allowsExplicitIDs: true,
});
expect(service.allowsExplicitIDs('allowsExplicitIDs-type-1')).toBe(true);
});

it('correctly defaults to false when the specified type is not registered', () => {
expect(service.allowsExplicitIDs('unknown-type-1')).toBe(false);
});
});

describe('#stripOrDecryptAttributes', () => {
it('does not strip attributes from unknown types', async () => {
const attributes = { attrOne: 'one', attrTwo: 'two', attrThree: 'three' };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export interface EncryptedSavedObjectTypeRegistration {
readonly type: string;
readonly attributesToEncrypt: ReadonlySet<string | AttributeToEncrypt>;
readonly attributesToExcludeFromAAD?: ReadonlySet<string>;
readonly allowsExplicitIDs?: boolean;
}

/**
Expand Down Expand Up @@ -121,6 +122,14 @@ export class EncryptedSavedObjectsService {
return this.typeDefinitions.has(type);
}

/**
* Returns whether the type allows explciit IDs when creating a saved object
* resolves to `false` if the type is not registered
*/
public allowsExplicitIDs(type: string) {
return this.typeDefinitions.get(type)?.allowsExplicitIDs ?? false;
}

/**
* Takes saved object attributes for the specified type and, depending on the type definition,
* either decrypts or strips encrypted attributes (e.g. in case AAD or encryption key has changed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,14 @@ beforeEach(() => {
{ key: 'attrNotSoSecret', dangerouslyExposeValue: true },
]),
},
{
type: 'known-type-with-allowed-explicit-ids',
allowsExplicitIDs: true,
attributesToEncrypt: new Set([
'attrSecret',
{ key: 'attrNotSoSecret', dangerouslyExposeValue: true },
]),
},
]);

wrapper = new EncryptedSavedObjectsClientWrapper({
Expand Down Expand Up @@ -121,6 +129,48 @@ describe('#create', () => {
);
});

it('allows explicit IDs when specified type allows it', async () => {
const attributes = {
attrOne: 'one',
attrSecret: 'secret',
attrNotSoSecret: 'not-so-secret',
attrThree: 'three',
};
const options = { overwrite: true, id: 'custom-uuid' };
const mockedResponse = {
id: 'custom-uuid',
type: 'known-type-with-allowed-explicit-ids',
attributes: {
attrOne: 'one',
attrSecret: '*secret*',
attrNotSoSecret: '*not-so-secret*',
attrThree: 'three',
},
references: [],
};

mockBaseClient.create.mockResolvedValue(mockedResponse);

expect(
await wrapper.create('known-type-with-allowed-explicit-ids', attributes, options)
).toEqual({
...mockedResponse,
attributes: { attrOne: 'one', attrNotSoSecret: 'not-so-secret', attrThree: 'three' },
});

expect(mockBaseClient.create).toHaveBeenCalledTimes(1);
expect(mockBaseClient.create).toHaveBeenCalledWith(
'known-type-with-allowed-explicit-ids',
{
attrOne: 'one',
attrSecret: '*secret*',
attrNotSoSecret: '*not-so-secret*',
attrThree: 'three',
},
{ id: 'custom-uuid', overwrite: true }
);
});

describe('namespace', () => {
const doTest = async (namespace: string, expectNamespaceInDescriptor: boolean) => {
const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' };
Expand Down Expand Up @@ -234,6 +284,66 @@ describe('#bulkCreate', () => {
);
});

it('allows explicit IDs when specified type allows it', async () => {
const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' };
const options = { namespace: 'some-namespace' };
const mockedResponse = {
saved_objects: [
{
id: 'uuid-v4-id',
type: 'known-type-with-allowed-explicit-ids',
attributes,
references: [],
},
{
id: 'custom-uuid',
type: 'known-type-with-allowed-explicit-ids',
attributes,
references: [],
},
{
id: 'some-id',
type: 'unknown-type',
attributes,
references: [],
},
],
};

mockBaseClient.bulkCreate.mockResolvedValue(mockedResponse);

const bulkCreateParams = [
{ type: 'known-type-with-allowed-explicit-ids', attributes },
{ id: 'custom-uuid', type: 'known-type-with-allowed-explicit-ids', attributes },
{ id: 'some-id', type: 'unknown-type', attributes },
];

await expect(wrapper.bulkCreate(bulkCreateParams, options)).resolves.toEqual({
saved_objects: [
{ ...mockedResponse.saved_objects[0], attributes: { attrOne: 'one', attrThree: 'three' } },
{ ...mockedResponse.saved_objects[1], attributes: { attrOne: 'one', attrThree: 'three' } },
mockedResponse.saved_objects[2],
],
});

expect(mockBaseClient.bulkCreate).toHaveBeenCalledTimes(1);
expect(mockBaseClient.bulkCreate).toHaveBeenCalledWith(
[
{
...bulkCreateParams[0],
id: 'uuid-v4-id',
attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' },
},
{
...bulkCreateParams[1],
attributes: { attrOne: 'one', attrSecret: '*secret*', attrThree: 'three' },
},
bulkCreateParams[2],
],
options
);
});

it('fails if ID is specified for registered type', async () => {
const attributes = { attrOne: 'one', attrSecret: 'secret', attrThree: 'three' };

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon
// Saved objects with encrypted attributes should have IDs that are hard to guess especially
// since IDs are part of the AAD used during encryption, that's why we control them within this
// wrapper and don't allow consumers to specify their own IDs directly.
if (options.id) {
if (options.id && !this.options.service.allowsExplicitIDs(type)) {
throw new Error(
'Predefined IDs are not allowed for saved objects with encrypted attributes.'
);
}

const id = generateID();
const id = options.id ?? generateID();
const namespace = getDescriptorNamespace(
this.options.baseTypeRegistry,
type,
Expand Down Expand Up @@ -103,13 +103,13 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon
// Saved objects with encrypted attributes should have IDs that are hard to guess especially
// since IDs are part of the AAD used during encryption, that's why we control them within this
// wrapper and don't allow consumers to specify their own IDs directly.
if (object.id) {
if (object.id && !this.options.service.allowsExplicitIDs(object.type)) {
throw new Error(
'Predefined IDs are not allowed for saved objects with encrypted attributes.'
);
}

const id = generateID();
const id = object.id ?? generateID();
const namespace = getDescriptorNamespace(
this.options.baseTypeRegistry,
object.type,
Expand Down
Loading

0 comments on commit d0c0249

Please sign in to comment.