Skip to content

Commit

Permalink
PR review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@jportner
jportner committed Aug 26, 2021
1 parent dcec828 commit fab5c21
Show file tree
Hide file tree
Showing 6 changed files with 35 additions and 46 deletions.
1 change: 1 addition & 0 deletions src/core/server/saved_objects/service/lib/internal_utils.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ describe('#rawDocExistsInNamespaces', () => {
expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['default'])).toBe(false);
expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, ['some-space'])).toBe(false);
expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['some-space'])).toBe(false);
expect(rawDocExistsInNamespaces(registry, docInNoSpace, ['*'])).toBe(false);
expect(rawDocExistsInNamespaces(registry, docInDefaultSpace, [])).toBe(false);
expect(rawDocExistsInNamespaces(registry, docInSomeSpace, [])).toBe(false);
expect(rawDocExistsInNamespaces(registry, docInAllSpaces, [])).toBe(false);
Expand Down
3 changes: 2 additions & 1 deletion src/core/server/saved_objects/service/lib/internal_utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,8 @@ export function rawDocExistsInNamespaces(

if (namespacesToCheck.size === 0 || existingNamespaces.length === 0) {
return false;
} else if (namespacesToCheck.has(ALL_NAMESPACES_STRING)) {
}
if (namespacesToCheck.has(ALL_NAMESPACES_STRING)) {
return true;
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/server/saved_objects/service/lib/repository.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1004,7 +1004,7 @@ export class SavedObjectsRepository {
const bulkGetDocs = expectedBulkGetResults
.filter(isRight)
.map(({ value: { type, id, fields, namespaces } }) => ({
_id: this._serializer.generateRawId(getNamespaceId(namespaces), type, id),
_id: this._serializer.generateRawId(getNamespaceId(namespaces), type, id), // the namespace prefix is only used for single-namespace object types
_index: this.getIndexForType(type),
_source: { includes: includedFields(type, fields) },
}));
Expand Down
36 changes: 12 additions & 24 deletions x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,25 +165,19 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';
{ type: 'foo', id: '1', key: 'val' },
{ type: 'bar', id: '2', key: 'val' },
{ type: 'baz', id: '3', key: 'val' }, // this should be replaced with a 400 error
{ type: 'foo', id: '4', error: { statusCode: 403 } },
{ type: 'bar', id: '5', error: { statusCode: 403 } },
{ type: 'baz', id: '6', error: { statusCode: 403 } }, // this should be not be replaced with a 400 error because it has a 403 error which is higher priority
{ type: 'foo', id: '7', key: 'val' },
{ type: 'bar', id: '8', key: 'val' },
{ type: 'baz', id: '9', key: 'val' }, // this should not be replaced with a 400 error because the user did not search for it in '*' all spaces
{ type: 'foo', id: '4', key: 'val' },
{ type: 'bar', id: '5', key: 'val' },
{ type: 'baz', id: '6', key: 'val' }, // this should not be replaced with a 400 error because the user did not search for it in '*' all spaces
] as unknown) as SavedObject[],
});

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

