Skip to content

Allow users with read access to view Integrations app#113925

Merged
joshdover merged 10 commits intoelastic:masterfrom
joshdover:int-read-access
Oct 13, 2021
Merged

Allow users with read access to view Integrations app#113925
joshdover merged 10 commits intoelastic:masterfrom
joshdover:int-read-access

Conversation

@joshdover
Copy link
Contributor

@joshdover joshdover commented Oct 5, 2021

Summary

Fixes #94322

This makes the following changes:

  • Removes the superuser requirement from the Integrations app
  • Hides the "Policies" and "Settings" tabs from the Integration detail view for read-only users
  • Filters the "Assets" tab based on what the user has privileges to access
  • Adds tooltip for users without enough privileges to install an integration
  • Adds tooltip and callout for clusters without security enabled + link to guide on what needs to be configured

Release Note

Users without the superuser role may view integrations and their assets in the Integrations app if they have "read" permissions for Fleet & Integrations.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@joshdover joshdover added the Feature:Unified Integrations Unified Integrations view feature label Oct 5, 2021
@joshdover joshdover mentioned this pull request Oct 5, 2021
63 tasks
@mostlyjason
Copy link
Contributor

mostlyjason commented Oct 5, 2021

@joshdover Thanks for thinking through this scenario. We didn't define it in our doc but its important to consider because self-managed clusters do not have security enabled by default. I want to make sure I understand your proposal. Today when a user starts a self-managed cluster, security is disabled by default. When they visit the integrations page, they see the list of elastic agent integrations but get an error if they lack superuser or security is not enabled. In this PR, you are proposing that when this user visits the integrations page they do not see any elastic agent integrations nor any notice that they are missing integrations? If so, that seems like a regression since they lose visibility to both the integrations and the requirements to enable them. I'm worried that a large percentage of users in new self-managed clusters will be unaware that they are losing visibility.

One option is consistent with our design for when the user lacks privileges to add agent integrations. We show a read-only mode, disable the add integration button and potentially add hovertext to explain that they need to enable security. The advantage is that users get visibility to the full set of integrations and a message explaining how to enable them. If they are not interested in using Elastic Agent integrations they can use the filter on the browse tab or clear the EPR URL.
image

A second option is to show a info box similar to when EPR is not accessible. This tells the user they need to enable security to see the full set of integrations. In this case, users get defaulted to Beats automatically. However, I'm not sure that users with security disabled necessarily prefer Beats over Elastic Agent. It could just be they didn't around to enabling security yet, but they intend to do so eventually.
image

I prefer the first option since it offers more visibility to agent integrations, but curious what you and @alexfrancoeur think too.

@joshdover
Copy link
Contributor Author

joshdover commented Oct 5, 2021

In this PR, you are proposing that when this user visits the integrations page they do not see any elastic agent integrations nor any notice that they are missing integrations? If so, that seems like a regression since they lose visibility to both the integrations and the requirements to enable them. I'm worried that a large percentage of users in new self-managed clusters will be unaware that they are losing visibility.

Good point on losing discoverability of integrations here. My thinking was that it wasn't a regression since they already couldn't access them, but you are right that we would be removing one key way to discover that Elastic Agent integrations exist.

Poking around a bit more in the code, I'm not too worried about the scope required to support either of the experiences you suggest above. If anything, option (1) is easier. I do wonder if we'd be making things confusing with option (1) since most of the tiles shown to the user in this case wouldn't be actionable for them unless they enable security. Could that reduce ingestion rates for on-prem clusters in 7.16?

One option could be to make the Agents vs. Beats toggle switch persisted for the user in localStorage so that they can see the view that makes the most sense for them.
EDIT: I see that this is already part of the scope for that toggle

@mostlyjason
Copy link
Contributor

Thinking through this again, I'd love to provide users a link to documentation so they can discover how to enable security. Its not an easy or intuitive process. We have a full page banner in Fleet with links to docs, but not in the Integrations app.

@dborodyansky
what are your thoughts on how to show a message with a docs link when security needs to be enabled?

@mostlyjason
Copy link
Contributor

Just had talk with Dmitry and he's going to take this on. Also, it looks like the security check in Fleet is only partially enabled since its implemented in the wrong order #83001. We also need to add an API key check.

@joshdover
Copy link
Contributor Author

joshdover commented Oct 6, 2021

Just had talk with Dmitry and he's going to take this on. Also, it looks like the security check in Fleet is only partially enabled since its implemented in the wrong order #83001. We also need to add an API key check.

