Fixes to /fleet/queries/run endpoint #14909
Conversation
| } | ||
| if allResultsForbidden { | ||
| return nil, authz.ForbiddenWithInternal("All Live Query results were forbidden.", authz.UserFromContext(ctx), nil, nil) | ||
| } |
There was a problem hiding this comment.
Let's check with the product team what the expected behavior is when sending multiple query_ids and the user is not authorized to run some of them:
- Should the request fail and no queries be executed?
- Should the request not fail and only run the queries that the user is authorized to run?
API: https://fleetdm.com/docs/rest-api/rest-api#parameters97
There was a problem hiding this comment.
@marko-lisica did we define a similar behavior for running MDM commands that we can borrow here?
There was a problem hiding this comment.
Current behavior for mix of authorized/unauthorized live queries is that user will get back an array of results. Good results will be valid, and unauthorized results will have "error":"forbidden"
There was a problem hiding this comment.
Sounds good. Could we take the chance to document this behavior in the rest-api.md?
There was a problem hiding this comment.
(Can be done later on another PR.)
There was a problem hiding this comment.
After chatting with Victor about it earlier, this behavior of mixed results makes sense to me, but I definitely agree we should document the behavior. @getvictor if you don't mind adding that to this PR, that'd be awesome. Or I'd be happy to take a stab at it after this is merged, just let me know
There was a problem hiding this comment.
@noahtalerman In the CLI we have error message for this use case - figma link. Regarding API, seems there's 403: forbidden error, but not sure when do we return this one.
There was a problem hiding this comment.
@rachaelshaw I added PR #14956 for rest-api.md updates.
- now returns 403 for an unauthorized user - now returns 400 when query_ids or host_ids are not specified
28e69d0 to
cca3505
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #14909 +/- ##
==========================================
+ Coverage 58.85% 58.87% +0.02%
==========================================
Files 953 953
Lines 80241 80274 +33
Branches 2222 2222
==========================================
+ Hits 47223 47265 +42
+ Misses 29341 29336 -5
+ Partials 3677 3673 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
| @@ -48,21 +50,45 @@ func runLiveQueryEndpoint(ctx context.Context, request interface{}, svc fleet.Se | |||
| logging.WithExtras(ctx, "live_query_rest_period_err", err) | |||
| } | |||
|
|
|||
| // Only allow a host to be specified once in HostIDs | |||
| req.HostIDs = server.RemoveDuplicatesFromSlice(req.HostIDs) | |||
There was a problem hiding this comment.
Out of curiosity: Is there a bug or just a sanity check to not cause unnecessary load?
There was a problem hiding this comment.
Small bug. User can specify the same host twice, but service will only return 1 result. So, TargetedHostCount(2) will never match RespondedHostCount(1).
From discussions on PR #14909
Fixes to /fleet/queries/run endpoint:
#11446 and #11901
Checklist for submitter
If some of the following don't apply, delete the relevant line.
API clarifications are in a separate PR #14956
changes/ororbit/changes/.See Changes files for more information.