Expand All @@ -197,25 +191,19 @@ const ERROR_NAMESPACE_SPECIFIED = 'Spaces currently determines the namespaces';
'"namespaces" can only specify a single space when used with space-isolated types'
).output.payload,
},
{ type: 'foo', id: '4', error: { statusCode: 403 } },
{ type: 'bar', id: '5', error: { statusCode: 403 } },
{ type: 'baz', id: '6', error: { statusCode: 403 } },
{ type: 'foo', id: '7', key: 'val' },
{ type: 'bar', id: '8', key: 'val' },
{ type: 'baz', id: '9', key: 'val' },
{ type: 'foo', id: '4', key: 'val' },
{ type: 'bar', id: '5', key: 'val' },
{ type: 'baz', id: '6', key: 'val' },
]);
expect(baseClient.bulkGet).toHaveBeenCalledWith(
[
{ type: 'foo', id: '1', namespaces: ['available-space-a', 'available-space-b'] },
{ type: 'bar', id: '2', namespaces: ['available-space-a', 'available-space-b'] },
{ type: 'baz', id: '3', namespaces: ['available-space-a', 'available-space-b'] },
{ type: 'foo', id: '4', namespaces: ['available-space-a', 'available-space-b'] },
{ type: 'bar', id: '5', namespaces: ['available-space-a', 'available-space-b'] },
{ type: 'baz', id: '6', namespaces: ['available-space-a', 'available-space-b'] },
// even if another space doesn't exist, it can be specified explicitly
{ type: 'foo', id: '7', namespaces: ['another-space'] },
{ type: 'bar', id: '8', namespaces: ['another-space'] },
{ type: 'baz', id: '9', namespaces: ['another-space'] },
{ type: 'foo', id: '4', namespaces: ['another-space'] },
{ type: 'bar', id: '5', namespaces: ['another-space'] },
{ type: 'baz', id: '6', namespaces: ['another-space'] },
],
{ namespace: currentSpace.expectedNamespace }
);
Expand Down
5 changes: 2 additions & 3 deletions x-pack/plugins/spaces/server/saved_objects/spaces_saved_objects_client.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -262,9 +262,8 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
? 'Left'
: 'Right';
return { tag, value: { ...object, namespaces: await getAvailableSpaces() } };
} else {
return { tag: 'Right', value: object };
}
return { tag: 'Right', value: object };
})
);

Expand All @@ -278,7 +277,7 @@ export class SpacesSavedObjectsClient implements SavedObjectsClientContract {
return {
saved_objects: expectedResults.map((expectedResult, i) => {
const actualResult = responseObjects[i];
if (isLeft(expectedResult) && actualResult?.error?.statusCode !== 403) {
if (isLeft(expectedResult)) {
const { type, id } = expectedResult.value;
return ({
type,
Expand Down
34 changes: 17 additions & 17 deletions x-pack/test/saved_object_api_integration/security_and_spaces/apis/bulk_get.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,28 +82,28 @@ export default function ({ getService }: FtrProviderContext) {
createTestDefinitions(normalTypes, false, { singleRequest: true }),
createTestDefinitions(hiddenType, true),
].flat();
const crossNamespaceAuthorizedAtSpace = crossNamespace.reduce<{
authorized: BulkGetTestCase[];
unauthorized: BulkGetTestCase[];
}>(
({ authorized, unauthorized }, cur) => {
// A user who is only authorized in a single space will be authorized to execute some of the cross-namespace test cases, but not all
if (cur.namespaces.some((x) => ![ALL_SPACES_ID, spaceId].includes(x))) {
return { authorized, unauthorized: [...unauthorized, cur] };
}
return { authorized: [...authorized, cur], unauthorized };
},
{ authorized: [], unauthorized: [] }
);

return {
unauthorized: createTestDefinitions(allTypes, true),
authorizedAtSpace: [
authorizedCommon,
...(() => {
const [authorized, unauthorized] = crossNamespace.reduce<
[BulkGetTestCase[], BulkGetTestCase[]]
>(
([left, right], cur) => {
if (cur.namespaces.some((x) => ![ALL_SPACES_ID, spaceId].includes(x))) {
return [left, [...right, cur]];
}
return [[...left, cur], right];
},
[[], []]
);
return [
...createTestDefinitions(authorized, false, { singleRequest: true }),
...createTestDefinitions(unauthorized, true),
];
})(),
createTestDefinitions(crossNamespaceAuthorizedAtSpace.authorized, false, {
singleRequest: true,
}),
createTestDefinitions(crossNamespaceAuthorizedAtSpace.unauthorized, true),
createTestDefinitions(allTypes, true, { singleRequest: true }),
].flat(),
authorizedEverywhere: [
Expand Down

0 comments on commit fab5c21

Please sign in to comment.