Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] Allow users to edit max_signals field for custom rules #179680
[Security Solution] Allow users to edit max_signals field for custom rules #179680
Changes from 45 commits
15db4e4
af60e77
bb5a1d2
8015b84
264cdb6
69dc936
ca8b462
d396a12
07927d1
3522cb7
1a1c121
0319c94
1749631
19ebcf0
813c807
00e942a
cd4eb47
5ad1b65
ff4b232
2053068
bec6bf7
aadde40
34588b2
61fbdc1
7aaa62a
3e749f6
bd01d55
aa2c565
975490e
25c05e3
b47deff
e709722
9886913
89bd1e7
304db6f
db88782
dd0a87e
bedbd7d
f400f99
8add109
e4e8af5
54b8041
ad64d19
1cb02a2
259d52e
bcca901
346e10e
ce2f06e
940f925
92d3ed2
b1ce138
995462b
69f4b87
dad1f8d
9244392
8d02476
d0e0432
18a724f
0b71b77
07d81e6
c850892
3cbb10f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use
exposeToBrowser
, then in theory we don't need this added to the triggers_actions_ui config route. ThoughexposeToBrowser
only makes it available to the browsers alerting plugin, not t_a_ui - though obviously we could make it accessible.But I don't think we need both, so we should pick one and not do the other. We don't need two ways to do the same thing ...
Or perhaps I missed something ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if I understand, the only
triggers_actions_ui
code that has been modified here was some test files to align with the mock alerting plugin. But to your larger point, I agree - theexposeToBrowser
has been implemented here and other plugins can use them if they so choose. That's how we're utilizing it in security solutionThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way this is structured we are also changing the output of the HTTP endpoint
/internal/triggers_actions_ui/_config
- to return ALL the values underrun
,not just the alerts.max value -which is something we need to consider re: backwards compatibility, documentation, and security (should we be exposing these via API). Do we really need to change the output of this endpoint?
I believe it's also the case that the values returned by this endpoint are or can be calculated, so using the value from the config wouldn't work. Which is why we have this endpoint. Static values (I assume these are static) can just use the
exposeToBrowser
path.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the end, I would like to NOT change the output of the
_config
route (that's adding therun
props) - not so much because it's a security concern now, but feels like a slippery slope to having a problem later, if someone follows this pattern, and we do leak something we shouldn't.Feels like we need to remove
run
fromAlertingRulesConfig
, or change the_config
route in t_a_ui topick
the fields it should be returning from the bigger config object (the existing ones, but notrun
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second way is probably the more preferable method, just so we can use that
getConfig
function elsewhere in other plugins without changing the_config
route output. Right now we're using thegetConfig
function to compare on the server side in security solution as well which is why therun
props were added in the first place. If y'all are ok with exposing the config values underrun
to that internalgetConfig
method from the plugin setup object and modifying thetriggers_actions_ui
config route so that we're locked into the existing values, I can change that over.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a problem exposting this at the plugin level - my concern is at the HTTP response level.
Filtering IN just the props we want returned from that
_config
endpoint would be perfect, as it means we won't have to worry about accidently leaking things later. So, in theory,x-pack/plugins/triggers_actions_ui/server/routes/config.test.ts
won't have any changes, but presumably it's pairconfig.ts
will.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that all sounds good, I think we're on the same page 👍