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.

Sign up for GitHub

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

Jump to bottom

[APM] Show missing permissions message to the user on the Services overview #56374

Merged
merged 5 commits into from
Feb 7, 2020
Merged

[APM] Show missing permissions message to the user on the Services overview #56374

merged 5 commits into from
Feb 7, 2020

Conversation

cauemarcondes
Copy link
Contributor

closes #50052

Checks whether a user has permission to read the APM index. It does it by calling a new API /api/apm/security/permissions, it returns hasPermission: true | false. Based on this response shows the permission page on top of any APM page.

It doesn't block the UI completely, as the user can still manually close the view the open the APM page.

@cauemarcondes cauemarcondes requested a review from a team as a code owner January 30, 2020 12:28
cauemarcondes
cauemarcondes commented Jan 30, 2020
View reviewed changes
x-pack/legacy/plugins/apm/public/new-platform/plugin.tsx
))}
</Switch>
{/* Check if user has permission to read from apm index. */}
<Permission>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Permission component was added here, as it should show part of the original page, such as the Breadcrumbs

@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

smith
smith requested changes Jan 31, 2020
View reviewed changes
Copy link
Contributor

@smith smith left a comment

Choose a reason for hiding this comment

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

A few changes needed, but looks pretty nice.

x-pack/legacy/plugins/apm/server/lib/security/getPermissions.ts Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated
<EuiEmptyPrompt
iconType="apmApp"
// @ts-ignore
iconColor={null}
Copy link
Contributor

Choose a reason for hiding this comment

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

What's with this?

Copy link
Contributor Author

@cauemarcondes cauemarcondes Jan 31, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I changed it to iconColor={''} TS doesn't complain about it. But this is used to show the APM icon with color. Otherwise, it shows black and white. I will probably open a new issue because null should be valid as described here: https://elastic.github.io/eui/#/display/emptyprompt

x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
@cauemarcondes
Copy link
Contributor Author

@elasticmachine merge upstream

dgieselaar
dgieselaar approved these changes Feb 3, 2020
View reviewed changes
Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

looking good, just a couple of questions & suggestions

x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/shared/Links/ElasticDocsLink.tsx Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
dgieselaar
dgieselaar reviewed Feb 4, 2020
View reviewed changes
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/context/MatchedRouteContext.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/context/MatchedRouteContext.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/new-platform/plugin.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Main/ProvideBreadcrumbs.tsx Outdated Show resolved Hide resolved
x-pack/legacy/plugins/apm/public/components/app/Permission/index.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@cauemarcondes cauemarcondes merged commit 07cb4c1 into elastic:master Feb 7, 2020
@cauemarcondes cauemarcondes deleted the permission branch February 7, 2020 08:02
@cauemarcondes cauemarcondes mentioned this pull request Feb 7, 2020
Merged
cauemarcondes added a commit to cauemarcondes/kibana that referenced this pull request Feb 7, 2020
@cauemarcondes
[APM] Show missing permissions message to the user on the Services ov…
56ef098 
…erview (elastic#56374)

* creating permission page

* pr comments

* creating permission page

* fixing pr comments

* removing breadcrumb and adding static label to dismiss the permission page
cauemarcondes added a commit that referenced this pull request Feb 7, 2020
@cauemarcondes
[APM] Show missing permissions message to the user on the Services ov…
fa8becb 
…erview (#56374) (#57082)

* creating permission page

* pr comments

* creating permission page

* fixing pr comments

* removing breadcrumb and adding static label to dismiss the permission page
jloleysens added a commit to jloleysens/kibana that referenced this pull request Feb 7, 2020
@jloleysens
Merge branch 'console/feature/text-objects-in-saved-objects' of githu…
9e266c6 
…b.com:jloleysens/kibana into console/feature/text-objects-in-saved-objects

* 'console/feature/text-objects-in-saved-objects' of github.com:jloleysens/kibana: (103 commits)
  fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998)
  TS of esKuery\node_types  (elastic#56857)
  Kibana app migration: Move static code dependencies into kibana_legacy plugin, part 1 (elastic#56408)
  Retry ES API calls that fail with 410/Gone (elastic#56950)
  [APM] Show missing permissions message to the user on the Services overview (elastic#56374)
  Fixing flaky CI tests for custom appRoutes (elastic#55763)
  [State Management][Docs] State syncing utils docs (elastic#56479)
  [Index management] Remove index mapper setting in tests (elastic#57066)
  Exposed common EuiExpressions to separate components be able to reuse for building new for Alert Types  (elastic#56466)
  [SIEM] update url state between page if date is relative (elastic#56813)
  fix for chart_types test (elastic#57056)
  chore(NA): remove compress from dll minimizer (elastic#57023)
  [File upload] Migrate routing to NP & add route validation (elastic#52313)
  Adding docs for grouped nav advanced setting (elastic#57013)
  Use i18n titles for field formatters, human names for numeral locales (elastic#56348)
  [Maps] Remove EMS catalogue url from docs (elastic#57020)
  [Endpoint] ERT-82 ERT-83 ERT-84: Alert list API with pagination (elastic#56538)
  [DOCS] Adds Apple notarization info to install doc (elastic#57042)
  [ML] New Platform server shim: update results service routes to use new platform router (elastic#56886)
  Fix typo on detection engine rule (elastic#56993)
  ...
majagrubic pushed a commit to majagrubic/kibana that referenced this pull request Feb 10, 2020
@cauemarcondes
[APM] Show missing permissions message to the user on the Services ov…
3b0d84f 
…erview (elastic#56374)

* creating permission page

* pr comments

* creating permission page

* fixing pr comments

* removing breadcrumb and adding static label to dismiss the permission page
@cauemarcondes cauemarcondes mentioned this pull request Feb 10, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dgieselaar dgieselaar dgieselaar approved these changes

@smith smith smith approved these changes

Assignees
No one assigned
Labels
release_note:enhancement v7.7.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[APM] Show missing permissions message to the user on the Services overview
4 participants
@cauemarcondes @kibanamachine @smith @dgieselaar