Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,15 @@ export const performFind = async <T = unknown, A = unknown>(
// If the user is unauthorized to find *anything* they requested, return an empty response
return SavedObjectsUtils.createEmptyFindResponse<T, A>(options);
}
if (authorizationResult?.status === 'partially_authorized') {
if (
authorizationResult?.status === 'fully_authorized' ||
Copy link
Member Author

@afharo afharo Dec 3, 2025

Choose a reason for hiding this comment

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

This only ensures the same path is followed for both authorization levels, although, functionally, it achieves the same result if we don't add this clause in the if.

Happy to remove this line if we think that performance might be a problem.

I'd appreciate @azasypkin's thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be more defensive to express this as if (authorizationResult?.status) {?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like @elastic/kibana-security's input to help me answer that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing some other context here, but if you are concerned with having the overhead of additional conditional checks, then this can just check that authorizationResult is not undefined or that authorizationResult.status is not unauthorized. Though, just before this we're returning if unauthorized. By this point in the execution path, the status must be either partially or fully authorized.

Copy link
Member Author

Choose a reason for hiding this comment

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

Apologies for not being clear: what I meant is that, in the current code, the fully_authorized doesn't go through this path, so typeToNamespacesMap is not defined, and only allowedTypes is used.

Maybe it's not worth spending CPU cycles to calculate the intersection of the authorized and the allowed types because the intersection will always equal allowedTypes.

I only added it because it made the code consistent before the fix. But happy to revert this change if we think that the extra CPU cycles are not necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. Thanks for clarifying. You are correct, and I think it is fine to skip this logic for fully_authorized if there is any concern about performance here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a code comment for future us to know that's safe to be removed.

I left it for consistency, and because, while there are more CPU cycles used... I'd expect that, typically, users are partially_authorized, so we're just removing that branch for a low percentage of the users.

authorizationResult?.status === 'partially_authorized'
) {
Comment on lines +173 to +176
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (
authorizationResult?.status === 'fully_authorized' ||
authorizationResult?.status === 'partially_authorized'
) {
if (authorizationResult !== undefined) {
typeToNamespacesMap = new Map<string, string[]>();
for (const [objType, entry] of authorizationResult.typeMap) {
if (!entry.find) continue;
// Discard the types that the SO repository doesn't know about (typically hidden objects).
if (!allowedTypes.includes(objType)) continue;
Comment on lines +180 to +181
Copy link
Member Author

Choose a reason for hiding this comment

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

Mixing the user's authorization and the client's scope here (which can be odd).

However, typeToNamespacesMap doesn't really imply that it's bound to only one.

Copy link
Contributor

@jloleysens jloleysens Dec 5, 2025

Choose a reason for hiding this comment

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

Part of the issue/complexity for me is that we have types and typeToNamespacesMap and these can get out-of-sync. In that context, code like this looks a bit weird to me:

typeToNamespacesMap ? Array.from(typeToNamespacesMap.keys()) : type

Shouldn't our type: string | string[] | undefined be the source of truth for the types we are building queries for? Would another fix be to just use type as types when building our query (just on that line Iinked)? Or does that break something else?

Copy link
Member Author

Choose a reason for hiding this comment

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

IIUC, this would break the authorization part: if we only look at the requested type, users who don't have access to the type will be able to read it. Also, in the line that you shared, type can be undefined, leading to getTypes to return all registered types (no matter if the user has access or not).

The bug here is that type is "cleaned up" from the hidden SOs that the SO Client isn't allowed to read, but we don't apply such filter to typeToNamespacesMap.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for taking a closer look @afharo ! We also have a comment talking directly to the issue I expressed:

      typeToNamespacesMap, // If defined, this takes precedence over the `type` and `namespaces` fields

Still a bit odd, but I think that's OK!

// This ensures that the query DSL can filter only for object types that the user is authorized to access for a given space
const { authorizedSpaces, isGloballyAuthorized } = entry.find;
typeToNamespacesMap.set(objType, isGloballyAuthorized ? namespaces : authorizedSpaces);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ import {
generateIndexPatternSearchResults,
setupAuthorizeFunc,
setupAuthorizeFind,
HIDDEN_TYPE,
} from '../test_helpers/repository.test.common';
import { savedObjectsExtensionsMock } from '../mocks/saved_objects_extensions.mock';
import { arrayMapsAreEqual } from '@kbn/core-saved-objects-utils-server';
Expand Down Expand Up @@ -845,14 +846,108 @@ describe('SavedObjectsRepository Security Extension', () => {
});
});

test(`passes an empty type map to the query when partially authorized but no types are authorized for find or are hidden`, async () => {
setupAuthorizeFind(mockSecurityExt, 'partially_authorized');
setupRedactPassthrough(mockSecurityExt);

await findSuccess(
client,
repository,
{
type: [type, NAMESPACE_AGNOSTIC_TYPE, HIDDEN_TYPE],
namespaces: [namespace, 'ns-1'],
}, // include multiple types and spaces
namespace
);

expect(mockGetSearchDsl.mock.calls[0].length).toBe(3); // Find success verifies this is called once, this should always pass
const {
typeToNamespacesMap: actualMap,
}: { typeToNamespacesMap: Map<string, string[] | undefined> } =
mockGetSearchDsl.mock.calls[0][2];

expect(actualMap).not.toBeUndefined();
const expectedMap = new Map<string, string[] | undefined>();

expect(arrayMapsAreEqual(actualMap, expectedMap)).toBeTruthy();
});

test(`passes an empty type map to the query when fully authorized but no types are authorized for find or are hidden`, async () => {
setupAuthorizeFind(mockSecurityExt, 'fully_authorized');
setupRedactPassthrough(mockSecurityExt);

await findSuccess(
client,
repository,
{
type: [type, NAMESPACE_AGNOSTIC_TYPE, HIDDEN_TYPE],
namespaces: [namespace, 'ns-1'],
}, // include multiple types and spaces
namespace
);

expect(mockGetSearchDsl.mock.calls[0].length).toBe(3); // Find success verifies this is called once, this should always pass
const {
typeToNamespacesMap: actualMap,
}: { typeToNamespacesMap: Map<string, string[] | undefined> } =
mockGetSearchDsl.mock.calls[0][2];

expect(actualMap).not.toBeUndefined();
const expectedMap = new Map<string, string[] | undefined>();

expect(arrayMapsAreEqual(actualMap, expectedMap)).toBeTruthy();
});

test(`uses the authorization map when partially authorized`, async () => {
setupAuthorizeFind(mockSecurityExt, 'partially_authorized');
setupRedactPassthrough(mockSecurityExt);

await findSuccess(
client,
repository,
{ type: [type, NAMESPACE_AGNOSTIC_TYPE], namespaces: [namespace, 'ns-1'] }, // include multiple types and spaces
{
type: [
type,
'foo',
Copy link
Member Author

Choose a reason for hiding this comment

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

I had to add foo to the requested type so that it is kept in the auth map.

// Explicitly request the hidden type despite the repository not having access to it to confirm that it's not authorized.
HIDDEN_TYPE,
NAMESPACE_AGNOSTIC_TYPE,
],
namespaces: [namespace, 'ns-1'],
}, // include multiple types and spaces
namespace
);

expect(mockGetSearchDsl.mock.calls[0].length).toBe(3); // Find success verifies this is called once, this should always pass
const {
typeToNamespacesMap: actualMap,
}: { typeToNamespacesMap: Map<string, string[] | undefined> } =
mockGetSearchDsl.mock.calls[0][2];

expect(actualMap).not.toBeUndefined();
const expectedMap = new Map<string, string[] | undefined>();
expectedMap.set('foo', ['bar']); // this is what is hard-coded in authMap

expect(arrayMapsAreEqual(actualMap, expectedMap)).toBeTruthy();
});

test(`uses the authorization map when fully authorized`, async () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

just repeating the test above, but for fully authorized.

setupAuthorizeFind(mockSecurityExt, 'fully_authorized');
setupRedactPassthrough(mockSecurityExt);

await findSuccess(
client,
repository,
{
type: [
type,
'foo',
// Explicitly request the hidden type despite the repository not having access to it to confirm that it's not authorized.
HIDDEN_TYPE,
NAMESPACE_AGNOSTIC_TYPE,
],
namespaces: [namespace, 'ns-1'],
}, // include multiple types and spaces
namespace
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,11 @@ export interface TypeIdTuple {

export const mappings: SavedObjectsTypeMappingDefinition = {
properties: {
foo: {
properties: {
// No fields indexed
},
},
config: {
properties: {
otherField: {
Expand Down Expand Up @@ -242,7 +247,13 @@ export const mappings: SavedObjectsTypeMappingDefinition = {
export const authRecord: Record<string, AuthorizationTypeEntry> = {
find: { authorizedSpaces: ['bar'] },
};
export const authMap = Object.freeze(new Map([['foo', authRecord]]));
export const authMap = Object.freeze(
new Map([
['foo', authRecord],
// The user is authorized to read hidden types but tests will confirm that repositories without that hidden type listed, won't show this as authorized.
[HIDDEN_TYPE, authRecord],
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

])
);

export const checkAuthError = SavedObjectsErrorHelpers.createBadRequestError(
'Failed to check authorization'
Expand Down Expand Up @@ -353,6 +364,7 @@ export const createType = (

export const createRegistry = () => {
const registry = new SavedObjectTypeRegistry();
registry.registerType(createType('foo'));
registry.registerType(createType('config'));
registry.registerType(createType('index-pattern'));
registry.registerType(
Expand Down