[ES-UI] Authorization lib to control access to feature & app sections #40150

sebelga · 2019-07-02T14:17:33Z

I had a few comments on Jen's PR https://github.com/elastic/kibana/pull/39966/files/e20aefbac90dbc4048630f52694d73dc6108e734

So as it is merged I decided to create this proposal. 😊
My main point was that I thought it overkills to add an app state with a reducer just to make the permission available through context. Aside, a state implies IMO that it will change, and here we load the permissions when the app loads and never change them afterward.

This PR bring a proposal of a small (reusable) "lib" (never know how to call those) that, if we agree on the architecture, will have to find its place in our future "x-pack/plugins/elasticsearch-ui-shared" plugin (in that plugin we will also put my current work for the forms, our useRequest() hook, and all our reusable static code.

@cjcenizal @jen-huang @alisonelizabeth looking forward to hearing your thoughts on this, cheers!

elasticmachine · 2019-07-02T14:18:02Z

Pinging @elastic/es-ui

sebelga · 2019-07-02T14:21:43Z

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

-      </HashRouter>
-    </I18nContext>
+    <AuthorizationProvider
+      permissionEndpoint={httpService.addBasePath(`${API_BASE_PATH}permissions`)}


Maybe, instead of having each app creating their handler for the permissions, we could have a single handler in our shared plugin, that would expose something like:

GET /<our-shared-plugin_name>/authorization/permissions?app=snapshot_restore

We could then remove the need of this permissionEndpoint prop here.

a single handler may get complex since I think the needs of each app will differ from each other. I like this approach for now

I don't think it would be complex. 75% of the handler code is generic and very little specific to the app. We would simply need to create 1 extension file / app for what is specific.
The benefit would be that we could then simply inject the AuthroizatioProvider anywhere, add an extension to the shared plugin "/privileges" endpoint handler and be done.

We can do that as a second pass 😊

elasticmachine · 2019-07-02T15:32:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga · 2019-07-02T16:43:53Z

retest

elasticmachine · 2019-07-02T17:10:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga · 2019-07-03T07:18:27Z

retest

elasticmachine · 2019-07-03T07:27:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

sebelga · 2019-07-03T14:27:44Z

retest

elasticmachine · 2019-07-03T15:28:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jen-huang

Overall I like this proposal, and I think it makes a lot of sense to build something re-usable. I have left some thoughts and comments that I think should be looked at before giving my final stamp 🙂 Thanks for doing this!

jen-huang · 2019-07-10T03:12:45Z

x-pack/legacy/plugins/snapshot_restore/public/app/sections/home/restore_list/restore_list.tsx

+            <section data-test-subj="restoreList">{content}</section>
+          ) : (
+            <NotAuthorizedSection
+              sectionName="snapshot restore status"


this needs to be localized

my comments in NotAuthorizedSection invalidates the above

jen-huang · 2019-07-10T03:12:58Z

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

+          ) : (
+            <EuiPageContent>
+              <NotAuthorizedSection
+                sectionName="Snapshot and Restore"


this needs to be localized

my comments in NotAuthorizedSection invalidates the above

jen-huang · 2019-07-10T03:14:53Z

.../legacy/plugins/snapshot_restore/public/app/lib/authorization/components/with_privileges.tsx

+  return false;
+};
+
+export const WithPrivileges = ({ privileges, children }: Props) => {


could we consider renaming this to something like PrivilegesProvider or something else that makes it clear that the usage is a HOC?

I prefer to keep the "Provider" term for our context Provider and not mix both. As, indeed, it is not an HOC, I renamed it <AuthPrivileges></AuthPrivileges>. Let me know what you think

jen-huang · 2019-07-10T03:19:02Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/not_authorized_section.tsx

+          id="xpack.snapshotRestore.restoreList.deniedPermissionTitle"
+          defaultMessage="You're missing {privilegeType} privileges"
+          values={{
+            privilegeType,


this will run into localization problems since cluster/index won't be part of the translation. see my next comment

jen-huang · 2019-07-10T03:21:59Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/not_authorized_section.tsx

+      <p>
+        <FormattedMessage
+          id="xpack.snapshotRestore.restoreList.deniedPermissionDescription"
+          defaultMessage="To view {sectionName}, you must have {privilegesCount,


the messaging for missing index-level privileges is slightly different: relevant LOC

how about changing this component so that the title and body are part of Props? this would make it similar to our SectionError and SectionLoading components

Good point, I will update the component

jen-huang · 2019-07-10T03:24:45Z

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

-    </div>
+    <WithPrivileges privileges="cluster">
+      {({ isLoading, hasPrivileges, missingPrivileges }) => (
+        <Fragment>


I think this Fragment isn't needed

jen-huang · 2019-07-10T03:29:55Z

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

-      </HashRouter>
-    </I18nContext>
+    <AuthorizationProvider
+      permissionEndpoint={httpService.addBasePath(`${API_BASE_PATH}permissions`)}


a single handler may get complex since I think the needs of each app will differ from each other. I like this approach for now

jen-huang · 2019-07-10T03:34:42Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/authorization_provider.tsx

+  return error ? (
+    <SectionError
+      title={
+        <FormattedMessage


not loving having to pass FormattedMessage just for this error message. what do you think about returning error as part of value so that the consumer can handle the error UI instead? this will also let us be more flexible with the error message, for example displaying Error checking Snapshot and Restore permissions

Yeah I was not very happy with it either. I updated the code to let the consumer deal with the error.

Note: I think we need, in a separate PR, to create our own <CoreProvider> that would just provide the core object, so we can use translation in our reusable components. I already needed it in a few places. So we would have a CoreProvider + AppDependencies (other plugins)

jen-huang · 2019-07-10T03:37:46Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/authorization_provider.tsx

+}: Props) => {
+  const { error, data: permissionsData } = useLoadPermissions(permissionEndpoint);
+
+  const isLoaded = typeof permissionsData !== 'undefined';


why not use loading that is returned from useLoadPermissions?

Not sure why, I will use it 😊

jen-huang · 2019-07-10T03:53:19Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/authorization_provider.tsx

+
+  // Note: here we have to serialize the HTTP response
+  // If we move forward with proposal, we would need to update the SR server response
+  // to implement the interface below so we don't need the serialization.


the permissions contract needs to be worked through more carefully. for SR, access to the entire app requires all cluster privileges, and access to Restore status requires all index privileges. the implementation logic in this PR works great for that use case, but what about for more granular UI details?

for example, maybe we want to check if user has access to just read snapshots/repositories and allow them overall app access, but disallow delete/edit/restore through UI if they don't have more admin privileges. read and admin privileges are both cluster type privileges in this case, so the current implementation wouldn't allow for this type of granular checking.

btw this comment does not block this PR! it is just some thoughts, after all I was the one who structured this app's permission response this way 😄 I am working on SLM UI for 7.4, which will also live inside Snapshot and Restore. I'm sure part of that work will be adjusting the app permissions to account for this new section of the app, so I am happy to iterate and improve the current implementation when I get there

I updated the logic to support this use case and be able to be granular.

Now, if we require all the privileges we need to provide cluster.*
If we need only some privileges we can provide cluster.someSpecifiPrivilege

cjcenizal

Great work @sebelga! I agree with Jen's suggestions about changing NotAuthorizedSection to accept title and body props, and to change the AuthorizationProvider to delegate handling of the error state to the consumer.

In addition to this, can we change "authorization" and "privileges" to "permissions" in the code and file names so that this concern is identified by one term instead of three?

cjcenizal · 2019-07-09T18:58:29Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/authorization_provider.tsx

+      title={
+        <FormattedMessage
+          id="xpack.snapshotRestore.app.checkingPermissionsErrorMessage"
+          defaultMessage="Error checking permissions"


Can we standardize on language, e.g. use "permissions" or "authorization" everywhere? For example, we could rename this file permissions_provider or change this message to "Error checking authorization."

I agree that 3 terms are too many 😊 But one thing is the Authorization service (which is the feature we are bringing here) and then what this feature provides. The Authorization provider can return different types of data: user roles, user privileges, user access, user permissions, ...

Jen seems to have used "privilege" and "permission" interchangeably. The API path is "/permissions" but privileges are returned 😊 I will stick with "permissions" and use that word everywhere.

[EDIT]: But then... Elasticsearch uses the word "privilege". :/ I'll use "privileges" to simplify things.

From our docs, all words are used: Authorization, roles, permissions and privileges 😊

cjcenizal · 2019-07-09T19:00:15Z

.../legacy/plugins/snapshot_restore/public/app/lib/authorization/components/with_privileges.tsx

+  return false;
+};
+
+export const WithPrivileges = ({ privileges, children }: Props) => {


Same with this file. Instead of "privileges" can we use "permissions" or "authorization"? By keeping the terms consistent we can reduce mental overhead for people figuring out what the code is doing.

I decided to align all the term to Elasticsearch API "privilege" naming. The provider is still the Authorization as explained above.

…ssion-state

sebelga · 2019-07-16T13:12:34Z

Thanks for the review @jen-huang and @cjcenizal I addressed all the feedback and made some comments. Could you have another look? Cheers!

@jen-huang I also updated the Server handler response to match the contract of the AuthorizationProvider.

elasticmachine · 2019-07-16T14:03:32Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-16T14:32:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…ssion-state

elasticmachine · 2019-07-17T07:08:46Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-17T08:01:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pugnascotia · 2019-07-22T09:49:12Z

Just for interest, I thought I'd mention what Cloud has done for permissions. Our permissions map 1-to-1 with Swagger operation IDs, so for the UI we generate a TypeScript enum of all the possible permissions. Then, we get type completion when checking the user's permissions, as well as typo prevention.

sebelga · 2019-07-22T13:28:58Z

Thanks for sharing @pugnascotia Although we don't have yet a centralized service for privileges. I'll keep it in mind if we decide to do so. cheers!

jen-huang

Thanks for making the changes Seb, LGTM!

I also tested locally to make sure there were no regressions with accessing the app with a fully privileged user and users of varying privileges.

Looking forward to trying this out in my new work for SR!

…ssion-state

sebelga · 2019-07-23T11:34:37Z

Cool, thanks for the review @jen-huang !

elasticmachine · 2019-07-23T12:27:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sebelga · 2019-07-23T12:54:23Z

@cjcenizal Do you still have concerns about naming? cheers

cjcenizal · 2019-07-23T15:54:49Z

@sebelga I can re-review this today, thanks for the ping!

cjcenizal

I did a grep for "permission" and found many references still in the code. Did you want to replace these with "privilege"? In a few places both terms refer to the same thing, which is a little confusing. Since this doesn't affect functionality I'm approving the PR to unblock it but I think it would be great if we could align on a single term.

cjcenizal · 2019-07-24T00:37:37Z

.../legacy/plugins/snapshot_restore/public/app/lib/authorization/components/auth_privileges.tsx

+const toArray = (value: string | string[]): string[] =>
+  Array.isArray(value) ? (value as string[]) : ([value] as string[]);
+
+export const AuthPrivileges = ({ requiredPrivileges, children }: Props) => {


Nit: AuthorizationPrivileges or simply Authorization would be more consistent with our other names, and removes any ambiguity around what auth stands for (authorization or authentication).

Ok, I will use AuthorizationPrivileges.

"Authorization" and "Privileges" are 2 different things 😊 The "Authorization" is the mechanism in place that will handle a user "privileges". It could, for example, also manage "roles" and would still be called "Authorization".
Here, the component is handling "privileges" so I'll change it to AuthorizationPrivileges.

Edit: I went back to my original <WithPrivileges> as it is what best describes this component. I just saw this convention used by the app search team.

<WithPrivileges privileges="index.*"> {/* anything here requires the above privileges */} </WithPrivileges>

cc @jen-huang

cjcenizal · 2019-07-24T00:38:15Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/authorization_provider.tsx

+ */
+
+import React, { createContext } from 'react';
+import { useLoadPermissions } from '../services';


Nit: Would useLoadPrivileges be more consistent?

cjcenizal · 2019-07-24T00:38:33Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/authorization_provider.tsx

+export const AuthorizationContext = createContext<Authorization>(initialValue);
+
+interface Props {
+  permissionEndpoint: string;


Nit: privilegesEndpoint?

cjcenizal · 2019-07-24T00:38:52Z

.../plugins/snapshot_restore/public/app/lib/authorization/components/authorization_provider.tsx

+}
+
+export const AuthorizationProvider = ({ permissionEndpoint, children }: Props) => {
+  const { loading, error, data: permissionsData } = useLoadPermissions(permissionEndpoint);


Nit: privilegesData?

cjcenizal · 2019-07-24T00:39:25Z

x-pack/legacy/plugins/snapshot_restore/common/types/app.ts

 * you may not use this file except in compliance with the Elastic License.
 */

 export interface AppPermissions {


Nit: should this be AppPrivileges or perhaps AppAuthorization? I learn towards the latter if that's overarching concept we're dealing with, of which "privileges" are a part.

Good point, I'll change it to AppAuthorization.

sebelga · 2019-07-29T13:43:02Z

Thanks for the review @cjcenizal I updated all references to use the word "privilege" instead of "permission".

…ssion-state # Conflicts: # x-pack/legacy/plugins/snapshot_restore/public/app/app.tsx

cjcenizal · 2019-07-29T15:16:03Z

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

-  permissionsResult.missingClusterPrivileges = extractMissingPrivileges(cluster || {});
-  permissionsResult.hasPermission = hasPermission;
+  privilegesResult.missingPrivileges.cluster = extractMissingPrivileges(cluster);
+  privilegesResult.hasAllPrivileges = hasPermission;


Did you want to change this to privileges too? I did a ctrl-f for "permission" on the diff of this PR and there are also quite a few references to privileges in the localization keys.

Not sure I follow here. Yes I did want to change "permission" to "privileges" here. Isn't that what you also wanted?

Ah you mean the "hasPermission" name? 😊

Ah you mean the "hasPermission" name

Yes, you can do a ctrl-f and you'll see all of the instances I have in mind as candidates for renaming.

++ I just updated everything that had the word "permission".

elasticmachine · 2019-07-29T15:58:51Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-07-29T16:39:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…elastic#40150)

…#40150) (#42168)

Authorization lib to control access to feature & app sections

f5aaedf

sebelga added non-issue Indicates to automation that a pull request should not appear in the release notes Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// labels Jul 2, 2019

sebelga commented Jul 2, 2019

View reviewed changes

sebelga added 2 commits July 2, 2019 16:24

Remove dev state

cd98f9b

Remove useLoadPermissions not needed anymore

5d1aca3

Small refactor

1544735

jen-huang added release_note:skip Skip the PR/issue when compiling release notes v7.4.0 v8.0.0 labels Jul 10, 2019

jen-huang reviewed Jul 10, 2019

View reviewed changes

cjcenizal suggested changes Jul 10, 2019

View reviewed changes

sebelga added 4 commits July 16, 2019 10:37

Merge remote-tracking branch 'upstream/master' into proposal-sr-permi…

9922cd2

…ssion-state

Move apiError handling to consumer

cb6a7f3

Allow to be more granular to define specific privilege for each section

2ff6413

Refactor: rename WithPrivileges --> AuthPrivileges

2359e5f

Update API endpoint payload for privileges

b2326d7

sebelga added 2 commits July 17, 2019 08:12

Merge remote-tracking branch 'upstream/master' into proposal-sr-permi…

72a5c0a

…ssion-state

Refactor var naming

87348ea

jen-huang approved these changes Jul 23, 2019

View reviewed changes

Merge remote-tracking branch 'upstream/master' into proposal-sr-permi…

b46b754

…ssion-state

cjcenizal approved these changes Jul 24, 2019

View reviewed changes

Refactor: remove reference to "permission" and use "privilege" instead

3822538

sebelga added 6 commits July 29, 2019 15:50

Revert to "WithPrivileges" instead of "AuthPrivileges"

6402b9c

Remove unnecessary AppState type

fb7edc9

Use "Privileges" interface from lib instead of app defined.

8b6ce92

Merge remote-tracking branch 'upstream/master' into proposal-sr-permi…

55b44fa

…ssion-state # Conflicts: # x-pack/legacy/plugins/snapshot_restore/public/app/app.tsx

Put back "policies" section

da2dcc3

Refactor: use the name "privileges" instead of "permission"

0869e48

cjcenizal reviewed Jul 29, 2019

View reviewed changes

Refactor: use the name "privileges" instead of "permission"

53b20e6

sebelga merged commit a45f28f into elastic:master Jul 29, 2019

sebelga deleted the proposal-sr-permission-state branch July 29, 2019 17:06

sebelga mentioned this pull request Jul 29, 2019

[7.x] [ES-UI] Authorization lib to control access to feature & app sections (#40150) #42168

Merged

sebelga added a commit to sebelga/kibana that referenced this pull request Jul 29, 2019

[ES-UI] Authorization lib to control access to feature & app sections (…

d62eccb

…elastic#40150)

cjcenizal pushed a commit that referenced this pull request Jul 29, 2019

[ES-UI] Authorization lib to control access to feature & app sections (…

738e1fb

…#40150) (#42168)

alisonelizabeth mentioned this pull request Aug 23, 2019

[IM] Handle user permissions appropriately #43894

Open

alisonelizabeth mentioned this pull request Dec 15, 2020

Refining UX around user privileges #86016

Open

[ES-UI] Authorization lib to control access to feature & app sections #40150

[ES-UI] Authorization lib to control access to feature & app sections #40150

Uh oh!

Conversation

sebelga commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jul 2, 2019

Uh oh!

sebelga Jul 2, 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 Jul 2, 2019

💔 Build Failed

Uh oh!

sebelga commented Jul 2, 2019

Uh oh!

elasticmachine commented Jul 2, 2019

💔 Build Failed

Uh oh!

sebelga commented Jul 3, 2019

Uh oh!

elasticmachine commented Jul 3, 2019

💔 Build Failed

Uh oh!

sebelga commented Jul 3, 2019

Uh oh!

elasticmachine commented Jul 3, 2019

💚 Build Succeeded

Uh oh!

jen-huang 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!

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!

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!

cjcenizal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebelga Jul 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

sebelga commented Jul 2, 2019 •

edited

Loading

sebelga Jul 2, 2019 •

edited

Loading

sebelga Jul 16, 2019 •

edited

Loading

sebelga Jul 16, 2019 •

edited

Loading

sebelga commented Jul 16, 2019 •

edited

Loading

sebelga commented Jul 23, 2019 •

edited

Loading