Read Only Mode in Canvas #23304

cqliu1 · 2018-09-19T03:25:20Z

Closes #22979.
Closes #23126.

This implements a read only mode in Canvas for users with only read permissions.

Changes

adds canUserWrite to transient state
sets canUserWrite to false when user doesn't have write permissions
adds isWriteable to workpad object
shows bottom toolbar when editing is disabled
hides page controls and page add button in page manager when editing is disabled
disables drag and drop for reordering pages when editing is disabled
hides the edit mode toggle if user does not have write permissions

Update 9/26/2018

renames/moves editing in transient state to readOnly to persistent workpad state. Now, you can export read only workpads and they will upload with edit mode disabled. Once uploaded and if the user has write permissions, the user can turn on edit mode.
omits workpad create/upload/clone/delete controls in workpad loader for users with read only permissions

Update 10/4/18

switches the eye icon to a lock icon for turning edit mode on/off. Thanks @ryankeairns for the new icon!

cqliu1 · 2018-09-19T03:36:55Z

x-pack/plugins/canvas/public/state/middleware/es_persist.js

@@ -43,6 +44,8 @@ export const esPersistMiddleware = ({ getState }) => {
    next(action);
    const newState = getState();

+    if (getReadOnly(newState)) return;


@rashidkpc I tried moving the current page from being stored as part of the workpad to transient state, but there were conflicts with the aeroelastic store where it relies the selected page to stay in the workpad state. I talked to @w33ble about the issues I was encountering, and we decided moving the selected page to transient state may be easier to implement after refactoring our redux implementation.

This was the simpler fix for preventing the user from updating the current page in the saved workpad object in elasticsearch.

cqliu1 · 2018-09-19T03:43:51Z

@alexfrancoeur @rashidkpc With the changes in this PR, it only sets Canvas in read only mode when the user doesn't have write permissions. Should I also provide an option somewhere to export the workpad as read only and read it off of the workpad JSON on upload? Maybe add a lock button in the workpad header to set the workpad to read only.

elasticmachine · 2018-09-19T04:53:29Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alexfrancoeur · 2018-09-19T16:07:35Z

@cqliu1 I like the idea of exporting in read only. That would allow us to add demos to demo.elastic.co and then users could download them as well. A lock button would be nice too. I'll try and take this for a spin today.

elasticmachine · 2018-09-19T17:27:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

alexfrancoeur · 2018-09-21T12:48:59Z

Opening an existing workpad now works like a charm. I'm using the "old" way to create a read only role. We may want to check with @kobelb and @legrego to see if the read only roles is significantly different with 6.5

As a read only user, I can still "create a workpad", just with constant errors. It makes for an awkward experience because every time you move an element you get a flurry of errors. Maybe we should disable the button?

@asawariS @gingerwizard if we provide a read only mode in this fashion, end users will not be able to create and/or manipulate workpads on demo.elastic.co. I think this could be fine. We're showing the art of the possible with example workpads but if they want to get a feel for how to build them, they should spin up a trial. Because of the way Canvas saves progress, the only other way I can think of providing the "build" experience without saving is to simply hide or ignore the errors with a flag or something.

legrego · 2018-09-21T13:08:21Z

We may want to check with @kobelb and @legrego to see if the read only roles is significantly different with 6.5

6.5 likely won't help here, at least not in its current state. 6.6/Granular app privs will provide the necessary hooks for all applications to make decisions based on the current user's capabilities, so if you can wait for 6.6, that'd be preferred.

There is an outside chance that we could provide a flag to the UI in 6.5 indicating if the user is a readonly user, but it would be a temporary measure, as 6.6/Granular App Privileges will fundamentally change the way this works.

I'm saying "outside chance" because we'll want to wait for Spaces to merge into master before evaluating the feasibility. Then time permitting, we can open a PR to provide this to Canvas. This flag would be available within x-pack, so it'd work as long as you don't need it in OSS

alexfrancoeur · 2018-09-21T14:58:28Z

Thanks @legrego, just trying to understand if the read only role with RBAC phase 1/2 changes how we plan on handling it. If it's the same as earlier, I think we'll be fine. @cqliu1 seems to be detecting a read only state already.

kobelb · 2018-09-21T15:30:38Z

From my understanding, we're trying to update the workpad with the existing workpad and if we get a 403, then we assume we're in read-only mode per:

              workpadService.update(params.id, fetchedWorkpad).catch(err => {
                if (err.response.status === 403) {
                  dispatch(setReadOnly(true));
                  dispatch(setEditing(false));
                }
              });

It's an interesting approach, that could potentially cause someone's changes to be overwritten if there's a certain timing of events, but from the security perspective it should work with RBAC and the legacy fallback to determine whether the user can edit workpads.

cqliu1 · 2018-09-26T23:25:47Z

x-pack/plugins/canvas/public/components/workpad_loader/workpad_dropzone/index.js

 import { getId } from '../../../lib/get_id';
 import { notify } from '../../../lib/notify';
 import { WorkpadDropzone as Component } from './workpad_dropzone';

 export const WorkpadDropzone = compose(
-  withState('isDropping', 'setDropping', false),


isDropping is unused as far as I can tell, so I just removed it.

alexfrancoeur · 2018-10-15T16:07:14Z

@legrego is there no easy way to check a users permissions upon entering the app? Something like https://www.elastic.co/guide/en/kibana/6.x/role-management-api-get.html but for users? I'm assuming not given your previous statement.

I wonder if we can gain any insight here into how dashboard only mode was introduced? I'm guessing that the app itself checks for the dashboard_only_role or any others from the advanced setting list. Which is a completely different approach but I wonder if we can get creative here.

legrego · 2018-10-15T19:53:40Z

@alexfrancoeur there's no great way to check a user's permissions when entering the app. The authorization logic is rather complex because of all of the permutations we need to allow:

Security + Spaces with the new privilege system
Security + Spaces with the legacy privilege system
Security w/o Spaces with the new privilege system
Security w/o Spaces with the legacy privilege system
Spaces w/o Security
All of the above with and w/o Canvas enabled

The implementation we have for 6.5 is very focused, and unfortunately not extensible for the canvas use case.

Dashboard only mode was also a very focused implementation, and from what I can tell, it's method isn't something we can apply here.

alexfrancoeur

Spoke with Catherine about this. We cannot avoid the "flash" of the editing controls upon first interaction within the Canvas app until we have granular application permissions in Kibana. @legrego & @kobelb are targeting this for 6.6/6.x. I'd like to have the logic using granular app permissions before GA if possible. While this experience isn't ideal, I feel that it's fine for beta. Once merged, I'll open an issue to track adding granular app permissions so we can know the users roles & permissions for an application when they enter it. This will allow us to control the experience appropriately.

That being said, LGTM.

elasticmachine · 2018-10-16T20:12:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-10-16T22:04:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

clintandrewhall

I spoke with @cqliu1 yesterday, and I'm suggesting we divide this read-only change into two concepts:

1/ The viewer does not have permission within Kibana/Canvas to write workpads, and
2/ A particular workpad is not writable by the viewer, (for whatever reason, though there is only one today).

So I'm recommending we take this opportunity to augment the user model to include permissions information around Canvas while at the same time dividing that concern from the permissions of an individual workpad.

I think I caught all of the places below, but YMMV...!

clintandrewhall · 2018-10-18T17:12:47Z

x-pack/plugins/canvas/public/apps/workpad/routes.js

@@ -11,6 +11,7 @@ import { setWorkpad } from '../../state/actions/workpad';
 import { setAssets, resetAssets } from '../../state/actions/assets';
 import { gotoPage } from '../../state/actions/pages';
 import { getWorkpad } from '../../state/selectors/workpad';
+import { setReadOnlyUser } from '../../state/actions/transient';


Consider setUserCanRead, (which would default to true) and setUserCanWrite, which would be set based on the success or error response.

Suggested change

import { setReadOnlyUser } from '../../state/actions/transient';

import { setUserCanRead, setUserCanWrite } from '../../state/actions/transient';

Currently there are only read or all privileges in Kibana, so I don't believe it's necessary to track read permissions since every user at a baseline has read privileges. It should be sufficient to just track if the user has write permissions.

clintandrewhall · 2018-10-18T17:13:50Z

x-pack/plugins/canvas/public/apps/workpad/routes.js

@@ -29,6 +30,8 @@ export const routes = [
            router.redirectTo('loadWorkpad', { id: newWorkpad.id, page: 1 });
          } catch (err) {
            notify.error(err, { title: `Couldn't create workpad` });
+            if (err.response.status === 403) dispatch(setReadOnlyUser(true));


Suggested change

if (err.response.status === 403) dispatch(setReadOnlyUser(true));

if (err.response.status === 403) dispatch(setUserCanWrite(false));

clintandrewhall · 2018-10-18T17:14:57Z

x-pack/plugins/canvas/public/apps/workpad/routes.js

+
+              // tests if user has permissions to write to workpads
+              workpadService.update(params.id, fetchedWorkpad).catch(err => {
+                if (err.response.status === 403) dispatch(setReadOnlyUser(true));


Suggested change

if (err.response.status === 403) dispatch(setReadOnlyUser(true));

if (err.response.status === 403) dispatch(setUserCanWrite(false));

clintandrewhall · 2018-10-18T17:15:56Z

x-pack/plugins/canvas/public/apps/workpad/routes.js

@@ -48,6 +51,11 @@ export const routes = [
              const { assets, ...workpad } = fetchedWorkpad;
              dispatch(setWorkpad(workpad));
              dispatch(setAssets(assets));
+
+              // tests if user has permissions to write to workpads
+              workpadService.update(params.id, fetchedWorkpad).catch(err => {


Since you mentioned there will be a new permissions API forthcoming, consider adding a TODO comment here with a follow-up task?

clintandrewhall · 2018-10-18T17:17:36Z

x-pack/plugins/canvas/public/apps/workpad/workpad_app/index.js

@@ -8,16 +8,16 @@ import { connect } from 'react-redux';
 import { compose, branch, renderComponent } from 'recompose';
 import { initializeWorkpad } from '../../../state/actions/workpad';
 import { selectElement } from '../../../state/actions/transient';
-import { getEditing, getAppReady } from '../../../state/selectors/app';
-import { getWorkpad } from '../../../state/selectors/workpad';
+import { getReadOnlyUser, getAppReady } from '../../../state/selectors/app';


Consider boolean-style getters.

Suggested change

import { getReadOnlyUser, getAppReady } from '../../../state/selectors/app';

import { canUserRead, canUserWrite, getAppReady } from '../../../state/selectors/app';

clintandrewhall · 2018-10-18T18:04:23Z

x-pack/plugins/canvas/public/state/initial_state.js

@@ -11,7 +11,7 @@ export const getInitialState = path => {
  const state = {
    app: {}, // Kibana stuff in here
    transient: {
-      editing: true,
+      readOnlyUser: false,


Suggested change

readOnlyUser: false,

canUserRead: true,

canUserWrite: true,

clintandrewhall · 2018-10-18T18:04:35Z

x-pack/plugins/canvas/public/state/middleware/es_persist.js

@@ -13,6 +13,7 @@ import * as transientActions from '../actions/transient';
 import * as resolvedArgsActions from '../actions/resolved_args';
 import { update } from '../../lib/workpad_service';
 import { notify } from '../../lib/notify';
+import { getReadOnlyUser } from '../selectors/app';


Suggested change

import { getReadOnlyUser } from '../selectors/app';

import { canUserWrite } from '../selectors/app';

clintandrewhall · 2018-10-18T18:04:52Z

x-pack/plugins/canvas/public/state/middleware/es_persist.js

@@ -43,6 +44,9 @@ export const esPersistMiddleware = ({ getState }) => {
    next(action);
    const newState = getState();

+    // skips the update request if in read only mode
+    if (getReadOnlyUser(newState)) return;


Suggested change

if (getReadOnlyUser(newState)) return;

if (!canUserWrite(newState)) return;

clintandrewhall · 2018-10-18T18:05:07Z

x-pack/plugins/canvas/public/state/reducers/transient.js

@@ -28,8 +28,8 @@ export const transientReducer = handleActions(
      );
    },

-    [actions.setEditing]: (transientState, { payload }) => {
-      return set(transientState, 'editing', Boolean(payload));
+    [actions.setReadOnlyUser]: (transientState, { payload }) => {


Suggested change

[actions.setReadOnlyUser]: (transientState, { payload }) => {

[actions.setCanUserWrite]: (transientState, { payload }) => {

clintandrewhall · 2018-10-18T18:05:19Z

x-pack/plugins/canvas/public/state/reducers/transient.js

-    [actions.setEditing]: (transientState, { payload }) => {
-      return set(transientState, 'editing', Boolean(payload));
+    [actions.setReadOnlyUser]: (transientState, { payload }) => {
+      return set(transientState, 'readOnlyUser', Boolean(payload));


Suggested change

return set(transientState, 'readOnlyUser', Boolean(payload));

return set(transientState, 'canUserWrite', Boolean(payload));

clintandrewhall

I spoke with @cqliu1 yesterday, and I'm suggesting we divide this read-only change into two concepts:

1/ The viewer does not have permission within Kibana/Canvas to write workpads, and
2/ A particular workpad is not writable by the viewer, (for whatever reason, though there is only one today).

So I'm recommending we take this opportunity to augment the user model to include permissions information around Canvas while at the same time dividing that concern from the permissions of an individual workpad.

I think I caught all of the places below, but YMMV...!

elasticmachine · 2018-10-18T19:31:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cqliu1 · 2018-10-18T20:05:07Z

x-pack/plugins/canvas/public/components/workpad/index.js

@@ -25,7 +25,6 @@ const mapStateToProps = state => ({
  totalElementCount: getAllElements(state).length,
  workpad: getWorkpad(state),
  isFullscreen: getFullscreen(state),
-  isEditing: getEditing(state),


This prop wasn't being used in the workpad component, so I removed it.

cqliu1 · 2018-10-18T20:35:57Z

@clintandrewhall This is ready for another look when you get a chance. I made all of the changes you requested and added a couple tests for the isWriteable workpad selector.

x-pack/plugins/canvas/public/components/workpad_loader/index.js

elasticmachine · 2018-10-18T22:55:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Added readOnly to transient state * Added check that user has permission to edit when loading workpads * Shows toolbar in readOnly mode * Disables page controls in read only mode * Disables add page button when editing disabled * Added comment to workpad routes * Cleaned up page manager styles * Added a comment to es_persist * Renamed transient.readOnly to transient.readOnlyUser * Replaced isEditing in transient state with readOnly in workpad state * Cleaned up workpad_header * Fixed logic in workpad_header * Disables workpad controls instead of hiding them and adds tooltips * Switched eye icon to lock icon in workpad header * Renamed readOnly and readOnlyUser variables and functions * Added dispatch to update canUserWrite in create/clone/delete workpads * Fixed typo * Removed unnecessary prop from workpad * Refactored tooltips in workpad_loader * Added workpad selector tests

cqliu1 added 7 commits September 17, 2018 15:17

Added readOnly to transient state

58ca391

Added check that user has permission to edit when loading workpads

37bb076

Shows toolbar in readOnly mode

e3717b7

Disables page controls in read only mode

0ab8152

Disables add page button when editing disabled

8b5252d

Added comment to workpad routes

2264d50

Cleaned up page manager styles

4ae0cd5

cqliu1 added review Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Sep 19, 2018

cqliu1 requested review from rashidkpc and alexfrancoeur September 19, 2018 03:25

Added a comment to es_persist

585a769

cqliu1 commented Sep 19, 2018

View reviewed changes

rashidkpc added this to In Progress in Canvas (6.6 @ 12/18, 6.7 @ 01/29) Sep 21, 2018

cqliu1 added 2 commits September 26, 2018 10:27

Merge branch 'master' into feat/read-only-mode

aea0b81

Renamed transient.readOnly to transient.readOnlyUser

2118571

rashidkpc changed the title ~~Feat: Read Only Mode in Canvas~~ [WIP] Read Only Mode in Canvas Sep 26, 2018

rashidkpc removed their request for review September 26, 2018 19:11

cqliu1 added 3 commits September 26, 2018 16:03

Replaced isEditing in transient state with readOnly in workpad state

e6f3c2f

Cleaned up workpad_header

f9fd32c

Fixed logic in workpad_header

6c20700

cqliu1 commented Sep 26, 2018

View reviewed changes

alexfrancoeur approved these changes Oct 16, 2018

View reviewed changes

Merge branch 'master' into feat/read-only-mode

01c0b3b

rashidkpc requested review from clintandrewhall and removed request for rashidkpc October 16, 2018 18:35

Merge branch 'master' into feat/read-only-mode

3acc54e

clintandrewhall reviewed Oct 18, 2018

View reviewed changes

cqliu1 added 4 commits October 18, 2018 15:34

Renamed readOnly and readOnlyUser variables and functions

6a7ee14

Added dispatch to update canUserWrite in create/clone/delete workpads

bcf9e30

Fixed typo

d429466

Removed unnecessary prop from workpad

6990d12

cqliu1 commented Oct 18, 2018

View reviewed changes

cqliu1 added 2 commits October 18, 2018 16:22

Refactored tooltips in workpad_loader

b8ce43d

Added workpad selector tests

2ef216a

cqliu1 commented Oct 18, 2018

View reviewed changes

x-pack/plugins/canvas/public/components/workpad_loader/index.js Show resolved Hide resolved

Merge branch 'master' into feat/read-only-mode

238a3a1

cqliu1 merged commit 9d424f9 into elastic:master Oct 22, 2018

Canvas (6.6 @ 12/18, 6.7 @ 01/29) automation moved this from In Progress to Done Oct 22, 2018

cqliu1 deleted the feat/read-only-mode branch October 22, 2018 21:01

cqliu1 mentioned this pull request Oct 22, 2018

[6.x] Read Only Mode in Canvas (#23304) #24367

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read Only Mode in Canvas #23304

Read Only Mode in Canvas #23304

cqliu1 commented Sep 19, 2018 •

edited

cqliu1 Sep 19, 2018

cqliu1 commented Sep 19, 2018 •

edited

elasticmachine commented Sep 19, 2018

alexfrancoeur commented Sep 19, 2018

elasticmachine commented Sep 19, 2018

alexfrancoeur commented Sep 21, 2018

legrego commented Sep 21, 2018

alexfrancoeur commented Sep 21, 2018

kobelb commented Sep 21, 2018 •

edited

cqliu1 Sep 26, 2018

alexfrancoeur commented Oct 15, 2018

legrego commented Oct 15, 2018

alexfrancoeur left a comment

elasticmachine commented Oct 16, 2018

elasticmachine commented Oct 16, 2018

clintandrewhall left a comment

clintandrewhall Oct 18, 2018

cqliu1 Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall Oct 18, 2018

clintandrewhall left a comment

elasticmachine commented Oct 18, 2018

cqliu1 Oct 18, 2018

cqliu1 commented Oct 18, 2018

elasticmachine commented Oct 18, 2018

	import { setReadOnlyUser } from '../../state/actions/transient';
	import { setUserCanRead, setUserCanWrite } from '../../state/actions/transient';

	if (err.response.status === 403) dispatch(setReadOnlyUser(true));
	if (err.response.status === 403) dispatch(setUserCanWrite(false));

	import { getReadOnlyUser, getAppReady } from '../../../state/selectors/app';
	import { canUserRead, canUserWrite, getAppReady } from '../../../state/selectors/app';

	import { getReadOnlyUser } from '../selectors/app';
	import { canUserWrite } from '../selectors/app';

	if (getReadOnlyUser(newState)) return;
	if (!canUserWrite(newState)) return;

	[actions.setReadOnlyUser]: (transientState, { payload }) => {
	[actions.setCanUserWrite]: (transientState, { payload }) => {

	return set(transientState, 'readOnlyUser', Boolean(payload));
	return set(transientState, 'canUserWrite', Boolean(payload));

Read Only Mode in Canvas #23304

Read Only Mode in Canvas #23304

Conversation

cqliu1 commented Sep 19, 2018 • edited

Choose a reason for hiding this comment

cqliu1 commented Sep 19, 2018 • edited

elasticmachine commented Sep 19, 2018

💔 Build Failed

alexfrancoeur commented Sep 19, 2018

elasticmachine commented Sep 19, 2018

💚 Build Succeeded

alexfrancoeur commented Sep 21, 2018

legrego commented Sep 21, 2018

alexfrancoeur commented Sep 21, 2018

kobelb commented Sep 21, 2018 • edited

Choose a reason for hiding this comment

alexfrancoeur commented Oct 15, 2018

legrego commented Oct 15, 2018

alexfrancoeur left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 16, 2018

💔 Build Failed

elasticmachine commented Oct 16, 2018

💔 Build Failed

clintandrewhall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintandrewhall left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 18, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

cqliu1 commented Oct 18, 2018

elasticmachine commented Oct 18, 2018

💚 Build Succeeded

cqliu1 commented Sep 19, 2018 •

edited

cqliu1 commented Sep 19, 2018 •

edited

kobelb commented Sep 21, 2018 •

edited