[SLO] hide service details link for read only users #170731

mgiota · 2023-11-07T11:22:18Z

Fixes #170610

apmmachine · 2023-11-07T11:22:34Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
/oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

elasticmachine · 2023-11-07T11:22:42Z

Pinging @elastic/actionable-observability (Team: Actionable Observability)

kdelemme

I think this is not the correct behaviour. We should show the button regardless of SLO read or write capabilities. Having SLO read capability should allow customers to debug and investigate issues, therefore allowing them to go into the APM service is expected/mandatory.

The only capability that could be used for showing or not this button might be the read/write APM capability. I'm saying could because I'm not even sure we should check APM related capabilities from here. IMO, we should let APM handles its access control when the user lands on the APM application. If the user experience they provide is not great, I believe we should open an issue for them to fix (with a redirection or something else)

mgiota · 2023-11-21T10:11:45Z

The only capability that could be used for showing or not this button might be the read/write APM capability. I'm saying could because I'm not even sure we should check APM related capabilities from here. IMO, we should let APM handles its access control when the user lands on the APM application. If the user experience they provide is not great, I believe we should open an issue for them to fix (with a redirection or something else)

@kdelemme You are right, this is not the right behavior. The SLO capabilities are irrelevant. I was about to create a ticket for APM to properly handle its access control when the user lands on the APM application. But I have a question before doing so. In my scenario user has None APM feature privilege and that's why the APM app doesn't show up at all in the sidebar navigation. So where would the user be redirected, if there is no APM app in the sidebar? In this case, maybe easiest would be to check the read/write APM capability to show or hide the link accordingly. What do you think?

kdelemme · 2023-11-21T12:33:56Z

The link is simply hidden from the menu, but it still exists. I'm still not sure we should handle apm capability from observability/SLO but that's not a battle I'm ready to die on the hill for 😀
Maybe @dgieselaar has some thoughts about that?

mgiota · 2023-11-21T13:02:08Z

@kdelemme Yep I know the link still exists, but I am trying to imagine what the workflow would be for the user.

There is no visible APM link
User goes to SLO and clicks on Service details link
User is redirected back to the SLO detail page with a proper message sent from the APM app. I don't know how this happens behind the scenes, but point is there should be a redirection back to the SLO page, right?

Anyway this is really an edge case, having a user with read only access to SLO and none for APM. I created an issue for APM and they decide how/if they can fix on their side.

dgieselaar · 2023-11-21T19:40:05Z

@kdelemme I think it makes more sense to handle it when linking to an app rather than having the app show an error page. It's just frustrating to the user if we give them a navigation option that results in an error. I dont think we should be thinking about app boundaries here either - if this was part of the Observability app (or the SLO app), would you also link to a page that results in error?

kdelemme · 2023-11-21T19:59:38Z

Thanks @dgieselaar for taking the time to give your input.
I guess my reasoning is that nothing prevents the customer from giving the APM link on another medium (could be an alert message including the link to APM), and the user with less privileges will still get an error trying to open the link. That's why I suggest handling this at the destination app level at a bare minimum.

That being said, I'm fine handling this from our app, as it avoids an unnecessary error being displayed to the less privileged user when they try to access that link directly from our app, but IMO that's a bandage e.g. a link from an alert message will still result in an error for less privileged users.

if this was part of the Observability app (or the SLO app), would you also link to a page that results in error?

I think this is different because in that case we own the capability and the app. We know when we change them, and can keep them in sync. But nothing prevents APM to decide that now the minimum permission must be write to access that particular page, and would we remember to update the SLO app? I know it will probably never happen, but that's my reasoning.

Anyway, I think it's too much philosophical discussion for the size of the fix, let's get it done and move on 😄

dgieselaar · 2023-11-21T20:18:58Z

@kdelemme I don't think it's a bandaid. We really shouldn't create links (anywhere, but especially not in the UI) that lead to error states. I'm also not sure whether APM is not handling this correctly. From the code it looks like this is a platform level decision (and "application not found" is consistent with the fact that we e.g. return 404s on API routes the user does not have access to).

I think this is different because in that case we own the capability and the app. We know when we change them, and can keep them in sync. But nothing prevents APM to decide that now the minimum permission must be write to access that particular page, and would we remember to update the SLO app? I know it will probably never happen, but that's my reasoning.

This can still be handled by APM or platform exposing an API that will tell you whether the user has access to this link. It shouldn't land on the user's plate to deal with this.

kdelemme · 2023-11-21T20:46:43Z

@dgieselaar

This can still be handled by APM or platform exposing an API that will tell you whether the user has access to this link. It shouldn't land on the user's plate to deal with this.

Agree! But relying on an API provided by the destination app is very different than relying on a the raw capability boolean.

mgiota · 2023-11-22T14:09:46Z

@dgieselaar Thanks for the input. As part of the PR I will check the raw APM capability and when/if APM team decides to provide such an API, then we can switch to using the API.

…_link_read_only

shahzad31 · 2023-11-28T12:14:12Z

x-pack/plugins/observability/public/pages/slo_details/components/header_control.tsx

