Skip to content
This repository has been archived by the owner on Aug 9, 2022. It is now read-only.

use session cookie for puppeteer to access url of security-enabled domain #129

Merged

Conversation

zhongnansu
Copy link
Member

@zhongnansu zhongnansu commented Oct 20, 2020

Issue #, if available:

Description of changes:

  • use session cookie for puppeteer to access url on security-enabled domain. Most changes in this filekibana-reports/server/routes/utils/visualReportHelper.ts.
    • extract cookie from request header and find the correct one named security_authentication
    • construct cookie object that puppeteer accepts
    • pass cookie object to puppeteer page.setCookie(cookie) before page.goto(queryUrl)
    • Removed the temp solution which uses puppeteer to simulate login.
  • Continue optimizing the project layout.
    • Extract and create createReport.ts, deliverReport.ts , createScheduleReport.ts, visualReportHelper.ts from reportHelper.ts. Remove reportHelpers
    • Extract function createSchedule from reportDefinition.ts and create a new file createSchedule.ts
    • extract all index update/save operation from routehandler to helpers.ts

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@zhongnansu zhongnansu marked this pull request as ready for review October 20, 2020 23:14
*/
return;
}
// else if (triggerType == TRIGGER_TYPE.alerting) {
Copy link
Contributor

Choose a reason for hiding this comment

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

np: I think we can get rid of the commented-out else if block since there is a TODO statement

Copy link
Member Author

Choose a reason for hiding this comment

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

yes will do

return res;
} else if (triggerType == TRIGGER_TYPE.onDemand) {
/*
* TODO: return nothing for on Demand report, because currently on-demand report is handled by client side,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the TODO here? It looks like we already return nothing for on-demand report?

Copy link
Contributor

@davidcui1225 davidcui1225 left a comment

Choose a reason for hiding this comment

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

LGTM- a few minor notes in the comments

@zhongnansu zhongnansu merged commit f52e04f into opendistro-for-elasticsearch:dev Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants