From c53f036f5d0caeff4759800bfaa893d0b8057b3b Mon Sep 17 00:00:00 2001 From: Joel Griffith Date: Mon, 29 Jun 2020 09:26:11 -0700 Subject: [PATCH] Ensure that security is enabled before doing user authentication checks (#70127) Co-authored-by: Elastic Machine --- .../server/routes/generation.test.ts | 3 ++ .../reporting/server/routes/jobs.test.ts | 9 +++++ .../lib/authorized_user_pre_routing.test.ts | 33 +++++++++++++++++-- .../routes/lib/authorized_user_pre_routing.ts | 2 +- 4 files changed, 43 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/reporting/server/routes/generation.test.ts b/x-pack/plugins/reporting/server/routes/generation.test.ts index 4474f2c95e1c3..7de7c68122125 100644 --- a/x-pack/plugins/reporting/server/routes/generation.test.ts +++ b/x-pack/plugins/reporting/server/routes/generation.test.ts @@ -57,6 +57,9 @@ describe('POST /api/reporting/generate', () => { }, }, security: { + license: { + isEnabled: () => true, + }, authc: { getCurrentUser: () => ({ id: '123', diff --git a/x-pack/plugins/reporting/server/routes/jobs.test.ts b/x-pack/plugins/reporting/server/routes/jobs.test.ts index 35594474685b0..a0e3379da12be 100644 --- a/x-pack/plugins/reporting/server/routes/jobs.test.ts +++ b/x-pack/plugins/reporting/server/routes/jobs.test.ts @@ -47,6 +47,9 @@ describe('GET /api/reporting/jobs/download', () => { legacy: { client: { callAsInternalUser: jest.fn() } }, }, security: { + license: { + isEnabled: () => true, + }, authc: { getCurrentUser: () => ({ id: '123', @@ -113,6 +116,9 @@ describe('GET /api/reporting/jobs/download', () => { // @ts-ignore ...core.pluginSetupDeps, security: { + license: { + isEnabled: () => true, + }, authc: { getCurrentUser: () => undefined, }, @@ -136,6 +142,9 @@ describe('GET /api/reporting/jobs/download', () => { // @ts-ignore ...core.pluginSetupDeps, security: { + license: { + isEnabled: () => true, + }, authc: { getCurrentUser: () => ({ id: '123', diff --git a/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.test.ts b/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.test.ts index b218f9e4607dd..f4591f8436bd2 100644 --- a/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.test.ts +++ b/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.test.ts @@ -46,7 +46,7 @@ describe('authorized_user_pre_routing', function () { mockCore = await createMockReportingCore(mockReportingConfig); }); - it('should return from handler with null user when security is disabled', async function () { + it('should return from handler with a "null" user when security plugin is not found', async function () { mockCore.getPluginSetupDeps = () => (({ // @ts-ignore @@ -66,12 +66,37 @@ describe('authorized_user_pre_routing', function () { expect(handlerCalled).toBe(true); }); - it('should return with 401 when security is enabled but no authenticated user', async function () { + it('should return from handler with a "null" user when security is disabled', async function () { mockCore.getPluginSetupDeps = () => (({ // @ts-ignore ...mockCore.pluginSetupDeps, security: { + license: { + isEnabled: () => false, + }, + }, // disable security + } as unknown) as ReportingInternalSetup); + const authorizedUserPreRouting = authorizedUserPreRoutingFactory(mockCore); + const mockResponseFactory = httpServerMock.createResponseFactory() as KibanaResponseFactory; + + let handlerCalled = false; + authorizedUserPreRouting((user: unknown) => { + expect(user).toBe(null); // verify the user is a null value + handlerCalled = true; + return Promise.resolve({ status: 200, options: {} }); + })(getMockContext(), getMockRequest(), mockResponseFactory); + + expect(handlerCalled).toBe(true); + }); + + it('should return with 401 when security is enabled and the request is unauthenticated', async function () { + mockCore.getPluginSetupDeps = () => + (({ + // @ts-ignore + ...mockCore.pluginSetupDeps, + security: { + license: { isEnabled: () => true }, authc: { getCurrentUser: () => null }, }, } as unknown) as ReportingInternalSetup); @@ -87,12 +112,13 @@ describe('authorized_user_pre_routing', function () { }); }); - it(`should return with 403 when security is enabled but user doesn't have allowed role`, async function () { + it(`should return with 403 when security is enabled but user doesn't have the allowed role`, async function () { mockCore.getPluginSetupDeps = () => (({ // @ts-ignore ...mockCore.pluginSetupDeps, security: { + license: { isEnabled: () => true }, authc: { getCurrentUser: () => ({ username: 'friendlyuser', roles: ['cowboy'] }) }, }, } as unknown) as ReportingInternalSetup); @@ -113,6 +139,7 @@ describe('authorized_user_pre_routing', function () { // @ts-ignore ...mockCore.pluginSetupDeps, security: { + license: { isEnabled: () => true }, authc: { getCurrentUser: () => ({ username: 'friendlyuser', roles: ['reporting_user'] }), }, diff --git a/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts b/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts index 2f5d4ebe1419a..74737b0a5d1e2 100644 --- a/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts +++ b/x-pack/plugins/reporting/server/routes/lib/authorized_user_pre_routing.ts @@ -24,7 +24,7 @@ export const authorizedUserPreRoutingFactory = function authorizedUserPreRouting return (handler: RequestHandlerUser): RequestHandler => { return (context, req, res) => { let user: ReportingUser = null; - if (setupDeps.security) { + if (setupDeps.security && setupDeps.security.license.isEnabled()) { // find the authenticated user, or null if security is not enabled user = getUser(req); if (!user) {