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.

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

[Security Solution] Implement normalization of ruleSource for API responses #188631

Merged
merged 6 commits into from
Jul 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
* 2.0.
*/

import { convertObjectKeysToSnakeCase } from '../../../../../../utils/object_case_converters';
import type { BaseRuleParams } from '../../../../rule_schema';
import { migrateLegacyInvestigationFields } from '../../../utils/utils';
import { normalizeRuleSource } from './normalize_rule_source';

export const commonParamsCamelToSnake = (params: BaseRuleParams) => {
return {
Expand Down Expand Up @@ -39,7 +39,10 @@ export const commonParamsCamelToSnake = (params: BaseRuleParams) => {
version: params.version,
exceptions_list: params.exceptionsList,
immutable: params.immutable,
rule_source: convertObjectKeysToSnakeCase(params.ruleSource),
rule_source: normalizeRuleSource({
immutable: params.immutable,
ruleSource: params.ruleSource,
}),
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not be normalizing data in case converters, as these functions are solely responsible for converting from camel case to snake case and vice versa. I think what you need is a converter from the alerting rule type to the rule response type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, thanks.

Created a normalization function to all params, applied in internalRuleToAPIResponse.

related_integrations: params.relatedIntegrations ?? [],
required_fields: params.requiredFields ?? [],
setup: params.setup ?? '',
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { normalizeRuleSource } from './normalize_rule_source';
import type { BaseRuleParams } from '../../../../rule_schema';

describe('normalizeRuleSource', () => {
it('should return rule_source of type `internal` when immutable is false and ruleSource is undefined', () => {
const result = normalizeRuleSource({
immutable: false,
ruleSource: undefined,
});
expect(result).toEqual({
type: 'internal',
});
});

it('should return rule_source of type `external` and `isCustomized: false` when immutable is true and ruleSource is undefined', () => {
const result = normalizeRuleSource({
immutable: true,
ruleSource: undefined,
});
expect(result).toEqual({
type: 'external',
is_customized: false,
});
});

it('should return snake_case version of rule_source when ruleSource is present', () => {
const externalRuleSource: BaseRuleParams['ruleSource'] = {
type: 'external',
isCustomized: true,
};
const externalResult = normalizeRuleSource({ immutable: true, ruleSource: externalRuleSource });
expect(externalResult).toEqual({
type: externalRuleSource.type,
is_customized: externalRuleSource.isCustomized,
});

const internalRuleSource: BaseRuleParams['ruleSource'] = {
type: 'internal',
};
const internalResult = normalizeRuleSource({
immutable: false,
ruleSource: internalRuleSource,
});
expect(internalResult).toEqual({
type: internalRuleSource.type,
});
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import type { RuleSource } from '../../../../../../../common/api/detection_engine';
import { convertObjectKeysToSnakeCase } from '../../../../../../utils/object_case_converters';
import type { BaseRuleParams } from '../../../../rule_schema';

interface NormalizeRuleSourceParams {
immutable: BaseRuleParams['immutable'];
ruleSource: BaseRuleParams['ruleSource'];
}
export const normalizeRuleSource = ({
immutable,
ruleSource,
}: NormalizeRuleSourceParams): RuleSource => {
if (!ruleSource) {
const normalizedRuleSource = immutable
? {
type: 'external',
isCustomized: false,
}
: {
type: 'internal',
};

return convertObjectKeysToSnakeCase(normalizedRuleSource) as RuleSource;
}
return convertObjectKeysToSnakeCase(ruleSource) as RuleSource;
};
Copy link
Contributor Author

@jpdjere jpdjere Jul 18, 2024

Choose a reason for hiding this comment

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

@xcrzx I remember you mentioning handling possible data inconsistencies. For example, data in ES being for whatever reason:

{
    "immutable": false,
    "ruleSource": {
        "type": "external",
        "isCustomized": true
    }
}

It's hard to me to think where these inconsitencies might arise from, but do you think it makes sense to rely always on immutable to calculate rule_source?

In the case above, calculating rule_source to be:

{
        "type": "internal"
}

Or if in ES data looks like:

{
    "immutable": true,
    "ruleSource": {
        "type": "internal",
    }
}

calculating rule_source to be:

{
        "type": "external",
        "is_customized": false
}

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think what you've implemented is correct. Relying on immutable in responses is not always possible because we also need to return isCustomized, which is calculated on writes. If the ruleSource field is available, we can return it as is. There's synchronization logic implemented on write, so ruleSource should always match immutable if present.