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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding authc.areAPIKeysEnabled which uses _xpack/usage #55255

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

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 @@ -16,6 +16,7 @@ const getCurrentUserThrow = jest.fn().mockImplementation(() => {
export const authenticationMock = {
create: (): jest.Mocked<Authentication> => ({
login: jest.fn(),
areAPIKeysEnabled: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser,
invalidateAPIKey: jest.fn(),
Expand All @@ -25,6 +26,7 @@ export const authenticationMock = {
}),
createInvalid: (): jest.Mocked<Authentication> => ({
login: jest.fn(),
areAPIKeysEnabled: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser: getCurrentUserThrow,
invalidateAPIKey: jest.fn(),
Expand Down
53 changes: 53 additions & 0 deletions x-pack/plugins/security/server/authentication/api_keys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,4 +142,57 @@ describe('API Keys', () => {
);
});
});

describe('areEnabled()', () => {
it('returns false when security feature is disabled', async () => {
mockLicense.isEnabled.mockReturnValue(false);
const result = await apiKeys.areEnabled();
expect(result).toBe(false);
expect(mockClusterClient.callAsInternalUser).not.toHaveBeenCalled();
});

describe('security feature enabled', () => {
it(`returns true when _xpack/usage responds with security.api_key_service.enabled: true`, async () => {
mockLicense.isEnabled.mockReturnValue(true);
mockClusterClient.callAsInternalUser.mockResolvedValueOnce({
security: {
api_key_service: {
enabled: true,
},
},
});
const result = await apiKeys.areEnabled();
expect(result).toBe(true);
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledWith('transport.request', {
method: 'GET',
path: '/_xpack/usage',
});
});

it(`returns false when _xpack/usage responds with security.api_key_service.enabled: false`, async () => {
mockLicense.isEnabled.mockReturnValue(true);
mockClusterClient.callAsInternalUser.mockResolvedValueOnce({
security: {
api_key_service: {
enabled: false,
},
},
});
const result = await apiKeys.areEnabled();
expect(result).toBe(false);
expect(mockClusterClient.callAsInternalUser).toHaveBeenCalledWith('transport.request', {
method: 'GET',
path: '/_xpack/usage',
});
});

it(`rejects promise when _xpack/usage throws an error`, async () => {
mockLicense.isEnabled.mockReturnValue(true);
const testError = new Error('something happened');
mockClusterClient.callAsInternalUser.mockRejectedValueOnce(testError);

await expect(apiKeys.areEnabled()).rejects.toEqual(testError);
});
});
});
});
15 changes: 15 additions & 0 deletions x-pack/plugins/security/server/authentication/api_keys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -164,4 +164,19 @@ export class APIKeys {

return result;
}

async areEnabled(): Promise<boolean> {
Copy link
Member

Choose a reason for hiding this comment

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

nit: I believe : Promise<boolean> isn't necessary and should be automatically inferred

if (!this.license.isEnabled()) {
return false;
}

// `transport.request` is potentially unsafe when combined with untrusted user input.
// Do not augment with such input.
const result = await this.clusterClient.callAsInternalUser('transport.request', {
method: 'GET',
path: '/_xpack/usage',
});

return result.security?.api_key_service?.enabled === true;
Copy link
Member

Choose a reason for hiding this comment

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

note: the only thing that worries me a bit here is that we don't have a proper ES documentation on the format of this response and it's not clear what BWC guarantees this API has. If format changes this line will "silently" start always returning false.

Ideally I'd have an API integration test (e.g. via plugin-fixture that depends on security plugin, luckily we can have NP plugin fixtures now), but it seems we don't have any integration tests for the API keys functionality so it's not a blocker for this PR, up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great point, let me add API integrations tests specifically for this.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { Authentication } from '.';
export const authenticationMock = {
create: (): jest.Mocked<Authentication> => ({
login: jest.fn(),
areAPIKeysEnabled: jest.fn(),
createAPIKey: jest.fn(),
getCurrentUser: jest.fn(),
invalidateAPIKey: jest.fn(),
Expand Down
15 changes: 15 additions & 0 deletions x-pack/plugins/security/server/authentication/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,21 @@ describe('setupAuthentication()', () => {
});
});

describe('apiAPIKeysEnabled', () => {
let areAPIKeysEnabled: () => Promise<boolean>;
beforeEach(async () => {
areAPIKeysEnabled = (await setupAuthentication(mockSetupAuthenticationParams))
.areAPIKeysEnabled;
});

it('calls apiKeys.areEnabled', async () => {
const apiKeysInstance = jest.requireMock('./api_keys').APIKeys.mock.instances[0];
apiKeysInstance.areEnabled.mockResolvedValueOnce(true);
await expect(areAPIKeysEnabled()).resolves.toEqual(true);
expect(apiKeysInstance.areEnabled).toHaveBeenCalled();
});
});

describe('createAPIKey()', () => {
let createAPIKey: (
request: KibanaRequest,
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/server/authentication/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ export async function setupAuthentication({
logout: authenticator.logout.bind(authenticator),
getSessionInfo: authenticator.getSessionInfo.bind(authenticator),
getCurrentUser,
areAPIKeysEnabled: () => apiKeys.areEnabled(),
createAPIKey: (request: KibanaRequest, params: CreateAPIKeyParams) =>
apiKeys.create(request, params),
invalidateAPIKey: (request: KibanaRequest, params: InvalidateAPIKeyParams) =>
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security/server/plugin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ describe('Security Plugin', () => {
"registerPrivilegesWithCluster": [Function],
},
"authc": Object {
"areAPIKeysEnabled": [Function],
"createAPIKey": [Function],
"getCurrentUser": [Function],
"getSessionInfo": [Function],
Expand Down
160 changes: 69 additions & 91 deletions x-pack/plugins/security/server/routes/api_keys/privileges.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,25 +14,35 @@ import { defineCheckPrivilegesRoutes } from './privileges';

interface TestOptions {
licenseCheckResult?: LicenseCheck;
apiResponses?: Array<() => Promise<unknown>>;
asserts: { statusCode: number; result?: Record<string, any>; apiArguments?: unknown[][] };
hasPrivilegesImpl?: () => Promise<unknown>;
areAPIKeysEnabledImpl?: () => Promise<boolean>;
asserts: {
statusCode: number;
result?: Record<string, any>;
};
}

describe('Check API keys privileges', () => {
const getPrivilegesTest = (
description: string,
{
licenseCheckResult = { state: LICENSE_CHECK_STATE.Valid },
apiResponses = [],
hasPrivilegesImpl,
areAPIKeysEnabledImpl,
asserts,
}: TestOptions
) => {
test(description, async () => {
const mockRouteDefinitionParams = routeDefinitionParamsMock.create();
const mockScopedClusterClient = elasticsearchServiceMock.createScopedClusterClient();
mockRouteDefinitionParams.clusterClient.asScoped.mockReturnValue(mockScopedClusterClient);
for (const apiResponse of apiResponses) {
mockScopedClusterClient.callAsCurrentUser.mockImplementationOnce(apiResponse);
if (hasPrivilegesImpl) {
mockScopedClusterClient.callAsCurrentUser.mockImplementationOnce(hasPrivilegesImpl);
}
if (areAPIKeysEnabledImpl) {
Copy link
Member

Choose a reason for hiding this comment

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

optional nit: new line between if's

mockRouteDefinitionParams.authc.areAPIKeysEnabled.mockImplementationOnce(
areAPIKeysEnabledImpl
);
}

defineCheckPrivilegesRoutes(mockRouteDefinitionParams);
Expand All @@ -52,17 +62,22 @@ describe('Check API keys privileges', () => {
expect(response.status).toBe(asserts.statusCode);
expect(response.payload).toEqual(asserts.result);

if (Array.isArray(asserts.apiArguments)) {
for (const apiArguments of asserts.apiArguments) {
expect(mockRouteDefinitionParams.clusterClient.asScoped).toHaveBeenCalledWith(
mockRequest
);
expect(mockScopedClusterClient.callAsCurrentUser).toHaveBeenCalledWith(...apiArguments);
}
expect(mockContext.licensing.license.check).toHaveBeenCalledWith('security', 'basic');
if (hasPrivilegesImpl) {
expect(mockScopedClusterClient.callAsCurrentUser).toHaveBeenCalledWith(
'shield.hasPrivileges',
{
body: { cluster: ['manage_security', 'manage_api_key'] },
}
);
} else {
expect(mockScopedClusterClient.callAsCurrentUser).not.toHaveBeenCalled();
}
expect(mockContext.licensing.license.check).toHaveBeenCalledWith('security', 'basic');
if (areAPIKeysEnabledImpl) {
expect(mockRouteDefinitionParams.authc.areAPIKeysEnabled).toHaveBeenCalled();
} else {
expect(mockRouteDefinitionParams.authc.areAPIKeysEnabled).not.toHaveBeenCalled();
}
});
};

Expand All @@ -73,18 +88,28 @@ describe('Check API keys privileges', () => {
});

const error = Boom.notAcceptable('test not acceptable message');
getPrivilegesTest('returns error from cluster client', {
apiResponses: [
async () => {
throw error;
getPrivilegesTest('returns error from hasPrivilegesImpl', {
hasPrivilegesImpl: async () => {
throw error;
},
areAPIKeysEnabledImpl: async () => true,
asserts: {
statusCode: 406,
result: error,
},
});

getPrivilegesTest('returns error from areAPIKeysEnabled', {
hasPrivilegesImpl: async () => ({
cluster: {
manage_security: true,
manage_api_key: true,
},
async () => {},
],
}),
areAPIKeysEnabledImpl: async () => {
throw error;
},
asserts: {
apiArguments: [
['shield.hasPrivileges', { body: { cluster: ['manage_security', 'manage_api_key'] } }],
['shield.getAPIKeys', { owner: true }],
],
statusCode: 406,
result: error,
},
Expand All @@ -93,92 +118,45 @@ describe('Check API keys privileges', () => {

describe('success', () => {
getPrivilegesTest('returns areApiKeysEnabled and isAdmin', {
apiResponses: [
async () => ({
username: 'elastic',
has_all_requested: true,
cluster: { manage_api_key: true, manage_security: true },
index: {},
application: {},
}),
async () => ({
api_keys: [
{
id: 'si8If24B1bKsmSLTAhJV',
name: 'my-api-key',
creation: 1574089261632,
expiration: 1574175661632,
invalidated: false,
username: 'elastic',
realm: 'reserved',
},
],
}),
],
hasPrivilegesImpl: async () => ({
cluster: {
manage_security: true,
manage_api_key: true,
},
}),
areAPIKeysEnabledImpl: async () => true,
asserts: {
apiArguments: [
['shield.getAPIKeys', { owner: true }],
['shield.hasPrivileges', { body: { cluster: ['manage_security', 'manage_api_key'] } }],
],
statusCode: 200,
result: { areApiKeysEnabled: true, isAdmin: true },
},
});

getPrivilegesTest(
'returns areApiKeysEnabled=false when getAPIKeys error message includes "api keys are not enabled"',
'returns areApiKeysEnabled=false when authc.areAPIKeysEnabled returns false"',
{
apiResponses: [
async () => ({
username: 'elastic',
has_all_requested: true,
cluster: { manage_api_key: true, manage_security: true },
index: {},
application: {},
}),
async () => {
throw Boom.unauthorized('api keys are not enabled');
hasPrivilegesImpl: async () => ({
cluster: {
manage_security: true,
manage_api_key: true,
},
],
}),
areAPIKeysEnabledImpl: async () => false,
asserts: {
apiArguments: [
['shield.getAPIKeys', { owner: true }],
['shield.hasPrivileges', { body: { cluster: ['manage_security', 'manage_api_key'] } }],
],
statusCode: 200,
result: { areApiKeysEnabled: false, isAdmin: true },
},
}
);

getPrivilegesTest('returns isAdmin=false when user has insufficient privileges', {
apiResponses: [
async () => ({
username: 'elastic',
has_all_requested: true,
cluster: { manage_api_key: false, manage_security: false },
index: {},
application: {},
}),
async () => ({
api_keys: [
{
id: 'si8If24B1bKsmSLTAhJV',
name: 'my-api-key',
creation: 1574089261632,
expiration: 1574175661632,
invalidated: false,
username: 'elastic',
realm: 'reserved',
},
],
}),
],
hasPrivilegesImpl: async () => ({
cluster: {
manage_security: false,
manage_api_key: false,
},
}),
areAPIKeysEnabledImpl: async () => true,
asserts: {
apiArguments: [
['shield.getAPIKeys', { owner: true }],
['shield.hasPrivileges', { body: { cluster: ['manage_security', 'manage_api_key'] } }],
],
statusCode: 200,
result: { areApiKeysEnabled: true, isAdmin: false },
},
Expand Down
Loading