From a94281662f9a92dfa28a87d999e5eda057d1bb0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Fern=C3=A1ndez=20Haro?= Date: Thu, 19 Oct 2023 17:35:06 +0200 Subject: [PATCH 1/2] Unskip #169161 --- src/plugins/telemetry/server/routes/index.ts | 8 +- .../routes/telemetry_usage_stats.test.ts | 67 +++++++++++--- .../server/routes/telemetry_usage_stats.ts | 90 ++++++++++--------- .../telemetry/snapshot_telemetry.ts | 3 +- 4 files changed, 113 insertions(+), 55 deletions(-) diff --git a/src/plugins/telemetry/server/routes/index.ts b/src/plugins/telemetry/server/routes/index.ts index 5a47b4a00ac18..627c18a206e84 100644 --- a/src/plugins/telemetry/server/routes/index.ts +++ b/src/plugins/telemetry/server/routes/index.ts @@ -33,7 +33,13 @@ export function registerRoutes(options: RegisterRoutesParams) { options; registerTelemetryOptInRoutes(options); registerTelemetryConfigRoutes(options); - registerTelemetryUsageStatsRoutes(router, telemetryCollectionManager, isDev, getSecurity); + registerTelemetryUsageStatsRoutes( + router, + telemetryCollectionManager, + isDev, + getSecurity, + options.logger + ); registerTelemetryOptInStatsRoutes(router, telemetryCollectionManager); registerTelemetryUserHasSeenNotice(router, options.currentKibanaVersion); registerTelemetryLastReported(router, savedObjectsInternalClient$); diff --git a/src/plugins/telemetry/server/routes/telemetry_usage_stats.test.ts b/src/plugins/telemetry/server/routes/telemetry_usage_stats.test.ts index 4cbb1381c0566..2a4a8fb51143d 100644 --- a/src/plugins/telemetry/server/routes/telemetry_usage_stats.test.ts +++ b/src/plugins/telemetry/server/routes/telemetry_usage_stats.test.ts @@ -11,6 +11,7 @@ import { coreMock, httpServerMock } from '@kbn/core/server/mocks'; import type { RequestHandlerContext, IRouter } from '@kbn/core/server'; import { securityMock } from '@kbn/security-plugin/server/mocks'; import { telemetryCollectionManagerPluginMock } from '@kbn/telemetry-collection-manager-plugin/server/mocks'; +import { loggerMock } from '@kbn/logging-mocks'; async function runRequest( mockRouter: IRouter, @@ -27,6 +28,7 @@ async function runRequest( } describe('registerTelemetryUsageStatsRoutes', () => { + const logger = loggerMock.create(); const router = { handler: undefined, config: undefined, @@ -49,7 +51,13 @@ describe('registerTelemetryUsageStatsRoutes', () => { describe('clusters/_stats POST route', () => { it('registers _stats POST route and accepts body configs', () => { - registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true, getSecurity); + registerTelemetryUsageStatsRoutes( + mockRouter, + telemetryCollectionManager, + true, + getSecurity, + logger + ); expect(mockRouter.versioned.post).toBeCalledTimes(1); const [routeConfig, handler] = (mockRouter.versioned.post as jest.Mock).mock.results[0].value .addVersion.mock.calls[0]; @@ -58,7 +66,13 @@ describe('registerTelemetryUsageStatsRoutes', () => { }); it('responds with encrypted stats with no cache refresh by default', async () => { - registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true, getSecurity); + registerTelemetryUsageStatsRoutes( + mockRouter, + telemetryCollectionManager, + true, + getSecurity, + logger + ); const { mockResponse } = await runRequest(mockRouter); expect(telemetryCollectionManager.getStats).toBeCalledWith({ @@ -70,7 +84,13 @@ describe('registerTelemetryUsageStatsRoutes', () => { }); it('when unencrypted is set getStats is called with unencrypted and refreshCache', async () => { - registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true, getSecurity); + registerTelemetryUsageStatsRoutes( + mockRouter, + telemetryCollectionManager, + true, + getSecurity, + logger + ); await runRequest(mockRouter, { unencrypted: true }); expect(telemetryCollectionManager.getStats).toBeCalledWith({ @@ -80,7 +100,13 @@ describe('registerTelemetryUsageStatsRoutes', () => { }); it('calls getStats with refreshCache when set in body', async () => { - registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true, getSecurity); + registerTelemetryUsageStatsRoutes( + mockRouter, + telemetryCollectionManager, + true, + getSecurity, + logger + ); await runRequest(mockRouter, { refreshCache: true }); expect(telemetryCollectionManager.getStats).toBeCalledWith({ unencrypted: undefined, @@ -89,7 +115,13 @@ describe('registerTelemetryUsageStatsRoutes', () => { }); it('calls getStats with refreshCache:true even if set to false in body when unencrypted is set to true', async () => { - registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true, getSecurity); + registerTelemetryUsageStatsRoutes( + mockRouter, + telemetryCollectionManager, + true, + getSecurity, + logger + ); await runRequest(mockRouter, { refreshCache: false, unencrypted: true, @@ -106,7 +138,13 @@ describe('registerTelemetryUsageStatsRoutes', () => { securityStartMock.authz.mode.useRbacForRequest.mockReturnValue(false); return securityStartMock; }); - registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true, getSecurity); + registerTelemetryUsageStatsRoutes( + mockRouter, + telemetryCollectionManager, + true, + getSecurity, + logger + ); await runRequest(mockRouter, { refreshCache: false, unencrypted: true, @@ -131,7 +169,8 @@ describe('registerTelemetryUsageStatsRoutes', () => { mockRouter, telemetryCollectionManager, true, - getSecurityMock + getSecurityMock, + logger ); const { mockResponse } = await runRequest(mockRouter, { refreshCache: false, @@ -143,7 +182,13 @@ describe('registerTelemetryUsageStatsRoutes', () => { it('returns 503 when Kibana is not healthy enough to generate the Telemetry report', async () => { telemetryCollectionManager.shouldGetTelemetry.mockResolvedValueOnce(false); - registerTelemetryUsageStatsRoutes(mockRouter, telemetryCollectionManager, true, () => void 0); + registerTelemetryUsageStatsRoutes( + mockRouter, + telemetryCollectionManager, + true, + () => void 0, + logger + ); const { mockResponse } = await runRequest(mockRouter, { refreshCache: false, unencrypted: true, @@ -167,7 +212,8 @@ describe('registerTelemetryUsageStatsRoutes', () => { mockRouter, telemetryCollectionManager, true, - getSecurityMock + getSecurityMock, + logger ); const { mockResponse } = await runRequest(mockRouter, { refreshCache: false, @@ -188,7 +234,8 @@ describe('registerTelemetryUsageStatsRoutes', () => { mockRouter, telemetryCollectionManager, true, - getSecurityMock + getSecurityMock, + logger ); const { mockResponse } = await runRequest(mockRouter, { refreshCache: false, diff --git a/src/plugins/telemetry/server/routes/telemetry_usage_stats.ts b/src/plugins/telemetry/server/routes/telemetry_usage_stats.ts index a2cee086ee563..de3ff948be734 100644 --- a/src/plugins/telemetry/server/routes/telemetry_usage_stats.ts +++ b/src/plugins/telemetry/server/routes/telemetry_usage_stats.ts @@ -7,7 +7,7 @@ */ import { schema } from '@kbn/config-schema'; -import type { IRouter } from '@kbn/core/server'; +import type { IRouter, Logger } from '@kbn/core/server'; import type { TelemetryCollectionManagerPluginSetup, StatsGetterConfig, @@ -23,59 +23,65 @@ export function registerTelemetryUsageStatsRoutes( router: IRouter, telemetryCollectionManager: TelemetryCollectionManagerPluginSetup, isDev: boolean, - getSecurity: SecurityGetter + getSecurity: SecurityGetter, + logger: Logger ) { const v2Handler: RequestHandler = async ( context, req, res ) => { - const { unencrypted, refreshCache } = req.body; + try { + const { unencrypted, refreshCache } = req.body; - if (!(await telemetryCollectionManager.shouldGetTelemetry())) { - // We probably won't reach here because there is a license check in the auth phase of the HTTP requests. - // But let's keep it here should that changes at any point. - return res.customError({ - statusCode: 503, - body: `Can't fetch telemetry at the moment because some services are down. Check the /status page for more details.`, - }); - } + if (!(await telemetryCollectionManager.shouldGetTelemetry())) { + // We probably won't reach here because there is a license check in the auth phase of the HTTP requests. + // But let's keep it here should that changes at any point. + return res.customError({ + statusCode: 503, + body: `Can't fetch telemetry at the moment because some services are down. Check the /status page for more details.`, + }); + } - const security = getSecurity(); - // We need to check useRbacForRequest to figure out if ES has security enabled before making the privileges check - if (security && unencrypted && security.authz.mode.useRbacForRequest(req)) { - // Normally we would use `options: { tags: ['access:decryptedTelemetry'] }` in the route definition to check authorization for an - // API action, however, we want to check this conditionally based on the `unencrypted` parameter. In this case we need to use the - // security API directly to check privileges for this action. Note that the 'decryptedTelemetry' API privilege string is only - // granted to users that have "Global All" or "Global Read" privileges in Kibana. - const { checkPrivilegesWithRequest, actions } = security.authz; - const privileges = { kibana: actions.api.get('decryptedTelemetry') }; - const { hasAllRequested } = await checkPrivilegesWithRequest(req).globally(privileges); - if (!hasAllRequested) { - return res.forbidden(); + const security = getSecurity(); + // We need to check useRbacForRequest to figure out if ES has security enabled before making the privileges check + if (security && unencrypted && security.authz.mode.useRbacForRequest(req)) { + // Normally we would use `options: { tags: ['access:decryptedTelemetry'] }` in the route definition to check authorization for an + // API action, however, we want to check this conditionally based on the `unencrypted` parameter. In this case we need to use the + // security API directly to check privileges for this action. Note that the 'decryptedTelemetry' API privilege string is only + // granted to users that have "Global All" or "Global Read" privileges in Kibana. + const { checkPrivilegesWithRequest, actions } = security.authz; + const privileges = { kibana: actions.api.get('decryptedTelemetry') }; + const { hasAllRequested } = await checkPrivilegesWithRequest(req).globally(privileges); + if (!hasAllRequested) { + return res.forbidden(); + } } - } - try { - const statsConfig: StatsGetterConfig = { - unencrypted, - refreshCache: unencrypted || refreshCache, - }; + try { + const statsConfig: StatsGetterConfig = { + unencrypted, + refreshCache: unencrypted || refreshCache, + }; - const body: v2.UnencryptedTelemetryPayload = await telemetryCollectionManager.getStats( - statsConfig - ); - return res.ok({ body }); - } catch (err) { - if (isDev) { - // don't ignore errors when running in dev mode - throw err; + const body: v2.UnencryptedTelemetryPayload = await telemetryCollectionManager.getStats( + statsConfig + ); + return res.ok({ body }); + } catch (err) { + if (isDev) { + // don't ignore errors when running in dev mode + throw err; + } + if (unencrypted && err.status === 403) { + return res.forbidden(); + } + // ignore errors and return empty set + return res.ok({ body: [] }); } - if (unencrypted && err.status === 403) { - return res.forbidden(); - } - // ignore errors and return empty set - return res.ok({ body: [] }); + } catch (err) { + logger.error(err); + throw err; } }; diff --git a/x-pack/test_serverless/api_integration/test_suites/observability/telemetry/snapshot_telemetry.ts b/x-pack/test_serverless/api_integration/test_suites/observability/telemetry/snapshot_telemetry.ts index f6f2d63d566bc..f0fc2a357156e 100644 --- a/x-pack/test_serverless/api_integration/test_suites/observability/telemetry/snapshot_telemetry.ts +++ b/x-pack/test_serverless/api_integration/test_suites/observability/telemetry/snapshot_telemetry.ts @@ -18,8 +18,7 @@ import type { UsageStatsPayloadTestFriendly } from '../../../../../test/api_inte export default function ({ getService }: FtrProviderContext) { const usageApi = getService('usageAPI'); - // FLAKY: https://github.com/elastic/kibana/issues/168625 - describe.skip('Snapshot telemetry', function () { + describe('Snapshot telemetry', function () { let stats: UsageStatsPayloadTestFriendly; before(async () => { From b07621c0ec161d4752060969df4187395e7d572f Mon Sep 17 00:00:00 2001 From: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Date: Thu, 19 Oct 2023 15:43:54 +0000 Subject: [PATCH 2/2] [CI] Auto-commit changed files from 'node scripts/lint_ts_projects --fix' --- src/plugins/telemetry/tsconfig.json | 1 + 1 file changed, 1 insertion(+) diff --git a/src/plugins/telemetry/tsconfig.json b/src/plugins/telemetry/tsconfig.json index 638bfb4f722a7..95601febd1aeb 100644 --- a/src/plugins/telemetry/tsconfig.json +++ b/src/plugins/telemetry/tsconfig.json @@ -35,6 +35,7 @@ "@kbn/core-http-browser-mocks", "@kbn/core-http-browser", "@kbn/core-http-server", + "@kbn/logging-mocks", ], "exclude": [ "target/**/*",