Bug 1779493: Disabled edit actions if the user has no permission #3484

vikram-raj · 2019-11-20T05:08:55Z

Bug - https://jira.coreos.com/browse/ODC-1791

This PR

Hide connector arrow
Hide context menu for node
Disabled drag and drop
Hide delete connector

TODO
~~- Disabled Drag and drop.~~
~~- Hide delete connector~~

vikram-raj · 2019-11-20T05:09:51Z

/cc @christianvogt @jeff-phillips-18

vikram-raj · 2019-11-21T12:36:49Z

/assign @christianvogt

jeff-phillips-18 · 2019-11-21T12:41:16Z

frontend/packages/topology/src/behavior/withRemoveConnector.tsx

@@ -1,5 +1,7 @@
 import * as React from 'react';
 import { observer } from 'mobx-react';
+import { referenceFor, modelFor } from '@console/internal/module/k8s';
+import { useAccessReview } from '@console/internal/components/utils';


The topology package shouldn't have dependencies on other @console packages (with the possible exception of shared, and then only with pure utils).

Even shared we want to avoid because we eventually want to move it to PF.
There is a small dependency on popper because recently I moved that code and didn't want to duplicate.

@jeff-phillips-18 @christianvogt Added the access check in BaseEdge component

christianvogt · 2019-11-22T04:06:44Z

frontend/packages/dev-console/src/components/topology2/components/nodes/BaseNode.tsx


  return (
    <g className="odc2-base-node">
      <NodeShadows />
      <g
        data-test-id="base-node-handler"
        onClick={onSelect}
-        onContextMenu={onContextMenu}
-        ref={refs}
+        {...(editAccess ? { onContextMenu } : {})}


This isn't right. Context menu actions already have their own access checks built in.

This is for hiding the contextMenu. Because we are hiding all the option which user has no access to.

I guess we don't have any context menu actions that don't act directly on the resource itself, so it's probably ok to hide it.
However:

Suggested change

{...(editAccess ? { onContextMenu } : {})}

onContextMenu={editAccess && onContextMenu}

Got this warning when I change it to onContextMenu={editAccess && onContextMenu}

How about this then.
The other way just doesn't seem very clear but maybe it's just me. @andrewballantyne @rohitkrai03 what do you suggest?

Suggested change

{...(editAccess ? { onContextMenu } : {})}

onContextMenu={editAccess && onContextMenu || null}

Not really, ternaries are the only other option... but it's essentially the same thing. But it might be cleaner to read:

Suggested change

{...(editAccess ? { onContextMenu } : {})}

onContextMenu={editAccess ? onContextMenu : null}

🤷‍♂

christianvogt · 2019-11-22T04:10:29Z

frontend/packages/dev-console/src/components/topology2/components/nodes/BaseNode.tsx

-        onContextMenu={onContextMenu}
-        ref={refs}
+        {...(editAccess ? { onContextMenu } : {})}
+        {...(editAccess ? { ref: refs } : {})}


refs provides drag to move behavior AND regroup behavior.
The regroup behavior needs to suppressed but not the drag.

Look at componentUtils#nodeDragSourceSpec. This is where the REGROUP_OPERATION is installed.
We may need to move to hooks instead of HOCs to gain better control of the behaviors.

christianvogt · 2019-11-22T04:31:33Z

frontend/packages/dev-console/src/components/topology2/components/nodes/BaseNode.tsx

+    name: element.getData().data.donutStatus.dc.metadata.name,
+    namespace: element.getData().data.donutStatus.dc.metadata.namespace,


Using donutStatus as the main resource is very odd.
I guess we don't have any other data point to read from here :(

We have the only donutStatus where resource data exist.

~~element.getData().resources.obj.metadata is available~~
Best to use the object returned from getTopologyResourceObject(element.getData())

christianvogt · 2019-11-22T04:35:09Z

frontend/packages/dev-console/src/components/topology2/components/edges/BaseEdge.tsx

@@ -32,7 +49,7 @@ const BaseEdge: React.FC<BaseEdgeProps> = ({
  return (
    <Layer id={dragging || hover ? 'top' : undefined}>
      <g
-        ref={hoverRef}
+        {...editAccess && { ref: hoverRef }}


What does hover have to do with editAccess?!
We need hover to provide a hover style treatment.
You need to prevent showing the connector if the user doesn't have edit access.
Furthermore, editing the visual connector edits the source node while the service request binding connector creates a new SBR. Two completely different access control scenarios that require different checks.

I do check editAccess here because the delete connection option appears on hover.

But we do other things on hover such as changing the color of the line to be blue and raised.

removed the editAccess checked from hoverRef.

frontend/packages/dev-console/src/components/topology2/componentUtils.ts

frontend/packages/dev-console/src/components/topology2/components/nodes/BaseNode.tsx

openshift-ci-robot · 2019-12-04T05:00:05Z

@vikram-raj: This pull request references Bugzilla bug 1779493, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-bot · 2019-12-04T06:01:32Z

/retest