Skip to content
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

Introduce capabilities provider and switcher to file upload plugin #96593

Merged
merged 5 commits into from May 14, 2021

Conversation

legrego
Copy link
Member

@legrego legrego commented Apr 8, 2021

Summary

Adds a capabilities provider and switcher to the File Upload plugin. This provides a new fileUpload.show UI Capability, which can be used to tell if the current user should be shown the File Upload feature.

Users with the ability to create index patterns (in the current space) will be shown this feature. Users who are lacking this privilege will not be shown this feature.

^ We have the flexibility to change these privilege checks to whatever we need...I just picked something to start the conversation. Should we also test that the user can create pipelines?

The file upload plugin requires its own capabilities provider & switcher because we don't have the concept of a "composite feature" today, where it relies on both Kibana and Elasticsearch privileges in order to function. This is a limitation of the current authorization model, and is being tracked in #96598

@legrego
Copy link
Member Author

legrego commented Apr 8, 2021

@nreese @jgowdyelastic here's a potential solution to the file upload problem we discussed today. Let me know your thoughts!

@legrego
Copy link
Member Author

legrego commented May 6, 2021

let me know if this is still needed

@legrego legrego closed this May 6, 2021
@nreese
Copy link
Contributor

nreese commented May 10, 2021

let me know if this is still needed

Thanks @legrego for putting this PR together. This is still something very much that we are interested in. We hope to use this in the current minor sprint to make it possible to have a ingest button in the home application that will work for users without specific access to ML or maps feature privileges. I am not sure on the timing but this will be a requirement for this effort.

@nreese nreese reopened this May 10, 2021
@legrego legrego added auto-backport Deprecated: Automatically backport this PR after it's merged Feature:File Upload release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0 labels May 10, 2021
Comment on lines +31 to +36
const { hasImportPermission } = await checkFileUploadPrivileges({
authorization: security?.authz,
request,
checkCreateIndexPattern: true,
checkHasManagePipeline: false,
});
Copy link
Member Author

Choose a reason for hiding this comment

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

We have the flexibility to change these privilege checks to whatever we need...I just picked something to start the conversation. Should we also test that the user can create pipelines?

@legrego legrego marked this pull request as ready for review May 10, 2021 19:54
@legrego legrego requested a review from a team as a code owner May 10, 2021 19:54
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM from the security side, left just a few nits and one question about anonymous request.

x-pack/plugins/security/server/index.ts Outdated Show resolved Hide resolved
x-pack/plugins/file_upload/server/capabilities.ts Outdated Show resolved Hide resolved
});

core.capabilities.registerSwitcher(async (request, capabilities, useDefaultCapabilities) => {
const isAnonymousRequest = !request.route.options.authRequired;
Copy link
Member

Choose a reason for hiding this comment

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

question: is there any reason why we cannot use request.auth.isAuthenticated (or request.route.options.authRequired !== true && !request.auth.isAuthenticated) here instead? If authRequired === 'optional' and request is not authenticated, it seems we can treat it as a legitimate anonymous request as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wish I had a better memory -- there used to be a reason we couldn't use request.auth.isAuthenticated with the other capability switchers, but I can't remember the details. That said, it looks like the security switcher is using this very check, so maybe it's safe to do so again.

I'll experiment with this and report back!

Copy link
Member Author

Choose a reason for hiding this comment

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

I modeled this switcher after the security plugin's switcher, and it appears to be working correctly!


const checkPrivileges = authorizationService.checkPrivilegesDynamicallyWithRequest(request);
const checkPrivilegesResp = await checkPrivileges(checkPrivilegesPayload);
const { hasImportPermission } = await checkFileUploadPrivileges({
Copy link
Member

Choose a reason for hiding this comment

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

this route (/internal/file_upload/has_import_permission) might not be needed now if we can use the capabilities on the client side in the fileDataVisualiser plugin.
but that change can be done in a follow up.

Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

LGTM

@legrego legrego requested a review from azasypkin May 12, 2021 16:40
@legrego
Copy link
Member Author

legrego commented May 12, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
security 31 32 +1

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
security 12 15 +3
Unknown metric groups

API count

id before after diff
security 72 73 +1

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 4 2 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 5 3 -2
licensing 18 15 -3
monitoring 109 56 -53
total -73

History

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

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM

@legrego legrego merged commit c572ddd into elastic:master May 14, 2021
@legrego legrego deleted the fileUpload/capabilities branch May 14, 2021 12:31
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request May 14, 2021
…lastic#96593)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request May 14, 2021
…96593) (#100114)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

Co-authored-by: Larry Gregory <larry.gregory@elastic.co>
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 17, 2021
…lastic#96593)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
…lastic#96593)

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated: Automatically backport this PR after it's merged Feature:File Upload release_note:skip Skip the PR/issue when compiling release notes v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants