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

[Guided onboarding] Use Kibana features to grant access #155065

Merged
merged 22 commits into from
Apr 26, 2023

Conversation

yuliacech
Copy link
Contributor

@yuliacech yuliacech commented Apr 17, 2023

Summary

Fixes #149132

This PR adds a Kibana feature for the guided onboarding plugin for better permissions handling. By default kibana_admin and editor roles are granted access to guided onboarding. The role viewer on the other hand doesn't have enough permissions to see or use guided onboarding. For any roles that don't have the correct permissions, guided onboarding is completely disabled, the same as it's disabled on-prem.
When creating a new role, the feature "Setup guides" can be enabled or disabled.

How to test

  1. Add xpack.cloud.id: 'testID' to /config/kibana.dev.yml
  2. Start ES with yarn es snapshot and Kibana with `yarn start``
  3. Login as elastic and create a test user with the role viewer
  4. Clear everything from your browser's local storage
  5. Login as the test user and check the following
  • On the first visit, the "on-prem" welcome message is shown (not the guided onboarding landing page)
  • The url /app/home#/getting_started is unknown and redirects back to the home page
  • There is no button "Setup guides" in the header
  • There is no link "Setup guides" in the help menu

Checklist

@yuliacech yuliacech added Team:Journey/Onboarding Platform Journey Onboarding team release_note:skip Skip the PR/issue when compiling release notes v8.8.0 labels Apr 17, 2023
@@ -57,7 +57,8 @@ export default function navLinksTests({ getService }: FtrProviderContext) {
'enterpriseSearchContent',
'enterpriseSearchAnalytics',
'appSearch',
'workplaceSearch'
'workplaceSearch',
'guidedOnboarding'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure what is the correct approach to fix these tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/kibana-security Another question for the team: if you own these tests, could you please provide some guidance on what is being tested here and what is the best approach to fix those?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yuliacech Sorry for the delay. The except method matches the feature ID to set the expected value to false. The feature ID is actually 'guidedOnboardingFeature',

export const PLUGIN_FEATURE = 'guidedOnboardingFeature';

This explains why it was still being set to true and those 3 tests were failing. I think you'll want to move the 'everything_space_all at everything_space' case. If I understand the feature/privileges correctly, this case should enable the navlink.

@yuliacech yuliacech marked this pull request as ready for review April 20, 2023 17:57
@yuliacech yuliacech requested review from a team as code owners April 20, 2023 17:57
@elasticmachine
Copy link
Contributor

Pinging @elastic/platform-onboarding (Team:Journey/Onboarding)

Copy link
Contributor

@thomheymann thomheymann left a comment

Choose a reason for hiding this comment

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

Security changes LGTM!

Copy link
Contributor

@alisonelizabeth alisonelizabeth 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 working on this @yuliacech! Changes LGTM. Verified locally. I see some tests are failing, but it looks like you're already aware and looking into it.

I have one question about Kibana features. This is probably more for Kibana Security team, but - do we document the features anywhere? I'm just wondering if users are going to know what "Setup guides" is when it appears in the list.

Screen Shot 2023-04-21 at 11 59 08 AM

@yuliacech
Copy link
Contributor Author

Thanks a lot for your review, @alisonelizabeth!
That is great questions about the documentation for features, I haven't thought about that. @elastic/kibana-security What is the recommended way of documenting a new feature when we register it?

Copy link
Member

@lukeelmers lukeelmers 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 jumping on this so quickly @yuliacech!

I'm just wondering if users are going to know what "Setup guides" is when it appears in the list.

Probably something I should know the answer to, but @elastic/kibana-security is there a way to hide this from the privileges UI altogether? I don't think there's a reason someone would ever want/need to remove these permissions?

@@ -10,3 +10,5 @@ export const PLUGIN_ID = 'guidedOnboarding';
export const PLUGIN_NAME = 'guidedOnboarding';

export const API_BASE_PATH = '/api/guided_onboarding';
Copy link
Member

Choose a reason for hiding this comment

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

This API really should be /internal/guided_onboarding. I can't think of any reason why a user would need to use this API directly, and by using the /api path prefix, we are indicating it is public, meaning you can't make any breaking changes to it without approvals, a long deprecation period, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree @lukeelmers, I opened this issue to change the route's prefix. Our team was not aware of this convention when setting up the plugin.
What do you think, do we have to handle the prefix issue as a breaking change or can we update it in the next minor? The plugin was first released in 8.7

Copy link
Member

Choose a reason for hiding this comment

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

Since it was introduced recently & is undocumented (and also something folks would be unlikely to discover and get much utility out of), then IMO if you fix it quickly, you are probably safe to change it, treat this as a bug, and document it in the release notes.

@legrego
Copy link
Member

legrego commented Apr 24, 2023

Probably something I should know the answer to, but @elastic/kibana-security is there a way to hide this from the privileges UI altogether? I don't think there's a reason someone would ever want/need to remove these permissions?

@lukeelmers Yes, there are two ways for us to do this, depending on the privileges required: we can either enable this for all users, or we can enable this for users with the kibana_admin or equivalent role. If your needs don't fit into these two categories, then the only way to grant access is to expose it on this screen.

I discussed these options with @yuliacech, and IIUC the product team decided that exposing it was the best approach for their needs. I don't want to put words into their mouths, so perhaps Yulia can confirm this.

@legrego
Copy link
Member

legrego commented Apr 24, 2023

@elastic/kibana-security What is the recommended way of documenting a new feature when we register it?

@yuliacech we don't document these toggles in a central location. There is a public API that can be called to get the list of available features, but it doesn't offer any additional information than what you see in the UI: https://www.elastic.co/guide/en/kibana/master/features-api-get.html

@yuliacech
Copy link
Contributor Author

@lukeelmers @legrego Yes, I can confirm that we want to show the guided onboarding feature in the capabilities UI. I think it's important for deployment admins to be able to configure this feature for different roles.

Copy link
Contributor

@jeramysoucy jeramysoucy left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I found the issue.

@@ -57,7 +57,8 @@ export default function navLinksTests({ getService }: FtrProviderContext) {
'enterpriseSearchContent',
'enterpriseSearchAnalytics',
'appSearch',
'workplaceSearch'
'workplaceSearch',
'guidedOnboarding'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'guidedOnboarding'
'guidedOnboardingFeature'

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternative - change the ID to 'guidedOnboarding'. This may be more consistent with other feature ID's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot for this suggestions, @jeramysoucy! I think that should fix the tests.

@@ -57,7 +57,8 @@ export default function navLinksTests({ getService }: FtrProviderContext) {
'enterpriseSearchContent',
'enterpriseSearchAnalytics',
'appSearch',
'workplaceSearch'
'workplaceSearch',
'guidedOnboarding'
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @yuliacech Sorry for the delay. The except method matches the feature ID to set the expected value to false. The feature ID is actually 'guidedOnboardingFeature',

export const PLUGIN_FEATURE = 'guidedOnboardingFeature';

This explains why it was still being set to true and those 3 tests were failing. I think you'll want to move the 'everything_space_all at everything_space' case. If I understand the feature/privileges correctly, this case should enable the navlink.

Copy link
Contributor

@rshen91 rshen91 left a comment

Choose a reason for hiding this comment

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

Changes LGTM - tested locally

@yuliacech yuliacech enabled auto-merge (squash) April 25, 2023 08:45
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

LGTM

@yuliacech yuliacech merged commit b75546f into elastic:main Apr 26, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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
guidedOnboarding 56 57 +1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
home 159.6KB 159.7KB +128.0B

Page load bundle

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

id before after diff
cloudLinks 3.0KB 3.1KB +93.0B
guidedOnboarding 27.2KB 27.2KB +50.0B
total +143.0B
Unknown metric groups

API count

id before after diff
guidedOnboarding 57 58 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 17 19 +2
securitySolution 399 402 +3
total +5

References to deprecated APIs

id before after diff
guidedOnboarding 0 3 +3

Total ESLint disabled count

id before after diff
enterpriseSearch 18 20 +2
securitySolution 479 482 +3
total +5

History

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

@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 26, 2023
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 26, 2023
* main: (1294 commits)
  [SecuritySolution] Refactor security packages (elastic#155365)
  [Discover] Show "Temporary" badge for ad-hoc data views in Alerts flyout (elastic#155717)
  [RAM] Conditional actions feedback on pr review (elastic#155804)
  [Files] Adds bulk delete method (elastic#155628)
  [Lens] Use proper way to generate absolute short URL (elastic#155512)
  [Guided onboarding] Use Kibana features to grant access (elastic#155065)
  [Index Management] Fix duped mock (elastic#155844)
  [Lens] Enhance visualization modifier popup with layer palette (elastic#155280)
  Fix flaky combobox tests on role management screen (elastic#155711)
  [Infrastructure UI] Create InventoryViewsService and InventoryViewsClient (elastic#155126)
  [Fleet] always create agent upload write indices (elastic#155729)
  [Fleet] [Cloud Security Posture] Add CloudFormation agent install method (elastic#155045)
  Add tech preview label for search applications (elastic#155649)
  [ML] AIOps: Stabilize flaky functional tests. (elastic#155710)
  [ES UI Shared] Migrate JsonEditor to monaco (elastic#155610)
  [Security Solution] Fixes security_solution storybooks always rendering in a flyout (elastic#155814)
  [Synthetics] Make error popover disappear `onMouseLeave` of metric item card (elastic#155800)
  Remove Exploratory View components from Observability (elastic#155629)
  [Discover] Remove redundant "Filter was added" toast (elastic#155645)
  [RAM][Security Solution][Alerts] Support the ability to trigger a rule action per alert generated (elastic#153611) (elastic#155384)
  ...
yuliacech added a commit that referenced this pull request May 3, 2023
## Summary

This PR fixes a bug introduced in
#155065 that I noticed when
testing guided onboarding for v8.9.0: the help link was missing from the
help menu. I added a functional test for the help link but we also need
a test that the link is hidden when the user doesn't have access to
guided onboarding (opened an issue for that
[here](#156410)).

### How to test
1. Add xpack.cloud.id: 'testID' to /config/kibana.dev.yml
2. Start ES with yarn es snapshot and Kibana with `yarn start``
3. Check that the help link "Setup guides" is displayed in the help menu

