Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unskip #169161 #169421

Closed
wants to merge 2 commits into from
Closed

Unskip #169161 #169421

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a942816
Unskip #169161
afharo Oct 19, 2023
b07621c
[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…
kibanamachine Oct 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/plugins/telemetry/server/routes/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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$);
Expand Down
67 changes: 57 additions & 10 deletions src/plugins/telemetry/server/routes/telemetry_usage_stats.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<RequestHandlerContext>,
Expand All @@ -27,6 +28,7 @@ async function runRequest(
}

describe('registerTelemetryUsageStatsRoutes', () => {
const logger = loggerMock.create();
const router = {
handler: undefined,
config: undefined,
Expand All @@ -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];
Expand All @@ -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({
Expand All @@ -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({
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -131,7 +169,8 @@ describe('registerTelemetryUsageStatsRoutes', () => {
mockRouter,
telemetryCollectionManager,
true,
getSecurityMock
getSecurityMock,
logger
);
const { mockResponse } = await runRequest(mockRouter, {
refreshCache: false,
Expand All @@ -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,
Expand All @@ -167,7 +212,8 @@ describe('registerTelemetryUsageStatsRoutes', () => {
mockRouter,
telemetryCollectionManager,
true,
getSecurityMock
getSecurityMock,
logger
);
const { mockResponse } = await runRequest(mockRouter, {
refreshCache: false,
Expand All @@ -188,7 +234,8 @@ describe('registerTelemetryUsageStatsRoutes', () => {
mockRouter,
telemetryCollectionManager,
true,
getSecurityMock
getSecurityMock,
logger
);
const { mockResponse } = await runRequest(mockRouter, {
refreshCache: false,
Expand Down
90 changes: 48 additions & 42 deletions src/plugins/telemetry/server/routes/telemetry_usage_stats.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -23,59 +23,65 @@ export function registerTelemetryUsageStatsRoutes(
router: IRouter,
telemetryCollectionManager: TelemetryCollectionManagerPluginSetup,
isDev: boolean,
getSecurity: SecurityGetter
getSecurity: SecurityGetter,
logger: Logger
) {
const v2Handler: RequestHandler<undefined, undefined, UsageStatsBody> = 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;
Comment on lines +82 to +84
Copy link
Member Author

Choose a reason for hiding this comment

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

DO NOT MERGE! We shouldn't log the error like this. It's only to confirm the error.

}
};

Expand Down
1 change: 1 addition & 0 deletions src/plugins/telemetry/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"@kbn/core-http-browser-mocks",
"@kbn/core-http-browser",
"@kbn/core-http-server",
"@kbn/logging-mocks",
],
"exclude": [
"target/**/*",
Expand Down
3 changes: 1 addition & 2 deletions ...test_serverless/api_integration/test_suites/observability/telemetry/snapshot_telemetry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down