Great, makes sense. I'll update this PR to support the following in both scenarios (where security is disabled in ES only and where security is disabled in Kibana and ES):

  • Integrations app is visible and functional
  • EPR packages are included in integrations grid
  • UI is displayed as if user is superuser:
    • The "Add integration" button is visible and enabled (clicking leads to security disabled UI)
    • Only the overview tab is displayed on integrations detail view
  • Clicking on any actions that lead to an API that requires superuser shows an error about not having superuser (which will be improved by [Fleet] Display security checks when security is disabled #83001 to instead show the instructions for enabling security requirements)

We can then leverage the fix for #83001 and add any additional UX improvements for this scenario in a separate PR. Does that make sense to you, @mostlyjason?

@joshdover
Copy link
Contributor Author

I've pushed up changes to allow the same behavior regardless of how security is disabled (in ES, or ES and Kibana). @mostlyjason let me know if the plan I proposed above works and we can set this to ready for review.

@dborodyansky
Copy link
Contributor

@mostlyjason @joshdover Please let me know if I understood this correctly. My assumptions are limitations result in inability to add an integration. Limitations include:

  1. Security being disabled, which may be remedied by the user with doc guidance.
  2. Privilege limitations, which require administrator assist.

With this in mind, what do you think of the following proposal:

image

@mostlyjason
Copy link
Contributor

mostlyjason commented Oct 7, 2021

Thanks @dborodyansky! We should only show the tabs and actions appropriate for read-only mode of the UI. You can read up on those requirements in my google doc titled "Allow non-superusers to access the integrations app". Also, where does the "Steps in this guide" link to? Does it handle both security being disabled and the API key being disabled?

@joshdover I'd modify your proposal for when security is disabled in this way

  • UI is displayed as if user is a read only user:
    • The "Add integration" button is disabled
    • Only the overview tab and asset tabs are displayed on integrations detail view
  • The user is not allowed to click on any actions that require superuser
  • API calls that requires superuser shows an error about not having superuser (or about not having security enabled)
  • Include Dmitry's designs

@joshdover
Copy link
Contributor Author

  • UI is displayed as if user is a read only user:
    • The "Add integration" button is disabled
    • Only the overview tab and asset tabs are displayed on integrations detail view
  • The user is not allowed to click on any actions that require superuser
  • API calls that requires superuser shows an error about not having superuser (or about not having security enabled)

All of this is already covered in this PR for read-only users. The only difference here when security is disabled is that the Asset tab won't be present (since integrations can't be installed), so they'll only see the Overview tab.

  • Include Dmitry's designs

👍

@joshdover
Copy link
Contributor Author

@dborodyansky @mostlyjason a few questions related to the designs:

  • Should the "follow this guide" link just send the user to /app/fleet where the user will see the instructions once [Fleet] Display security checks when security is disabled #83001 is fixed?
  • FYI when security is disabled, there's no way of telling if the user (since there is no user) is someone who has access to the cluster's configuration. So I can't differentiate between the top-right and bottom-right situations. Maybe we should use copy on the tooltip that covers both cases?

@mostlyjason
Copy link
Contributor

mostlyjason commented Oct 11, 2021

@joshdover I think if security is disabled then they cannot log in as a superuser role, so the top right use case is not possible. The bottom left indicates the next step they need to take.

I think it'd be fine to link to Fleet to show the dialog. We need to make sure the two checks align so the user doesn't get redirected and see nothing.

@dborodyansky
Copy link
Contributor

Does the following look accurate?
image

@joshdover
Copy link
Contributor Author

I think if security is disabled then they cannot log in as a superuser role, so the top right use case is not possible. The bottom left indicates the next step they need to take.

++ this is what I was trying to explain, but this was more clearly put 😅

Does the following look accurate?

Yes, thanks all. I'll have this in review tomorrow.

@joshdover joshdover added auto-backport Deprecated - use backport:version if exact versions are needed Team:Fleet Team label for Observability Data Collection Fleet team v7.16.0 labels Oct 13, 2021
@joshdover joshdover added v8.0.0 enhancement New value added to drive a business result labels Oct 13, 2021
@joshdover joshdover marked this pull request as ready for review October 13, 2021 11:05
@joshdover joshdover requested a review from a team as a code owner October 13, 2021 11:05
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@joshdover joshdover added release_note:enhancement and removed enhancement New value added to drive a business result labels Oct 13, 2021
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 - Created a read-only user w/ the viewer role in Kibana and was able to view integrations on this branch.

@joshdover joshdover enabled auto-merge (squash) October 13, 2021 14:32
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

lgtm

can confirm:

  • created user with viewer-role
  • can access integration page
  • cannot install an AWS package
  • tooltip shows with extra info
  • asset-tab is hidden (except when package is installed)

Wasn't sure how to verify the callout for missing security-enablement.

@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
fleet 630.2KB 630.7KB +499.0B

Page load bundle

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

id before after diff
fleet 104.9KB 105.0KB +142.0B

History

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

@joshdover joshdover merged commit 3ab04b6 into elastic:master Oct 13, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 113925

@thomasneirynck
Copy link
Contributor

this backport is failing because we're backed up a little on backports of prereqs. Working through this as we speak. Will create new backport once we're through all the outstanding prereqs (this one likely #114432 (comment))

@thomasneirynck
Copy link
Contributor

once #114910 merges, I'll backport this one

thomasneirynck pushed a commit to thomasneirynck/kibana that referenced this pull request Oct 13, 2021
thomasneirynck added a commit that referenced this pull request Oct 14, 2021
)

Co-authored-by: Josh Dover <1813008+joshdover@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 - use backport:version if exact versions are needed Feature:Unified Integrations Unified Integrations view feature release_note:enhancement Team:Fleet Team label for Observability Data Collection Fleet team v7.16.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Fleet] Allow read-only access to integrations

7 participants