[SR] Adjust app permissions by jen-huang · Pull Request #39966 · elastic/kibana

jen-huang · 2019-06-28T23:07:41Z

Summary

This PR adjusts the app permissions for Snapshot and Restore to reflect additional features done in #39193.

Overall app permissions now need these cluster privileges because the UI now supports additional actions with snapshots (as opposed to readonly view):

[
  'cluster:admin/snapshot',
  'cluster:admin/repository',
]

An index privilege is now also checked, monitor. Not having this privilege on at least one index will not block the user's access to the app, but will block the Restore Status content from being shown. Prior to this PR, the UX was not very nice:

Testing

This PR can be tested by creating a new user role (and a user who uses that role plus kibana_user) and adjusting its privileges to see the various UX changes:

Missing cluster privilege, no app access

POST _security/role/sr_user
{
  "cluster": ["cluster:admin/repository"]
}

Missing index privilege, app access but no Recovery Status access

POST _security/role/sr_user
{
  "cluster": ["cluster:admin/repository", "cluster:admin/snapshot"]
}

Some indices privilege, app access and Recovery Status access for just those indices

POST _security/role/sr_user
{
  "cluster": ["cluster:admin/repository", "cluster:admin/snapshot"],
  "index" : [
    {
      "names": ["logstash-1", "logstash-2"],
      "privileges": ["monitor"]
    }
  ]
}

All indices privilege (or superuser), app access and full Recovery Status access

POST _security/role/sr_user
{
  "cluster": ["cluster:admin/repository", "cluster:admin/snapshot"],
  "index" : [
    {
      "names": ["*"],
      "privileges": ["monitor"]
    }
  ]
}

elasticmachine · 2019-06-28T23:07:43Z

Pinging @elastic/es-ui

jen-huang · 2019-06-28T23:14:02Z

x-pack/legacy/plugins/snapshot_restore/public/app/app.tsx

