Skip to content

[File Data Visualizer] Fixing security plugin use in filebeat config generation#98264

Closed
jgowdyelastic wants to merge 5 commits intoelastic:masterfrom
jgowdyelastic:fixing-generated-filebeat-config-security-check
Closed

[File Data Visualizer] Fixing security plugin use in filebeat config generation#98264
jgowdyelastic wants to merge 5 commits intoelastic:masterfrom
jgowdyelastic:fixing-generated-filebeat-config-security-check

Conversation

@jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Apr 26, 2021

The file beat config generator requires the security setup contract, but was incorrectly using the start contract.
This PR separates the file data visuaizer plugin's setup and start dependencies so the correct security is used.

Adds a basic functional test to ensure the flyout opens correctly when the link is clicked.

@jgowdyelastic jgowdyelastic self-assigned this Apr 26, 2021
@jgowdyelastic jgowdyelastic added :ml Feature:File and Index Data Viz ML file and index data visualizer Feature:File Upload non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.13.0 v7.14.0 v8.0.0 labels Apr 26, 2021
@jgowdyelastic jgowdyelastic marked this pull request as ready for review April 26, 2021 09:54
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner April 26, 2021 09:54
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM. Just left a minor comment!

await ml.dataVisualizerFileBased.startImportAndWaitForProcessing();

await ml.testExecution.logTestStep('creates filebeat config');
await ml.dataVisualizerFileBased.selectCreateFileBeatConfig();
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - Filebeat is one word, so this should be selectCreateFilebeatConfig I think.

});
},

async selectCreateFileBeatConfig() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit - Filebeat is one word, so this should be selectCreateFilebeatConfig I think.

Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

Tested and LGTM

@jgowdyelastic jgowdyelastic added the auto-backport Deprecated - use backport:version if exact versions are needed label Apr 26, 2021
> {
public setup() {}
public setup(core: CoreSetup, plugins: FileDataVisualizerSetupDependencies) {
setSetupServices(plugins);
Copy link
Contributor

Choose a reason for hiding this comment

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

@elastic/kibana-security Would it be possible to expose authc on start as well as setup? File data visualizer uses authc outside of plugin setup.

@jgowdyelastic From my understanding, setup services should only be used during setup and not after the plugin has started.

Copy link
Member

Choose a reason for hiding this comment

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

@nreese No objections from me. IMO the services exposed by authc are more appropriate for the start phase anyway

Copy link
Member Author

@jgowdyelastic jgowdyelastic Apr 26, 2021

Choose a reason for hiding this comment

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

authc is just used here to get the current username.
@elastic/kibana-security what is the correct way to get the current user at runtime?

Copy link
Member

Choose a reason for hiding this comment

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

authc.getCurrentUser() is the best way to get the current user

Copy link
Member Author

Choose a reason for hiding this comment

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

@legrego is this something that could be done for 7.13?
If not, I think i'll have to keep this change as is 7.13 and change it for future versions once authc is available.

Copy link
Member

Choose a reason for hiding this comment

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

Probably. Follow along here: #98307

Copy link
Member

Choose a reason for hiding this comment

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

#98307 has merged. Backports are in progress

@jgowdyelastic jgowdyelastic requested a review from a team as a code owner April 26, 2021 14:51
@jgowdyelastic
Copy link
Member Author

I've raised a new PR #98312 to cover the changes in this PR which aren't related to the security plugin.
In case this PR isn't needed because of #98307

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
fileDataVisualizer 1.0MB 1.0MB +7.7KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
fileDataVisualizer 11.8KB 12.1KB +317.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @jgowdyelastic

@jgowdyelastic
Copy link
Member Author

Closing in favour of #98312

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Deprecated - use backport:version if exact versions are needed Feature:File and Index Data Viz ML file and index data visualizer Feature:File Upload :ml non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.13.0 v7.14.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants