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

[Connection Details] Check if user has permissions to manage own API keys #183286

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
7c80f73
expose ability to check for security key privileges
vadimkibana May 13, 2024
0051b6b
wire in security key privilege check into conn details flyout
vadimkibana May 13, 2024
04d5865
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine May 13, 2024
e491f22
handle case when fetching permissions is forbidden
vadimkibana May 13, 2024
d95075d
Merge remote-tracking branch 'origin/connection-details-permissions' …
vadimkibana May 13, 2024
2f163c1
Merge branch 'main' into connection-details-permissions
kibanamachine May 15, 2024
ee4258c
Merge remote-tracking branch 'upstream/main' into connection-details-…
vadimkibana May 16, 2024
63aa379
remove import from security plugin
vadimkibana May 16, 2024
b8303ab
import security type from public
vadimkibana May 16, 2024
e9e4942
Merge branch 'main' into connection-details-permissions
kibanamachine May 16, 2024
a54b758
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine May 16, 2024
114db76
add new api method mocks
vadimkibana May 16, 2024
e99527b
update test snapshots
vadimkibana May 17, 2024
5fb7a0d
Merge branch 'main' into connection-details-permissions
kibanamachine May 17, 2024
9b66af5
Update packages/cloud/connection_details/kibana/kibana_connection_det…
vadimkibana May 17, 2024
e9a7b1d
Revert "Update packages/cloud/connection_details/kibana/kibana_connec…
vadimkibana May 17, 2024
7141325
add permissions check test
vadimkibana May 17, 2024
da49c22
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine May 17, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { coreMock } from '@kbn/core/public/mocks';
import { createOpts } from './kibana_connection_details_provider';

describe('createOpts()', () => {
it('allows editing API keys, when user can manage own API keys', async () => {
const props = await createOpts({
start: {
core: coreMock.createStart(),
plugins: {
security: {
authz: {
getCurrentUserApiKeyPrivileges: jest
.fn()
.mockResolvedValue({ canManageOwnApiKeys: true }),
},
} as any,
},
},
});
const hasPermission = await props.apiKeys!.hasPermission();

expect(hasPermission).toBe(true);
});

it('does not allow editing API keys, when user can manage own API keys', async () => {
const props = await createOpts({
start: {
core: coreMock.createStart(),
plugins: {
security: {
authz: {
getCurrentUserApiKeyPrivileges: jest
.fn()
.mockResolvedValue({ canManageOwnApiKeys: false }),
},
} as any,
},
},
});
const hasPermission = await props.apiKeys!.hasPermission();

expect(hasPermission).toBe(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,18 @@ import type { CoreStart } from '@kbn/core-lifecycle-browser';
import type { CloudStart } from '@kbn/cloud-plugin/public';
import type { SharePluginStart } from '@kbn/share-plugin/public';
import type { CreateAPIKeyParams, CreateAPIKeyResult } from '@kbn/security-plugin-types-server';
import type { SecurityPluginStart } from '@kbn/security-plugin-types-public';
import { ConnectionDetailsOptsProvider } from '../context';
import { ConnectionDetailsOpts } from '../types';
import { useAsyncMemo } from '../hooks/use_async_memo';

const createOpts = async (props: KibanaConnectionDetailsProviderProps) => {
export const createOpts = async (props: KibanaConnectionDetailsProviderProps) => {
const { options, start } = props;
const { http, docLinks } = start.core;
const locator = start.plugins?.share?.url?.locators.get('MANAGEMENT_APP_LOCATOR');
const manageKeysLink = await locator?.getUrl({ sectionId: 'security', appId: 'api_keys' });
const security = start.plugins?.security;

const result: ConnectionDetailsOpts = {
...options,
navigateToUrl: start.core.application
Expand Down Expand Up @@ -65,7 +68,18 @@ const createOpts = async (props: KibanaConnectionDetailsProviderProps) => {
},
};
},
hasPermission: async () => true,
hasPermission: security
vadimkibana marked this conversation as resolved.
Show resolved Hide resolved
? async () => {
try {
return (await security.authz.getCurrentUserApiKeyPrivileges()).canManageOwnApiKeys;
} catch (error) {
if (error instanceof Error && error.message === 'Forbidden') {
vadimkibana marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
throw error;
}
}
: async () => true,
...options?.apiKeys,
},
};
Expand All @@ -87,6 +101,7 @@ export interface KibanaConnectionDetailsProviderProps {
plugins?: {
cloud?: CloudStart;
share?: SharePluginStart;
security?: SecurityPluginStart;
};
};
}
Expand Down
2 changes: 2 additions & 0 deletions packages/cloud/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,7 @@
"@kbn/cloud-plugin",
"@kbn/share-plugin",
"@kbn/security-plugin-types-server",
"@kbn/security-plugin-types-public",
"@kbn/core",
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not import from core's "plugin entry point" from package code. Imports from @kbn/core/server|public should be replaced with imports from core's corresponding domain packages instead.

Looking at the changes in this package, it seems this was added because of this import:

import { coreMock } from '@kbn/core/public/mocks';

used for

coreMock.createStart(),

Please use

import { coreLifecycleMock } from '@kbn/core-lifecycle-browser-mocks';

//...

coreLifecycleMock.createCoreStart()

instead

]
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ export interface AuthorizationServiceSetup {
* Determines if role management is enabled.
*/
isRoleManagementEnabled: () => boolean | undefined;

/**
* Retrieve current user's API key privileges.
*/
getCurrentUserApiKeyPrivileges: () => Promise<AuthorizationCurrentUserApiKeyPrivilegesResponse>;
}

export interface AuthorizationCurrentUserApiKeyPrivilegesResponse {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use GetAPIKeysResult interface here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code is in a package, but GetAPIKeysResult is the server-side plugin. I'm thinking this would create a circular dependency problem, no?

The GetAPIKeysResult would need to be moved to shared package to be reused? Happy to move it somewhere, if you could please tell me what is the right place in the security code.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have plugin_types_common, but I would probably ask for second opinion also before moving things around.
cc @jeramysoucy

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the ping! I added my feedback here

canManageApiKeys: boolean;
canManageCrossClusterApiKeys: boolean;
canManageOwnApiKeys: boolean;
}

/**
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security/public/authentication/index.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,10 @@ export const authenticationMock = {
export const authorizationMock = {
createSetup: (): jest.Mocked<AuthorizationServiceSetup> => ({
isRoleManagementEnabled: jest.fn(),
getCurrentUserApiKeyPrivileges: jest.fn(),
}),
createStart: (): jest.Mocked<AuthorizationServiceStart> => ({
isRoleManagementEnabled: jest.fn(),
getCurrentUserApiKeyPrivileges: jest.fn(),
}),
};
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,30 @@
* 2.0.
*/

import type { HttpSetup } from '@kbn/core/public';
import type { AuthorizationServiceSetup } from '@kbn/security-plugin-types-public';
import type { AuthorizationCurrentUserApiKeyPrivilegesResponse } from '@kbn/security-plugin-types-public/src/authorization/authorization_service';

import type { ConfigType } from '../config';

interface SetupParams {
config: ConfigType;
http: HttpSetup;
}

export class AuthorizationService {
public setup({ config }: SetupParams): AuthorizationServiceSetup {
public setup({ config, http }: SetupParams): AuthorizationServiceSetup {
const isRoleManagementEnabled = () => config.roleManagementEnabled;

return { isRoleManagementEnabled };
const getCurrentUserApiKeyPrivileges = async () => {
const { canManageApiKeys, canManageCrossClusterApiKeys, canManageOwnApiKeys } =
(await http.get(
'/internal/security/api_key'
)) as AuthorizationCurrentUserApiKeyPrivilegesResponse;

return { canManageApiKeys, canManageCrossClusterApiKeys, canManageOwnApiKeys };
};

return { isRoleManagementEnabled, getCurrentUserApiKeyPrivileges };
Comment on lines +23 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

GET /internal/security/api_key returns all API keys, which if there are many could make this slow. Additionally, this endpoint is being dropped and replaced with a query endpoint. See #168970

I'd suggest instead augmenting our authentication service to include the functionality that you want. This is where we already expose API key functions. See

You could add a getCurrentUserApiKeyPrivileges function in the authentication service that calls a new internal endpoint that ONLY returns the three flags you are looking for (canManageApiKeys, canManageCrossClusterApiKeys, canManageOwnApiKeys), e.g. GET /internal/security/api_key/_check_privileges

Additionally, this would be exposed on the server side as well here:

}
}
6 changes: 5 additions & 1 deletion x-pack/plugins/security/public/plugin.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ describe('Security Plugin', () => {
})
).toEqual({
authc: { getCurrentUser: expect.any(Function), areAPIKeysEnabled: expect.any(Function) },
authz: { isRoleManagementEnabled: expect.any(Function) },
authz: {
isRoleManagementEnabled: expect.any(Function),
getCurrentUserApiKeyPrivileges: expect.any(Function),
},
license: {
isLicenseAvailable: expect.any(Function),
isEnabled: expect.any(Function),
Expand Down Expand Up @@ -126,6 +129,7 @@ describe('Security Plugin', () => {
"getCurrentUser": [Function],
},
"authz": Object {
"getCurrentUserApiKeyPrivileges": [Function],
"isRoleManagementEnabled": [Function],
},
"navControlService": Object {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/public/plugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,7 @@ export class SecurityPlugin

this.authz = this.authorizationService.setup({
config: this.config,
http: core.http,
});

this.securityApiClients = {
Expand Down