Skip to content

Commit

Permalink
Added suggestions from PR
Browse files Browse the repository at this point in the history
  • Loading branch information
thomheymann committed Dec 4, 2020
1 parent 5b91bb8 commit 459e0c4
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 46 deletions.
2 changes: 1 addition & 1 deletion src/core/server/saved_objects/service/lib/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ export class SavedObjectsUtils {
}

/**
* Validates that a saved object ID matches UUID format.
* Validates that a saved object ID has been randomly generated.
*
* @param {string} id The ID of a saved object.
* @todo Use `uuid.validate` once upgraded to v5.3+
Expand Down
18 changes: 10 additions & 8 deletions x-pack/plugins/actions/server/actions_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,14 +392,16 @@ export class ActionsClient {
const bulkGetOpts = actionSavedObjectsIds.map((id) => ({ id, type: 'action' }));
const bulkGetResult = await this.unsecuredSavedObjectsClient.bulkGet<RawAction>(bulkGetOpts);

ids.forEach((id) =>
this.auditLogger?.log(
connectorAuditEvent({
action: ConnectorAuditAction.GET,
savedObject: { type: 'action', id },
})
)
);
bulkGetResult.saved_objects.forEach(({ id, error }) => {
if (!error && this.auditLogger) {
this.auditLogger.log(
connectorAuditEvent({
action: ConnectorAuditAction.GET,
savedObject: { type: 'action', id },
})
);
}
});

for (const action of bulkGetResult.saved_objects) {
if (action.error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon
return await this.options.baseClient.create(type, attributes, options);
}

// 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.

// only allow a specified ID if we're overwriting an existing ESO with a Version
// this helps us ensure that the document really was previously created using ESO
// and not being used to get around the specified ID limitation
const canSpecifyID =
(options.overwrite && options.version) || SavedObjectsUtils.isRandomId(options.id);
if (options.id && !canSpecifyID) {
throw new Error(
'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
);
}

const id = options.id ?? SavedObjectsUtils.generateId();
const id = getValidId(options.id, options.version, options.overwrite);
const namespace = getDescriptorNamespace(
this.options.baseTypeRegistry,
type,
Expand Down Expand Up @@ -108,18 +93,7 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon
return object;
}

// 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 unless overwriting the original document.
const canSpecifyID =
(options?.overwrite && object.version) || SavedObjectsUtils.isRandomId(object.id);
if (object.id && !canSpecifyID) {
throw new Error(
'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
);
}

const id = object.id ?? SavedObjectsUtils.generateId();
const id = getValidId(object.id, object.version, options?.overwrite);
const namespace = getDescriptorNamespace(
this.options.baseTypeRegistry,
object.type,
Expand Down Expand Up @@ -321,3 +295,26 @@ export class EncryptedSavedObjectsClientWrapper implements SavedObjectsClientCon
return response;
}
}

// 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 unless overwriting the original document.
function getValidId(
id: string | undefined,
version: string | undefined,
overwrite: boolean | undefined
) {
if (id) {
// only allow a specified ID if we're overwriting an existing ESO with a Version
// this helps us ensure that the document really was previously created using ESO
// and not being used to get around the specified ID limitation
const canSpecifyID = (overwrite && version) || SavedObjectsUtils.isRandomId(id);
if (!canSpecifyID) {
throw new Error(
'Predefined IDs are not allowed for saved objects with encrypted attributes, unless the ID has been generated using `SavedObjectsUtils.generateId`.'
);
}
return id;
}
return SavedObjectsUtils.generateId();
}
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ describe('#bulkGet', () => {
});

test(`adds audit event when successful`, async () => {
const apiCallReturnValue = { saved_objects: [], foo: 'bar' };
const apiCallReturnValue = { saved_objects: [obj1, obj2], foo: 'bar' };
clientOpts.baseClient.bulkGet.mockReturnValue(apiCallReturnValue as any);
const objects = [obj1, obj2];
const options = { namespace };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,14 +294,16 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra

const response = await this.baseClient.bulkGet<T>(objects, options);

objects.forEach(({ type, id }) =>
this.auditLogger.log(
savedObjectEvent({
action: SavedObjectAction.GET,
savedObject: { type, id },
})
)
);
response.saved_objects.forEach(({ error, type, id }) => {
if (!error) {
this.auditLogger.log(
savedObjectEvent({
action: SavedObjectAction.GET,
savedObject: { type, id },
})
);
}
});

return await this.redactSavedObjectsNamespaces(response, [options.namespace]);
}
Expand Down

0 comments on commit 459e0c4

Please sign in to comment.