### Screenshot 
<img width="336" alt="Screenshot 2023-05-02 at 17 27 20"
src="https://user-images.githubusercontent.com/6585477/235712599-812b422d-3092-45b3-a726-37f6b90e81c1.png">
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 3, 2023
## Summary

This PR fixes a bug introduced in
elastic#155065 that I noticed when
testing guided onboarding for v8.9.0: the help link was missing from the
help menu. I added a functional test for the help link but we also need
a test that the link is hidden when the user doesn't have access to
guided onboarding (opened an issue for that
[here](elastic#156410)).

### How to test
1. Add xpack.cloud.id: 'testID' to /config/kibana.dev.yml
2. Start ES with yarn es snapshot and Kibana with `yarn start``
3. Check that the help link "Setup guides" is displayed in the help menu

### Screenshot
<img width="336" alt="Screenshot 2023-05-02 at 17 27 20"
src="https://user-images.githubusercontent.com/6585477/235712599-812b422d-3092-45b3-a726-37f6b90e81c1.png">

(cherry picked from commit d00e2a3)
kibanamachine added a commit that referenced this pull request May 3, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [[Guided onboarding] Fix the missing help link
(#156399)](#156399)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Yulia
Čech","email":"6585477+yuliacech@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-05-03T14:00:09Z","message":"[Guided
onboarding] Fix the missing help link (#156399)\n\n##
Summary\r\n\r\nThis PR fixes a bug introduced
in\r\nhttps://github.com//pull/155065/ that I noticed
when\r\ntesting guided onboarding for v8.9.0: the help link was missing
from the\r\nhelp menu. I added a functional test for the help link but
we also need\r\na test that the link is hidden when the user doesn't
have access to\r\nguided onboarding (opened an issue for
that\r\n[here](https://github.com/elastic/kibana/issues/156410)).\r\n\r\n###
How to test\r\n1. Add xpack.cloud.id: 'testID' to
/config/kibana.dev.yml\r\n2. Start ES with yarn es snapshot and Kibana
with `yarn start``\r\n3. Check that the help link \"Setup guides\" is
displayed in the help menu\r\n\r\n### Screenshot \r\n<img width=\"336\"
alt=\"Screenshot 2023-05-02 at 17 27
20\"\r\nsrc=\"https://user-images.githubusercontent.com/6585477/235712599-812b422d-3092-45b3-a726-37f6b90e81c1.png\">","sha":"d00e2a366f872d80b5cd598765f8a5aee50cc399","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Journey/Onboarding","v8.8.0","v8.9.0"],"number":156399,"url":"https://github.com/elastic/kibana/pull/156399","mergeCommit":{"message":"[Guided
onboarding] Fix the missing help link (#156399)\n\n##
Summary\r\n\r\nThis PR fixes a bug introduced
in\r\nhttps://github.com//pull/155065/ that I noticed
when\r\ntesting guided onboarding for v8.9.0: the help link was missing
from the\r\nhelp menu. I added a functional test for the help link but
we also need\r\na test that the link is hidden when the user doesn't
have access to\r\nguided onboarding (opened an issue for
that\r\n[here](https://github.com/elastic/kibana/issues/156410)).\r\n\r\n###
How to test\r\n1. Add xpack.cloud.id: 'testID' to
/config/kibana.dev.yml\r\n2. Start ES with yarn es snapshot and Kibana
with `yarn start``\r\n3. Check that the help link \"Setup guides\" is
displayed in the help menu\r\n\r\n### Screenshot \r\n<img width=\"336\"
alt=\"Screenshot 2023-05-02 at 17 27
20\"\r\nsrc=\"https://user-images.githubusercontent.com/6585477/235712599-812b422d-3092-45b3-a726-37f6b90e81c1.png\">","sha":"d00e2a366f872d80b5cd598765f8a5aee50cc399"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/156399","number":156399,"mergeCommit":{"message":"[Guided
onboarding] Fix the missing help link (#156399)\n\n##
Summary\r\n\r\nThis PR fixes a bug introduced
in\r\nhttps://github.com//pull/155065/ that I noticed
when\r\ntesting guided onboarding for v8.9.0: the help link was missing
from the\r\nhelp menu. I added a functional test for the help link but
we also need\r\na test that the link is hidden when the user doesn't
have access to\r\nguided onboarding (opened an issue for
that\r\n[here](https://github.com/elastic/kibana/issues/156410)).\r\n\r\n###
How to test\r\n1. Add xpack.cloud.id: 'testID' to
/config/kibana.dev.yml\r\n2. Start ES with yarn es snapshot and Kibana
with `yarn start``\r\n3. Check that the help link \"Setup guides\" is
displayed in the help menu\r\n\r\n### Screenshot \r\n<img width=\"336\"
alt=\"Screenshot 2023-05-02 at 17 27
20\"\r\nsrc=\"https://user-images.githubusercontent.com/6585477/235712599-812b422d-3092-45b3-a726-37f6b90e81c1.png\">","sha":"d00e2a366f872d80b5cd598765f8a5aee50cc399"}}]}]
BACKPORT-->

Co-authored-by: Yulia Čech <6585477+yuliacech@users.noreply.github.com>
@yuliacech yuliacech deleted the guided_onboarding/8.8/roles branch February 15, 2024 12:04
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 release_note:skip Skip the PR/issue when compiling release notes Team:Journey/Onboarding Platform Journey Onboarding team v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Guided onboarding] Handle missing permissions