Skip to content

Commit

Permalink
Avoid redundant saved objects authorization checks
Browse files Browse the repository at this point in the history
  • Loading branch information
@jportner
jportner committed Oct 30, 2020
1 parent fcb590e commit 15b57dc
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 47 deletions.
41 changes: 26 additions & 15 deletions x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ const createSecureSavedObjectsClientWrapperOptions = () => {
createBadRequestError: jest.fn().mockImplementation((message) => new Error(message)),
isNotFoundError: jest.fn().mockReturnValue(false),
} as unknown) as jest.Mocked<SavedObjectsClientContract['errors']>;
const getSpacesService = jest.fn().mockReturnValue(true);
const getSpacesService = jest.fn().mockReturnValue({
namespaceToSpaceId: (namespace?: string) => (namespace ? namespace : 'default'),
});

return {
actions,
Expand Down Expand Up @@ -174,7 +176,9 @@ const expectObjectNamespaceFiltering = async (
);
expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith(
'login:',
namespaces.filter((x) => x !== '*') // when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
['some-other-namespace']
// when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
// we don't check privileges for authorizedNamespace either, as that was already checked earlier in the operation
);
};

Expand Down Expand Up @@ -206,12 +210,17 @@ const expectObjectsNamespaceFiltering = async (fn: Function, args: Record<string
getMockCheckPrivilegesFailure // privilege check for namespace filtering
);

const authorizedNamespace = args.options.namespace || 'default';
// the 'find' operation has options.namespaces, the others have options.namespace
const authorizedNamespaces = args.options.namespaces
? args.options.namespaces
: args.options.namespace
? [args.options.namespace]
: ['default'];
const returnValue = {
saved_objects: [
{ namespaces: ['foo'] },
{ namespaces: [authorizedNamespace] },
{ namespaces: ['foo', authorizedNamespace] },
{ namespaces: ['*'] },
{ namespaces: authorizedNamespaces },
{ namespaces: ['some-other-namespace', ...authorizedNamespaces] },
],
};

Expand All @@ -224,17 +233,19 @@ const expectObjectsNamespaceFiltering = async (fn: Function, args: Record<string
const result = await fn.bind(client)(...Object.values(args));
expect(result).toEqual({
saved_objects: [
{ namespaces: ['?'] },
{ namespaces: [authorizedNamespace] },
{ namespaces: [authorizedNamespace, '?'] },
{ namespaces: ['*'] },
{ namespaces: authorizedNamespaces },
{ namespaces: [...authorizedNamespaces, '?'] },
],
});

expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenCalledTimes(2);
expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith('login:', [
'foo',
authorizedNamespace,
]);
expect(clientOpts.checkSavedObjectsPrivilegesAsCurrentUser).toHaveBeenLastCalledWith(
'login:',
['some-other-namespace']
// when we check what namespaces to redact, we don't check privileges for '*', only actual space IDs
// we don't check privileges for authorizedNamespaces either, as that was already checked earlier in the operation
);
};

function getMockCheckPrivilegesSuccess(actions: string | string[], namespaces?: string | string[]) {
Expand Down Expand Up @@ -964,8 +975,8 @@ describe('#get', () => {
describe('#deleteFromNamespaces', () => {
const type = 'foo';
const id = `${type}-id`;
const namespace1 = 'foo-namespace';
const namespace2 = 'bar-namespace';
const namespace1 = 'default';
const namespace2 = 'another-namespace';
const namespaces = [namespace1, namespace2];
const privilege = `mock-saved_object:${type}/share_to_space`;

Expand Down
84 changes: 52 additions & 32 deletions x-pack/plugins/security/server/saved_objects/secure_saved_objects_client_wrapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,9 +90,9 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
attributes: T = {} as T,
options: SavedObjectsCreateOptions = {}
) {
const namespaces = [options.namespace, ...(options.initialNamespaces || [])];
try {
const args = { type, attributes, options };
const namespaces = [options.namespace, ...(options.initialNamespaces || [])];
await this.ensureAuthorized(type, 'create', namespaces, { args });
} catch (error) {
this.auditLogger.log(
Expand All @@ -113,7 +113,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const savedObject = await this.baseClient.create(type, attributes, options);
return await this.redactSavedObjectNamespaces(savedObject);
return await this.redactSavedObjectNamespaces(savedObject, namespaces);
}

public async checkConflicts(
Expand All @@ -135,15 +135,12 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
objects: Array<SavedObjectsBulkCreateObject<T>>,
options: SavedObjectsBaseOptions = {}
) {
const namespaces = objects.reduce(
(acc, { initialNamespaces = [] }) => acc.concat(initialNamespaces),
[options.namespace]
);
try {
const args = { objects, options };
const namespaces = objects.reduce(
(acc, { initialNamespaces = [] }) => {
return acc.concat(initialNamespaces);
},
[options.namespace]
);

await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_create', namespaces, {
args,
});
Expand All @@ -170,7 +167,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.bulkCreate(objects, options);
return await this.redactSavedObjectsNamespaces(response);
return await this.redactSavedObjectsNamespaces(response, namespaces);
}

public async delete(type: string, id: string, options: SavedObjectsBaseOptions = {}) {
Expand Down Expand Up @@ -249,7 +246,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
)
);

return await this.redactSavedObjectsNamespaces(response);
return await this.redactSavedObjectsNamespaces(response, options.namespaces ?? [undefined]);
}

public async bulkGet<T = unknown>(
Expand Down Expand Up @@ -290,7 +287,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
)
);

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

public async get<T = unknown>(type: string, id: string, options: SavedObjectsBaseOptions = {}) {
Expand All @@ -317,7 +314,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
})
);

return await this.redactSavedObjectNamespaces(savedObject);
return await this.redactSavedObjectNamespaces(savedObject, [options.namespace]);
}