+  // Update app state with permissions data
+  useEffect(
+    () => {
+      dispatch({


@elastic/es-ui
I think permissions is a good candidate for the type of data that should be stored in React context. Here, the permissions are fetched when the app mounts and I dispatch the resulting data into the app state so that other components can access it without having to send another request to get permissions info. This can let us get really granular with the way the UI behaves at different permission levels. I've only used it in one section of this app for now, but it would be relatively easy to check for more granular permissions and disable buttons such as Delete and Restore.
What do you think?

Agreed @jen-huang. And should be a reusable solution across our apps. Once the Provider wraps the app, I see something like this in our components

import { Authorize } from 'plugins/our_shared_es_ui_plugin/permissions'; const SomeComponent = () => ( <Authorize role="someRole"> <EuiButton>Only visible to users with enough privileges</EuiButton> </Authorize> )

I think permissions is a good candidate for the type of data that should be stored in React context.

Yes, I agree. I think a compelling rationale is also that this lets us design the app so that users are free to navigate throughout it while the permissions request is still resolving. As you pointed out, discrete parts of the app will be gated on permissions. These specific parts of the UI can show a spinner while the user uses the parts of the app that are not gated on permissions.

EDIT: I re-read your comment and it sounds like maybe we're saying the same thing in different ways. :)

elasticmachine · 2019-06-29T00:04:40Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-06-29T01:17:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-01T17:59:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jen-huang · 2019-07-01T18:10:26Z

Retest

elasticmachine · 2019-07-01T19:10:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

Tested locally, code LGTM! I found a few typos and had a suggestion for making some code easier to follow.

cjcenizal · 2019-07-01T18:34:46Z

x-pack/legacy/plugins/snapshot_restore/common/constants.ts

 };

-export const APP_PERMISSIONS = ['create_snapshot', 'cluster:admin/repository'];
+export const APP_REQUIRED_CLUSTER_PRIVILAGES = [


Typo nit in a few places: PRIVILAGES -> PRIVILEGES

cjcenizal · 2019-07-01T18:43:02Z

x-pack/legacy/plugins/snapshot_restore/public/app/app.tsx

+  // Update app state with permissions data
+  useEffect(
+    () => {
+      dispatch({


I think permissions is a good candidate for the type of data that should be stored in React context.

Yes, I agree. I think a compelling rationale is also that this lets us design the app so that users are free to navigate throughout it while the permissions request is still resolving. As you pointed out, discrete parts of the app will be gated on permissions. These specific parts of the UI can show a spinner while the user uses the parts of the app that are not gated on permissions.

EDIT: I re-read your comment and it sounds like maybe we're saying the same thing in different ways. :)

cjcenizal · 2019-07-01T18:45:59Z

x-pack/legacy/plugins/snapshot_restore/server/routes/api/app.ts

+    method: 'GET',
+  });
+
+  // Check if they have at the required index privilages for at least one index


privilages -> privileges

As a side note, the more I read this word, this more it looks wrong to me!

privileges doesn't feel like a real word to me any more 😆

cjcenizal · 2019-07-01T19:37:58Z

x-pack/legacy/plugins/snapshot_restore/server/routes/api/app.ts

+    method: 'GET',
+  });
+
+  // Check if they have at the required index privilages for at least one index


typo: at -> all?

cjcenizal · 2019-07-01T19:40:08Z

x-pack/legacy/plugins/snapshot_restore/server/routes/api/app.ts

+  const oneIndexWithAllPrivileges = indices.find(({ privileges }: { privileges: string[] }) => {
+    return (
+      privileges.includes('all') ||
+      APP_RESTORE_INDEX_PRIVILAGES.filter(privilage => !privileges.includes(privilage)).length === 0


I had to read through this a few times to figure out the logic. Could we extract this condition and add a variable to communicate meaning? Here's my suggestion, it's a couple more lines but I find it easier to understand. (Warning, I haven't tested it!)

// Check if they have all the required index privilages for at least one index const hasIndexWithRestorePrivileges = indices.find(({ privileges }: { privileges: string[] }) => { if (privileges.includes('all')) { return true; } const indexHasRestorePrivileges = APP_RESTORE_INDEX_PRIVILEGES.every(restorePrivilege => privileges.includes(restorePrivilege)); return indexHasRestorePrivileges; });

I know we could eliminate a line by getting rid of the indexHasRestorePrivileges variable, but its name makes it so much easier for me to follow the logic. :)

elasticmachine · 2019-07-01T21:39:03Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Adjust SR app permissions for new features * Fix app state and reducer typings * Fix i18n key * Fix typos * Make logic more clear

jen-huang added 2 commits June 28, 2019 15:20

Adjust SR app permissions for new features

1e84d8d

Fix app state and reducer typings

e20aefb

jen-huang added Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:Snapshot and Restore Elasticsearch snapshots and repositories UI v7.3.0 v7.4.0 labels Jun 28, 2019

jen-huang requested review from alisonelizabeth, cjcenizal and sebelga June 28, 2019 23:07

jen-huang added v8.0.0 and removed v7.4.0 labels Jun 28, 2019

jen-huang commented Jun 28, 2019

View reviewed changes

Fix i18n key

e02f7c2

Merge remote-tracking branch 'upstream/master' into fix/sr-permissions

2125710

cjcenizal approved these changes Jul 1, 2019

View reviewed changes

jen-huang added 2 commits July 1, 2019 12:44

Fix typos

cb0efab

Make logic more clear

4707f5b

jen-huang merged commit f60b536 into elastic:master Jul 1, 2019

jen-huang deleted the fix/sr-permissions branch July 1, 2019 21:41

jen-huang added a commit to jen-huang/kibana that referenced this pull request Jul 1, 2019

[SR] Adjust app permissions (elastic#39966)

9e26bd2

* Adjust SR app permissions for new features * Fix app state and reducer typings * Fix i18n key * Fix typos * Make logic more clear

jen-huang mentioned this pull request Jul 1, 2019

[7.x] [SR] Adjust app permissions (#39966) #40100

Merged

jen-huang added a commit that referenced this pull request Jul 1, 2019

[SR] Adjust app permissions (#39966) (#40100)

5e4d6af

* Adjust SR app permissions for new features * Fix app state and reducer typings * Fix i18n key * Fix typos * Make logic more clear

Conversation

jen-huang commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Missing cluster privilege, no app access

Missing index privilege, app access but no Recovery Status access

Some indices privilege, app access and Recovery Status access for just those indices

All indices privilege (or superuser), app access and full Recovery Status access

Uh oh!

elasticmachine commented Jun 28, 2019

Uh oh!

jen-huang Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jun 29, 2019

💔 Build Failed

Uh oh!

elasticmachine commented Jun 29, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Jul 1, 2019

💔 Build Failed

Uh oh!

jen-huang commented Jul 1, 2019

Uh oh!

elasticmachine commented Jul 1, 2019

💚 Build Succeeded

Uh oh!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 1, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jen-huang commented Jun 28, 2019 •

edited

Loading

jen-huang Jun 28, 2019 •

edited

Loading