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

[Cases] Prevent fetching connectors if the user does not have read access to the actions plugin #127872

Merged
merged 14 commits into from
Mar 21, 2022

Conversation

cnasikas
Copy link
Member

@cnasikas cnasikas commented Mar 16, 2022

Summary

Fixes: #126784

Screenshots

Screenshot 2022-03-17 at 1 33 17 PM

Screenshot 2022-03-17 at 1 33 08 PM

Screenshot 2022-03-17 at 1 32 58 PM

Screenshot 2022-03-17 at 3 30 14 PM

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@cnasikas cnasikas added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.2.0 labels Mar 16, 2022
@cnasikas cnasikas self-assigned this Mar 16, 2022
@cnasikas cnasikas requested a review from a team as a code owner March 16, 2022 13:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops-cases (Feature:Cases)

@cnasikas cnasikas marked this pull request as draft March 16, 2022 13:04
@cnasikas cnasikas requested a review from academo March 17, 2022 11:31
@cnasikas cnasikas marked this pull request as ready for review March 17, 2022 11:31
@cnasikas cnasikas force-pushed the fix_connectors_priv branch from f75bc25 to 87402b4 Compare March 17, 2022 11:32
@@ -0,0 +1,109 @@
/*
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved it from x-pack/plugins/cases/public/containers/configure/mock.ts to a new common file.

@cnasikas
Copy link
Member Author

@elasticmachine merge upstream

crud: !!casesCapabilities?.crud_cases,
read: !!casesCapabilities?.read_cases,
},
visualize: { crud: !!capabilities.visualize?.save, read: !!capabilities.visualize?.show },
Copy link
Contributor

@academo academo Mar 21, 2022

Choose a reason for hiding this comment

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

I'm not a fan of proxying values like this. Because we can simply put this code in the place we need it and that's it. That's why I kept visualize as a single boolean value that checks both conditions instead of separate ones.

Copy link
Member Author

@cnasikas cnasikas Mar 21, 2022

Choose a reason for hiding this comment

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

There are scenarios in which you need to know if the user has read or crud access and not both. Also, as a consumer of the hook if I have a single boolean I am not sure what kind of permissions the user has. I have to look at the code of the hook. By having separate properties I can immediately understand what is going on, it leaves no ambiguity and avoids security mistakes.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, we can use the kibana capabilities object directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Each plugin has a different name to define crud or read operations. As a developer, I need to know every time what this property is called and what is about. Also, the capabilities object is untyped. Having the hook add typing and there is no need to know the names. It will always be crud and read. Also, some of them have more than two capabilities. For example, the actions plugin has read, execute, delete, save. Surprisingly read, execute are both considered as read. A new developer may make a mistake and use the wrong one. For all that reasons, I think is fine having this hook that a) force types b) export the same contract c) centralize all cases dependencies regarding capabilities.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was not aware the capabilities was totally untyped. that's a good point. I see your point then as using this for standardizing them.

// TODO remove this check after the lens plugin fixes this bug
appCapabilities.dashboard
appCapabilities?.dashboard.crud
Copy link
Contributor

Choose a reason for hiding this comment

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

After this #127963 merged we can remove this additional dashboard check.

Copy link
Contributor

@academo academo left a comment

Choose a reason for hiding this comment

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

Thanks for the test changes!

My only comment is about proxying the capabilities object. I see more problematic that we take the existing values from kibana capabilities, put them in a custom object and re-export the same attributes with a different name. e.g. crud instead of save.

I rather we either use kibana capabilities directly or we actually enhance the capabilities.

@cnasikas cnasikas enabled auto-merge (squash) March 21, 2022 09:12
@cnasikas cnasikas merged commit f7c6183 into elastic:main Mar 21, 2022
@kibana-ci
Copy link
Collaborator

💚 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
cases 292.9KB 292.4KB -503.0B

Page load bundle

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

id before after diff
cases 84.2KB 85.1KB +870.0B

History

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

cc @cnasikas

@cnasikas cnasikas deleted the fix_connectors_priv branch March 21, 2022 11:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting bug Fixes for quality problems that affect the customer experience Feature:Cases Cases feature release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Cases] "Create case" page shows error for minimally-privileged user
5 participants