Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multiple namespaces support to PIT search and finder #109062

Merged
merged 11 commits into from
Aug 23, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,9 @@ export class PointInTimeFinder<T = unknown, A = unknown>

private async open() {
try {
const { id } = await this.#client.openPointInTimeForType(this.#findOptions.type);
const { id } = await this.#client.openPointInTimeForType(this.#findOptions.type, {
namespaces: this.#findOptions.namespaces,
});
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
this.#pitId = id;
this.#open = true;
} catch (e) {
Expand Down
31 changes: 29 additions & 2 deletions src/core/server/saved_objects/service/lib/repository.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1863,9 +1863,36 @@ export class SavedObjectsRepository {
*/
async openPointInTimeForType(
type: string | string[],
{ keepAlive = '5m', preference }: SavedObjectsOpenPointInTimeOptions = {}
{
keepAlive = '5m',
preference,
namespaces,
typeToNamespacesMap,
}: SavedObjectsOpenPointInTimeOptions = {}
): Promise<SavedObjectsOpenPointInTimeResponse> {
const types = Array.isArray(type) ? type : [type];
if (!type && !typeToNamespacesMap) {
throw SavedObjectsErrorHelpers.createBadRequestError(
'options.type must be a string or an array of strings'
);
} else if (namespaces?.length === 0 && !typeToNamespacesMap) {
throw SavedObjectsErrorHelpers.createBadRequestError(
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
'options.namespaces cannot be an empty array'
);
} else if (type && typeToNamespacesMap) {
throw SavedObjectsErrorHelpers.createBadRequestError(
'options.type must be an empty string when options.typeToNamespacesMap is used'
);
} else if ((!namespaces || namespaces?.length) && typeToNamespacesMap) {
throw SavedObjectsErrorHelpers.createBadRequestError(
'options.namespaces must be an empty array when options.typeToNamespacesMap is used'
);
}

const types = type
? Array.isArray(type)
? type
: [type]
: Array.from(typeToNamespacesMap!.keys());
const allowedTypes = types.filter((t) => this._allowedTypes.includes(t));
if (allowedTypes.length === 0) {
throw SavedObjectsErrorHelpers.createGenericNotFoundError();
Expand Down
14 changes: 13 additions & 1 deletion src/core/server/saved_objects/service/saved_objects_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,7 @@ export interface SavedObjectsResolveResponse<T = unknown> {
/**
* @public
*/
export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOptions {
export interface SavedObjectsOpenPointInTimeOptions {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer extend SavedObjectsBaseOptions to remove the namespace option, as this new namespaces option was added.

Copy link
Contributor

@jportner jportner Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in #109062 (review), I think we should leave the SavedObjectsOpenPointInTimeOptions options as-is.

Copy link
Contributor Author

@pgayvallet pgayvallet Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with your approach, so typeToNamespacesMap is no longer necessary here and I will remove it. However SavedObjectsOpenPointInTimeOptions had a namespace?: string option due to this extension, that needs to be changed to a namespaces?: string[], for the security wrapper to use it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was going too fast, yep agreed

/**
* Optionally specify how long ES should keep the PIT alive until the next request. Defaults to `5m`.
*/
Expand All @@ -343,6 +343,18 @@ export interface SavedObjectsOpenPointInTimeOptions extends SavedObjectsBaseOpti
* An optional ES preference value to be used for the query.
*/
preference?: string;
/**
* An optional list of namespaces to be used by the PIT.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our separation between core / security makes documentation really hard here. But in the absence of another good place to write documentation it feels like we should maybe add some details about how this option behaves when the spaces plugin is enabled? Otherwise it's very difficult to figure out how to use this.

Suggested change
* An optional list of namespaces to be used by the PIT.
* An optional list of namespaces to be used by the PIT.
*
* When the spaces plugin is enabled:
*. - this will default to the user's current space as determined by the URL
* - if namespaces is specified the user's current space will be ignored
* - `['*']` will search across all available spaces

Copy link
Contributor Author

@pgayvallet pgayvallet Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that's fair. I can't think of any better place...

*/
namespaces?: string[];
/**
* This map defines each type to search for, and the namespace(s) to search for the type in; this is only intended to be used by a saved
* object client wrapper.
* If this is defined, it supersedes the `type` and `namespaces` fields when building the Elasticsearch query.
* Any types that are not included in this map will be excluded entirely.
* If a type is included but its value is undefined, the operation will search for that type in the Default namespace.
*/
typeToNamespacesMap?: Map<string, string[] | undefined>;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import type {
SavedObjectsUpdateOptions,
} from 'src/core/server';

import { SavedObjectsUtils } from '../../../../../src/core/server';
import { SavedObjectsErrorHelpers, SavedObjectsUtils } from '../../../../../src/core/server';
import { ALL_SPACES_ID, UNKNOWN_SPACE } from '../../common/constants';
import type { AuditLogger, SecurityAuditLogger } from '../audit';
import { SavedObjectAction, savedObjectEvent } from '../audit';
Expand All @@ -54,6 +54,7 @@ interface SecureSavedObjectsClientWrapperOptions {
baseClient: SavedObjectsClientContract;
errors: SavedObjectsClientContract['errors'];
checkSavedObjectsPrivilegesAsCurrentUser: CheckSavedObjectsPrivileges;

getSpacesService(): SpacesService | undefined;
}

Expand All @@ -75,10 +76,12 @@ interface LegacyEnsureAuthorizedResult {
status: 'fully_authorized' | 'partially_authorized' | 'unauthorized';
typeMap: Map<string, LegacyEnsureAuthorizedTypeResult>;
}

interface LegacyEnsureAuthorizedTypeResult {
authorizedSpaces: string[];
isGloballyAuthorized?: boolean;
}

export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContract {
private readonly actions: Actions;
private readonly legacyAuditLogger: PublicMethodsOf<SecurityAuditLogger>;
Expand Down Expand Up @@ -236,11 +239,6 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
`_find across namespaces is not permitted when the Spaces plugin is disabled.`
);
}
if (options.pit && Array.isArray(options.namespaces) && options.namespaces.length > 1) {
throw this.errors.createBadRequestError(
'_find across namespaces is not permitted when using the `pit` option.'
);
}

const args = { options };
const { status, typeMap } = await this.legacyEnsureAuthorized(
Expand Down Expand Up @@ -508,32 +506,52 @@ export class SecureSavedObjectsClientWrapper implements SavedObjectsClientContra
type: string | string[],
options: SavedObjectsOpenPointInTimeOptions
) {
try {
const args = { type, options };
await this.legacyEnsureAuthorized(type, 'open_point_in_time', options?.namespace, {
const args = { type, options };
const { status, typeMap } = await this.legacyEnsureAuthorized(
type,
'open_point_in_time',
options?.namespaces,
{
args,
// Partial authorization is acceptable in this case because this method is only designed
// to be used with `find`, which already allows for partial authorization.
requireFullAuthorization: false,
});
} catch (error) {
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
}
);

if (status === 'unauthorized') {
this.auditLogger.log(
savedObjectEvent({
action: SavedObjectAction.OPEN_POINT_IN_TIME,
error,
error: new Error(status),
})
);
throw error;
throw SavedObjectsErrorHelpers.decorateForbiddenError(new Error(status));
Comment on lines -526 to +528
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the same scenario, the find wrapper returns SavedObjectsUtils.createEmptyFindResponse, however this is not possible for openPointInTimeForType, as the expected return type is a SavedObjectsOpenPointInTimeResponse. I'm throwing a forbidden error in that case, but really not sure this is the correct error type?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW this makes sense to me: If you are opening a PIT against a type & you don't have permissions to access that type's backing index, I'd expect a forbidden error. But let's see what @jportner thinks.

Copy link
Contributor

@jportner jportner Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the forbidden error seems like the best approach here. Note that if you are only authorized to access a type in space A, and you attempt to open a PIT for that type only in space B, you'll run into a forbidden error. But that seems acceptable.

Copy link
Contributor Author

@pgayvallet pgayvallet Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you are only authorized to access a type in space A, and you attempt to open a PIT for that type only in space B, you'll run into a forbidden error. But that seems acceptable.

I would even think that this is expected and not just acceptable? OTOH that's right that it differs from the find behavior that just returns an empty find response without throwing in the same situation.

The alternative would be to call baseClient.openPointInTimeForType with an empty list of types, and have it return a stubbed PIT finder impl that returns no results in that case, but that feels a little over-complicated to me?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, actually we can't do that, as it would need to be done in createPointInTimeFinder, so it doesn't solve the issue when calling openPointInTimeForType directly. So throwing here seems the only viable option anyway

pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
}

const typeToNamespacesMap = Array.from(typeMap).reduce<Map<string, string[] | undefined>>(
(acc, [currentType, { authorizedSpaces, isGloballyAuthorized }]) =>
isGloballyAuthorized
? acc.set(currentType, options.namespaces)
: acc.set(currentType, authorizedSpaces),
new Map()
);

this.auditLogger.log(
savedObjectEvent({
action: SavedObjectAction.OPEN_POINT_IN_TIME,
outcome: 'unknown',
})
);

return await this.baseClient.openPointInTimeForType(type, options);
return await this.baseClient.openPointInTimeForType(
status === 'partially_authorized' ? '' : type, // if the user is partially authorized, type must be an empty string to use typeToNamespacesMap instead
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
{
...options,
typeToNamespacesMap: undefined, // if the user is fully authorized, use `undefined` as the typeToNamespacesMap to prevent privilege escalation
...(status === 'partially_authorized' && { typeToNamespacesMap, namespaces: [] }), // the repository requires that `type` and `namespaces` must be empty if `typeToNamespacesMap` is defined
}
);
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
}

public async closePointInTime(id: string, options?: SavedObjectsClosePointInTimeOptions) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ import type {
SavedObjectsUpdateOptions,
} from 'src/core/server';

import { SavedObjectsUtils } from '../../../../../src/core/server';
import { SavedObjectsErrorHelpers, SavedObjectsUtils } from '../../../../../src/core/server';
import { ALL_SPACES_ID } from '../../common/constants';
import { spaceIdToNamespace } from '../lib/utils/namespace';
import type { ISpacesClient } from '../spaces_client';
Expand Down Expand Up @@ -177,30 +177,19 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
public async find<T = unknown, A = unknown>(options: SavedObjectsFindOptions) {
throwErrorIfNamespaceSpecified(options);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why this was there, there isn't a namespace property on SavedObjectsFindOptions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code! thanks for removing it


let namespaces = options.namespaces;
if (namespaces) {
try {
const availableSpaces = await this.spacesClient.getAll({ purpose: 'findSavedObjects' });
if (namespaces.includes(ALL_SPACES_ID)) {
namespaces = availableSpaces.map((space) => space.id);
} else {
namespaces = namespaces.filter((namespace) =>
availableSpaces.some((space) => space.id === namespace)
);
}
if (namespaces.length === 0) {
// return empty response, since the user is unauthorized in this space (or these spaces), but we don't return forbidden errors for `find` operations
return SavedObjectsUtils.createEmptyFindResponse<T, A>(options);
}
} catch (err) {
if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
// return empty response, since the user is unauthorized in any space, but we don't return forbidden errors for `find` operations
return SavedObjectsUtils.createEmptyFindResponse<T, A>(options);
}
throw err;
let namespaces: string[];
try {
namespaces = await this.getSearchableSpaces(options.namespaces);
} catch (err) {
if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
// return empty response, since the user is unauthorized in any space, but we don't return forbidden errors for `find` operations
return SavedObjectsUtils.createEmptyFindResponse<T, A>(options);
}
} else {
namespaces = [this.spaceId];
throw err;
}
if (namespaces.length === 0) {
// return empty response, since the user is unauthorized in this space (or these spaces), but we don't return forbidden errors for `find` operations
return SavedObjectsUtils.createEmptyFindResponse<T, A>(options);
}

return await this.client.find<T, A>({
Expand All @@ -212,6 +201,21 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
});
}

private async getSearchableSpaces(namespaces?: string[]): Promise<string[]> {
if (namespaces) {
const availableSpaces = await this.spacesClient.getAll({ purpose: 'findSavedObjects' });
if (namespaces.includes(ALL_SPACES_ID)) {
return availableSpaces.map((space) => space.id);
} else {
return namespaces.filter((namespace) =>
availableSpaces.some((space) => space.id === namespace)
);
}
} else {
return [this.spaceId];
}
}

/**
* Returns an array of objects by id
*
Expand Down Expand Up @@ -397,9 +401,16 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
options: SavedObjectsOpenPointInTimeOptions = {}
) {
throwErrorIfNamespaceSpecified(options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can get rid of this too


const namespaces = await this.getSearchableSpaces(options.namespaces);
if (namespaces.length === 0) {
// throw bad request if no valid spaces were found.
throw SavedObjectsErrorHelpers.createBadRequestError();
}

return await this.client.openPointInTimeForType(type, {
...options,
namespace: spaceIdToNamespace(this.spaceId),
namespaces,
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
});
}

Expand Down