public async update<T = unknown>(
Expand Down Expand Up @@ -348,7 +345,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const savedObject = await this.baseClient.update(type, id, attributes, options);
return await this.redactSavedObjectNamespaces(savedObject);
return await this.redactSavedObjectNamespaces(savedObject, [options.namespace]);
}

public async addToNamespaces(
Expand All @@ -357,9 +354,9 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
namespaces: string[],
options: SavedObjectsAddToNamespacesOptions = {}
) {
const { namespace } = options;
try {
const args = { type, id, namespaces, options };
const { namespace } = options;
// To share an object, the user must have the "share_to_space" permission in each of the destination namespaces.
await this.ensureAuthorized(type, 'share_to_space', namespaces, {
args,
Expand Down Expand Up @@ -395,7 +392,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.addToNamespaces(type, id, namespaces, options);
return await this.redactSavedObjectNamespaces(response);
return await this.redactSavedObjectNamespaces(response, [namespace, ...namespaces]);
}

public async deleteFromNamespaces(
Expand Down Expand Up @@ -432,20 +429,20 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.deleteFromNamespaces(type, id, namespaces, options);
return await this.redactSavedObjectNamespaces(response);
return await this.redactSavedObjectNamespaces(response, namespaces);
}

public async bulkUpdate<T = unknown>(
objects: Array<SavedObjectsBulkUpdateObject<T>> = [],
options: SavedObjectsBaseOptions = {}
) {
const objectNamespaces = objects
// The repository treats an `undefined` object namespace is treated as the absence of a namespace, falling back to options.namespace;
// in this case, filter it out here so we don't accidentally check for privileges in the Default space when we shouldn't be doing so.
.filter(({ namespace }) => namespace !== undefined)
.map(({ namespace }) => namespace!);
const namespaces = [options?.namespace, ...objectNamespaces];
try {
const objectNamespaces = objects
// The repository treats an `undefined` object namespace is treated as the absence of a namespace, falling back to options.namespace;
// in this case, filter it out here so we don't accidentally check for privileges in the Default space when we shouldn't be doing so.
.filter(({ namespace }) => namespace !== undefined)
.map(({ namespace }) => namespace!);
const namespaces = [options?.namespace, ...objectNamespaces];
const args = { objects, options };
await this.ensureAuthorized(this.getUniqueObjectTypes(objects), 'bulk_update', namespaces, {
args,
Expand Down Expand Up @@ -473,7 +470,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
);

const response = await this.baseClient.bulkUpdate<T>(objects, options);
return await this.redactSavedObjectsNamespaces(response);
return await this.redactSavedObjectsNamespaces(response, namespaces);
}

private async checkPrivileges(
Expand Down Expand Up @@ -596,14 +593,21 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
return map;
}

private redactAndSortNamespaces(spaceIds: string[], privilegeMap: Record<string, boolean>) {
private redactAndSortNamespaces(
spaceIds: string[],
privilegeMap: Record<string, boolean>,
authorizedSpaceIds: string[]
) {
return spaceIds
.map((x) => (x === ALL_SPACES_ID || privilegeMap[x] ? x : UNKNOWN_SPACE))
.map((x) =>
x === ALL_SPACES_ID || privilegeMap[x] || authorizedSpaceIds.includes(x) ? x : UNKNOWN_SPACE
)
.sort(namespaceComparator);
}

private async redactSavedObjectNamespaces<T extends SavedObjectNamespaces>(
savedObject: T
savedObject: T,
authorizedNamespaces: Array<string | undefined>
): Promise<T> {
if (
this.getSpacesService() === undefined ||
Expand All @@ -613,7 +617,13 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
return savedObject;
}

const namespaces = savedObject.namespaces.filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
const authorizedSpaceIds = authorizedNamespaces.map((x) =>
this.getSpacesService()!.namespaceToSpaceId(x)
);
// all users can see the "all spaces" ID, and we don't need to recheck authorization for any namespaces that we just checked earlier
const namespaces = savedObject.namespaces.filter(
(x) => x !== ALL_SPACES_ID && !authorizedSpaceIds.includes(x)
);
if (namespaces.length === 0) {
return savedObject;
}
Expand All @@ -622,20 +632,30 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra

return {
...savedObject,
namespaces: this.redactAndSortNamespaces(savedObject.namespaces, privilegeMap),
namespaces: this.redactAndSortNamespaces(
savedObject.namespaces,
privilegeMap,
authorizedSpaceIds
),
};
}

private async redactSavedObjectsNamespaces<T extends SavedObjectsNamespaces>(
response: T
response: T,
authorizedNamespaces: Array<string | undefined>
): Promise<T> {
if (this.getSpacesService() === undefined) {
return response;
}

const authorizedSpaceIds = authorizedNamespaces.map((x) =>
this.getSpacesService()!.namespaceToSpaceId(x)
);
const { saved_objects: savedObjects } = response;
// all users can see the "all spaces" ID, and we don't need to recheck authorization for any namespaces that we just checked earlier
const namespaces = uniq(
savedObjects.flatMap((savedObject) => savedObject.namespaces || [])
).filter((x) => x !== ALL_SPACES_ID); // all users can see the "all spaces" ID
).filter((x) => x !== ALL_SPACES_ID && !authorizedSpaceIds.includes(x));
if (namespaces.length === 0) {
return response;
}
Expand All @@ -648,7 +668,7 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
...savedObject,
namespaces:
savedObject.namespaces &&
this.redactAndSortNamespaces(savedObject.namespaces, privilegeMap),
this.redactAndSortNamespaces(savedObject.namespaces, privilegeMap, authorizedSpaceIds),
})),
};
}
Expand Down

0 comments on commit 15b57dc

Please sign in to comment.