From 4d7c7b55f7df813b69b3a75d78f1bf1234ddf384 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Wed, 5 Feb 2020 10:29:21 +0100 Subject: [PATCH 1/5] Properly handle password change for users authenticated with provider other than `basic`. (#55206) --- .../server/alerts_client_factory.test.ts | 2 +- .../plugins/reporting/server/lib/get_user.ts | 19 +--- .../routes/lib/authorized_user_pre_routing.ts | 2 +- x-pack/legacy/plugins/security/index.js | 2 +- .../common/model/authenticated_user.mock.ts | 1 + .../common/model/authenticated_user.ts | 5 + .../account_management_page.test.tsx | 6 +- .../authentication/authenticator.test.ts | 71 +----------- .../server/authentication/authenticator.ts | 23 ++-- .../server/authentication/index.test.ts | 75 ++++++------- .../security/server/authentication/index.ts | 20 +--- .../server/authentication/providers/base.ts | 17 ++- .../server/authentication/providers/basic.ts | 5 + .../authentication/providers/kerberos.test.ts | 10 +- .../authentication/providers/kerberos.ts | 5 + .../authentication/providers/oidc.test.ts | 6 +- .../server/authentication/providers/oidc.ts | 7 +- .../authentication/providers/pki.test.ts | 12 +- .../server/authentication/providers/pki.ts | 5 + .../authentication/providers/saml.test.ts | 6 +- .../server/authentication/providers/saml.ts | 7 +- .../authentication/providers/token.test.ts | 10 +- .../server/authentication/providers/token.ts | 7 +- .../routes/authentication/common.test.ts | 13 +-- .../server/routes/authentication/common.ts | 8 +- .../routes/users/change_password.test.ts | 106 +++++++++++++++--- .../server/routes/users/change_password.ts | 77 +++++++------ .../apis/security/basic_login.js | 2 + .../apis/security/kerberos_login.ts | 2 + .../apis/authorization_code_flow/oidc_auth.js | 2 + .../apis/implicit_flow/oidc_auth.ts | 1 + .../apis/security/pki_auth.ts | 3 + .../apis/security/saml_login.ts | 2 + 33 files changed, 284 insertions(+), 255 deletions(-) diff --git a/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts b/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts index 754e02a3f1e5e..14c685237bf92 100644 --- a/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts +++ b/x-pack/legacy/plugins/alerting/server/alerts_client_factory.test.ts @@ -86,7 +86,7 @@ test('getUserName() returns a name when security is enabled', async () => { factory.create(KibanaRequest.from(fakeRequest), fakeRequest); const constructorCall = jest.requireMock('./alerts_client').AlertsClient.mock.calls[0][0]; - securityPluginSetup.authc.getCurrentUser.mockResolvedValueOnce({ username: 'bob' }); + securityPluginSetup.authc.getCurrentUser.mockReturnValueOnce({ username: 'bob' }); const userNameResult = await constructorCall.getUserName(); expect(userNameResult).toEqual('bob'); }); diff --git a/x-pack/legacy/plugins/reporting/server/lib/get_user.ts b/x-pack/legacy/plugins/reporting/server/lib/get_user.ts index 350004ddb78f8..ab02dfe0743f0 100644 --- a/x-pack/legacy/plugins/reporting/server/lib/get_user.ts +++ b/x-pack/legacy/plugins/reporting/server/lib/get_user.ts @@ -6,27 +6,14 @@ import { Legacy } from 'kibana'; import { KibanaRequest } from '../../../../../../src/core/server'; -import { Logger, ServerFacade } from '../../types'; +import { ServerFacade } from '../../types'; import { ReportingSetupDeps } from '../plugin'; -export function getUserFactory( - server: ServerFacade, - security: ReportingSetupDeps['security'], - logger: Logger -) { +export function getUserFactory(server: ServerFacade, security: ReportingSetupDeps['security']) { /* * Legacy.Request because this is called from routing middleware */ return async (request: Legacy.Request) => { - if (!security) { - return null; - } - - try { - return await security.authc.getCurrentUser(KibanaRequest.from(request)); - } catch (err) { - logger.error(err, ['getUser']); - return null; - } + return security?.authc.getCurrentUser(KibanaRequest.from(request)) ?? null; }; } diff --git a/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts b/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts index 874027251570c..57c3fcee222da 100644 --- a/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts +++ b/x-pack/legacy/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts @@ -22,7 +22,7 @@ export const authorizedUserPreRoutingFactory = function authorizedUserPreRouting plugins: ReportingSetupDeps, logger: Logger ) { - const getUser = getUserFactory(server, plugins.security, logger); + const getUser = getUserFactory(server, plugins.security); const config = server.config(); return async function authorizedUserPreRouting(request: Legacy.Request) { diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js index 4988c30a1398b..9016398463b5f 100644 --- a/x-pack/legacy/plugins/security/index.js +++ b/x-pack/legacy/plugins/security/index.js @@ -130,7 +130,7 @@ export const security = kibana => ); server.expose({ - getUser: request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)), + getUser: async request => securityPlugin.authc.getCurrentUser(KibanaRequest.from(request)), }); initLoginView(securityPlugin, server); diff --git a/x-pack/plugins/security/common/model/authenticated_user.mock.ts b/x-pack/plugins/security/common/model/authenticated_user.mock.ts index 3a93efc57b5f6..220b284e76591 100644 --- a/x-pack/plugins/security/common/model/authenticated_user.mock.ts +++ b/x-pack/plugins/security/common/model/authenticated_user.mock.ts @@ -15,6 +15,7 @@ export function mockAuthenticatedUser(user: Partial = {}) { enabled: true, authentication_realm: { name: 'native1', type: 'native' }, lookup_realm: { name: 'native1', type: 'native' }, + authentication_provider: 'basic', ...user, }; } diff --git a/x-pack/plugins/security/common/model/authenticated_user.ts b/x-pack/plugins/security/common/model/authenticated_user.ts index 1b660ae919c09..6465b4a23eb48 100644 --- a/x-pack/plugins/security/common/model/authenticated_user.ts +++ b/x-pack/plugins/security/common/model/authenticated_user.ts @@ -26,6 +26,11 @@ export interface AuthenticatedUser extends User { * The name and type of the Realm where the user information were retrieved from. */ lookup_realm: UserRealm; + + /** + * Name of the Kibana authentication provider that used to authenticate user. + */ + authentication_provider: string; } export function canUserChangePassword(user: AuthenticatedUser) { diff --git a/x-pack/plugins/security/public/account_management/account_management_page.test.tsx b/x-pack/plugins/security/public/account_management/account_management_page.test.tsx index b7cf8e6dd1418..4caf3d25e887b 100644 --- a/x-pack/plugins/security/public/account_management/account_management_page.test.tsx +++ b/x-pack/plugins/security/public/account_management/account_management_page.test.tsx @@ -10,6 +10,7 @@ import { AuthenticatedUser } from '../../common/model'; import { AccountManagementPage } from './account_management_page'; import { coreMock } from 'src/core/public/mocks'; +import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock'; import { securityMock } from '../mocks'; import { userAPIClientMock } from '../management/users/index.mock'; @@ -19,11 +20,10 @@ interface Options { realm?: string; } const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }: Options = {}) => { - return { + return mockAuthenticatedUser({ full_name: withFullName ? 'Casey Smith' : '', username: 'csmith', email: withEmail ? 'csmith@domain.com' : '', - enabled: true, roles: [], authentication_realm: { type: realm, @@ -33,7 +33,7 @@ const createUser = ({ withFullName = true, withEmail = true, realm = 'native' }: type: realm, name: realm, }, - }; + }); }; function getSecuritySetupMock({ currentUser }: { currentUser: AuthenticatedUser }) { diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index dd580c890bf94..8be1762133db6 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -jest.mock('./providers/basic', () => ({ BasicAuthenticationProvider: jest.fn() })); +jest.mock('./providers/basic'); import Boom from 'boom'; import { duration, Duration } from 'moment'; @@ -225,75 +225,6 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).not.toHaveBeenCalled(); expect(mockSessionStorage.clear).toHaveBeenCalled(); }); - - describe('stateless login', () => { - it('does not create session even if authentication provider returns state', async () => { - const user = mockAuthenticatedUser(); - const request = httpServerMock.createKibanaRequest(); - const authorization = `Basic ${Buffer.from('foo:bar').toString('base64')}`; - - mockBasicAuthenticationProvider.login.mockResolvedValue( - AuthenticationResult.succeeded(user, { state: { authorization } }) - ); - - const authenticationResult = await authenticator.login(request, { - provider: 'basic', - value: {}, - stateless: true, - }); - expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); - - expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null); - expect(mockSessionStorage.get).not.toHaveBeenCalled(); - expect(mockSessionStorage.set).not.toHaveBeenCalled(); - expect(mockSessionStorage.clear).not.toHaveBeenCalled(); - }); - - it('does not clear session even if provider asked to do so.', async () => { - const user = mockAuthenticatedUser(); - const request = httpServerMock.createKibanaRequest(); - - mockBasicAuthenticationProvider.login.mockResolvedValue( - AuthenticationResult.succeeded(user, { state: null }) - ); - - const authenticationResult = await authenticator.login(request, { - provider: 'basic', - value: {}, - stateless: true, - }); - expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); - - expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null); - expect(mockSessionStorage.get).not.toHaveBeenCalled(); - expect(mockSessionStorage.set).not.toHaveBeenCalled(); - expect(mockSessionStorage.clear).not.toHaveBeenCalled(); - }); - - it('does not clear session even if provider failed with 401.', async () => { - const request = httpServerMock.createKibanaRequest(); - - const failureReason = Boom.unauthorized(); - mockBasicAuthenticationProvider.login.mockResolvedValue( - AuthenticationResult.failed(failureReason) - ); - - const authenticationResult = await authenticator.login(request, { - provider: 'basic', - value: {}, - stateless: true, - }); - expect(authenticationResult.failed()).toBe(true); - expect(authenticationResult.error).toBe(failureReason); - - expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null); - expect(mockSessionStorage.get).not.toHaveBeenCalled(); - expect(mockSessionStorage.set).not.toHaveBeenCalled(); - expect(mockSessionStorage.clear).not.toHaveBeenCalled(); - }); - }); }); describe('`authenticate` method', () => { diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index be952a154cee4..ea7792e902ec1 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -80,13 +80,6 @@ export interface ProviderLoginAttempt { * Login attempt can have any form and defined by the specific provider. */ value: unknown; - - /** - * Indicates whether login attempt should be performed in a "stateless" manner. If `true` provider - * performing login will neither be able to retrieve or update existing state if any nor persist - * any new state it may produce as a result of the login attempt. It's `false` by default. - */ - stateless?: boolean; } export interface AuthenticatorOptions { @@ -107,12 +100,12 @@ const providerMap = new Map< providerSpecificOptions?: AuthenticationProviderSpecificOptions ) => BaseAuthenticationProvider >([ - ['basic', BasicAuthenticationProvider], - ['kerberos', KerberosAuthenticationProvider], - ['saml', SAMLAuthenticationProvider], - ['token', TokenAuthenticationProvider], - ['oidc', OIDCAuthenticationProvider], - ['pki', PKIAuthenticationProvider], + [BasicAuthenticationProvider.type, BasicAuthenticationProvider], + [KerberosAuthenticationProvider.type, KerberosAuthenticationProvider], + [SAMLAuthenticationProvider.type, SAMLAuthenticationProvider], + [TokenAuthenticationProvider.type, TokenAuthenticationProvider], + [OIDCAuthenticationProvider.type, OIDCAuthenticationProvider], + [PKIAuthenticationProvider.type, PKIAuthenticationProvider], ]); function assertRequest(request: KibanaRequest) { @@ -254,7 +247,7 @@ export class Authenticator { // If we detect an existing session that belongs to a different provider than the one requested // to perform a login we should clear such session. - let existingSession = attempt.stateless ? null : await this.getSessionValue(sessionStorage); + let existingSession = await this.getSessionValue(sessionStorage); if (existingSession && existingSession.provider !== attempt.provider) { this.logger.debug( `Clearing existing session of another ("${existingSession.provider}") provider.` @@ -281,7 +274,7 @@ export class Authenticator { (authenticationResult.failed() && getErrorStatusCode(authenticationResult.error) === 401); if (existingSession && shouldClearSession) { sessionStorage.clear(); - } else if (!attempt.stateless && authenticationResult.shouldUpdateState()) { + } else if (authenticationResult.shouldUpdateState()) { const { idleTimeoutExpiration, lifespanExpiration } = this.calculateExpiry(existingSession); sessionStorage.set({ state: authenticationResult.state, diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index 0c1095b56e6e2..d0de6d571b7a0 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -10,7 +10,6 @@ jest.mock('./api_keys'); jest.mock('./authenticator'); import Boom from 'boom'; -import { errors } from 'elasticsearch'; import { first } from 'rxjs/operators'; import { @@ -27,7 +26,6 @@ import { AuthToolkit, IClusterClient, CoreSetup, - ElasticsearchErrorHelpers, KibanaRequest, LoggerFactory, ScopedClusterClient, @@ -289,67 +287,66 @@ describe('setupAuthentication()', () => { }); describe('getCurrentUser()', () => { - let getCurrentUser: (r: KibanaRequest) => Promise; + let getCurrentUser: (r: KibanaRequest) => AuthenticatedUser | null; beforeEach(async () => { getCurrentUser = (await setupAuthentication(mockSetupAuthenticationParams)).getCurrentUser; }); - it('returns `null` if Security is disabled', async () => { + it('returns `null` if Security is disabled', () => { mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false); - await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(null); + expect(getCurrentUser(httpServerMock.createKibanaRequest())).toBe(null); }); - it('fails if `authenticate` call fails', async () => { - const failureReason = new Error('Something went wrong'); - mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); + it('returns user from the auth state.', () => { + const mockUser = mockAuthenticatedUser(); - await expect(getCurrentUser(httpServerMock.createKibanaRequest())).rejects.toBe( - failureReason - ); + const mockAuthGet = mockSetupAuthenticationParams.http.auth.get as jest.Mock; + mockAuthGet.mockReturnValue({ state: mockUser }); + + const mockRequest = httpServerMock.createKibanaRequest(); + expect(getCurrentUser(mockRequest)).toBe(mockUser); + expect(mockAuthGet).toHaveBeenCalledTimes(1); + expect(mockAuthGet).toHaveBeenCalledWith(mockRequest); }); - it('returns result of `authenticate` call.', async () => { - const mockUser = mockAuthenticatedUser(); - mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(mockUser); + it('returns null if auth state is not available.', () => { + const mockAuthGet = mockSetupAuthenticationParams.http.auth.get as jest.Mock; + mockAuthGet.mockReturnValue({}); - await expect(getCurrentUser(httpServerMock.createKibanaRequest())).resolves.toBe(mockUser); + const mockRequest = httpServerMock.createKibanaRequest(); + expect(getCurrentUser(mockRequest)).toBeNull(); + expect(mockAuthGet).toHaveBeenCalledTimes(1); + expect(mockAuthGet).toHaveBeenCalledWith(mockRequest); }); }); describe('isAuthenticated()', () => { - let isAuthenticated: (r: KibanaRequest) => Promise; + let isAuthenticated: (r: KibanaRequest) => boolean; beforeEach(async () => { isAuthenticated = (await setupAuthentication(mockSetupAuthenticationParams)).isAuthenticated; }); - it('returns `true` if Security is disabled', async () => { - mockSetupAuthenticationParams.license.isEnabled.mockReturnValue(false); - - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true); - }); + it('returns `true` if request is authenticated', () => { + const mockIsAuthenticated = mockSetupAuthenticationParams.http.auth + .isAuthenticated as jest.Mock; + mockIsAuthenticated.mockReturnValue(true); - it('returns `true` if `authenticate` succeeds.', async () => { - const mockUser = mockAuthenticatedUser(); - mockScopedClusterClient.callAsCurrentUser.mockResolvedValue(mockUser); - - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(true); - }); - - it('returns `false` if `authenticate` fails with 401.', async () => { - const failureReason = ElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error()); - mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); - - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).resolves.toBe(false); + const mockRequest = httpServerMock.createKibanaRequest(); + expect(isAuthenticated(mockRequest)).toBe(true); + expect(mockIsAuthenticated).toHaveBeenCalledTimes(1); + expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest); }); - it('fails if `authenticate` call fails with unknown reason', async () => { - const failureReason = new errors.BadRequest(); - mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); + it('returns `false` if request is not authenticated', () => { + const mockIsAuthenticated = mockSetupAuthenticationParams.http.auth + .isAuthenticated as jest.Mock; + mockIsAuthenticated.mockReturnValue(false); - await expect(isAuthenticated(httpServerMock.createKibanaRequest())).rejects.toBe( - failureReason - ); + const mockRequest = httpServerMock.createKibanaRequest(); + expect(isAuthenticated(mockRequest)).toBe(false); + expect(mockIsAuthenticated).toHaveBeenCalledTimes(1); + expect(mockIsAuthenticated).toHaveBeenCalledWith(mockRequest); }); }); diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 1002ad7709b80..4b73430ff13c4 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -55,14 +55,12 @@ export async function setupAuthentication({ * Retrieves currently authenticated user associated with the specified request. * @param request */ - const getCurrentUser = async (request: KibanaRequest) => { + const getCurrentUser = (request: KibanaRequest) => { if (!license.isEnabled()) { return null; } - return (await clusterClient - .asScoped(request) - .callAsCurrentUser('shield.authenticate')) as AuthenticatedUser; + return (http.auth.get(request).state ?? null) as AuthenticatedUser | null; }; const isValid = (sessionValue: ProviderSession) => { @@ -180,18 +178,6 @@ export async function setupAuthentication({ apiKeys.create(request, params), invalidateAPIKey: (request: KibanaRequest, params: InvalidateAPIKeyParams) => apiKeys.invalidate(request, params), - isAuthenticated: async (request: KibanaRequest) => { - try { - await getCurrentUser(request); - } catch (err) { - // Don't swallow server errors. - if (getErrorStatusCode(err) !== 401) { - throw err; - } - return false; - } - - return true; - }, + isAuthenticated: (request: KibanaRequest) => http.auth.isAuthenticated(request), }; } diff --git a/x-pack/plugins/security/server/authentication/providers/base.ts b/x-pack/plugins/security/server/authentication/providers/base.ts index 12253f8d8f548..a40732768810d 100644 --- a/x-pack/plugins/security/server/authentication/providers/base.ts +++ b/x-pack/plugins/security/server/authentication/providers/base.ts @@ -11,6 +11,7 @@ import { IClusterClient, Headers, } from '../../../../../../src/core/server'; +import { deepFreeze } from '../../../../../../src/core/utils'; import { AuthenticatedUser } from '../../../common/model'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; @@ -35,6 +36,11 @@ export type AuthenticationProviderSpecificOptions = Record; * Base class that all authentication providers should extend. */ export abstract class BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type: string; + /** * Logger instance bound to a specific provider context. */ @@ -85,8 +91,13 @@ export abstract class BaseAuthenticationProvider { * @param [authHeaders] Optional `Headers` dictionary to send with the request. */ protected async getUser(request: KibanaRequest, authHeaders: Headers = {}) { - return (await this.options.client - .asScoped({ headers: { ...request.headers, ...authHeaders } }) - .callAsCurrentUser('shield.authenticate')) as AuthenticatedUser; + return deepFreeze({ + ...(await this.options.client + .asScoped({ headers: { ...request.headers, ...authHeaders } }) + .callAsCurrentUser('shield.authenticate')), + // We use `this.constructor` trick to get access to the static `type` field of the specific + // `BaseAuthenticationProvider` subclass. + authentication_provider: (this.constructor as any).type, + } as AuthenticatedUser); } } diff --git a/x-pack/plugins/security/server/authentication/providers/basic.ts b/x-pack/plugins/security/server/authentication/providers/basic.ts index 07d141b4e1b2b..a8e4e8705a7a8 100644 --- a/x-pack/plugins/security/server/authentication/providers/basic.ts +++ b/x-pack/plugins/security/server/authentication/providers/basic.ts @@ -34,6 +34,11 @@ interface ProviderState { * Provider that supports request authentication via Basic HTTP Authentication. */ export class BasicAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'basic'; + /** * Performs initial login request using username and password. * @param request Request instance. diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts index e609afb6ae3f3..e4b4df3feeae2 100644 --- a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts @@ -155,7 +155,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers.authorization).toBe('negotiate spnego'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer some-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -193,7 +193,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers.authorization).toBe('negotiate spnego'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer some-token' }); expect(authenticationResult.authResponseHeaders).toEqual({ 'WWW-Authenticate': 'Negotiate response-token', @@ -337,7 +337,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -370,7 +370,7 @@ describe('KerberosAuthenticationProvider', () => { expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer newfoo' }); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.state).toEqual({ accessToken: 'newfoo', refreshToken: 'newbar' }); expect(request.headers).not.toHaveProperty('authorization'); }); @@ -437,7 +437,7 @@ describe('KerberosAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'kerberos' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.ts index 767eab7b4311d..b8e3b7bc23790 100644 --- a/x-pack/plugins/security/server/authentication/providers/kerberos.ts +++ b/x-pack/plugins/security/server/authentication/providers/kerberos.ts @@ -43,6 +43,11 @@ const WWWAuthenticateHeaderName = 'WWW-Authenticate'; * Provider that supports Kerberos request authentication. */ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'kerberos'; + /** * Performs Kerberos request authentication. * @param request Request instance. diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.test.ts b/x-pack/plugins/security/server/authentication/providers/oidc.test.ts index dd4f6820f89f3..dae3774955859 100644 --- a/x-pack/plugins/security/server/authentication/providers/oidc.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/oidc.test.ts @@ -311,7 +311,7 @@ describe('OIDCAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'oidc' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -380,7 +380,7 @@ describe('OIDCAuthenticationProvider', () => { expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer new-access-token', }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'oidc' }); expect(authenticationResult.state).toEqual({ accessToken: 'new-access-token', refreshToken: 'new-refresh-token', @@ -492,7 +492,7 @@ describe('OIDCAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'oidc' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.ts b/x-pack/plugins/security/server/authentication/providers/oidc.ts index 3737123645379..f13a2ec05231a 100644 --- a/x-pack/plugins/security/server/authentication/providers/oidc.ts +++ b/x-pack/plugins/security/server/authentication/providers/oidc.ts @@ -6,9 +6,9 @@ import Boom from 'boom'; import type from 'type-detect'; -import { canRedirectRequest } from '../'; import { KibanaRequest } from '../../../../../../src/core/server'; import { AuthenticationResult } from '../authentication_result'; +import { canRedirectRequest } from '../can_redirect_request'; import { DeauthenticationResult } from '../deauthentication_result'; import { Tokens, TokenPair } from '../tokens'; import { @@ -62,6 +62,11 @@ interface ProviderState extends Partial { * Provider that supports authentication using an OpenID Connect realm in Elasticsearch. */ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'oidc'; + /** * Specifies Elasticsearch OIDC realm name that Kibana should use. */ diff --git a/x-pack/plugins/security/server/authentication/providers/pki.test.ts b/x-pack/plugins/security/server/authentication/providers/pki.test.ts index 76442733e7368..a2dda88c4680c 100644 --- a/x-pack/plugins/security/server/authentication/providers/pki.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/pki.test.ts @@ -200,7 +200,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -242,7 +242,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -287,7 +287,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -331,7 +331,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer access-token' }); expect(authenticationResult.authResponseHeaders).toBeUndefined(); expect(authenticationResult.state).toEqual({ @@ -448,7 +448,7 @@ describe('PKIAuthenticationProvider', () => { expect(authenticationResult.authHeaders).toEqual({ authorization: `Bearer ${state.accessToken}`, }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -491,7 +491,7 @@ describe('PKIAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'pki' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/pki.ts b/x-pack/plugins/security/server/authentication/providers/pki.ts index c7d431422a248..6d5aa9f01f2ea 100644 --- a/x-pack/plugins/security/server/authentication/providers/pki.ts +++ b/x-pack/plugins/security/server/authentication/providers/pki.ts @@ -44,6 +44,11 @@ function getRequestAuthenticationScheme(request: KibanaRequest) { * Provider that supports PKI request authentication. */ export class PKIAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'pki'; + /** * Performs PKI request authentication. * @param request Request instance. diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index a5d1010a1bec8..c4fdf0b25061b 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -635,7 +635,7 @@ describe('SAMLAuthenticationProvider', () => { expect(request.headers).not.toHaveProperty('authorization'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toEqual({ authorization }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'saml' }); expect(authenticationResult.state).toBeUndefined(); }); @@ -696,7 +696,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer new-access-token', }); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'saml' }); expect(authenticationResult.state).toEqual({ username: 'user', accessToken: 'new-access-token', @@ -842,7 +842,7 @@ describe('SAMLAuthenticationProvider', () => { expect(request.headers.authorization).toBe('Bearer some-valid-token'); expect(authenticationResult.succeeded()).toBe(true); expect(authenticationResult.authHeaders).toBeUndefined(); - expect(authenticationResult.user).toBe(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'saml' }); expect(authenticationResult.state).toBeUndefined(); }); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index faa19239fcc3b..a817159fcd445 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -10,8 +10,8 @@ import { KibanaRequest } from '../../../../../../src/core/server'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; import { AuthenticationProviderOptions, BaseAuthenticationProvider } from './base'; +import { canRedirectRequest } from '../can_redirect_request'; import { Tokens, TokenPair } from '../tokens'; -import { canRedirectRequest } from '..'; /** * The state supported by the provider (for the SAML handshake or established session). @@ -66,6 +66,11 @@ export function isSAMLRequestQuery(query: any): query is { SAMLRequest: string } * Provider that supports SAML request authentication. */ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'saml'; + /** * Specifies Elasticsearch SAML realm name that Kibana should use. */ diff --git a/x-pack/plugins/security/server/authentication/providers/token.test.ts b/x-pack/plugins/security/server/authentication/providers/token.test.ts index 3d377140cb42e..0a55219e25d91 100644 --- a/x-pack/plugins/security/server/authentication/providers/token.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/token.test.ts @@ -48,7 +48,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.login(request, credentials); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toEqual(tokenPair); expect(authenticationResult.authHeaders).toEqual({ authorization }); }); @@ -140,7 +140,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.authHeaders).toBeUndefined(); expect(authenticationResult.state).toBeUndefined(); }); @@ -158,7 +158,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request, tokenPair); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toBeUndefined(); expect(authenticationResult.authHeaders).toEqual({ authorization }); expect(request.headers).not.toHaveProperty('authorization'); @@ -192,7 +192,7 @@ describe('TokenAuthenticationProvider', () => { sinon.assert.calledOnce(mockOptions.tokens.refresh); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toEqual({ accessToken: 'newfoo', refreshToken: 'newbar' }); expect(authenticationResult.authHeaders).toEqual({ authorization: 'Bearer newfoo' }); expect(request.headers).not.toHaveProperty('authorization'); @@ -231,7 +231,7 @@ describe('TokenAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request, tokenPair); expect(authenticationResult.succeeded()).toBe(true); - expect(authenticationResult.user).toEqual(user); + expect(authenticationResult.user).toEqual({ ...user, authentication_provider: 'token' }); expect(authenticationResult.state).toBeUndefined(); expect(authenticationResult.authHeaders).toBeUndefined(); expect(request.headers.authorization).toEqual('Bearer foo-from-header'); diff --git a/x-pack/plugins/security/server/authentication/providers/token.ts b/x-pack/plugins/security/server/authentication/providers/token.ts index c5f8f07e50b11..03fd003e2cbde 100644 --- a/x-pack/plugins/security/server/authentication/providers/token.ts +++ b/x-pack/plugins/security/server/authentication/providers/token.ts @@ -9,8 +9,8 @@ import { KibanaRequest } from '../../../../../../src/core/server'; import { AuthenticationResult } from '../authentication_result'; import { DeauthenticationResult } from '../deauthentication_result'; import { BaseAuthenticationProvider } from './base'; +import { canRedirectRequest } from '../can_redirect_request'; import { Tokens, TokenPair } from '../tokens'; -import { canRedirectRequest } from '..'; /** * Describes the parameters that are required by the provider to process the initial login request. @@ -29,6 +29,11 @@ type ProviderState = TokenPair; * Provider that supports token-based request authentication. */ export class TokenAuthenticationProvider extends BaseAuthenticationProvider { + /** + * Type of the provider. + */ + static readonly type = 'token'; + /** * Performs initial login request using username and password. * @param request Request instance. diff --git a/x-pack/plugins/security/server/routes/authentication/common.test.ts b/x-pack/plugins/security/server/routes/authentication/common.test.ts index 5d5868d4cc593..4666b5abad756 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.test.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.test.ts @@ -176,20 +176,9 @@ describe('Common authentication routes', () => { expect(routeConfig.validate).toBe(false); }); - it('returns 500 if cannot retrieve current user due to unhandled exception.', async () => { - const unhandledException = new Error('Something went wrong.'); - authc.getCurrentUser.mockRejectedValue(unhandledException); - - const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); - - expect(response.status).toBe(500); - expect(response.payload).toEqual(unhandledException); - expect(authc.getCurrentUser).toHaveBeenCalledWith(mockRequest); - }); - it('returns current user.', async () => { const mockUser = mockAuthenticatedUser(); - authc.getCurrentUser.mockResolvedValue(mockUser); + authc.getCurrentUser.mockReturnValue(mockUser); const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index cb4ec196459ee..c9856e9dff7f1 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -59,7 +59,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef for (const path of ['/internal/security/me', '/api/security/v1/me']) { router.get( { path, validate: false }, - createLicensedRouteHandler(async (context, request, response) => { + createLicensedRouteHandler((context, request, response) => { if (path === '/api/security/v1/me') { logger.warn( `The "${basePath.serverBasePath}${path}" endpoint is deprecated and will be removed in the next major version.`, @@ -67,11 +67,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef ); } - try { - return response.ok({ body: (await authc.getCurrentUser(request)) as any }); - } catch (error) { - return response.customError(wrapIntoCustomErrorResponse(error)); - } + return response.ok({ body: authc.getCurrentUser(request)! }); }) ); } diff --git a/x-pack/plugins/security/server/routes/users/change_password.test.ts b/x-pack/plugins/security/server/routes/users/change_password.test.ts index 80a25e03ede62..34509edc2e9d2 100644 --- a/x-pack/plugins/security/server/routes/users/change_password.test.ts +++ b/x-pack/plugins/security/server/routes/users/change_password.test.ts @@ -4,6 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ +import { errors } from 'elasticsearch'; import { ObjectType } from '@kbn/config-schema'; import { IClusterClient, @@ -13,6 +14,7 @@ import { RequestHandler, RequestHandlerContext, RouteConfig, + ScopeableRequest, } from '../../../../../../src/core/server'; import { LICENSE_CHECK_STATE } from '../../../../licensing/server'; import { Authentication, AuthenticationResult } from '../../authentication'; @@ -38,10 +40,7 @@ describe('Change password', () => { let routeConfig: RouteConfig; let mockContext: RequestHandlerContext; - function checkPasswordChangeAPICall( - username: string, - request: ReturnType - ) { + function checkPasswordChangeAPICall(username: string, request: ScopeableRequest) { expect(mockClusterClient.asScoped).toHaveBeenCalledTimes(1); expect(mockClusterClient.asScoped).toHaveBeenCalledWith(request); expect(mockScopedClusterClient.callAsCurrentUser).toHaveBeenCalledTimes(1); @@ -55,8 +54,14 @@ describe('Change password', () => { router = httpServiceMock.createRouter(); authc = authenticationMock.create(); - authc.getCurrentUser.mockResolvedValue(mockAuthenticatedUser({ username: 'user' })); + authc.getCurrentUser.mockReturnValue(mockAuthenticatedUser({ username: 'user' })); authc.login.mockResolvedValue(AuthenticationResult.succeeded(mockAuthenticatedUser())); + authc.getSessionInfo.mockResolvedValue({ + now: Date.now(), + idleTimeoutExpiration: null, + lifespanExpiration: null, + provider: 'basic', + }); mockScopedClusterClient = elasticsearchServiceMock.createScopedClusterClient(); mockClusterClient = elasticsearchServiceMock.createClusterClient(); @@ -122,14 +127,24 @@ describe('Change password', () => { }); it('returns 403 if old password is wrong.', async () => { - const loginFailureReason = new Error('Something went wrong.'); - authc.login.mockResolvedValue(AuthenticationResult.failed(loginFailureReason)); + const changePasswordFailure = new (errors.AuthenticationException as any)('Unauthorized', { + body: { error: { header: { 'WWW-Authenticate': 'Negotiate' } } }, + }); + mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(changePasswordFailure); const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); expect(response.status).toBe(403); - expect(response.payload).toEqual(loginFailureReason); - expect(mockScopedClusterClient.callAsCurrentUser).not.toHaveBeenCalled(); + expect(response.payload).toEqual(changePasswordFailure); + + expect(mockScopedClusterClient.callAsCurrentUser).toHaveBeenCalledTimes(1); + expect(mockClusterClient.asScoped).toHaveBeenCalledTimes(1); + expect(mockClusterClient.asScoped).toHaveBeenCalledWith({ + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); }); it(`returns 401 if user can't authenticate with new password.`, async () => { @@ -148,10 +163,15 @@ describe('Change password', () => { expect(response.status).toBe(401); expect(response.payload).toEqual(loginFailureReason); - checkPasswordChangeAPICall(username, mockRequest); + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); }); - it('returns 500 if password update request fails.', async () => { + it('returns 500 if password update request fails with non-401 error.', async () => { const failureReason = new Error('Request failed.'); mockScopedClusterClient.callAsCurrentUser.mockRejectedValue(failureReason); @@ -160,7 +180,12 @@ describe('Change password', () => { expect(response.status).toBe(500); expect(response.payload).toEqual(failureReason); - checkPasswordChangeAPICall(username, mockRequest); + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); }); it('successfully changes own password if provided old password is correct.', async () => { @@ -169,7 +194,62 @@ describe('Change password', () => { expect(response.status).toBe(204); expect(response.payload).toBeUndefined(); - checkPasswordChangeAPICall(username, mockRequest); + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); + + expect(authc.login).toHaveBeenCalledTimes(1); + expect(authc.login).toHaveBeenCalledWith(mockRequest, { + provider: 'basic', + value: { username, password: 'new-password' }, + }); + }); + + it('successfully changes own password if provided old password is correct for non-basic provider.', async () => { + const mockUser = mockAuthenticatedUser({ + username: 'user', + authentication_provider: 'token', + }); + authc.getCurrentUser.mockReturnValue(mockUser); + authc.login.mockResolvedValue(AuthenticationResult.succeeded(mockUser)); + + const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); + + expect(response.status).toBe(204); + expect(response.payload).toBeUndefined(); + + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); + + expect(authc.login).toHaveBeenCalledTimes(1); + expect(authc.login).toHaveBeenCalledWith(mockRequest, { + provider: 'token', + value: { username, password: 'new-password' }, + }); + }); + + it('successfully changes own password but does not re-login if current session does not exist.', async () => { + authc.getSessionInfo.mockResolvedValue(null); + const response = await routeHandler(mockContext, mockRequest, kibanaResponseFactory); + + expect(response.status).toBe(204); + expect(response.payload).toBeUndefined(); + + checkPasswordChangeAPICall(username, { + headers: { + ...mockRequest.headers, + authorization: `Basic ${Buffer.from(`${username}:old-password`).toString('base64')}`, + }, + }); + + expect(authc.login).not.toHaveBeenCalled(); }); }); diff --git a/x-pack/plugins/security/server/routes/users/change_password.ts b/x-pack/plugins/security/server/routes/users/change_password.ts index b9d04b4bd1e0e..fc3ca4573d500 100644 --- a/x-pack/plugins/security/server/routes/users/change_password.ts +++ b/x-pack/plugins/security/server/routes/users/change_password.ts @@ -5,7 +5,8 @@ */ import { schema } from '@kbn/config-schema'; -import { wrapIntoCustomErrorResponse } from '../../errors'; +import { canUserChangePassword } from '../../../common/model'; +import { getErrorStatusCode, wrapIntoCustomErrorResponse } from '../../errors'; import { createLicensedRouteHandler } from '../licensed_route_handler'; import { RouteDefinitionParams } from '..'; @@ -13,7 +14,6 @@ export function defineChangeUserPasswordRoutes({ authc, router, clusterClient, - config, }: RouteDefinitionParams) { router.post( { @@ -27,54 +27,65 @@ export function defineChangeUserPasswordRoutes({ }, }, createLicensedRouteHandler(async (context, request, response) => { - const username = request.params.username; - const { password, newPassword } = request.body; - const isCurrentUser = username === (await authc.getCurrentUser(request))!.username; + const { username } = request.params; + const { password: currentPassword, newPassword } = request.body; - // We should prefer `token` over `basic` if possible. - const providerToLoginWith = config.authc.providers.includes('token') ? 'token' : 'basic'; + const currentUser = authc.getCurrentUser(request); + const isUserChangingOwnPassword = + currentUser && currentUser.username === username && canUserChangePassword(currentUser); + const currentSession = isUserChangingOwnPassword ? await authc.getSessionInfo(request) : null; - // If user tries to change own password, let's check if old password is valid first by trying - // to login. - if (isCurrentUser) { - try { - const authenticationResult = await authc.login(request, { - provider: providerToLoginWith, - value: { username, password }, - // We shouldn't alter authentication state just yet. - stateless: true, - }); - - if (!authenticationResult.succeeded()) { - return response.forbidden({ body: authenticationResult.error }); - } - } catch (error) { - return response.customError(wrapIntoCustomErrorResponse(error)); - } - } + // If user is changing their own password they should provide a proof of knowledge their + // current password via sending it in `Authorization: Basic base64(username:current password)` + // HTTP header no matter how they logged in to Kibana. + const scopedClusterClient = clusterClient.asScoped( + isUserChangingOwnPassword + ? { + headers: { + ...request.headers, + authorization: `Basic ${Buffer.from(`${username}:${currentPassword}`).toString( + 'base64' + )}`, + }, + } + : request + ); try { - await clusterClient.asScoped(request).callAsCurrentUser('shield.changePassword', { + await scopedClusterClient.callAsCurrentUser('shield.changePassword', { username, body: { password: newPassword }, }); + } catch (error) { + // This may happen only if user's credentials are rejected meaning that current password + // isn't correct. + if (isUserChangingOwnPassword && getErrorStatusCode(error) === 401) { + return response.forbidden({ body: error }); + } + + return response.customError(wrapIntoCustomErrorResponse(error)); + } - // Now we authenticate user with the new password again updating current session if any. - if (isCurrentUser) { + // If user previously had an active session and changed their own password we should re-login + // user with the new password and update session. We check this since it's possible to update + // password even if user is authenticated via HTTP headers and hence doesn't have an active + // session and in such cases we shouldn't create a new one. + if (isUserChangingOwnPassword && currentSession) { + try { const authenticationResult = await authc.login(request, { - provider: providerToLoginWith, + provider: currentUser!.authentication_provider, value: { username, password: newPassword }, }); if (!authenticationResult.succeeded()) { return response.unauthorized({ body: authenticationResult.error }); } + } catch (error) { + return response.customError(wrapIntoCustomErrorResponse(error)); } - - return response.noContent(); - } catch (error) { - return response.customError(wrapIntoCustomErrorResponse(error)); } + + return response.noContent(); }) ); } diff --git a/x-pack/test/api_integration/apis/security/basic_login.js b/x-pack/test/api_integration/apis/security/basic_login.js index 62deedad6c35c..d4b41603944f6 100644 --- a/x-pack/test/api_integration/apis/security/basic_login.js +++ b/x-pack/test/api_integration/apis/security/basic_login.js @@ -126,6 +126,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be(validUsername); }); @@ -165,6 +166,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be(validUsername); }); diff --git a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts index 203f90c55aa82..570d7026cf99e 100644 --- a/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts +++ b/x-pack/test/kerberos_api_integration/apis/security/kerberos_login.ts @@ -74,6 +74,7 @@ export default function({ getService }: FtrProviderContext) { expect(user.username).to.eql(username); expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); + expect(user.authentication_provider).to.eql('basic'); }); describe('initiating SPNEGO', () => { @@ -129,6 +130,7 @@ export default function({ getService }: FtrProviderContext) { enabled: true, authentication_realm: { name: 'kerb1', type: 'kerberos' }, lookup_realm: { name: 'kerb1', type: 'kerberos' }, + authentication_provider: 'kerberos', }); }); diff --git a/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js b/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js index 57bcd1b041c49..094537fd61436 100644 --- a/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js +++ b/x-pack/test/oidc_api_integration/apis/authorization_code_flow/oidc_auth.js @@ -173,6 +173,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('user1'); @@ -222,6 +223,7 @@ export default function({ getService }) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('user2'); diff --git a/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts b/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts index 1f5a64835416a..7d013e97732f9 100644 --- a/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts +++ b/x-pack/test/oidc_api_integration/apis/implicit_flow/oidc_auth.ts @@ -142,6 +142,7 @@ export default function({ getService }: FtrProviderContext) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('user1'); diff --git a/x-pack/test/pki_api_integration/apis/security/pki_auth.ts b/x-pack/test/pki_api_integration/apis/security/pki_auth.ts index 186ed824b3b6c..1ae7488fcf379 100644 --- a/x-pack/test/pki_api_integration/apis/security/pki_auth.ts +++ b/x-pack/test/pki_api_integration/apis/security/pki_auth.ts @@ -90,6 +90,7 @@ export default function({ getService }: FtrProviderContext) { expect(user.username).to.eql(username); expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); + expect(user.authentication_provider).to.eql('basic'); }); it('should properly set cookie and authenticate user', async () => { @@ -118,6 +119,7 @@ export default function({ getService }: FtrProviderContext) { }, authentication_realm: { name: 'pki1', type: 'pki' }, lookup_realm: { name: 'pki1', type: 'pki' }, + authentication_provider: 'pki', }); // Cookie should be accepted. @@ -160,6 +162,7 @@ export default function({ getService }: FtrProviderContext) { }, authentication_realm: { name: 'pki1', type: 'pki' }, lookup_realm: { name: 'pki1', type: 'pki' }, + authentication_provider: 'pki', }); checkCookieIsSet(request.cookie(response.headers['set-cookie'][0])!); diff --git a/x-pack/test/saml_api_integration/apis/security/saml_login.ts b/x-pack/test/saml_api_integration/apis/security/saml_login.ts index 0436d59906ea8..6ede8aadeb5a7 100644 --- a/x-pack/test/saml_api_integration/apis/security/saml_login.ts +++ b/x-pack/test/saml_api_integration/apis/security/saml_login.ts @@ -56,6 +56,7 @@ export default function({ getService }: FtrProviderContext) { 'enabled', 'authentication_realm', 'lookup_realm', + 'authentication_provider', ]); expect(apiResponse.body.username).to.be('a@b.c'); @@ -88,6 +89,7 @@ export default function({ getService }: FtrProviderContext) { expect(user.username).to.eql(username); expect(user.authentication_realm).to.eql({ name: 'reserved', type: 'reserved' }); + expect(user.authentication_provider).to.eql('basic'); }); describe('capture URL fragment', () => { From a0b73cf2996863491b75f002d2eaddc86f26c6a2 Mon Sep 17 00:00:00 2001 From: Anton Dosov Date: Wed, 5 Feb 2020 13:43:09 +0100 Subject: [PATCH 2/5] improve validation on tuncate field formater editor (#56521) Co-authored-by: Elastic Machine --- .../__snapshots__/truncate.test.js.snap | 1 + .../editors/truncate/truncate.js | 11 +++- .../editors/truncate/truncate.test.js | 56 +++++++++++++++++++ .../field_formats/converters/truncate.test.ts | 7 +++ 4 files changed, 73 insertions(+), 2 deletions(-) diff --git a/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/__snapshots__/truncate.test.js.snap b/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/__snapshots__/truncate.test.js.snap index c0954491a2a47..729487dfae5d7 100644 --- a/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/__snapshots__/truncate.test.js.snap +++ b/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/__snapshots__/truncate.test.js.snap @@ -21,6 +21,7 @@ exports[`TruncateFormatEditor should render normally 1`] = ` > diff --git a/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.js b/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.js index fdae8627ead69..9a9b6c954b78d 100644 --- a/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.js +++ b/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.js @@ -38,7 +38,7 @@ export class TruncateFormatEditor extends DefaultFormatEditor { } render() { - const { formatParams } = this.props; + const { formatParams, onError } = this.props; const { error, samples } = this.state; return ( @@ -55,8 +55,15 @@ export class TruncateFormatEditor extends DefaultFormatEditor { > { - this.onChange({ fieldLength: e.target.value ? Number(e.target.value) : null }); + if (e.target.checkValidity()) { + this.onChange({ + fieldLength: e.target.value ? Number(e.target.value) : null, + }); + } else { + onError(e.target.validationMessage); + } }} isInvalid={!!error} /> diff --git a/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.test.js b/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.test.js index e98dd4edca386..7ab6f2a9cbeb0 100644 --- a/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.test.js +++ b/src/legacy/ui/public/field_editor/components/field_format_editor/editors/truncate/truncate.test.js @@ -19,6 +19,7 @@ import React from 'react'; import { shallow } from 'enzyme'; +import { EuiFieldNumber } from '@elastic/eui'; import { TruncateFormatEditor } from './truncate'; @@ -34,6 +35,11 @@ const onChange = jest.fn(); const onError = jest.fn(); describe('TruncateFormatEditor', () => { + beforeEach(() => { + onChange.mockClear(); + onError.mockClear(); + }); + it('should have a formatId', () => { expect(TruncateFormatEditor.formatId).toEqual('truncate'); }); @@ -50,4 +56,54 @@ describe('TruncateFormatEditor', () => { ); expect(component).toMatchSnapshot(); }); + + it('should fire error, when input is invalid', async () => { + const component = shallow( + + ); + const input = component.find(EuiFieldNumber); + + const changeEvent = { + target: { + value: '123.3', + checkValidity: () => false, + validationMessage: 'Error!', + }, + }; + await input.invoke('onChange')(changeEvent); + + expect(onError).toBeCalledWith(changeEvent.target.validationMessage); + expect(onChange).not.toBeCalled(); + }); + + it('should fire change, when input changed and is valid', async () => { + const component = shallow( + + ); + const input = component.find(EuiFieldNumber); + + const changeEvent = { + target: { + value: '123', + checkValidity: () => true, + validationMessage: null, + }, + }; + onError.mockClear(); + await input.invoke('onChange')(changeEvent); + expect(onError).not.toBeCalled(); + expect(onChange).toBeCalledWith({ fieldLength: 123 }); + }); }); diff --git a/src/plugins/data/common/field_formats/converters/truncate.test.ts b/src/plugins/data/common/field_formats/converters/truncate.test.ts index 472d9673346d7..3a0abc918fa98 100644 --- a/src/plugins/data/common/field_formats/converters/truncate.test.ts +++ b/src/plugins/data/common/field_formats/converters/truncate.test.ts @@ -43,4 +43,11 @@ describe('String TruncateFormat', () => { expect(truncate.convert('This is some text')).toBe('This is some text'); }); + + test('does not truncate whole text when non integer is passed in', () => { + // https://github.com/elastic/kibana/issues/29648 + const truncate = new TruncateFormat({ fieldLength: 3.2 }, jest.fn()); + + expect(truncate.convert('This is some text')).toBe('Thi...'); + }); }); From 3cb85d407027a118103ff02c90747673529f1af1 Mon Sep 17 00:00:00 2001 From: Shahzad Date: Wed, 5 Feb 2020 13:53:34 +0100 Subject: [PATCH 3/5] [Uptime] Kuery Bar Filters break overview page (#56793) * update filter * fix status panel not using filter group --- .../components/connected/charts/ping_histogram.tsx | 9 +++++---- .../connected/charts/snapshot_container.tsx | 11 +++++++---- .../connected/pages/overview_container.ts | 11 ++++++++++- .../uptime/public/hooks/update_kuery_string.ts | 14 -------------- .../plugins/uptime/public/pages/overview.tsx | 14 ++++++++++++-- .../plugins/uptime/public/state/selectors/index.ts | 7 ++++++- 6 files changed, 40 insertions(+), 26 deletions(-) diff --git a/x-pack/legacy/plugins/uptime/public/components/connected/charts/ping_histogram.tsx b/x-pack/legacy/plugins/uptime/public/components/connected/charts/ping_histogram.tsx index cbdd921a36e81..50f91be4ff09f 100644 --- a/x-pack/legacy/plugins/uptime/public/components/connected/charts/ping_histogram.tsx +++ b/x-pack/legacy/plugins/uptime/public/components/connected/charts/ping_histogram.tsx @@ -19,7 +19,7 @@ import { useUrlParams } from '../../../hooks'; type Props = ResponsiveWrapperProps & Pick & - DispatchProps & { lastRefresh: number; monitorId?: string }; + DispatchProps & { lastRefresh: number; monitorId?: string; esKuery?: string }; const PingHistogramContainer: React.FC = ({ data, @@ -28,6 +28,7 @@ const PingHistogramContainer: React.FC = ({ lastRefresh, height, loading, + esKuery, }) => { const [getUrlParams] = useUrlParams(); const { @@ -36,12 +37,11 @@ const PingHistogramContainer: React.FC = ({ dateRangeStart: dateStart, dateRangeEnd: dateEnd, statusFilter, - filters, } = getUrlParams(); useEffect(() => { - loadData({ monitorId, dateStart, dateEnd, statusFilter, filters }); - }, [loadData, dateStart, dateEnd, monitorId, filters, statusFilter, lastRefresh]); + loadData({ monitorId, dateStart, dateEnd, statusFilter, filters: esKuery }); + }, [loadData, dateStart, dateEnd, monitorId, statusFilter, lastRefresh, esKuery]); return ( = ({ height, lastRefresh, loading, + esKuery, loadSnapshotCount, }: Props) => { const [getUrlParams] = useUrlParams(); - const { dateRangeStart, dateRangeEnd, statusFilter, filters } = getUrlParams(); + const { dateRangeStart, dateRangeEnd, statusFilter } = getUrlParams(); useEffect(() => { - loadSnapshotCount(dateRangeStart, dateRangeEnd, filters, statusFilter); - }, [dateRangeStart, dateRangeEnd, filters, lastRefresh, loadSnapshotCount, statusFilter]); + loadSnapshotCount(dateRangeStart, dateRangeEnd, esKuery, statusFilter); + }, [dateRangeStart, dateRangeEnd, esKuery, lastRefresh, loadSnapshotCount, statusFilter]); return ; }; @@ -66,11 +68,12 @@ export const Container: React.FC = ({ */ const mapStateToProps = ({ snapshot: { count, loading }, - ui: { lastRefresh }, + ui: { lastRefresh, esKuery }, }: AppState): StoreProps => ({ count, lastRefresh, loading, + esKuery, }); /** diff --git a/x-pack/legacy/plugins/uptime/public/components/connected/pages/overview_container.ts b/x-pack/legacy/plugins/uptime/public/components/connected/pages/overview_container.ts index 406fab8f5bf01..cbd1fae77c518 100644 --- a/x-pack/legacy/plugins/uptime/public/components/connected/pages/overview_container.ts +++ b/x-pack/legacy/plugins/uptime/public/components/connected/pages/overview_container.ts @@ -8,7 +8,16 @@ import { connect } from 'react-redux'; import { OverviewPageComponent } from '../../../pages/overview'; import { selectIndexPattern } from '../../../state/selectors'; import { AppState } from '../../../state'; +import { setEsKueryString } from '../../../state/actions'; + +interface DispatchProps { + setEsKueryFilters: typeof setEsKueryString; +} + +const mapDispatchToProps = (dispatch: any): DispatchProps => ({ + setEsKueryFilters: (esFilters: string) => dispatch(setEsKueryString(esFilters)), +}); const mapStateToProps = (state: AppState) => ({ indexPattern: selectIndexPattern(state) }); -export const OverviewPage = connect(mapStateToProps)(OverviewPageComponent); +export const OverviewPage = connect(mapStateToProps, mapDispatchToProps)(OverviewPageComponent); diff --git a/x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts b/x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts index 8c9eec3abe223..8a4ae01a72b4b 100644 --- a/x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts +++ b/x-pack/legacy/plugins/uptime/public/hooks/update_kuery_string.ts @@ -6,20 +6,8 @@ import { combineFiltersAndUserSearch, stringifyKueries } from '../lib/helper'; import { esKuery } from '../../../../../../src/plugins/data/common/es_query'; -import { store } from '../state'; -import { setEsKueryString } from '../state/actions'; import { IIndexPattern } from '../../../../../../src/plugins/data/common/index_patterns'; -const updateEsQueryForFilterGroup = (filterQueryString: string, indexPattern: IIndexPattern) => { - // Update EsQuery in Redux to be used in FilterGroup - const searchDSL: string = filterQueryString - ? JSON.stringify( - esKuery.toElasticsearchQuery(esKuery.fromKueryExpression(filterQueryString), indexPattern) - ) - : ''; - store.dispatch(setEsKueryString(searchDSL)); -}; - const getKueryString = (urlFilters: string): string => { let kueryString = ''; // We are using try/catch here because this is user entered value @@ -56,8 +44,6 @@ export const useUpdateKueryString = ( esFilters = JSON.stringify(elasticsearchQuery); } - updateEsQueryForFilterGroup(filterQueryString, indexPattern); - return [esFilters]; } catch (err) { return [urlFilters, err]; diff --git a/x-pack/legacy/plugins/uptime/public/pages/overview.tsx b/x-pack/legacy/plugins/uptime/public/pages/overview.tsx index df6ffba6154e5..ce5fb619aca02 100644 --- a/x-pack/legacy/plugins/uptime/public/pages/overview.tsx +++ b/x-pack/legacy/plugins/uptime/public/pages/overview.tsx @@ -5,7 +5,7 @@ */ import { EuiFlexGroup, EuiFlexItem, EuiSpacer } from '@elastic/eui'; -import React, { useContext } from 'react'; +import React, { useContext, useEffect } from 'react'; import styled from 'styled-components'; import { EmptyState, @@ -27,6 +27,7 @@ interface OverviewPageProps { autocomplete: DataPublicPluginStart['autocomplete']; setBreadcrumbs: UMUpdateBreadcrumbs; indexPattern: IIndexPattern; + setEsKueryFilters: (esFilters: string) => void; } type Props = OverviewPageProps; @@ -40,7 +41,12 @@ const EuiFlexItemStyled = styled(EuiFlexItem)` } `; -export const OverviewPageComponent = ({ autocomplete, setBreadcrumbs, indexPattern }: Props) => { +export const OverviewPageComponent = ({ + autocomplete, + setBreadcrumbs, + indexPattern, + setEsKueryFilters, +}: Props) => { const { colors } = useContext(UptimeThemeContext); const [getUrlParams] = useUrlParams(); const { absoluteDateRangeStart, absoluteDateRangeEnd, ...params } = getUrlParams(); @@ -60,6 +66,10 @@ export const OverviewPageComponent = ({ autocomplete, setBreadcrumbs, indexPatte const [esFilters, error] = useUpdateKueryString(indexPattern, search, urlFilters); + useEffect(() => { + setEsKueryFilters(esFilters ?? ''); + }, [esFilters, setEsKueryFilters]); + const sharedProps = { dateRangeStart, dateRangeEnd, diff --git a/x-pack/legacy/plugins/uptime/public/state/selectors/index.ts b/x-pack/legacy/plugins/uptime/public/state/selectors/index.ts index fe6a7a1b7eade..25498cc0cb0ee 100644 --- a/x-pack/legacy/plugins/uptime/public/state/selectors/index.ts +++ b/x-pack/legacy/plugins/uptime/public/state/selectors/index.ts @@ -34,5 +34,10 @@ export const selectIndexPattern = ({ indexPattern }: AppState) => { }; export const selectPingHistogram = ({ ping, ui }: AppState) => { - return { data: ping.pingHistogram, loading: ping.loading, lastRefresh: ui.lastRefresh }; + return { + data: ping.pingHistogram, + loading: ping.loading, + lastRefresh: ui.lastRefresh, + esKuery: ui.esKuery, + }; }; From 2bb46732e19663331a7207df5c015b69a9fc54c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Fern=C3=A1ndez=20Haro?= Date: Wed, 5 Feb 2020 14:08:30 +0000 Subject: [PATCH 4/5] [Telemetry] Report Kibana distro in local collectors + Usage Collectors in TS (#55859) * [Telemetry] Report Kibana distro in local collectors + Usage Collectors in TS * Ensure isReady is a function * Move CollectorSet tests to TS + Jest * Fix test Co-authored-by: Elastic Machine --- .../usage/telemetry_usage_collector.test.ts | 2 +- .../{get_kibana.js => get_kibana.ts} | 45 ++++-- .../telemetry_collection/get_local_stats.ts | 11 +- .../collector/{collector.js => collector.ts} | 67 ++++---- ...collector_set.js => collector_set.test.ts} | 145 +++++++++++++----- .../server/collector/collector_set.ts | 27 ++-- .../server/collector/index.ts | 2 - .../server/collector/usage_collector.js | 51 ------ .../server/collector/usage_collector.ts | 37 +++++ .../collectors/cloud_usage_collector.test.ts | 5 +- 10 files changed, 239 insertions(+), 153 deletions(-) rename src/legacy/core_plugins/telemetry/server/telemetry_collection/{get_kibana.js => get_kibana.ts} (62%) rename src/plugins/usage_collection/server/collector/{collector.js => collector.ts} (59%) rename src/plugins/usage_collection/server/collector/{__tests__/collector_set.js => collector_set.test.ts} (53%) delete mode 100644 src/plugins/usage_collection/server/collector/usage_collector.js create mode 100644 src/plugins/usage_collection/server/collector/usage_collector.ts diff --git a/src/legacy/core_plugins/telemetry/server/collectors/usage/telemetry_usage_collector.test.ts b/src/legacy/core_plugins/telemetry/server/collectors/usage/telemetry_usage_collector.test.ts index cf6059faf0c05..78685cd6becc8 100644 --- a/src/legacy/core_plugins/telemetry/server/collectors/usage/telemetry_usage_collector.test.ts +++ b/src/legacy/core_plugins/telemetry/server/collectors/usage/telemetry_usage_collector.test.ts @@ -136,7 +136,7 @@ describe('telemetry_usage_collector', () => { const collectorOptions = createTelemetryUsageCollector(usageCollector, server); expect(collectorOptions.type).toBe('static_telemetry'); - expect(await collectorOptions.fetch()).toEqual(expectedObject); + expect(await collectorOptions.fetch({} as any)).toEqual(expectedObject); // Sending any as the callCluster client because it's not needed in this collector but TS requires it when calling it. }); }); }); diff --git a/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_kibana.js b/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_kibana.ts similarity index 62% rename from src/legacy/core_plugins/telemetry/server/telemetry_collection/get_kibana.js rename to src/legacy/core_plugins/telemetry/server/telemetry_collection/get_kibana.ts index e65606a83afc8..537d5a85911cd 100644 --- a/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_kibana.js +++ b/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_kibana.ts @@ -17,9 +17,27 @@ * under the License. */ -import { get, omit } from 'lodash'; +import { omit } from 'lodash'; +import { UsageCollectionSetup } from 'src/plugins/usage_collection/server'; +import { CallCluster } from 'src/legacy/core_plugins/elasticsearch'; -export function handleKibanaStats(server, response) { +export interface KibanaUsageStats { + kibana: { + index: string; + }; + kibana_stats: { + os: { + platform: string; + platformRelease: string; + distro?: string; + distroRelease?: string; + }; + }; + + [plugin: string]: any; +} + +export function handleKibanaStats(server: any, response?: KibanaUsageStats) { if (!response) { server.log( ['warning', 'telemetry', 'local-stats'], @@ -30,8 +48,17 @@ export function handleKibanaStats(server, response) { const { kibana, kibana_stats: kibanaStats, ...plugins } = response; - const platform = get(kibanaStats, 'os.platform', 'unknown'); - const platformRelease = get(kibanaStats, 'os.platformRelease', 'unknown'); + const os = { + platform: 'unknown', + platformRelease: 'unknown', + ...kibanaStats.os, + }; + const formattedOsStats = Object.entries(os).reduce((acc, [key, value]) => { + return { + ...acc, + [`${key}s`]: [{ [key]: value, count: 1 }], + }; + }, {}); const version = server .config() @@ -44,16 +71,16 @@ export function handleKibanaStats(server, response) { ...omit(kibana, 'index'), // discard index count: 1, indices: 1, - os: { - platforms: [{ platform, count: 1 }], - platformReleases: [{ platformRelease, count: 1 }], - }, + os: formattedOsStats, versions: [{ version, count: 1 }], plugins, }; } -export async function getKibana(usageCollection, callWithInternalUser) { +export async function getKibana( + usageCollection: UsageCollectionSetup, + callWithInternalUser: CallCluster +): Promise { const usage = await usageCollection.bulkFetch(callWithInternalUser); return usageCollection.toObject(usage); } diff --git a/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_local_stats.ts b/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_local_stats.ts index a4ea2eb534226..8adb6d237bee8 100644 --- a/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_local_stats.ts +++ b/src/legacy/core_plugins/telemetry/server/telemetry_collection/get_local_stats.ts @@ -22,18 +22,25 @@ import { get, omit } from 'lodash'; import { getClusterInfo } from './get_cluster_info'; import { getClusterStats } from './get_cluster_stats'; // @ts-ignore -import { getKibana, handleKibanaStats } from './get_kibana'; +import { getKibana, handleKibanaStats, KibanaUsageStats } from './get_kibana'; import { StatsGetter } from '../collection_manager'; /** * Handle the separate local calls by combining them into a single object response that looks like the * "cluster_stats" document from X-Pack monitoring. * + * @param {Object} server ?? * @param {Object} clusterInfo Cluster info (GET /) * @param {Object} clusterStats Cluster stats (GET /_cluster/stats) + * @param {Object} kibana The Kibana Usage stats * @return {Object} A combined object containing the different responses. */ -export function handleLocalStats(server: any, clusterInfo: any, clusterStats: any, kibana: any) { +export function handleLocalStats( + server: any, + clusterInfo: any, + clusterStats: any, + kibana: KibanaUsageStats +) { return { timestamp: new Date().toISOString(), cluster_uuid: get(clusterInfo, 'cluster_uuid'), diff --git a/src/plugins/usage_collection/server/collector/collector.js b/src/plugins/usage_collection/server/collector/collector.ts similarity index 59% rename from src/plugins/usage_collection/server/collector/collector.js rename to src/plugins/usage_collection/server/collector/collector.ts index 54d18ec2b8a7f..e102dc2a64ee8 100644 --- a/src/plugins/usage_collection/server/collector/collector.js +++ b/src/plugins/usage_collection/server/collector/collector.ts @@ -17,7 +17,30 @@ * under the License. */ -export class Collector { +import { Logger } from 'kibana/server'; +import { CallCluster } from 'src/legacy/core_plugins/elasticsearch'; + +export type CollectorFormatForBulkUpload = (result: T) => { type: string; payload: U }; + +export interface CollectorOptions { + type: string; + init?: Function; + fetch: (callCluster: CallCluster) => Promise | T; + /* + * A hook for allowing the fetched data payload to be organized into a typed + * data model for internal bulk upload. See defaultFormatterForBulkUpload for + * a generic example. + */ + formatForBulkUpload?: CollectorFormatForBulkUpload; + isReady: () => Promise | boolean; +} + +export class Collector { + public readonly type: CollectorOptions['type']; + public readonly init?: CollectorOptions['init']; + public readonly fetch: CollectorOptions['fetch']; + private readonly _formatForBulkUpload?: CollectorFormatForBulkUpload; + public readonly isReady: CollectorOptions['isReady']; /* * @param {Object} logger - logger object * @param {String} options.type - property name as the key for the data @@ -27,8 +50,8 @@ export class Collector { * @param {Function} options.rest - optional other properties */ constructor( - logger, - { type, init, fetch, formatForBulkUpload = null, isReady = null, ...options } = {} + protected readonly log: Logger, + { type, init, fetch, formatForBulkUpload, isReady, ...options }: CollectorOptions ) { if (type === undefined) { throw new Error('Collector must be instantiated with a options.type string property'); @@ -42,41 +65,27 @@ export class Collector { throw new Error('Collector must be instantiated with a options.fetch function property'); } - this.log = logger; - Object.assign(this, options); // spread in other properties and mutate "this" this.type = type; this.init = init; this.fetch = fetch; - - const defaultFormatterForBulkUpload = result => ({ type, payload: result }); - this._formatForBulkUpload = formatForBulkUpload || defaultFormatterForBulkUpload; - if (typeof isReady === 'function') { - this.isReady = isReady; - } + this.isReady = typeof isReady === 'function' ? isReady : () => true; + this._formatForBulkUpload = formatForBulkUpload; } - /* - * @param {Function} callCluster - callCluster function - */ - fetchInternal(callCluster) { - if (typeof callCluster !== 'function') { - throw new Error('A `callCluster` function must be passed to the fetch methods of collectors'); + public formatForBulkUpload(result: T) { + if (this._formatForBulkUpload) { + return this._formatForBulkUpload(result); + } else { + return this.defaultFormatterForBulkUpload(result); } - return this.fetch(callCluster); - } - - /* - * A hook for allowing the fetched data payload to be organized into a typed - * data model for internal bulk upload. See defaultFormatterForBulkUpload for - * a generic example. - */ - formatForBulkUpload(result) { - return this._formatForBulkUpload(result); } - isReady() { - throw `isReady() must be implemented in ${this.type} collector`; + protected defaultFormatterForBulkUpload(result: T) { + return { + type: this.type, + payload: result, + }; } } diff --git a/src/plugins/usage_collection/server/collector/__tests__/collector_set.js b/src/plugins/usage_collection/server/collector/collector_set.test.ts similarity index 53% rename from src/plugins/usage_collection/server/collector/__tests__/collector_set.js rename to src/plugins/usage_collection/server/collector/collector_set.test.ts index 397499650e054..c85880c34d72b 100644 --- a/src/plugins/usage_collection/server/collector/__tests__/collector_set.js +++ b/src/plugins/usage_collection/server/collector/collector_set.test.ts @@ -18,58 +18,62 @@ */ import { noop } from 'lodash'; -import sinon from 'sinon'; -import expect from '@kbn/expect'; -import { Collector } from '../collector'; -import { CollectorSet } from '../collector_set'; -import { UsageCollector } from '../usage_collector'; - -const mockLogger = () => ({ - debug: sinon.spy(), - warn: sinon.spy(), -}); +import { Collector } from './collector'; +import { CollectorSet } from './collector_set'; +import { UsageCollector } from './usage_collector'; +import { loggingServiceMock } from '../../../../core/server/mocks'; + +const logger = loggingServiceMock.createLogger(); + +const loggerSpies = { + debug: jest.spyOn(logger, 'debug'), + warn: jest.spyOn(logger, 'warn'), +}; describe('CollectorSet', () => { describe('registers a collector set and runs lifecycle events', () => { - let init; - let fetch; + let init: Function; + let fetch: Function; beforeEach(() => { init = noop; fetch = noop; + loggerSpies.debug.mockRestore(); + loggerSpies.warn.mockRestore(); }); + const mockCallCluster = () => Promise.resolve({ passTest: 1000 }); + it('should throw an error if non-Collector type of object is registered', () => { - const logger = mockLogger(); const collectors = new CollectorSet({ logger }); const registerPojo = () => { collectors.registerCollector({ type: 'type_collector_test', init, fetch, - }); + } as any); // We are intentionally sending it wrong. }; - expect(registerPojo).to.throwException(({ message }) => { - expect(message).to.be('CollectorSet can only have Collector instances registered'); - }); + expect(registerPojo).toThrowError( + 'CollectorSet can only have Collector instances registered' + ); }); it('should log debug status of fetching from the collector', async () => { - const mockCallCluster = () => Promise.resolve({ passTest: 1000 }); - const logger = mockLogger(); const collectors = new CollectorSet({ logger }); collectors.registerCollector( new Collector(logger, { type: 'MY_TEST_COLLECTOR', - fetch: caller => caller(), + fetch: (caller: any) => caller(), + isReady: () => true, }) ); - const result = await collectors.bulkFetch(mockCallCluster); - const calls = logger.debug.getCalls(); - expect(calls.length).to.be(1); - expect(calls[0].args).to.eql(['Fetching data from MY_TEST_COLLECTOR collector']); - expect(result).to.eql([ + const result = await collectors.bulkFetch(mockCallCluster as any); + expect(loggerSpies.debug).toHaveBeenCalledTimes(1); + expect(loggerSpies.debug).toHaveBeenCalledWith( + 'Fetching data from MY_TEST_COLLECTOR collector' + ); + expect(result).toStrictEqual([ { type: 'MY_TEST_COLLECTOR', result: { passTest: 1000 }, @@ -78,32 +82,90 @@ describe('CollectorSet', () => { }); it('should gracefully handle a collector fetch method throwing an error', async () => { - const mockCallCluster = () => Promise.resolve({ passTest: 1000 }); - const logger = mockLogger(); const collectors = new CollectorSet({ logger }); collectors.registerCollector( new Collector(logger, { type: 'MY_TEST_COLLECTOR', fetch: () => new Promise((_resolve, reject) => reject()), + isReady: () => true, }) ); let result; try { - result = await collectors.bulkFetch(mockCallCluster); + result = await collectors.bulkFetch(mockCallCluster as any); } catch (err) { // Do nothing } // This must return an empty object instead of null/undefined - expect(result).to.eql([]); + expect(result).toStrictEqual([]); + }); + + it('should not break if isReady is not a function', async () => { + const collectors = new CollectorSet({ logger }); + collectors.registerCollector( + new Collector(logger, { + type: 'MY_TEST_COLLECTOR', + fetch: () => ({ test: 1 }), + isReady: true as any, + }) + ); + + const result = await collectors.bulkFetch(mockCallCluster as any); + expect(result).toStrictEqual([ + { + type: 'MY_TEST_COLLECTOR', + result: { test: 1 }, + }, + ]); + }); + + it('should not break if isReady is not provided', async () => { + const collectors = new CollectorSet({ logger }); + collectors.registerCollector( + new Collector(logger, { + type: 'MY_TEST_COLLECTOR', + fetch: () => ({ test: 1 }), + } as any) + ); + + const result = await collectors.bulkFetch(mockCallCluster as any); + expect(result).toStrictEqual([ + { + type: 'MY_TEST_COLLECTOR', + result: { test: 1 }, + }, + ]); + }); + + it('should infer the types from the implementations of fetch and formatForBulkUpload', async () => { + const collectors = new CollectorSet({ logger }); + collectors.registerCollector( + new Collector(logger, { + type: 'MY_TEST_COLLECTOR', + fetch: () => ({ test: 1 }), + formatForBulkUpload: result => ({ + type: 'MY_TEST_COLLECTOR', + payload: { test: result.test * 2 }, + }), + isReady: () => true, + }) + ); + + const result = await collectors.bulkFetch(mockCallCluster as any); + expect(result).toStrictEqual([ + { + type: 'MY_TEST_COLLECTOR', + result: { test: 1 }, // It matches the return of `fetch`. `formatForBulkUpload` is used later on + }, + ]); }); }); describe('toApiFieldNames', () => { - let collectorSet; + let collectorSet: CollectorSet; beforeEach(() => { - const logger = mockLogger(); collectorSet = new CollectorSet({ logger }); }); @@ -126,7 +188,7 @@ describe('CollectorSet', () => { }; const result = collectorSet.toApiFieldNames(apiData); - expect(result).to.eql({ + expect(result).toStrictEqual({ os: { load: { '15m': 2.3525390625, '1m': 2.22412109375, '5m': 2.4462890625 }, memory: { free_bytes: 458280960, total_bytes: 17179869184, used_bytes: 16721588224 }, @@ -155,7 +217,7 @@ describe('CollectorSet', () => { }; const result = collectorSet.toApiFieldNames(apiData); - expect(result).to.eql({ + expect(result).toStrictEqual({ days_of_the_week: [ { day_index: 1, day_name: 'monday' }, { day_index: 2, day_name: 'tuesday' }, @@ -166,21 +228,20 @@ describe('CollectorSet', () => { }); describe('isUsageCollector', () => { - const collectorOptions = { type: 'MY_TEST_COLLECTOR', fetch: () => {} }; + const collectorOptions = { type: 'MY_TEST_COLLECTOR', fetch: () => {}, isReady: () => true }; it('returns true only for UsageCollector instances', () => { - const logger = mockLogger(); const collectors = new CollectorSet({ logger }); const usageCollector = new UsageCollector(logger, collectorOptions); const collector = new Collector(logger, collectorOptions); const randomClass = new (class Random {})(); - expect(collectors.isUsageCollector(usageCollector)).to.be(true); - expect(collectors.isUsageCollector(collector)).to.be(false); - expect(collectors.isUsageCollector(randomClass)).to.be(false); - expect(collectors.isUsageCollector({})).to.be(false); - expect(collectors.isUsageCollector(null)).to.be(false); - expect(collectors.isUsageCollector('')).to.be(false); - expect(collectors.isUsageCollector()).to.be(false); + expect(collectors.isUsageCollector(usageCollector)).toEqual(true); + expect(collectors.isUsageCollector(collector)).toEqual(false); + expect(collectors.isUsageCollector(randomClass)).toEqual(false); + expect(collectors.isUsageCollector({})).toEqual(false); + expect(collectors.isUsageCollector(null)).toEqual(false); + expect(collectors.isUsageCollector('')).toEqual(false); + expect(collectors.isUsageCollector(void 0)).toEqual(false); }); }); }); diff --git a/src/plugins/usage_collection/server/collector/collector_set.ts b/src/plugins/usage_collection/server/collector/collector_set.ts index a87accc47535e..6cc5d057b080a 100644 --- a/src/plugins/usage_collection/server/collector/collector_set.ts +++ b/src/plugins/usage_collection/server/collector/collector_set.ts @@ -20,39 +20,37 @@ import { snakeCase } from 'lodash'; import { Logger } from 'kibana/server'; import { CallCluster } from 'src/legacy/core_plugins/elasticsearch'; -// @ts-ignore -import { Collector } from './collector'; -// @ts-ignore +import { Collector, CollectorOptions } from './collector'; import { UsageCollector } from './usage_collector'; interface CollectorSetConfig { logger: Logger; - maximumWaitTimeForAllCollectorsInS: number; - collectors?: Collector[]; + maximumWaitTimeForAllCollectorsInS?: number; + collectors?: Array>; } export class CollectorSet { private _waitingForAllCollectorsTimestamp?: number; private logger: Logger; private readonly maximumWaitTimeForAllCollectorsInS: number; - private collectors: Collector[] = []; + private collectors: Array> = []; constructor({ logger, maximumWaitTimeForAllCollectorsInS, collectors = [] }: CollectorSetConfig) { this.logger = logger; this.collectors = collectors; this.maximumWaitTimeForAllCollectorsInS = maximumWaitTimeForAllCollectorsInS || 60; } - public makeStatsCollector = (options: any) => { + public makeStatsCollector = (options: CollectorOptions) => { return new Collector(this.logger, options); }; - public makeUsageCollector = (options: any) => { + public makeUsageCollector = (options: CollectorOptions) => { return new UsageCollector(this.logger, options); }; /* * @param collector {Collector} collector object */ - public registerCollector = (collector: Collector) => { + public registerCollector = (collector: Collector) => { // check instanceof if (!(collector instanceof Collector)) { throw new Error('CollectorSet can only have Collector instances registered'); @@ -115,7 +113,7 @@ export class CollectorSet { public bulkFetch = async ( callCluster: CallCluster, - collectors: Collector[] = this.collectors + collectors: Array> = this.collectors ) => { const responses = []; for (const collector of collectors) { @@ -123,7 +121,7 @@ export class CollectorSet { try { responses.push({ type: collector.type, - result: await collector.fetchInternal(callCluster), + result: await collector.fetch(callCluster), }); } catch (err) { this.logger.warn(err); @@ -148,14 +146,13 @@ export class CollectorSet { }; // convert an array of fetched stats results into key/object - public toObject = (statsData: any) => { - if (!statsData) return {}; - return statsData.reduce((accumulatedStats: any, { type, result }: any) => { + public toObject = (statsData: Array<{ type: string; result: T }> = []) => { + return statsData.reduce((accumulatedStats, { type, result }) => { return { ...accumulatedStats, [type]: result, }; - }, {}); + }, {} as Result); }; // rename fields to use api conventions diff --git a/src/plugins/usage_collection/server/collector/index.ts b/src/plugins/usage_collection/server/collector/index.ts index 962f61474c250..0d3939e1dc681 100644 --- a/src/plugins/usage_collection/server/collector/index.ts +++ b/src/plugins/usage_collection/server/collector/index.ts @@ -18,7 +18,5 @@ */ export { CollectorSet } from './collector_set'; -// @ts-ignore export { Collector } from './collector'; -// @ts-ignore export { UsageCollector } from './usage_collector'; diff --git a/src/plugins/usage_collection/server/collector/usage_collector.js b/src/plugins/usage_collection/server/collector/usage_collector.js deleted file mode 100644 index 54863474dbd01..0000000000000 --- a/src/plugins/usage_collection/server/collector/usage_collector.js +++ /dev/null @@ -1,51 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { KIBANA_STATS_TYPE } from '../../common/constants'; -import { Collector } from './collector'; - -export class UsageCollector extends Collector { - /* - * @param {Object} logger - logger object - * @param {String} options.type - property name as the key for the data - * @param {Function} options.init (optional) - initialization function - * @param {Function} options.fetch - function to query data - * @param {Function} options.formatForBulkUpload - optional - * @param {Function} options.rest - optional other properties - */ - constructor(logger, { type, init, fetch, formatForBulkUpload = null, ...options } = {}) { - super(logger, { type, init, fetch, formatForBulkUpload, ...options }); - - /* - * Currently, for internal bulk uploading, usage stats are part of - * `kibana_stats` type, under the `usage` namespace in the document. - */ - const defaultUsageFormatterForBulkUpload = result => { - return { - type: KIBANA_STATS_TYPE, - payload: { - usage: { - [type]: result, - }, - }, - }; - }; - this._formatForBulkUpload = formatForBulkUpload || defaultUsageFormatterForBulkUpload; - } -} diff --git a/src/plugins/usage_collection/server/collector/usage_collector.ts b/src/plugins/usage_collection/server/collector/usage_collector.ts new file mode 100644 index 0000000000000..05c701bd3abf4 --- /dev/null +++ b/src/plugins/usage_collection/server/collector/usage_collector.ts @@ -0,0 +1,37 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { KIBANA_STATS_TYPE } from '../../common/constants'; +import { Collector } from './collector'; + +export class UsageCollector extends Collector< + T, + U +> { + protected defaultUsageFormatterForBulkUpload(result: T) { + return { + type: KIBANA_STATS_TYPE, + payload: { + usage: { + [this.type]: result, + }, + }, + }; + } +} diff --git a/x-pack/plugins/cloud/server/collectors/cloud_usage_collector.test.ts b/x-pack/plugins/cloud/server/collectors/cloud_usage_collector.test.ts index a731de341435c..7ec3888e4e1e4 100644 --- a/x-pack/plugins/cloud/server/collectors/cloud_usage_collector.test.ts +++ b/x-pack/plugins/cloud/server/collectors/cloud_usage_collector.test.ts @@ -21,12 +21,13 @@ describe('createCloudUsageCollector', () => { }); describe('Fetched Usage data', () => { - it('return isCloudEnabled boolean', () => { + it('return isCloudEnabled boolean', async () => { const mockConfigs = getMockConfigs(true); const usageCollection = mockUsageCollection() as any; const collector = createCloudUsageCollector(usageCollection, mockConfigs); + const callCluster = {} as any; // Sending any as the callCluster client because it's not needed in this collector but TS requires it when calling it. - expect(collector.fetch().isCloudEnabled).toBe(true); + expect((await collector.fetch(callCluster)).isCloudEnabled).toBe(true); // Adding the await because the fetch can be a Promise or a synchronous method and TS complains in the test if not awaited }); }); }); From 367086b6fa41220ce5eb4e01cc57c7577dfab883 Mon Sep 17 00:00:00 2001 From: Daniil Suleiman <31325372+sulemanof@users.noreply.github.com> Date: Wed, 5 Feb 2020 17:25:01 +0300 Subject: [PATCH 5/5] Remove the editor config provider registry (#56501) * Remove the editor_config_providers * Remove unused translations * Fix eslint errors Co-authored-by: Elastic Machine --- .../public/components/agg_param_props.ts | 3 +- .../public/components/agg_params.test.tsx | 6 +- .../public/components/agg_params.tsx | 18 +- .../components/agg_params_helper.test.ts | 10 +- .../public/components/agg_params_helper.ts | 2 +- .../public/components/controls/test_utils.ts | 3 +- .../components/controls/time_interval.tsx | 2 +- .../public/components/utils/editor_config.ts | 135 +++++++++++ .../public/components/utils}/index.ts | 3 +- .../public/legacy_imports.ts | 1 - .../config/editor_config_providers.test.ts | 210 ------------------ .../vis/config/editor_config_providers.ts | 168 -------------- src/legacy/ui/public/vis/config/types.ts | 57 ----- .../data/public/index_patterns/index.ts | 8 +- .../index_patterns/index_patterns/index.ts | 1 + .../index_patterns/index_pattern.ts | 7 +- .../index_patterns/index_patterns/types.ts | 35 +++ .../lens/public/indexpattern_plugin/loader.ts | 38 ++-- .../operations/definitions/date_histogram.tsx | 2 +- .../lens/public/indexpattern_plugin/types.ts | 13 +- x-pack/legacy/plugins/rollup/public/legacy.ts | 2 - .../plugins/rollup/public/legacy_imports.ts | 1 - x-pack/legacy/plugins/rollup/public/plugin.ts | 11 +- .../rollup/public/visualize/editor_config.js | 84 ------- .../translations/translations/ja-JP.json | 2 - .../translations/translations/zh-CN.json | 2 - 26 files changed, 221 insertions(+), 603 deletions(-) create mode 100644 src/legacy/core_plugins/vis_default_editor/public/components/utils/editor_config.ts rename src/legacy/{ui/public/vis/config => core_plugins/vis_default_editor/public/components/utils}/index.ts (86%) delete mode 100644 src/legacy/ui/public/vis/config/editor_config_providers.test.ts delete mode 100644 src/legacy/ui/public/vis/config/editor_config_providers.ts delete mode 100644 src/legacy/ui/public/vis/config/types.ts create mode 100644 src/plugins/data/public/index_patterns/index_patterns/types.ts delete mode 100644 x-pack/legacy/plugins/rollup/public/visualize/editor_config.js diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/agg_param_props.ts b/src/legacy/core_plugins/vis_default_editor/public/components/agg_param_props.ts index fc535884c69ff..c858fb62045ca 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/agg_param_props.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/components/agg_param_props.ts @@ -19,8 +19,9 @@ import { Field } from 'src/plugins/data/public'; import { VisState } from 'src/legacy/core_plugins/visualizations/public'; -import { IAggConfig, AggParam, EditorConfig } from '../legacy_imports'; +import { IAggConfig, AggParam } from '../legacy_imports'; import { ComboBoxGroupedOptions } from '../utils'; +import { EditorConfig } from './utils'; // NOTE: we cannot export the interface with export { InterfaceName } // as there is currently a bug on babel typescript transform plugin for it diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.test.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.test.tsx index 5636059394bac..af851aa9b4418 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.test.tsx +++ b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.test.tsx @@ -36,10 +36,8 @@ const mockEditorConfig = { }; jest.mock('ui/new_platform'); -jest.mock('ui/vis/config', () => ({ - editorConfigProviders: { - getConfigForAgg: jest.fn(() => mockEditorConfig), - }, +jest.mock('./utils', () => ({ + getEditorConfig: jest.fn(() => mockEditorConfig), })); jest.mock('./agg_params_helper', () => ({ getAggParamsToRender: jest.fn(() => ({ diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.tsx index 1b450957f3b26..e9583ab4cec79 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.tsx +++ b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params.tsx @@ -23,14 +23,7 @@ import { i18n } from '@kbn/i18n'; import useUnmount from 'react-use/lib/useUnmount'; import { IndexPattern } from 'src/plugins/data/public'; -import { - IAggConfig, - AggGroupNames, - editorConfigProviders, - FixedParam, - TimeIntervalParam, - EditorParamConfig, -} from '../legacy_imports'; +import { IAggConfig, AggGroupNames } from '../legacy_imports'; import { DefaultEditorAggSelect } from './agg_select'; import { DefaultEditorAggParam } from './agg_param'; @@ -46,6 +39,7 @@ import { initAggParamsState, } from './agg_params_state'; import { DefaultEditorCommonProps } from './agg_common_props'; +import { EditorParamConfig, TimeIntervalParam, FixedParam, getEditorConfig } from './utils'; const FIXED_VALUE_PROP = 'fixedValue'; const DEFAULT_PROP = 'default'; @@ -93,10 +87,12 @@ function DefaultEditorAggParams({ values: { schema: agg.schema.title }, }) : ''; - - const editorConfig = useMemo(() => editorConfigProviders.getConfigForAgg(indexPattern, agg), [ + const aggTypeName = agg.type?.name; + const fieldName = agg.params?.field?.name; + const editorConfig = useMemo(() => getEditorConfig(indexPattern, aggTypeName, fieldName), [ indexPattern, - agg, + aggTypeName, + fieldName, ]); const params = useMemo(() => getAggParamsToRender({ agg, editorConfig, metricAggs, state }), [ agg, diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts index ec56d22143699..b9fb81fccd32c 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.test.ts @@ -19,20 +19,14 @@ import { IndexPattern, Field } from 'src/plugins/data/public'; import { VisState } from 'src/legacy/core_plugins/visualizations/public'; -import { - IAggConfig, - IAggType, - AggGroupNames, - BUCKET_TYPES, - IndexedArray, - EditorConfig, -} from '../legacy_imports'; +import { IAggConfig, IAggType, AggGroupNames, BUCKET_TYPES, IndexedArray } from '../legacy_imports'; import { getAggParamsToRender, getAggTypeOptions, isInvalidParamsTouched, } from './agg_params_helper'; import { FieldParamEditor, OrderByParamEditor } from './controls'; +import { EditorConfig } from './utils'; jest.mock('../utils', () => ({ groupAndSortBy: jest.fn(() => ['indexedFields']), diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts index 5a9d95725c8e4..25aa21fc83b31 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/components/agg_params_helper.ts @@ -33,8 +33,8 @@ import { AggParam, IFieldParamType, IAggType, - EditorConfig, } from '../legacy_imports'; +import { EditorConfig } from './utils'; interface ParamInstanceBase { agg: IAggConfig; diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/test_utils.ts b/src/legacy/core_plugins/vis_default_editor/public/components/controls/test_utils.ts index 894bc594a08d7..4280f85c901d7 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/test_utils.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/test_utils.ts @@ -18,7 +18,8 @@ */ import { VisState } from 'src/legacy/core_plugins/visualizations/public'; -import { IAggConfig, AggParam, EditorConfig } from '../../legacy_imports'; +import { IAggConfig, AggParam } from '../../legacy_imports'; +import { EditorConfig } from '../utils'; export const aggParamCommonPropsMock = { agg: {} as IAggConfig, diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/controls/time_interval.tsx b/src/legacy/core_plugins/vis_default_editor/public/components/controls/time_interval.tsx index 6168890c2f2da..5da0d6462a8ba 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/components/controls/time_interval.tsx +++ b/src/legacy/core_plugins/vis_default_editor/public/components/controls/time_interval.tsx @@ -107,7 +107,7 @@ function TimeIntervalParamEditor({ const onChange = (opts: EuiComboBoxOptionProps[]) => { const selectedOpt: ComboBoxOption = get(opts, '0'); - setValue(selectedOpt ? selectedOpt.key : selectedOpt); + setValue(selectedOpt ? selectedOpt.key : ''); if (selectedOpt) { agg.write(); diff --git a/src/legacy/core_plugins/vis_default_editor/public/components/utils/editor_config.ts b/src/legacy/core_plugins/vis_default_editor/public/components/utils/editor_config.ts new file mode 100644 index 0000000000000..80a64b7289f8c --- /dev/null +++ b/src/legacy/core_plugins/vis_default_editor/public/components/utils/editor_config.ts @@ -0,0 +1,135 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { i18n } from '@kbn/i18n'; +import { IndexPattern } from 'src/plugins/data/public'; + +/** + * A hidden parameter can be hidden from the UI completely. + */ +interface Param { + hidden?: boolean; + help?: string; +} + +/** + * A fixed parameter has a fixed value for a specific field. + * It can optionally also be hidden. + */ +export type FixedParam = Partial & { + fixedValue: any; +}; + +/** + * Numeric interval parameters must always be set in the editor to a multiple of + * the specified base. It can optionally also be hidden. + */ +export type NumericIntervalParam = Partial & { + base: number; +}; + +/** + * Time interval parameters must always be set in the editor to a multiple of + * the specified base. It can optionally also be hidden. + */ +export type TimeIntervalParam = Partial & { + default: string; + timeBase: string; +}; + +export type EditorParamConfig = NumericIntervalParam | TimeIntervalParam | FixedParam | Param; + +export interface EditorConfig { + [paramName: string]: EditorParamConfig; +} + +export function getEditorConfig( + indexPattern: IndexPattern, + aggTypeName: string, + fieldName: string +): EditorConfig { + const aggRestrictions = indexPattern.getAggregationRestrictions(); + + if (!aggRestrictions || !aggTypeName || !fieldName) { + return {}; + } + + // Exclude certain param options for terms: + // otherBucket, missingBucket, orderBy, orderAgg + if (aggTypeName === 'terms') { + return { + otherBucket: { + hidden: true, + }, + missingBucket: { + hidden: true, + }, + }; + } + + const fieldAgg = aggRestrictions[aggTypeName] && aggRestrictions[aggTypeName][fieldName]; + + if (!fieldAgg) { + return {}; + } + + // Set interval and base interval for histograms based on agg restrictions + if (aggTypeName === 'histogram') { + const interval = fieldAgg.interval; + return interval + ? { + intervalBase: { + fixedValue: interval, + }, + interval: { + base: interval, + help: i18n.translate('visDefaultEditor.editorConfig.histogram.interval.helpText', { + defaultMessage: 'Must be a multiple of configuration interval: {interval}', + values: { interval }, + }), + }, + } + : {}; + } + + // Set date histogram time zone based on agg restrictions + if (aggTypeName === 'date_histogram') { + // Interval is deprecated on date_histogram rollups, but may still be present + // See https://github.com/elastic/kibana/pull/36310 + const interval = fieldAgg.calendar_interval || fieldAgg.fixed_interval; + return { + useNormalizedEsInterval: { + fixedValue: false, + }, + interval: { + default: interval, + timeBase: interval, + help: i18n.translate( + 'visDefaultEditor.editorConfig.dateHistogram.customInterval.helpText', + { + defaultMessage: 'Must be a multiple of configuration interval: {interval}', + values: { interval }, + } + ), + }, + }; + } + + return {}; +} diff --git a/src/legacy/ui/public/vis/config/index.ts b/src/legacy/core_plugins/vis_default_editor/public/components/utils/index.ts similarity index 86% rename from src/legacy/ui/public/vis/config/index.ts rename to src/legacy/core_plugins/vis_default_editor/public/components/utils/index.ts index ee7385518a85d..14570356103b1 100644 --- a/src/legacy/ui/public/vis/config/index.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/components/utils/index.ts @@ -17,5 +17,4 @@ * under the License. */ -export { editorConfigProviders, EditorConfigProviderRegistry } from './editor_config_providers'; -export * from './types'; +export * from './editor_config'; diff --git a/src/legacy/core_plugins/vis_default_editor/public/legacy_imports.ts b/src/legacy/core_plugins/vis_default_editor/public/legacy_imports.ts index f023b808cb0a7..a700995ec596b 100644 --- a/src/legacy/core_plugins/vis_default_editor/public/legacy_imports.ts +++ b/src/legacy/core_plugins/vis_default_editor/public/legacy_imports.ts @@ -54,4 +54,3 @@ export { getDocLink } from 'ui/documentation_links'; export { documentationLinks } from 'ui/documentation_links/documentation_links'; export { move } from 'ui/utils/collection'; export * from 'ui/vis/lib'; -export * from 'ui/vis/config'; diff --git a/src/legacy/ui/public/vis/config/editor_config_providers.test.ts b/src/legacy/ui/public/vis/config/editor_config_providers.test.ts deleted file mode 100644 index d52c9119dd76a..0000000000000 --- a/src/legacy/ui/public/vis/config/editor_config_providers.test.ts +++ /dev/null @@ -1,210 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { IAggConfig } from 'ui/agg_types'; -import { EditorConfigProviderRegistry } from './editor_config_providers'; -import { EditorParamConfig, FixedParam, NumericIntervalParam, TimeIntervalParam } from './types'; - -jest.mock('ui/new_platform'); - -describe('EditorConfigProvider', () => { - let registry: EditorConfigProviderRegistry; - const indexPattern = { - id: '1234', - title: 'logstash-*', - fields: [ - { - name: 'response', - type: 'number', - esTypes: ['integer'], - aggregatable: true, - filterable: true, - searchable: true, - }, - ], - } as any; - - beforeEach(() => { - registry = new EditorConfigProviderRegistry(); - }); - - it('should call registered providers with given parameters', () => { - const provider = jest.fn(() => ({})); - registry.register(provider); - expect(provider).not.toHaveBeenCalled(); - const aggConfig = {} as IAggConfig; - registry.getConfigForAgg(indexPattern, aggConfig); - expect(provider).toHaveBeenCalledWith(indexPattern, aggConfig); - }); - - it('should call all registered providers with given parameters', () => { - const provider = jest.fn(() => ({})); - const provider2 = jest.fn(() => ({})); - registry.register(provider); - registry.register(provider2); - expect(provider).not.toHaveBeenCalled(); - expect(provider2).not.toHaveBeenCalled(); - const aggConfig = {} as IAggConfig; - registry.getConfigForAgg(indexPattern, aggConfig); - expect(provider).toHaveBeenCalledWith(indexPattern, aggConfig); - expect(provider2).toHaveBeenCalledWith(indexPattern, aggConfig); - }); - - describe('merging configs', () => { - function singleConfig(paramConfig: EditorParamConfig) { - return () => ({ singleParam: paramConfig }); - } - - function getOutputConfig(reg: EditorConfigProviderRegistry) { - return reg.getConfigForAgg(indexPattern, {} as IAggConfig).singleParam; - } - - it('should have hidden true if at least one config was hidden true', () => { - registry.register(singleConfig({ hidden: false })); - registry.register(singleConfig({ hidden: true })); - registry.register(singleConfig({ hidden: false })); - const config = getOutputConfig(registry); - expect(config.hidden).toBe(true); - }); - - it('should merge the same fixed values', () => { - registry.register(singleConfig({ fixedValue: 'foo' })); - registry.register(singleConfig({ fixedValue: 'foo' })); - const config = getOutputConfig(registry) as FixedParam; - expect(config).toHaveProperty('fixedValue'); - expect(config.fixedValue).toBe('foo'); - }); - - it('should throw having different fixed values', () => { - registry.register(singleConfig({ fixedValue: 'foo' })); - registry.register(singleConfig({ fixedValue: 'bar' })); - expect(() => { - getOutputConfig(registry); - }).toThrowError(); - }); - - it('should allow same base values', () => { - registry.register(singleConfig({ base: 5 })); - registry.register(singleConfig({ base: 5 })); - const config = getOutputConfig(registry) as NumericIntervalParam; - expect(config).toHaveProperty('base'); - expect(config.base).toBe(5); - }); - - it('should merge multiple base values, using least common multiple', () => { - registry.register(singleConfig({ base: 2 })); - registry.register(singleConfig({ base: 5 })); - registry.register(singleConfig({ base: 8 })); - const config = getOutputConfig(registry) as NumericIntervalParam; - expect(config).toHaveProperty('base'); - expect(config.base).toBe(40); - }); - - it('should throw on combining fixedValue with base', () => { - registry.register(singleConfig({ fixedValue: 'foo' })); - registry.register(singleConfig({ base: 5 })); - expect(() => { - getOutputConfig(registry); - }).toThrowError(); - }); - - it('should allow same timeBase values', () => { - registry.register(singleConfig({ timeBase: '2h', default: '2h' })); - registry.register(singleConfig({ timeBase: '2h', default: '2h' })); - const config = getOutputConfig(registry) as TimeIntervalParam; - expect(config).toHaveProperty('timeBase'); - expect(config).toHaveProperty('default'); - expect(config.timeBase).toBe('2h'); - expect(config.default).toBe('2h'); - }); - - it('should merge multiple compatible timeBase values, using least common interval', () => { - registry.register(singleConfig({ timeBase: '2h', default: '2h' })); - registry.register(singleConfig({ timeBase: '3h', default: '3h' })); - registry.register(singleConfig({ timeBase: '4h', default: '4h' })); - const config = getOutputConfig(registry) as TimeIntervalParam; - expect(config).toHaveProperty('timeBase'); - expect(config).toHaveProperty('default'); - expect(config.timeBase).toBe('12h'); - expect(config.default).toBe('12h'); - }); - - it('should throw on combining incompatible timeBase values', () => { - registry.register(singleConfig({ timeBase: '2h', default: '2h' })); - registry.register(singleConfig({ timeBase: '1d', default: '1d' })); - expect(() => { - getOutputConfig(registry); - }).toThrowError(); - }); - - it('should throw on invalid timeBase values', () => { - registry.register(singleConfig({ timeBase: '2w', default: '2w' })); - expect(() => { - getOutputConfig(registry); - }).toThrowError(); - }); - - it('should throw if timeBase and default are different', () => { - registry.register(singleConfig({ timeBase: '1h', default: '2h' })); - expect(() => { - getOutputConfig(registry); - }).toThrowError(); - }); - - it('should merge hidden together with fixedValue', () => { - registry.register(singleConfig({ fixedValue: 'foo', hidden: true })); - registry.register(singleConfig({ fixedValue: 'foo', hidden: false })); - const config = getOutputConfig(registry) as FixedParam; - expect(config).toHaveProperty('fixedValue'); - expect(config).toHaveProperty('hidden'); - expect(config.fixedValue).toBe('foo'); - expect(config.hidden).toBe(true); - }); - - it('should merge hidden together with base', () => { - registry.register(singleConfig({ base: 2, hidden: false })); - registry.register(singleConfig({ base: 13, hidden: false })); - const config = getOutputConfig(registry) as NumericIntervalParam; - expect(config).toHaveProperty('base'); - expect(config).toHaveProperty('hidden'); - expect(config.base).toBe(26); - expect(config.hidden).toBe(false); - }); - - it('should merge hidden together with timeBase', () => { - registry.register(singleConfig({ timeBase: '2h', default: '2h', hidden: false })); - registry.register(singleConfig({ timeBase: '4h', default: '4h', hidden: false })); - const config = getOutputConfig(registry) as TimeIntervalParam; - expect(config).toHaveProperty('timeBase'); - expect(config).toHaveProperty('default'); - expect(config).toHaveProperty('hidden'); - expect(config.timeBase).toBe('4h'); - expect(config.default).toBe('4h'); - expect(config.hidden).toBe(false); - }); - - it('should merge helps together into one string', () => { - registry.register(singleConfig({ help: 'Warning' })); - registry.register(singleConfig({ help: 'Another help' })); - const config = getOutputConfig(registry); - expect(config).toHaveProperty('help'); - expect(config.help).toBe('Warning\n\nAnother help'); - }); - }); -}); diff --git a/src/legacy/ui/public/vis/config/editor_config_providers.ts b/src/legacy/ui/public/vis/config/editor_config_providers.ts deleted file mode 100644 index ec82597d5fb19..0000000000000 --- a/src/legacy/ui/public/vis/config/editor_config_providers.ts +++ /dev/null @@ -1,168 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -import { IndexPattern } from 'src/plugins/data/public'; -import { IAggConfig } from 'ui/agg_types'; -import { parseEsInterval } from '../../../../core_plugins/data/public'; -import { - TimeIntervalParam, - EditorConfig, - EditorParamConfig, - FixedParam, - NumericIntervalParam, -} from './types'; -import { leastCommonInterval, leastCommonMultiple } from '../lib'; - -type EditorConfigProvider = (indexPattern: IndexPattern, aggConfig: IAggConfig) => EditorConfig; - -class EditorConfigProviderRegistry { - private providers: Set = new Set(); - - public register(configProvider: EditorConfigProvider): void { - this.providers.add(configProvider); - } - - public getConfigForAgg(indexPattern: IndexPattern, aggConfig: IAggConfig): EditorConfig { - const configs = Array.from(this.providers).map(provider => provider(indexPattern, aggConfig)); - return this.mergeConfigs(configs); - } - - private isTimeBaseParam(config: EditorParamConfig): config is TimeIntervalParam { - return config.hasOwnProperty('default') && config.hasOwnProperty('timeBase'); - } - - private isBaseParam(config: EditorParamConfig): config is NumericIntervalParam { - return config.hasOwnProperty('base'); - } - - private isFixedParam(config: EditorParamConfig): config is FixedParam { - return config.hasOwnProperty('fixedValue'); - } - - private mergeHidden(current: EditorParamConfig, merged: EditorParamConfig): boolean { - return Boolean(current.hidden || merged.hidden); - } - - private mergeHelp(current: EditorParamConfig, merged: EditorParamConfig): string | undefined { - if (!current.help) { - return merged.help; - } - - return merged.help ? `${merged.help}\n\n${current.help}` : current.help; - } - - private mergeFixedAndBase( - current: EditorParamConfig, - merged: EditorParamConfig, - paramName: string - ): { fixedValue: unknown } | { base: number } | {} { - if ( - this.isFixedParam(current) && - this.isFixedParam(merged) && - current.fixedValue !== merged.fixedValue - ) { - // In case multiple configurations provided a fixedValue, these must all be the same. - // If not we'll throw an error. - throw new Error(`Two EditorConfigProviders provided different fixed values for field ${paramName}: - ${merged.fixedValue} !== ${current.fixedValue}`); - } - - if ( - (this.isFixedParam(current) && this.isBaseParam(merged)) || - (this.isBaseParam(current) && this.isFixedParam(merged)) - ) { - // In case one config tries to set a fixed value and another setting a base value, - // we'll throw an error. This could be solved more elegantly, by allowing fixedValues - // that are the multiple of the specific base value, but since there is no use-case for that - // right now, this isn't implemented. - throw new Error(`Tried to provide a fixedValue and a base for param ${paramName}.`); - } - - if (this.isBaseParam(current) && this.isBaseParam(merged)) { - // In case where both had interval values, just use the least common multiple between both interval - return { - base: leastCommonMultiple(current.base, merged.base), - }; - } - - // In this case we haven't had a fixed value of base for that param yet, we use the one specified - // in the current config - if (this.isFixedParam(current)) { - return { - fixedValue: current.fixedValue, - }; - } - if (this.isBaseParam(current)) { - return { - base: current.base, - }; - } - - return {}; - } - - private mergeTimeBase( - current: TimeIntervalParam, - merged: EditorParamConfig, - paramName: string - ): { timeBase: string; default: string } { - if (current.default !== current.timeBase) { - throw new Error(`Tried to provide differing default and timeBase values for ${paramName}.`); - } - - if (this.isTimeBaseParam(merged)) { - // In case both had where interval values, just use the least common multiple between both intervals - const timeBase = leastCommonInterval(current.timeBase, merged.timeBase); - return { - default: timeBase, - timeBase, - }; - } - - // This code is simply here to throw an error in case the `timeBase` is not a valid ES interval - parseEsInterval(current.timeBase); - return { - default: current.timeBase, - timeBase: current.timeBase, - }; - } - - private mergeConfigs(configs: EditorConfig[]): EditorConfig { - return configs.reduce((output, conf) => { - Object.entries(conf).forEach(([paramName, paramConfig]) => { - if (!output[paramName]) { - output[paramName] = {}; - } - - output[paramName] = { - hidden: this.mergeHidden(paramConfig, output[paramName]), - help: this.mergeHelp(paramConfig, output[paramName]), - ...(this.isTimeBaseParam(paramConfig) - ? this.mergeTimeBase(paramConfig, output[paramName], paramName) - : this.mergeFixedAndBase(paramConfig, output[paramName], paramName)), - }; - }); - return output; - }, {}); - } -} - -const editorConfigProviders = new EditorConfigProviderRegistry(); - -export { editorConfigProviders, EditorConfigProviderRegistry }; diff --git a/src/legacy/ui/public/vis/config/types.ts b/src/legacy/ui/public/vis/config/types.ts deleted file mode 100644 index 61c0ced3cd519..0000000000000 --- a/src/legacy/ui/public/vis/config/types.ts +++ /dev/null @@ -1,57 +0,0 @@ -/* - * Licensed to Elasticsearch B.V. under one or more contributor - * license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright - * ownership. Elasticsearch B.V. licenses this file to you under - * the Apache License, Version 2.0 (the "License"); you may - * not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, - * software distributed under the License is distributed on an - * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY - * KIND, either express or implied. See the License for the - * specific language governing permissions and limitations - * under the License. - */ - -/** - * A hidden parameter can be hidden from the UI completely. - */ -interface Param { - hidden?: boolean; - help?: string; -} - -/** - * A fixed parameter has a fixed value for a specific field. - * It can optionally also be hidden. - */ -export type FixedParam = Partial & { - fixedValue: any; -}; - -/** - * Numeric interval parameters must always be set in the editor to a multiple of - * the specified base. It can optionally also be hidden. - */ -export type NumericIntervalParam = Partial & { - base: number; -}; - -/** - * Time interval parameters must always be set in the editor to a multiple of - * the specified base. It can optionally also be hidden. - */ -export type TimeIntervalParam = Partial & { - default: string; - timeBase: string; -}; - -export type EditorParamConfig = NumericIntervalParam | TimeIntervalParam | FixedParam | Param; - -export interface EditorConfig { - [paramName: string]: EditorParamConfig; -} diff --git a/src/plugins/data/public/index_patterns/index.ts b/src/plugins/data/public/index_patterns/index.ts index 7444126ee6cae..3d902ebbb7c23 100644 --- a/src/plugins/data/public/index_patterns/index.ts +++ b/src/plugins/data/public/index_patterns/index.ts @@ -47,4 +47,10 @@ export const indexPatterns = { export { Field, FieldList, IFieldList } from './fields'; // TODO: figure out how to replace IndexPatterns in get_inner_angular. -export { IndexPattern, IndexPatterns, IndexPatternsContract } from './index_patterns'; +export { + IndexPattern, + IndexPatterns, + IndexPatternsContract, + TypeMeta, + AggregationRestrictions, +} from './index_patterns'; diff --git a/src/plugins/data/public/index_patterns/index_patterns/index.ts b/src/plugins/data/public/index_patterns/index_patterns/index.ts index 4ca7e053a4492..fca82025cdc66 100644 --- a/src/plugins/data/public/index_patterns/index_patterns/index.ts +++ b/src/plugins/data/public/index_patterns/index_patterns/index.ts @@ -22,3 +22,4 @@ export * from './format_hit'; export * from './index_pattern'; export * from './index_patterns'; export * from './index_patterns_api_client'; +export * from './types'; diff --git a/src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts b/src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts index 5c09e22b6dbb4..c09c9f4828799 100644 --- a/src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts +++ b/src/plugins/data/public/index_patterns/index_patterns/index_pattern.ts @@ -38,6 +38,7 @@ import { formatHitProvider } from './format_hit'; import { flattenHitWrapper } from './flatten_hit'; import { IIndexPatternsApiClient } from './index_patterns_api_client'; import { getNotifications, getFieldFormats } from '../../services'; +import { TypeMeta } from './types'; const MAX_ATTEMPTS_TO_RESOLVE_CONFLICTS = 3; const type = 'index-pattern'; @@ -49,7 +50,7 @@ export class IndexPattern implements IIndexPattern { public title: string = ''; public type?: string; public fieldFormatMap: any; - public typeMeta: any; + public typeMeta?: TypeMeta; public fields: IFieldList; public timeFieldName: string | undefined; public formatHit: any; @@ -336,6 +337,10 @@ export class IndexPattern implements IIndexPattern { return this.fields.getByName(name); } + getAggregationRestrictions() { + return this.typeMeta?.aggs; + } + isWildcard() { return _.includes(this.title, '*'); } diff --git a/src/plugins/data/public/index_patterns/index_patterns/types.ts b/src/plugins/data/public/index_patterns/index_patterns/types.ts new file mode 100644 index 0000000000000..b2060dd1d48ba --- /dev/null +++ b/src/plugins/data/public/index_patterns/index_patterns/types.ts @@ -0,0 +1,35 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +export type AggregationRestrictions = Record< + string, + { + agg?: string; + interval?: number; + fixed_interval?: string; + calendar_interval?: string; + delay?: string; + time_zone?: string; + } +>; + +export interface TypeMeta { + aggs?: Record; + [key: string]: any; +} diff --git a/x-pack/legacy/plugins/lens/public/indexpattern_plugin/loader.ts b/x-pack/legacy/plugins/lens/public/indexpattern_plugin/loader.ts index c9473c1869868..f0692f120f9b5 100644 --- a/x-pack/legacy/plugins/lens/public/indexpattern_plugin/loader.ts +++ b/x-pack/legacy/plugins/lens/public/indexpattern_plugin/loader.ts @@ -15,13 +15,12 @@ import { IndexPatternPersistedState, IndexPatternPrivateState, IndexPatternField, - AggregationRestrictions, } from './types'; import { updateLayerIndexPattern } from './state_helpers'; import { DateRange, ExistingFields } from '../../common/types'; import { BASE_API_URL } from '../../common'; import { documentField } from './document_field'; -import { isNestedField, IFieldType } from '../../../../../../src/plugins/data/public'; +import { isNestedField, IFieldType, TypeMeta } from '../../../../../../src/plugins/data/public'; interface SavedIndexPatternAttributes extends SavedObjectAttributes { title: string; @@ -31,12 +30,7 @@ interface SavedIndexPatternAttributes extends SavedObjectAttributes { typeMeta: string; } -interface SavedRestrictionsObject { - aggs: Record; -} - type SetState = StateSetter; -type SavedRestrictionsInfo = SavedRestrictionsObject | undefined; type SavedObjectsClient = Pick; type ErrorHandler = (err: Error) => void; @@ -275,9 +269,7 @@ function fromSavedObject( fields: (JSON.parse(attributes.fields) as IFieldType[]) .filter(field => !isNestedField(field) && (!!field.aggregatable || !!field.scripted)) .concat(documentField) as IndexPatternField[], - typeMeta: attributes.typeMeta - ? (JSON.parse(attributes.typeMeta) as SavedRestrictionsInfo) - : undefined, + typeMeta: attributes.typeMeta ? (JSON.parse(attributes.typeMeta) as TypeMeta) : undefined, fieldFormatMap: attributes.fieldFormatMap ? JSON.parse(attributes.fieldFormatMap) : undefined, }; @@ -286,21 +278,23 @@ function fromSavedObject( return indexPattern; } - const aggs = Object.keys(typeMeta.aggs); - const newFields = [...(indexPattern.fields as IndexPatternField[])]; - newFields.forEach((field, index) => { - const restrictionsObj: IndexPatternField['aggregationRestrictions'] = {}; - aggs.forEach(agg => { - const restriction = typeMeta.aggs[agg] && typeMeta.aggs[agg][field.name]; - if (restriction) { - restrictionsObj[agg] = restriction; + + if (typeMeta.aggs) { + const aggs = Object.keys(typeMeta.aggs); + newFields.forEach((field, index) => { + const restrictionsObj: IndexPatternField['aggregationRestrictions'] = {}; + aggs.forEach(agg => { + const restriction = typeMeta.aggs && typeMeta.aggs[agg] && typeMeta.aggs[agg][field.name]; + if (restriction) { + restrictionsObj[agg] = restriction; + } + }); + if (Object.keys(restrictionsObj).length) { + newFields[index] = { ...field, aggregationRestrictions: restrictionsObj }; } }); - if (Object.keys(restrictionsObj).length) { - newFields[index] = { ...field, aggregationRestrictions: restrictionsObj }; - } - }); + } return { id: indexPattern.id, diff --git a/x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.tsx b/x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.tsx index ae12be90ddd05..4cfd7195d1fc0 100644 --- a/x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.tsx +++ b/x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations/definitions/date_histogram.tsx @@ -27,7 +27,7 @@ import { updateColumnParam } from '../../state_helpers'; import { OperationDefinition } from '.'; import { FieldBasedIndexPatternColumn } from './column_types'; import { autoIntervalFromDateRange } from '../../auto_date'; -import { AggregationRestrictions } from '../../types'; +import { AggregationRestrictions } from '../../../../../../../../src/plugins/data/public'; const autoInterval = 'auto'; const calendarOnlyIntervals = new Set(['w', 'M', 'q', 'y']); diff --git a/x-pack/legacy/plugins/lens/public/indexpattern_plugin/types.ts b/x-pack/legacy/plugins/lens/public/indexpattern_plugin/types.ts index e556ddda10679..546d148be1e29 100644 --- a/x-pack/legacy/plugins/lens/public/indexpattern_plugin/types.ts +++ b/x-pack/legacy/plugins/lens/public/indexpattern_plugin/types.ts @@ -5,6 +5,7 @@ */ import { IndexPatternColumn } from './operations'; +import { AggregationRestrictions } from '../../../../../../src/plugins/data/public'; export interface IndexPattern { id: string; @@ -20,18 +21,6 @@ export interface IndexPattern { >; } -export type AggregationRestrictions = Record< - string, - { - agg?: string; - interval?: number; - fixed_interval?: string; - calendar_interval?: string; - delay?: string; - time_zone?: string; - } ->; - export interface IndexPatternField { name: string; type: string; diff --git a/x-pack/legacy/plugins/rollup/public/legacy.ts b/x-pack/legacy/plugins/rollup/public/legacy.ts index a70f181dc86fb..17597672a898e 100644 --- a/x-pack/legacy/plugins/rollup/public/legacy.ts +++ b/x-pack/legacy/plugins/rollup/public/legacy.ts @@ -5,7 +5,6 @@ */ import { npSetup, npStart } from 'ui/new_platform'; -import { editorConfigProviders } from 'ui/vis/config'; import { aggTypeFilters } from 'ui/agg_types'; import { aggTypeFieldFilters } from 'ui/agg_types'; import { addSearchStrategy } from '../../../../../src/plugins/data/public'; @@ -20,7 +19,6 @@ export const setup = plugin.setup(npSetup.core, { __LEGACY: { aggTypeFilters, aggTypeFieldFilters, - editorConfigProviders, addSearchStrategy, addBadgeExtension, addToggleExtension, diff --git a/x-pack/legacy/plugins/rollup/public/legacy_imports.ts b/x-pack/legacy/plugins/rollup/public/legacy_imports.ts index 4fece0fddfa3e..07155a4b0a60e 100644 --- a/x-pack/legacy/plugins/rollup/public/legacy_imports.ts +++ b/x-pack/legacy/plugins/rollup/public/legacy_imports.ts @@ -9,4 +9,3 @@ export { findIllegalCharactersInIndexName, INDEX_ILLEGAL_CHARACTERS_VISIBLE } fr export { AggTypeFilters } from 'ui/agg_types'; export { AggTypeFieldFilters } from 'ui/agg_types'; -export { EditorConfigProviderRegistry } from 'ui/vis/config'; diff --git a/x-pack/legacy/plugins/rollup/public/plugin.ts b/x-pack/legacy/plugins/rollup/public/plugin.ts index 97c03fd1fdfc2..4dae078c90f57 100644 --- a/x-pack/legacy/plugins/rollup/public/plugin.ts +++ b/x-pack/legacy/plugins/rollup/public/plugin.ts @@ -6,11 +6,7 @@ import { i18n } from '@kbn/i18n'; import { CoreSetup, CoreStart, Plugin } from 'kibana/public'; -import { - EditorConfigProviderRegistry, - AggTypeFilters, - AggTypeFieldFilters, -} from './legacy_imports'; +import { AggTypeFilters, AggTypeFieldFilters } from './legacy_imports'; import { SearchStrategyProvider } from '../../../../../src/plugins/data/public'; import { ManagementSetup as ManagementSetupLegacy } from '../../../../../src/legacy/core_plugins/management/public/np_ready'; import { rollupBadgeExtension, rollupToggleExtension } from './extend_index_management'; @@ -23,8 +19,6 @@ import { getRollupSearchStrategy } from './search/rollup_search_strategy'; import { initAggTypeFilter } from './visualize/agg_type_filter'; // @ts-ignore import { initAggTypeFieldFilter } from './visualize/agg_type_field_filter'; -// @ts-ignore -import { initEditorConfig } from './visualize/editor_config'; import { CONFIG_ROLLUPS } from '../common'; import { FeatureCatalogueCategory, @@ -42,7 +36,6 @@ export interface RollupPluginSetupDependencies { __LEGACY: { aggTypeFilters: AggTypeFilters; aggTypeFieldFilters: AggTypeFieldFilters; - editorConfigProviders: EditorConfigProviderRegistry; addSearchStrategy: (searchStrategy: SearchStrategyProvider) => void; managementLegacy: ManagementSetupLegacy; addBadgeExtension: (badgeExtension: any) => void; @@ -59,7 +52,6 @@ export class RollupPlugin implements Plugin { __LEGACY: { aggTypeFilters, aggTypeFieldFilters, - editorConfigProviders, addSearchStrategy, managementLegacy, addBadgeExtension, @@ -81,7 +73,6 @@ export class RollupPlugin implements Plugin { addSearchStrategy(getRollupSearchStrategy(core.http.fetch)); initAggTypeFilter(aggTypeFilters); initAggTypeFieldFilter(aggTypeFieldFilters); - initEditorConfig(editorConfigProviders); } if (home) { diff --git a/x-pack/legacy/plugins/rollup/public/visualize/editor_config.js b/x-pack/legacy/plugins/rollup/public/visualize/editor_config.js deleted file mode 100644 index 5c1eb7c8ee3b7..0000000000000 --- a/x-pack/legacy/plugins/rollup/public/visualize/editor_config.js +++ /dev/null @@ -1,84 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License; - * you may not use this file except in compliance with the Elastic License. - */ - -import { i18n } from '@kbn/i18n'; - -export function initEditorConfig(editorConfigProviders) { - // Limit agg params based on rollup capabilities - editorConfigProviders.register((indexPattern, aggConfig) => { - if (indexPattern.type !== 'rollup') { - return {}; - } - - const aggTypeName = aggConfig.type && aggConfig.type.name; - - // Exclude certain param options for terms: - // otherBucket, missingBucket, orderBy, orderAgg - if (aggTypeName === 'terms') { - return { - otherBucket: { - hidden: true, - }, - missingBucket: { - hidden: true, - }, - }; - } - - const rollupAggs = indexPattern.typeMeta && indexPattern.typeMeta.aggs; - const field = aggConfig.params && aggConfig.params.field && aggConfig.params.field.name; - const fieldAgg = - rollupAggs && field && rollupAggs[aggTypeName] && rollupAggs[aggTypeName][field]; - - if (!rollupAggs || !field || !fieldAgg) { - return {}; - } - - // Set interval and base interval for histograms based on rollup capabilities - if (aggTypeName === 'histogram') { - const interval = fieldAgg.interval; - return interval - ? { - intervalBase: { - fixedValue: interval, - }, - interval: { - base: interval, - help: i18n.translate('xpack.rollupJobs.editorConfig.histogram.interval.helpText', { - defaultMessage: 'Must be a multiple of rollup configuration interval: {interval}', - values: { interval }, - }), - }, - } - : {}; - } - - // Set date histogram time zone based on rollup capabilities - if (aggTypeName === 'date_histogram') { - // Interval is deprecated on date_histogram rollups, but may still be present - // See https://github.com/elastic/kibana/pull/36310 - const interval = fieldAgg.calendar_interval || fieldAgg.fixed_interval || fieldAgg.interval; - return { - useNormalizedEsInterval: { - fixedValue: false, - }, - interval: { - default: interval, - timeBase: interval, - help: i18n.translate( - 'xpack.rollupJobs.editorConfig.dateHistogram.customInterval.helpText', - { - defaultMessage: 'Must be a multiple of rollup configuration interval: {interval}', - values: { interval }, - } - ), - }, - }; - } - - return {}; - }); -} diff --git a/x-pack/plugins/translations/translations/ja-JP.json b/x-pack/plugins/translations/translations/ja-JP.json index 351bc9def3082..b98eabad466ed 100644 --- a/x-pack/plugins/translations/translations/ja-JP.json +++ b/x-pack/plugins/translations/translations/ja-JP.json @@ -10394,8 +10394,6 @@ "xpack.rollupJobs.detailPanel.jobActionMenu.buttonLabel": "管理", "xpack.rollupJobs.detailPanel.loadingLabel": "ロールアップジョブを読み込み中…", "xpack.rollupJobs.detailPanel.notFoundLabel": "ロールアップジョブが見つかりません", - "xpack.rollupJobs.editorConfig.dateHistogram.customInterval.helpText": "ロールアップ構成の間隔の倍数でなければなりません: {interval}", - "xpack.rollupJobs.editorConfig.histogram.interval.helpText": "ロールアップ構成の間隔の倍数でなければなりません: {interval}", "xpack.rollupJobs.editRollupIndexPattern.createIndex.defaultButtonDescription": "要約データに制限された集約を実行します。", "xpack.rollupJobs.editRollupIndexPattern.createIndex.defaultButtonText": "ロールアップインデックスパターン", "xpack.rollupJobs.editRollupIndexPattern.createIndex.defaultTypeName": "ロールアップインデックスパターン", diff --git a/x-pack/plugins/translations/translations/zh-CN.json b/x-pack/plugins/translations/translations/zh-CN.json index 066bafd990fe3..1c87541ae107a 100644 --- a/x-pack/plugins/translations/translations/zh-CN.json +++ b/x-pack/plugins/translations/translations/zh-CN.json @@ -10393,8 +10393,6 @@ "xpack.rollupJobs.detailPanel.jobActionMenu.buttonLabel": "管理", "xpack.rollupJobs.detailPanel.loadingLabel": "正在加载汇总/打包作业……", "xpack.rollupJobs.detailPanel.notFoundLabel": "未找到汇总/打包作业", - "xpack.rollupJobs.editorConfig.dateHistogram.customInterval.helpText": "必须是汇总/打包配置时间间隔的倍数:{interval}", - "xpack.rollupJobs.editorConfig.histogram.interval.helpText": "必须是汇总/打包配置时间间隔的倍数:{interval}", "xpack.rollupJobs.editRollupIndexPattern.createIndex.defaultButtonDescription": "针对汇总数据执行有限聚合", "xpack.rollupJobs.editRollupIndexPattern.createIndex.defaultButtonText": "汇总/打包索引模式", "xpack.rollupJobs.editRollupIndexPattern.createIndex.defaultTypeName": "汇总/打包索引模式",