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

SavedObjects aggregation: add validation / path rewriting for sort parameter #115153

Open
pgayvallet opened this issue Oct 15, 2021 · 2 comments
Open
Labels
bug Fixes for quality problems that affect the customer experience Feature:Saved Objects impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@pgayvallet
Copy link
Contributor

Initial discussion: https://github.com/elastic/kibana/pull/114585/files#r729414544

We're not currently validating and rewriting attribute paths used inside the sort parameter (used in multiple aggregation types, even if top_hits is the only one supported in the subset we've implemented for SOs at the moment)

This means consumers currently have to use

aggs: {
    myAggr: {
        top_hits: {
            sort: [
                { [`${myType}.someAttribute`]: { ... } }
            ],
        },
    },
}

instead of

aggs: {
    myAggr: {
        top_hits: {
            sort: [
                { [`${myType}.attributes.someAttribute`]: { ... } }
            ],
        },
    },
}

Which is inconsistent with the rest of the API, in addition to skipping all the validation/rewrite we want to perform when attribute paths are used in aggregations (even if I don't think there is any security concerns here, given what sort does)

However, this validation is more complex than all we're currently supporting, due to the possible shapes of the sort option:

  1. This can either be a sort value or an array of sort value
  2. Each value can have different shapes, either records or plain strings.

E.g

// one field, short syntax 
{ 
   sort: 'my-field', 
}

// multiple fields, short syntax
{ 
   sort: ['my-field', 'my-other-field'], 
}

// one field, long syntax
{ 
   sort: { 
      'my-field' : { order: 'asc' } 
   } 
}

// multiple fields, long syntax
{ 
   sort:  { 
        'my-field' : { order: 'asc' },
        'my-other-field' : { order: 'asc' }
    }, 
}

// multiple fields, long syntax using one object per field (yea, this is supported)
{ 
   sort: [
      { 'my-field' : { order: 'asc' } },
      { 'my-other-field' : { order: 'asc' } },
   ], 
}

See the sortSchema for more info about the shape of this option:

const fieldSchema = s.string();
export const sortOrderSchema = s.oneOf([s.literal('asc'), s.literal('desc'), s.literal('_doc')]);
const sortModeSchema = s.oneOf([
s.literal('min'),
s.literal('max'),
s.literal('sum'),
s.literal('avg'),
s.literal('median'),
]);
const fieldSortSchema = s.object({
missing: s.maybe(s.oneOf([s.string(), s.number(), s.boolean()])),
mode: s.maybe(sortModeSchema),
order: s.maybe(sortOrderSchema),
// nested and unmapped_type not implemented yet
});
const sortContainerSchema = s.recordOf(s.string(), s.oneOf([sortOrderSchema, fieldSortSchema]));
const sortCombinationsSchema = s.oneOf([fieldSchema, sortContainerSchema]);
export const sortSchema = s.oneOf([sortCombinationsSchema, s.arrayOf(sortCombinationsSchema)]);

Until now, we're succeeded in using a naive hardcoded list of the attributes we needed to rewrite the key or the value for. But until now, we never encountered a situation where the validation and rewrite was depending on the shape of a polymorphic value. I'm not sure the current approach will work for this specific case, and I'm afraid we may have to rethink the validation (or to introduce specific code for this sort exception)

As a sidenote: we're not handling arrays in aggregation structure at all atm. It currently only work because the Object.entries call in

const recursiveRewrite = (
currentLevel: Record<string, any>,
context: ValidationContext,
parents: string[]

'reconstruct' an object with the array's indices as keys.

@pgayvallet pgayvallet added bug Fixes for quality problems that affect the customer experience Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects labels Oct 15, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@spong
Copy link
Member

spong commented Nov 9, 2021

As referenced in the description, there is one usage in SecuritySolution that'll be affected by this fix if still around (usage is deprecated and may be removed in early 8.x). Corresponding find_statues/find_rules FTR/integration tests will start failing once fixed as well.

spong added a commit that referenced this issue Nov 9, 2021
…Id is not a string (#117962)

## Summary

Resolves #116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue #115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Nov 9, 2021
…Id is not a string (elastic#117962)

## Summary

Resolves elastic#116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue elastic#115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Nov 9, 2021
…Id is not a string (elastic#117962)

## Summary

Resolves elastic#116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue elastic#115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
kibanamachine added a commit that referenced this issue Nov 9, 2021
…Id is not a string (#117962) (#118038)

## Summary

Resolves #116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue #115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
kibanamachine added a commit that referenced this issue Nov 9, 2021
…n alertId is not a string (#117962) (#118040)

* [SecuritySolution][Detections] Fixes rule status migration when alertId is not a string (#117962)

## Summary

Resolves #116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue #115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

* Fixes typecheck in test

Co-authored-by: Garrett Spong <spong@users.noreply.github.com>
Co-authored-by: Garrett Spong <garrett.spong@elastic.co>
kpatticha pushed a commit to kpatticha/kibana that referenced this issue Nov 10, 2021
…Id is not a string (elastic#117962)

## Summary

Resolves elastic#116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue elastic#115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
fkanout pushed a commit to fkanout/kibana that referenced this issue Nov 17, 2021
…Id is not a string (elastic#117962)

## Summary

Resolves elastic#116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue elastic#115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
roeehub pushed a commit to build-security/kibana that referenced this issue Dec 16, 2021
…Id is not a string (elastic#117962)

## Summary

Resolves elastic#116423, and adds an e2e test catching this behavior as we can't test via the migration test harness since the `siem-detection-engine-rule-status` SO isn't exposed within the SO Manager UI.

Also adds note with regards to changes necessary once core issue elastic#115153 is resolved. See https://github.com/elastic/kibana/pull/114585/files#r729620927. Note: existing `find_statuses`/`find_rules` integration tests will fail once fixed, so no additional tests necessary. 



### Checklist

Delete any items that are not applicable to this PR.

- [X] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Saved Objects impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

3 participants