Skip to content

Loosen apiClient ESLint rule on Security #252212

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

Merged
steliosmavro merged 6 commits into from
Feb 11, 2026
Merged

Loosen apiClient ESLint rule on Security #252212

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
269e67d
Loosen `apiClient` ESLint rule on Security
steliosmavro Feb 8, 2026
49c3f13
Merge branch 'main' into loosen-api-client-eslint-rule-on-security
steliosmavro Feb 9, 2026
329f8b9
Enhance with configurable option
steliosmavro Feb 9, 2026
9d2c805
Address feedback
steliosmavro Feb 9, 2026
6d6f29b
Merge branch 'main' into loosen-api-client-eslint-rule-on-security
steliosmavro Feb 10, 2026
a5ddb0d
Merge branch 'main' into loosen-api-client-eslint-rule-on-security
dmlemeshko Feb 10, 2026
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
25 changes: 23 additions & 2 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2701,8 +2701,9 @@ module.exports = {
],
},
},
// Custom rules for scout tests
{
// Custom rules for scout tests
// Platform & Solutions
files: [
'src/platform/plugins/**/test/{scout,scout_*}/**/*.ts',
'x-pack/platform/**/plugins/**/test/{scout,scout_*}/**/*.ts',
Expand All @@ -2712,9 +2713,29 @@ module.exports = {
'@kbn/eslint/scout_no_describe_configure': 'error',
'@kbn/eslint/scout_max_one_describe': 'error',
'@kbn/eslint/scout_test_file_naming': 'error',
'@kbn/eslint/scout_require_api_client_in_api_test': 'error',
'@kbn/eslint/scout_require_global_setup_hook_in_parallel_tests': 'error',
'@kbn/eslint/scout_no_es_archiver_in_parallel_tests': 'error',
'@kbn/eslint/scout_require_api_client_in_api_test': 'error',
'@kbn/eslint/require_include_in_check_a11y': 'warn',
},
},
{
// Security Solution
files: [
'x-pack/solutions/security/**/plugins/**/test/{scout,scout_*}/**/*.ts',
'x-pack/platform/**/plugins/**/security/**/test/{scout,scout_*}/**/*.ts',
],
rules: {
'@kbn/eslint/scout_require_api_client_in_api_test': [
'error',
{ alternativeFixtures: ['esClient'] },
],
},
},
Comment on lines +2722 to +2735

@steliosmavro steliosmavro Feb 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I say "Security Solution" but x-pack/platform/**/plugins/**/security/**/test/{scout,scout_*}/**/*.ts is not security solution - it's just a platform plugin, correct?

If yes, do you have any better name suggestions for this section?

cc @dmlemeshko

@dmlemeshko dmlemeshko Feb 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

let's allow it globally, it will be annoying to update .eslintrc.js every time a new team decided to use only esClient for api tests.

As Sophie pointed out, it is totally fine to use apiClient or esClient for API tests when Teams think this what they want to do.

{
// Ensure correct expect import path in solutions scout API tests
files: ['x-pack/solutions/**/plugins/**/test/scout/api/**/*.ts'],
rules: {
'@kbn/eslint/scout_expect_import': 'error',
'@kbn/eslint/require_include_in_check_a11y': 'warn',
},
Expand Down
76 changes: 53 additions & 23 deletions packages/kbn-eslint-plugin-eslint/rules/scout_require_api_client_in_api_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,23 @@
/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.CallExpression} CallExpression */
/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.Identifier} Identifier */

const ERROR_MSG =
'The `apiClient` fixture should be used in `apiTest` to call an endpoint and later verify response code and body.';

const traverse = require('eslint-traverse');

const isApiTestCall = (node) => node.callee.type === 'Identifier' && node.callee.name === 'apiTest';

/** Get local param name for `apiClient` (supports `{ apiClient: alias }`). */
const getApiClientLocalName = (fnNode) => {
/** Get local param name for a fixture (supports `{ fixtureName: alias }`). */
const getFixtureLocalName = (fnNode, fixtureName) => {
const firstParam = fnNode.params[0];
if (!firstParam || firstParam.type !== 'ObjectPattern') {
return 'apiClient';
return fixtureName;
}

const apiClientProp = firstParam.properties.find((prop) => {
const fixtureProp = firstParam.properties.find((prop) => {
if (prop.type === 'RestElement') return false;
return prop.key && prop.key.name === 'apiClient';
return prop.key && prop.key.name === fixtureName;
});

return apiClientProp?.value?.name || 'apiClient';
return fixtureProp?.value?.name || fixtureName;
};

/** Helper: Check if an identifier is used anywhere in a function body (simple recursion). */
Expand Down Expand Up @@ -65,18 +62,18 @@ const paramUsesIdentifierInBody = (node, paramName) => {
};

/**
* Determine whether `fnNode` uses `apiClient`.
* Determine whether `fnNode` uses the given `fixtureName`.
*
* Strategy: Single-pass traversal that:
* 1. Collects variable aliases pointing to apiClient (e.g., `const client = apiClient`)
* 1. Collects variable aliases pointing to the fixture (e.g., `const client = apiClient`)
* 2. Collects local function definitions and their parameter usage
* 3. Detects member access on apiClient or calls passing apiClient to functions that use it
* 3. Detects member access on the fixture or calls passing it to functions that use it
*
* We avoid the ESLint scope manager to maintain consistency with other rules in this package.
*/
const functionUsesApiClient = (fnNode, context) => {
const apiClientName = getApiClientLocalName(fnNode);
const variableAliases = new Set([apiClientName]);
const functionUsesFixture = (fnNode, context, fixtureName) => {
const localName = getFixtureLocalName(fnNode, fixtureName);
const variableAliases = new Set([localName]);
const localFnUsesParam = new Map();
let found = false;

Expand All @@ -95,7 +92,7 @@ const functionUsesApiClient = (fnNode, context) => {
// Pre-compute parameter usage for local function expressions
if (node.init.type === 'FunctionExpression' || node.init.type === 'ArrowFunctionExpression') {
const fnName = node.id.name;
const paramName = getApiClientLocalName(node.init);
const paramName = getFixtureLocalName(node.init, fixtureName);
localFnUsesParam.set(fnName, paramUsesIdentifierInBody(node.init.body, paramName));
// Skip traversing into nested function bodies to avoid false positives
return traverse.SKIP;
Expand All @@ -105,13 +102,13 @@ const functionUsesApiClient = (fnNode, context) => {
// Collect function declarations and pre-compute parameter usage
if (node.type === 'FunctionDeclaration' && node.id?.type === 'Identifier') {
const fnName = node.id.name;
const paramName = getApiClientLocalName(node);
const paramName = getFixtureLocalName(node, fixtureName);
localFnUsesParam.set(fnName, paramUsesIdentifierInBody(node.body, paramName));
// Skip traversing into nested function bodies to avoid false positives
return traverse.SKIP;
}

// Detect member access on apiClient (e.g., `apiClient.get(...)`)
// Detect member access on the fixture (e.g., `apiClient.get(...)`)
if (
node.type === 'MemberExpression' &&
node.object?.type === 'Identifier' &&
Expand All @@ -121,7 +118,7 @@ const functionUsesApiClient = (fnNode, context) => {
return traverse.SKIP;
}

// Detect calls with apiClient passed as argument
// Detect calls with the fixture passed as argument
if (node.type === 'CallExpression' && node.arguments) {
for (const arg of node.arguments) {
if (arg.type === 'Identifier' && variableAliases.has(arg.name)) {
Expand All @@ -140,7 +137,7 @@ const functionUsesApiClient = (fnNode, context) => {
}
}

// Also check if a local function is called that uses apiClient from closure (no args needed)
// Also check if a local function is called that uses the fixture from closure (no args needed)
const callee = node.callee;
if (callee.type === 'Identifier' && localFnUsesParam.has(callee.name)) {
if (localFnUsesParam.get(callee.name)) {
Expand All @@ -154,6 +151,18 @@ const functionUsesApiClient = (fnNode, context) => {
return found;
};

/** Build a human-readable error message listing all accepted fixtures. */
const buildErrorMsg = (acceptedFixtures) => {
if (acceptedFixtures.length === 1) {
return `The \`${acceptedFixtures[0]}\` fixture should be used in \`apiTest\` to call an endpoint and later verify response code and body.`;
}
const formatted = acceptedFixtures.map((f) => `\`${f}\``);
const last = formatted.pop();
return `One of ${formatted.join(
', '
)} or ${last} fixtures should be used in \`apiTest\` to interact with an endpoint and later verify the response.`;
};

/** @type {Rule} */
module.exports = {
meta: {
Expand All @@ -163,10 +172,27 @@ module.exports = {
category: 'Best Practices',
},
fixable: null,
schema: [],
schema: [
{
type: 'object',
properties: {
alternativeFixtures: {
type: 'array',
items: { type: 'string' },
uniqueItems: true,
},
},
additionalProperties: false,
},
],
},

create(context) {
const options = context.options[0] || {};
const alternativeFixtures = options.alternativeFixtures || [];
const acceptedFixtures = ['apiClient', ...alternativeFixtures];
const errorMsg = buildErrorMsg(acceptedFixtures);

return {
CallExpression(node) {
if (!isApiTestCall(node)) return;
Expand All @@ -193,10 +219,14 @@ module.exports = {
return;
}

if (!functionUsesApiClient(callbackArg, context)) {
const usesAcceptedFixture = acceptedFixtures.some((fixture) =>
functionUsesFixture(callbackArg, context, fixture)
);

if (!usesAcceptedFixture) {
context.report({
node,
message: ERROR_MSG,
message: errorMsg,
});
}
},
Expand Down
61 changes: 53 additions & 8 deletions packages/kbn-eslint-plugin-eslint/rules/scout_require_api_client_in_api_test.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,10 @@ const { RuleTester } = require('eslint');
const rule = require('./scout_require_api_client_in_api_test');
const dedent = require('dedent');

const ERROR_MSG =
const DEFAULT_ERROR_MSG =
'The `apiClient` fixture should be used in `apiTest` to call an endpoint and later verify response code and body.';
const ALT_ERROR_MSG =
'One of `apiClient` or `esArchiver` fixtures should be used in `apiTest` to interact with an endpoint and later verify the response.';

@dmlemeshko dmlemeshko Feb 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

apiClient or esClient.

esArchiver should be actually disallowed to be used in test block

const ruleTester = new RuleTester({
parser: require.resolve('@typescript-eslint/parser'),
parserOptions: {
Expand Down Expand Up @@ -161,7 +163,7 @@ ruleTester.run('@kbn/eslint/scout_require_api_client_in_api_test', rule, {
console.log(other);
});
`,
errors: [{ message: ERROR_MSG }],
errors: [{ message: DEFAULT_ERROR_MSG }],
},
// Nested apiTest inside apiTest.describe missing apiClient
{
Expand All @@ -170,7 +172,7 @@ ruleTester.run('@kbn/eslint/scout_require_api_client_in_api_test', rule, {
apiTest('inner missing', () => {});
});
`,
errors: [{ message: ERROR_MSG }],
errors: [{ message: DEFAULT_ERROR_MSG }],
},
// Nested blocks missing apiClient
{
Expand All @@ -181,7 +183,7 @@ ruleTester.run('@kbn/eslint/scout_require_api_client_in_api_test', rule, {
}
});
`,
errors: [{ message: ERROR_MSG }],
errors: [{ message: DEFAULT_ERROR_MSG }],
},
// Nested helper function missing apiClient
{
Expand All @@ -193,7 +195,7 @@ ruleTester.run('@kbn/eslint/scout_require_api_client_in_api_test', rule, {
helper(apiClient);
});
`,
errors: [{ message: ERROR_MSG }],
errors: [{ message: DEFAULT_ERROR_MSG }],
},
// Arrow function inside test
{
Expand All @@ -202,7 +204,7 @@ ruleTester.run('@kbn/eslint/scout_require_api_client_in_api_test', rule, {
const callApi = () => apiClient.get('/bar');
});
`,
errors: [{ message: ERROR_MSG }],
errors: [{ message: DEFAULT_ERROR_MSG }],
},
// External helper without apiClient usage
{
Expand All @@ -214,7 +216,7 @@ ruleTester.run('@kbn/eslint/scout_require_api_client_in_api_test', rule, {
externalHelper();
});
`,
errors: [{ message: ERROR_MSG }],
errors: [{ message: DEFAULT_ERROR_MSG }],
},
// Passing apiClient to a variable but never using it
{
Expand All @@ -224,7 +226,50 @@ ruleTester.run('@kbn/eslint/scout_require_api_client_in_api_test', rule, {
fn(apiClient);
});
`,
errors: [{ message: ERROR_MSG }],
errors: [{ message: DEFAULT_ERROR_MSG }],
},
],
});

// --- Tests with alternativeFixtures option ---
const altOptions = [{ alternativeFixtures: ['esArchiver'] }];

@dmlemeshko dmlemeshko Feb 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

apiClient has to be set here


ruleTester.run(
'@kbn/eslint/scout_require_api_client_in_api_test (with alternativeFixtures)',
rule,
{
valid: [
// apiClient still accepted when alternativeFixtures is configured
{
code: dedent`
apiTest('uses apiClient', async ({ apiClient }) => {
await apiClient.get('/foo');
});
`,
options: altOptions,
},
// Alternative fixture used instead of apiClient
{
code: dedent`
apiTest('uses esArchiver', async ({ esArchiver }) => {
await esArchiver.load('path/to/archive');
});
Comment on lines +253 to +256

@dmlemeshko dmlemeshko Feb 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

apiClient

`,
options: altOptions,
},
],

invalid: [
// Neither apiClient nor alternative fixture used
{
code: dedent`
apiTest('uses neither', async () => {
console.log('no fixture');
});
`,
options: altOptions,
errors: [{ message: ALT_ERROR_MSG }],
},
],
}
);
3 changes: 0 additions & 3 deletions x-pack/solutions/security/plugins/entity_store/test/scout/api/tests/dsl_translation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
* 2.0.
*/

// ignoring linter about api calls because we are testing DSL and esql only.
/* eslint-disable @kbn/eslint/scout_require_api_client_in_api_test */

import { expect } from '@kbn/scout-security/api';
import { apiTest } from '@kbn/scout-security';
import {
Expand Down
3 changes: 0 additions & 3 deletions x-pack/solutions/security/plugins/entity_store/test/scout/api/tests/esql_translation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
* 2.0.
*/

// ignoring linter about api calls because we are testing ESQL only.
/* eslint-disable @kbn/eslint/scout_require_api_client_in_api_test */

import { expect } from '@kbn/scout-security/api';
import { apiTest } from '@kbn/scout-security';
import {
Expand Down
2 changes: 0 additions & 2 deletions ...solutions/security/plugins/entity_store/test/scout/api/tests/painless_translation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
* 2.0.
*/

/* eslint-disable @kbn/eslint/scout_require_api_client_in_api_test */

import { expect } from '@kbn/scout-security/api';
import { apiTest } from '@kbn/scout-security';
import {
Expand Down
Loading