[Response Ops][Task Manager] Update functions do not handle API key invalidation#249109
[Response Ops][Task Manager] Update functions do not handle API key invalidation#249109doakalexi merged 13 commits intoelastic:mainfrom
Conversation
| ); | ||
| }); | ||
|
|
||
| test('uses regular repository when no request is provided even if docs have apiKey and userScope', async () => { |
There was a problem hiding this comment.
I updated this test to throw an error, but the changes make it seem like I changed the other tests too.
|
/oblt-deploy |
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
from #244918 (comment) :
Note that search query, for a scheduled report, isn't stored in the task at all, so it makes sense to parameterize the Likewise, if any doc in an update with fail with one of the new constraints (has an API key, request is null), ALL of the updates will fail. Again, we may want to revisit this later, but I think it's fine for now, as it seems unlikely folks would be mixing and matching tasks with and without API keys. |
pmuellr
left a comment
There was a problem hiding this comment.
LGTM, except for the timing on marking API keys invalid. Looks like it's too early to me, an error after processing those could potential cancel the update, but also invalidate the keys that are still in the tasks?
| if (todayMonth === runAtMonth) { | ||
| expect(runAtDay >= todayDay + 2).to.be(true); | ||
| } else if (todayMonth < runAtMonth) { | ||
| } else if (todayMonth < runAtMonth || todayYear < runAtYear) { |
There was a problem hiding this comment.
I notice this failed on Dec 29 - makes sense, fails starting on Dec 29, starts working again Jan 1 :-)
| ): Promise<BulkUpdateResult[]> { | ||
| const soClientToUpdate = this.getSoClientForUpdate(docs, options); | ||
| const apiKeyAndUserScopeMap = | ||
| (await this.regenerateApiKeyFromRequest(docs, options)) || new Map(); |
There was a problem hiding this comment.
We mark invalidated API keys in this new function, but it seems too early, if an error occurs somewhere after here and we throw an error instead of doing the update. Seems like we should only invalidate the keys if the SO bulk update succeeds.
There was a problem hiding this comment.
Oh okay that makes sense! I will update this, thanks!
⏳ Build in-progress, with failures
Failed CI Steps
Test Failures
History
|
pmuellr
left a comment
There was a problem hiding this comment.
LGTM, left a code style comment
|
|
||
| // If a task with an API key is updated with a request | ||
| if (hasEncryptedFields && options?.request && options?.regenerateApiKey) { | ||
| const docsWithApiKeys: ConcreteTaskInstance[] = []; |
There was a problem hiding this comment.
We could move these two lines up, to be in top-level function scope. And then also move the return { ... } down, replacing the current return null;. Then you wouldn't need the conditional assignment at the call-site.
…iew_cps * commit '5f7fec57cb01883038810bd735a0666683b49904': (116 commits) [Security Solution][Attacks/Alerts][Setup and miscellaneous] Advanced setting to control feature visibility (elastic#250157) (elastic#250830) Fix synthtrace `fetch` usage (elastic#250950) [APM] Add Nodes and Edges components and selection logic (elastic#250937) [Docs] Update alerting-settings.md and add serverless value for one parameter (elastic#250842) [Agent Builder] filestore: initial implementation (elastic#250043) [CPS] Support CPS in Vega ESQL (elastic#250693) Adjustments to cascade document esql helpers (elastic#250560) [Security Solutions] Trial Companion - adds ai chat and elastic agent detectors (elastic#250908) [Obs Presentation] Code Scanning Alert Fixes (elastic#250858) [performance] add return and refresh render scenarios to dashboard journeys (elastic#250939) skip failing test suite (elastic#245458) Add Cloud Forwarder onboarding tile to O11y Solution (elastic#250325) [Traces] Remove APM unified trace waterall embeddable registration (elastic#250808) [Discover] [Metrics] Fix: metrics grid titles do not update on order change (elastic#250963) [a11y] Fix Eui modal title annoucment (elastic#250459) [Cloud Security] [Fleet] Add cloud connector access scope for input or package level credential definitions (elastic#250280) [WorkplaceAI] SharePoint Online stack connector (elastic#248737) [Response Ops][Task Manager] Update functions do not handle API key invalidation (elastic#249109) [Osquery] Remove @kbn/timelines-plugin dependency from osquery plugin (elastic#250055) [One Discover][Logs UX] Update OpenTelemetry Semantic Conventions (elastic#250346) ...
…nvalidation (elastic#249109) Resolves elastic#244918 Resolves elastic#247560. I fixed a skipped test, bc I needed to write tests in that area. ## Summary This PR updates the bulk update function in to regenerate the API key on task update. There may be cases where we don't want to regenerate the API key, so I added an flag that can be passed in the with the request to indicate whether or not we want regenerate the API key. I made some changes to bulk update function to handle the following: - If a task with an API key is updated without a request, we cannot correctly get the encrypted saved objects client to use for update and so we throw an error. - If a task with an API key is updated with a request and the regenerate flag is set to true, we invalidate the old API key and create a new API key using the new request. - If a task without an API key is updated with a request, no changes are made bc we don't want to add an API key to this task - If a task without an API key is updated without a request, no changes are made. Note: I didn't update the `update` or `bulkPartialUpdate` functions because they are only used internally by task manager. Pls let me know if I'm misunderstanding, and I can add them there too. ### Checklist - [ ] [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 ### To verify 1. Update `scheduled_reports_service.ts` [here](https://github.com/elastic/kibana/blob/804c2bbd2aedd81fd708db7435492d9701ef90c5/x-pack/platform/plugins/private/reporting/server/services/scheduled_reports/scheduled_reports_service.ts#L496) (on line 496) with ``` await this.taskManager.bulkUpdateSchedules([id], schedule, { request: this.request, regenerateApiKey: true, }); ``` 3. Start Kibana 4. Go to discover and create a scheduled report and let it run 5. Go to dev tools and run the following command. Take note of the API key ``` POST .kibana_task_manager/_search { "query": { "term": { "task.taskType": { "value": "report:execute-scheduled" } } } } ``` 6. Go to the reporting page in stack management and edit the schedule date to be a couple minutes in the future 7. Repeat step 4 and verify that the API key has been updated. 8. Verify that after the update the scheduled report runs as expected.
…reports run schedule (#253767) Closes #245086 ## Summary As a follow-up to the changes in #249109 I am updating the call to bulkUpdateSchedules to regenerate the API key. I used #244918 as a reference where the description says: > If a task with an API key is updated with a request, we **should** invalidate the old API key and create a new API key using the new request. ## Testing 1. Create a scheduled report which should run soon 2. Verify that created scheduled report ran successfully in the Reporting > Exports tab 3. Update the report to run soon again via Reporting > Schedules or via API 4. Confirm that the edited scheduled report ran successfully
…reports run schedule (elastic#253767) Closes elastic#245086 ## Summary As a follow-up to the changes in elastic#249109 I am updating the call to bulkUpdateSchedules to regenerate the API key. I used elastic#244918 as a reference where the description says: > If a task with an API key is updated with a request, we **should** invalidate the old API key and create a new API key using the new request. ## Testing 1. Create a scheduled report which should run soon 2. Verify that created scheduled report ran successfully in the Reporting > Exports tab 3. Update the report to run soon again via Reporting > Schedules or via API 4. Confirm that the edited scheduled report ran successfully
Resolves #244918
Resolves #247560. I fixed a skipped test, bc I needed to write tests in that area.
Summary
This PR updates the bulk update function in to regenerate the API key on task update. There may be cases where we don't want to regenerate the API key, so I added an flag that can be passed in the with the request to indicate whether or not we want regenerate the API key.
I made some changes to bulk update function to handle the following:
Note: I didn't update the
updateorbulkPartialUpdatefunctions because they are only used internally by task manager. Pls let me know if I'm misunderstanding, and I can add them there too.Checklist
To verify
scheduled_reports_service.tshere (on line 496) with