@@ -202,6 +203,7 @@ export function HeaderControl({ isLoading, slo }: Props) {
                <EuiContextMenuItem
                  key="exploreInApm"
                  icon="bullseye"
+                  disabled={!hasApmReadCapabilities}


would be nice to show tooltip around on why button is disabled.

Yep I could, it would look like this. Does the text look good to you?

My only concern is that we don't show tooltips to the rest disabled icons. But this one I guess is different from the rest, cause it's the only one related to apm privileges and not SLO privileges.

@maciejforcone What do you think? Shall I show the tooltip in this case?

I was thinking of disabling the entire "Actions" button if there are no available actions for the user (with limited permissions, like in this scenario).

@maciejforcone Good idea! 'll merge current PR as it is to have the bug fixed and I will disable the Actions button in another PR with a few tests I want to cover.

elasticmachine · 2023-11-28T12:14:17Z

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

shahzad31

LGTM !!

mgiota · 2023-11-28T23:01:58Z

@elasticmachine merge upstream

mgiota · 2023-11-29T08:39:16Z

@elasticmachine merge upstream

kibana-ci · 2023-11-29T09:46:10Z

💚 Build Succeeded

Buildkite Build
Commit: 6d8520e

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
`observability`	1.1MB	1.1MB	+40.0B

History

💔 Build #179810 failed 800bf9b
💔 Build #179801 failed 303180d
💔 Build #179498 failed a18d779
💚 Build #173771 succeeded d4b452e

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

cc @mgiota

hide service details link for read only users

d4b452e

mgiota requested a review from a team as a code owner November 7, 2023 11:22

mgiota self-assigned this Nov 7, 2023

mgiota added the Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" label Nov 7, 2023

mgiota added v8.12.0 release_note:skip Skip the PR/issue when compiling release notes labels Nov 7, 2023

kdelemme requested changes Nov 7, 2023

View reviewed changes

mgiota marked this pull request as draft November 10, 2023 20:33

mgiota added 2 commits November 28, 2023 12:33

Merge branch 'main' of github.com:elastic/kibana into service_details…

7b26c5f

…_link_read_only

show Service details link if user has apm read access

a18d779

mgiota marked this pull request as ready for review November 28, 2023 12:00

mgiota requested a review from a team as a code owner November 28, 2023 12:00

shahzad31 reviewed Nov 28, 2023

View reviewed changes

botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Nov 28, 2023

shahzad31 approved these changes Nov 28, 2023

View reviewed changes

mgiota requested a review from kdelemme November 28, 2023 12:56

kdelemme approved these changes Nov 28, 2023

View reviewed changes

mock apm capabilities

303180d

mgiota enabled auto-merge (squash) November 28, 2023 23:00

Merge branch 'main' into service_details_link_read_only

800bf9b

Merge branch 'main' into service_details_link_read_only

6d8520e

mgiota merged commit 2914d25 into elastic:main Nov 29, 2023

kibanamachine added the backport:skip This commit does not require backporting label Nov 29, 2023

mgiota mentioned this pull request Nov 30, 2023

[SLO] Service details link for user with no APM kibana privilege is broken #171634

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SLO] hide service details link for read only users #170731

[SLO] hide service details link for read only users #170731

mgiota commented Nov 7, 2023

apmmachine commented Nov 7, 2023

elasticmachine commented Nov 7, 2023

kdelemme left a comment

mgiota commented Nov 21, 2023 •

edited

Loading

kdelemme commented Nov 21, 2023

mgiota commented Nov 21, 2023

dgieselaar commented Nov 21, 2023

kdelemme commented Nov 21, 2023

dgieselaar commented Nov 21, 2023

kdelemme commented Nov 21, 2023

mgiota commented Nov 22, 2023

shahzad31 Nov 28, 2023

mgiota Nov 28, 2023

maciejforcone Nov 28, 2023

mgiota Nov 28, 2023

elasticmachine commented Nov 28, 2023

shahzad31 left a comment

mgiota commented Nov 28, 2023

mgiota commented Nov 29, 2023

kibana-ci commented Nov 29, 2023

[SLO] hide service details link for read only users #170731

[SLO] hide service details link for read only users #170731

Conversation

mgiota commented Nov 7, 2023

apmmachine commented Nov 7, 2023

🤖 GitHub comments

elasticmachine commented Nov 7, 2023

kdelemme left a comment

Choose a reason for hiding this comment

mgiota commented Nov 21, 2023 • edited Loading

kdelemme commented Nov 21, 2023

mgiota commented Nov 21, 2023

dgieselaar commented Nov 21, 2023

kdelemme commented Nov 21, 2023

dgieselaar commented Nov 21, 2023

kdelemme commented Nov 21, 2023

mgiota commented Nov 22, 2023

shahzad31 Nov 28, 2023

Choose a reason for hiding this comment

mgiota Nov 28, 2023

Choose a reason for hiding this comment

maciejforcone Nov 28, 2023

Choose a reason for hiding this comment

mgiota Nov 28, 2023

Choose a reason for hiding this comment

elasticmachine commented Nov 28, 2023

shahzad31 left a comment

Choose a reason for hiding this comment

mgiota commented Nov 28, 2023

mgiota commented Nov 29, 2023

kibana-ci commented Nov 29, 2023

💚 Build Succeeded

Metrics [docs]

Async chunks

History

mgiota commented Nov 21, 2023 •

edited

Loading