Skip to content

Commit

Permalink
Update authZ checks for SpacesSavedObjectsClient
Browse files Browse the repository at this point in the history
1. Removed previous changes to pass unit tests, changed the tests
   instead. The tests were meant to be asserting for the "403 Forbidden"
   error.
2. Updated bulkGet to replace `'*'` with an empty namespaces array when
   a user is not authorized to access any spaces. This is primarily so
   that the API is consistent (the Security plugin will still check
   that they are authorized to access the type in the active space and
   return a more specific 403 error if not).
  • Loading branch information
jportner committed Aug 25, 2021
1 parent beedf6f commit e90b900
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,28 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';
);
expect(spacesClient.getAll).toHaveBeenCalledTimes(1);
});

test(`replaces object namespaces '*' with an empty array when the user doesn't have access to any spaces`, async () => {
const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient();
spacesClient.getAll.mockRejectedValue(Boom.forbidden());

const objects = [
{ type: 'foo', id: '1', namespaces: ['*'] },
{ type: 'bar', id: '2', namespaces: ['*', 'this-is-ignored'] },
{ type: 'baz', id: '3', namespaces: ['another-space'] },
];
await client.bulkGet(objects);

expect(baseClient.bulkGet).toHaveBeenCalledWith(
[
{ type: 'foo', id: '1', namespaces: [] },
{ type: 'bar', id: '2', namespaces: [] },
{ type: 'baz', id: '3', namespaces: ['another-space'] }, // even if another space doesn't exist, it can be specified explicitly
],
{ namespace: currentSpace.expectedNamespace }
);
expect(spacesClient.getAll).toHaveBeenCalledTimes(1);
});
});

describe('#find', () => {
Expand All @@ -207,7 +229,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';

test(`returns empty result if user is unauthorized in any space`, async () => {
const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient();
spacesClient.getAll.mockRejectedValue(Boom.unauthorized());
spacesClient.getAll.mockRejectedValue(Boom.forbidden());

const options = Object.freeze({ type: 'foo', namespaces: ['some-ns'] });
const actualReturnValue = await client.find(options);
Expand Down Expand Up @@ -560,7 +582,7 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';

test(`throws error if if user is unauthorized in any space`, async () => {
const { client, baseClient, spacesClient } = createSpacesSavedObjectsClient();
spacesClient.getAll.mockRejectedValue(Boom.unauthorized());
spacesClient.getAll.mockRejectedValue(Boom.forbidden());

await expect(
client.openPointInTimeForType('foo', { namespaces: ['bar'] })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
try {
namespaces = await this.getSearchableSpaces(options.namespaces);
} catch (err) {
if (Boom.isBoom(err) && err.output.payload.statusCode === 401) {
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);
}
Expand Down Expand Up @@ -222,7 +222,15 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
let availableSpaces: string[] | undefined;
const getAvailableSpaces = async () => {
if (!availableSpaces) {
availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]);
try {
availableSpaces = await this.getSearchableSpaces([ALL_SPACES_ID]);
} catch (err) {
if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
availableSpaces = []; // the user doesn't have access to any spaces
} else {
throw err;
}
}
}
return availableSpaces;
};
Expand Down Expand Up @@ -405,7 +413,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
try {
namespaces = await this.getSearchableSpaces(options.namespaces);
} catch (err) {
if (Boom.isBoom(err) && err.output.payload.statusCode === 401) {
if (Boom.isBoom(err) && err.output.payload.statusCode === 403) {
// throw bad request since the user is unauthorized in any space
throw SavedObjectsErrorHelpers.createBadRequestError();
}
Expand Down

0 comments on commit e90b900

Please sign in to comment.