Skip to content
Merged
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
190 changes: 190 additions & 0 deletions x-pack/plugins/cloud/public/plugin.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
/*
* 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; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { nextTick } from '@kbn/test/jest';
import { coreMock } from 'src/core/public/mocks';
import { homePluginMock } from 'src/plugins/home/public/mocks';
import { securityMock } from '../../security/public/mocks';
import { CloudPlugin } from './plugin';

describe('Cloud Plugin', () => {
describe('#start', () => {
function setupPlugin({
roles = [],
simulateUserError = false,
}: { roles?: string[]; simulateUserError?: boolean } = {}) {
const plugin = new CloudPlugin(
coreMock.createPluginInitializerContext({
id: 'cloudId',
base_url: 'https://cloud.elastic.co',
deployment_url: '/abc123',
profile_url: '/profile/alice',
organization_url: '/org/myOrg',
})
);
const coreSetup = coreMock.createSetup();
const homeSetup = homePluginMock.createSetupContract();
const securitySetup = securityMock.createSetup();
if (simulateUserError) {
securitySetup.authc.getCurrentUser.mockRejectedValue(new Error('Something happened'));
} else {
securitySetup.authc.getCurrentUser.mockResolvedValue(
securityMock.createMockAuthenticatedUser({
roles,
})
);
}

plugin.setup(coreSetup, { home: homeSetup, security: securitySetup });

return { coreSetup, securitySetup, plugin };
}

it('registers help support URL', async () => {
const { plugin } = setupPlugin();

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
plugin.start(coreStart, { security: securityStart });

expect(coreStart.chrome.setHelpSupportUrl).toHaveBeenCalledTimes(1);
expect(coreStart.chrome.setHelpSupportUrl.mock.calls[0]).toMatchInlineSnapshot(`
Array [
"https://support.elastic.co/",
]
`);
});

it('registers a custom nav link for superusers', async () => {
const { plugin } = setupPlugin({ roles: ['superuser'] });

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
plugin.start(coreStart, { security: securityStart });

await nextTick();

expect(coreStart.chrome.setCustomNavLink).toHaveBeenCalledTimes(1);
expect(coreStart.chrome.setCustomNavLink.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"euiIconType": "arrowLeft",
"href": "https://cloud.elastic.co/abc123",
"title": "Manage this deployment",
},
]
`);
});

it('registers a custom nav link when there is an error retrieving the current user', async () => {
const { plugin } = setupPlugin({ simulateUserError: true });

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
plugin.start(coreStart, { security: securityStart });

await nextTick();

expect(coreStart.chrome.setCustomNavLink).toHaveBeenCalledTimes(1);
expect(coreStart.chrome.setCustomNavLink.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Object {
"euiIconType": "arrowLeft",
"href": "https://cloud.elastic.co/abc123",
"title": "Manage this deployment",
},
]
`);
});

it('does not register a custom nav link for non-superusers', async () => {
const { plugin } = setupPlugin({ roles: ['not-a-superuser'] });

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
plugin.start(coreStart, { security: securityStart });

await nextTick();

expect(coreStart.chrome.setCustomNavLink).not.toHaveBeenCalled();
});

it('registers user profile links for superusers', async () => {
const { plugin } = setupPlugin({ roles: ['superuser'] });

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
plugin.start(coreStart, { security: securityStart });

await nextTick();

expect(securityStart.navControlService.addUserMenuLinks).toHaveBeenCalledTimes(1);
expect(securityStart.navControlService.addUserMenuLinks.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Array [
Object {
"href": "https://cloud.elastic.co/profile/alice",
"iconType": "user",
"label": "Profile",
"order": 100,
"setAsProfile": true,
},
Object {
"href": "https://cloud.elastic.co/org/myOrg",
"iconType": "gear",
"label": "Account & Billing",
"order": 200,
},
],
]
`);
});

it('registers profile links when there is an error retrieving the current user', async () => {
const { plugin } = setupPlugin({ simulateUserError: true });

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
plugin.start(coreStart, { security: securityStart });

await nextTick();

expect(securityStart.navControlService.addUserMenuLinks).toHaveBeenCalledTimes(1);
expect(securityStart.navControlService.addUserMenuLinks.mock.calls[0]).toMatchInlineSnapshot(`
Array [
Array [
Object {
"href": "https://cloud.elastic.co/profile/alice",
"iconType": "user",
"label": "Profile",
"order": 100,
"setAsProfile": true,
},
Object {
"href": "https://cloud.elastic.co/org/myOrg",
"iconType": "gear",
"label": "Account & Billing",
"order": 200,
},
],
]
`);
});

it('does not register profile links for non-superusers', async () => {
const { plugin } = setupPlugin({ roles: ['not-a-superuser'] });

const coreStart = coreMock.createStart();
const securityStart = securityMock.createStart();
plugin.start(coreStart, { security: securityStart });

await nextTick();

expect(securityStart.navControlService.addUserMenuLinks).not.toHaveBeenCalled();
});
});
});
64 changes: 49 additions & 15 deletions x-pack/plugins/cloud/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

import { CoreSetup, CoreStart, Plugin, PluginInitializerContext } from 'src/core/public';
import { i18n } from '@kbn/i18n';
import { SecurityPluginStart } from '../../security/public';
import { AuthenticatedUser, SecurityPluginSetup, SecurityPluginStart } from '../../security/public';
import { getIsCloudEnabled } from '../common/is_cloud_enabled';
import { ELASTIC_SUPPORT_LINK } from '../common/constants';
import { HomePublicPluginSetup } from '../../../../src/plugins/home/public';
Expand All @@ -25,6 +25,7 @@ export interface CloudConfigType {

interface CloudSetupDependencies {
home?: HomePublicPluginSetup;
security?: Pick<SecurityPluginSetup, 'authc'>;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NIT: as you're using the whole type in the tests via securityMock anyhow, any reason to use a Pick here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I leave that to Larry, but I personally like the fact that we explicitly outline the part of the dependency contract we really need (too bad we don't have a more expressive syntax to do that). This can make the life much easier if you're changing/fixing the behavior of a certain portion of your public contract and want to quickly know what plugins may be affected without following all the paths where the contract is passed to.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah I did this mostly out of habit for the reasons Aleh mentioned. I don't mind reverting if you'd prefer the original approach

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Na, this is fine

}

interface CloudStartDependencies {
Expand All @@ -44,13 +45,14 @@ export interface CloudSetup {
export class CloudPlugin implements Plugin<CloudSetup> {
private config!: CloudConfigType;
private isCloudEnabled: boolean;
private authenticatedUserPromise?: Promise<AuthenticatedUser | null>;

constructor(private readonly initializerContext: PluginInitializerContext) {
this.config = this.initializerContext.config.get<CloudConfigType>();
this.isCloudEnabled = false;
}

public setup(core: CoreSetup, { home }: CloudSetupDependencies) {
public setup(core: CoreSetup, { home, security }: CloudSetupDependencies) {
const {
id,
cname,
Expand All @@ -68,6 +70,10 @@ export class CloudPlugin implements Plugin<CloudSetup> {
}
}

if (security) {
this.authenticatedUserPromise = security.authc.getCurrentUser().catch(() => null);
}

return {
cloudId: id,
cname,
Expand All @@ -82,19 +88,47 @@ export class CloudPlugin implements Plugin<CloudSetup> {
public start(coreStart: CoreStart, { security }: CloudStartDependencies) {
const { deployment_url: deploymentUrl, base_url: baseUrl } = this.config;
coreStart.chrome.setHelpSupportUrl(ELASTIC_SUPPORT_LINK);
if (baseUrl && deploymentUrl) {
coreStart.chrome.setCustomNavLink({
title: i18n.translate('xpack.cloud.deploymentLinkLabel', {
defaultMessage: 'Manage this deployment',
}),
euiIconType: 'arrowLeft',
href: getFullCloudUrl(baseUrl, deploymentUrl),
});
}

if (security && this.isCloudEnabled) {
const userMenuLinks = createUserMenuLinks(this.config);
security.navControlService.addUserMenuLinks(userMenuLinks);
}
const setLinks = (authorized: boolean) => {
if (!authorized) return;

if (baseUrl && deploymentUrl) {
coreStart.chrome.setCustomNavLink({
title: i18n.translate('xpack.cloud.deploymentLinkLabel', {
defaultMessage: 'Manage this deployment',
}),
euiIconType: 'arrowLeft',
href: getFullCloudUrl(baseUrl, deploymentUrl),
});
}

if (security && this.isCloudEnabled) {
const userMenuLinks = createUserMenuLinks(this.config);
security.navControlService.addUserMenuLinks(userMenuLinks);
}
};

this.checkIfAuthorizedForLinks()
.then(setLinks)
// In the event of an unexpected error, fail *open*.
// Cloud admin console will always perform the actual authorization checks.
.catch(() => setLinks(true));
}

/**
* Determines if the current user should see links back to Cloud.
* This isn't a true authorization check, but rather a heuristic to
* see if the current user is *likely* a cloud deployment administrator.
*
* At this point, we do not have enough information to reliably make this determination,
* but we do know that all cloud deployment admins are superusers by default.
*/
private async checkIfAuthorizedForLinks() {
// Security plugin is disabled
if (!this.authenticatedUserPromise) return true;
// Otherwise check roles. If user is not defined due to an unexpected error, then fail *open*.
// Cloud admin console will always perform the actual authorization checks.
const user = await this.authenticatedUserPromise;
return user?.roles.includes('superuser') ?? true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

question: sorry if I missed that in #97308, but for how long we expect this workaround to exist? If we're not sure, I'm wondering if we may want to leave an escape hatch and expose something like xpack.cloud.roles: schema.arrayOf(schema.string(), { defaultValue: ['superuser'] }) config value, so that in the worst case users could create an empty "marker" role if they need to.... I don't have a strong opinion on that, so feel free to ignore.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This workaround will likely exist for a while - it's really up to the cloud team's roadmap to prioritize an API or similar mechanism for us to leverage.

I think it's premature to add an escape hatch, because we don't know what it would look like just yet. It's possible that we would instead want something under cloud's control, such as inspecting the user's metadata that their SSO realm attaches

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's premature to add an escape hatch, because we don't know what it would look like just yet. It's possible that we would instead want something under cloud's control, such as inspecting the user's metadata that their SSO realm attaches

That makes sense, thanks. Works for me as long as it works for Cloud.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,10 @@ import type { AuthenticatedUser } from './authenticated_user';

// We omit `roles` here since the original interface defines this field as `readonly string[]` that makes it hard to use
// in various mocks that expect mutable string array.
type AuthenticatedUserProps = Partial<Omit<AuthenticatedUser, 'roles'> & { roles: string[] }>;
export function mockAuthenticatedUser(user: AuthenticatedUserProps = {}) {
export type MockAuthenticatedUserProps = Partial<
Omit<AuthenticatedUser, 'roles'> & { roles: string[] }
>;
export function mockAuthenticatedUser(user: MockAuthenticatedUserProps = {}) {
return {
username: 'user',
email: 'email',
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/security/public/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
*/

import { licenseMock } from '../common/licensing/index.mock';
import type { MockAuthenticatedUserProps } from '../common/model/authenticated_user.mock';
import { mockAuthenticatedUser } from '../common/model/authenticated_user.mock';
import { authenticationMock } from './authentication/index.mock';
import { navControlServiceMock } from './nav_control/index.mock';
import { createSessionTimeoutMock } from './session/session_timeout.mock';
Expand All @@ -26,4 +28,6 @@ function createStartMock() {
export const securityMock = {
createSetup: createSetupMock,
createStart: createStartMock,
createMockAuthenticatedUser: (props: MockAuthenticatedUserProps = {}) =>
mockAuthenticatedUser(props),
};
4 changes: 4 additions & 0 deletions x-pack/plugins/security/server/mocks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
import type { ApiResponse } from '@elastic/elasticsearch';

import { licenseMock } from '../common/licensing/index.mock';
import type { MockAuthenticatedUserProps } from '../common/model/authenticated_user.mock';
import { mockAuthenticatedUser } from '../common/model/authenticated_user.mock';
import { auditServiceMock } from './audit/index.mock';
import { authenticationServiceMock } from './authentication/authentication_service.mock';
import { authorizationMock } from './authorization/index.mock';
Expand Down Expand Up @@ -62,4 +64,6 @@ export const securityMock = {
createSetup: createSetupMock,
createStart: createStartMock,
createApiResponse: createApiResponseMock,
createMockAuthenticatedUser: (props: MockAuthenticatedUserProps = {}) =>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't have to expose this from the server mock, but I felt like this was a good way to ensure consistency between the client and server mock contracts

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason why not just createMockAuthenticatedUser: mockAuthenticatedUser? No need to change anything, just curious.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I debated exposing mockAuthenticatedUser, but prefixing it with create was more consistent with the other mock functions (createSetup, createStart, ...), and it felt more correct. When I first imported mockAuthenticatedUser, I assumed based on the name that it was an object with a create property or similar on it (mockAuthenticatedUser.create())

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, I like the createMockAuthenticatedUser name, I was mostly curious if we need this additional arrow function or we can just assign mockAuthenticatedUser driectly to createMockAuthenticatedUser.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah sorry I should have read your comment a little closer 🙈

We don't need the additional arrow function, I added that so my IDE would give me type information as I wrote it out 😬

mockAuthenticatedUser(props),
};