Feature restrict submissions #1817 #1914

gothub · 2021-11-10T22:11:40Z

This PR makes these changes:

when NodeModel.js gets the node capabilties report from the CN
- the capabilities report for the current MN is inspected
  - if service restrictions are defined for MNStorage.create
    - if the current user, or an equivalent id is not in the list, they are not allowed to submit (create/update) to the MN
- if the MN isn't found in the CN capabilities report
  - the report is obtained directly from the MN
if the user is not allowed to submit
- the "Edit" button is never displayed in the MetadataView
- a warning is printed if they click on the Submit button

Issue #1817

laurenwalker · 2021-12-20T19:09:28Z

@gothub - I tested your changes for this PR today and it was not working for me. I changed a metacat instance to restrict submissions to a test group with only my ORCID as a member. I confirmed that this setting was shown in the MN and CN node descriptions.

As expected, when logged into my ORCID I was able to submit and everything looked fine. But when I logged into the Kepler ldap account, which is not in my test group, I had a bunch of issues. Below is a list of things to fix. I tried to add a checkbox next to each actionable item.

Improvements needed to the warning message

When I try to submit as a blocked user, the editor is still completely rendered and a warning message displays over it. I could still click on things and view the editor. We instead want to stop the rendering of the editor completely and show only the message on the page. Use the 404 error as an example of what this would look like:

This message:

Should look like:

Also the text was not very user friendly. e.g. people may not know what "DataONE user" means. I'd change to:

You are not authorized to submit or edit datasets. Please contact us at {email} to learn how to gain submission access.

This message should be in an HTML template so each repository can customize this message however they'd like. For example, ESS-DIVE might want to display more specific information and instructions about how to get added to the list and that simple message will not suffice.

Bug: Can still edit an existing dataset

I was able to click 'Edit' on an existing dataset and access the editor and save changes. I got an (expected) error back from Metacat. We should instead disable the Edit button with a tooltip message and show the same warning message as mentioned above.

Bug: Can click Publish

The Publish button on existing datasets should be disabled with a tooltip message and the Publish request should not be sent.

Bug: Can edit portals

I was able to click "Edit" on an existing portal and submit changes (with a resulting error from Metacat).

The portal editor should function just like the dataset editor. It should not render and display a message.

Bug: When I signed out, the warning message was displayed

When I clicked "Sign out" from the blocked account, the warning message was displayed on top of the Sign In page. Something caused it to render again.

Bug: Sometimes the warning message was not displayed at all

When I first logged in and clicked "Upload data", the warning message was displayed such as in the first screenshot I posted here. But most times after that, the message never displayed again and I could access the editor. It'll be helpful during development to do a lot of navigating around the app, refreshing the page, etc. to make sure access to the editor is always blocked.

I am going to close this PR for now and you can open it again after these issues are fixed. Thanks

gothub · 2021-12-27T18:25:37Z

@laurenwalker thanks for the review, I'll start working on fixing the bugs that you described.

Relates to #1789

Issue #1817

gothub · 2022-03-07T19:23:29Z

@laurenwalker all the issues you mentioned have been resolved. Regarding the error message displayed if a user is not in the allowed submitters list, I implemented it in the same way as error messages used for portals. That is, an error message is defined in AppModel.js, as is done for:

portalEditNotAuthCreateMessage
portalEditNotAuthEditMessage

Regarding the AppModel.js checkAllowedSubmitters attribute - the default is false, which means the allowed.submitters list from the node capabilities report is not used. If it is set to true, then they can only edit if their id is in the list or an equivalent identity allows them to edit.

Also included is a mercifully short video demonstrating how these changes affect the UI.

allow.submitters-issue-.1817-demo.mp4

laurenwalker

@gothub Thank you for making the changes we talked about and fixing most of those issues. Below is a list of manual UI tests I created for this PR. Each check mark is a test that passed and each empty check box is a failed test, with an Error message below it.

Please review and fix all the failed tests below. It would be helpful to have you run through the tests again before submitting another PR review.

Additional comments:

I wasn't sure if we had decided to support the denied submitter list in the UI for this PR. As you can see in the list below, each of the tests for the Denied Submitter list failed because I don't think it's checked in MetacatUI. We may decide to skip that functionality for now if it significantly delays the release of this work so far.
Regarding the checkAllowedSubmitters config: I think this can be set to true by default so that repo admins don't have to update the Metacat AND MetacatUI config for the feature to work. I originally suggested this as a feature flag because I thought it would prevent the node info doc from being grabbed for nodes that don't need to parse it for the allowed submitter list (which slows down the app load time by having to wait for another XHR to finish). But as I mentioned in my code comment above, I noticed that the node info doc is grabbed regardless. We should only send that extra request if either the member node is not in the CN node list OR if the checkAllowedSubmitters is set to true.

When AllowedSubmitters is disabled in MetacatUI and Metacat

When AllowedSubmitters is set to my ORCID

When AllowedSubmitters is set to my group

When I am not in the AllowedSubmitter list

The Edit button does not display for datasets
The Edit button does not display for portals in the portal list
The Edit button does not display for portals in the portal view
The "New" button does not display for portals
- Error: When I go to /portals to view the portal list there is a New Portal button

The dataset editor doesn't render and shows a message for a new dataset (/submit)
The dataset editor doesn't render and shows a message for an existing dataset (/submit/id)
The portal editor doesn't render and shows a message
- When navigating to the view
- When refreshing the page on the view
- For a new portal (/edit/portals)
  - Error: The portal editor still renders when creating a new portal (at /edit/portals) and there is no message

- [x] For an existing portal (/edit/portals/{id})

The provenance editor is not displayed

When I am in the DeniedSubmitter list

All of the below tests failed when my ORCID is in the denied submitter list, but the allowed submitter list is either empty or set to someone else. I think the denied submitter list is not checked at all. Did we decide that was out of scope for now?

The Edit button does not display for datasets
- Error: I still see the Edit and Publish buttons for datasets when I am in the denied submitter list:

DataONE theme testing

The following tests are for the DataONE theme, which supports the portal editor but not the dataset editor or provenance editor.

To test the dataone theme portal features with the dev.nceas member node, use the search-stage MetacatUI configuration in the metacatui-config repo.

DataONE theme only: When I am in the AllowedSubmitter list

The Edit button displays for portals in the portal list
The Edit button displays in the portal view
The "New" button displays for portals
The portal editor renders for new portals (/edit/portals)
The portal editor renders for an existing portal (/edit/portals/{id})
The "not auth" message still displays for portals I don't have write permissions for

DataONE theme only: When I am not in the AllowedSubmitter list

The Edit button does not display for portals in the portal list
- Error: The Edit button displays for my portals

The Edit button does not display in the portal view
- Error: The Edit buttons displays int he portal view
The "New" button does not display for portals
- Error: The New button displays in the portal list

The portal editor does not render for new portals (/edit/portals)
- Error: The new portal editor still renders

The portal editor does not render for an existing portal (/edit/portals/{id})
- Error: The portal editor still renders when I try to edit an existing portal

DataONE theme only: When I am in the DeniedSubmitter list

Again, not sure if this is out-of-scope for now

The Edit button does not display for portals in the portal list
- Error: The Edit button displays for each of my portals in the list
The Edit button does not display in the portal view
- Error: The Edit button displays for my portals
The "New" button does not display for portals
- Error The New button shows up in the portal list
The portal editor does not render for new portals (/edit/portals)
- - Error: The portal editor fully renders
The portal editor does not render for an existing portal (/edit/portals/{id})
- Error: The portal editor fully renders

laurenwalker · 2022-03-17T13:41:15Z

src/js/models/AppModel.js

+      * (see https://releases.dataone.org/online/api-documentation-v2.0/apis/CN_APIs.html#CNCore.listNodes)
+      * @type {string}
+      */
+      getCapabilitiesServiceUrl: null, 


Isn't this the same as the AppModel.nodeServiceUrl?

Ah, I see that the nodeServiceUrl is for the CN and getCapabilitiesServiceUrl is for the MN.

laurenwalker · 2022-03-17T13:46:49Z

src/js/models/NodeModel.js

        }
 			});
 		},
+
+        getCapabilities: function(){


I think this method name is misleading because more than the node capabilities are retrieved and set on the model. If we are only setting the node capabilities, then let's keep this name. But if we are setting all of the node properties (which it appears we are based on like 127 (thisModel.saveNodeInfo(xmlResponse);)), then let's name this method something more appropriate like getMemberNodeInfo()

Can you also please add a method description using JSDoc?

OK, I'll update the method call name and create the JSDoc entry.

laurenwalker · 2022-03-17T14:09:49Z

src/js/models/NodeModel.js

+                    if(!thisModel.get("nodeInfoFound")) {
+                        thisModel.getCapabilities();
+                    }


I notice that the app never sets this attribute (nodeInfoFound) so the member node doc will always be retrieved. When the CN node info doc is parsed, let's check if the current member node was found in the list and if not, set nodeInfoFound to false.

Even better, we could just use NodeModel.getMember() to see if the current member node is already set on the model. If getMember() doesn't return our node, then we know it wasn't found in the CN node list.

This does mean that the member node info doc needs to be synced with the CN for the changes in this PR to work. Right now I'm testing with dev.nceas and it hasn't synced with the CN, so everything has been working in my tests so far because the member node info doc is grabbed directly from dev.nceas.

Good idea on using NodeModel.getMember(), I'll implement the use of that.

The MN config change (updating allowed.submitters is only changed when the metcat admin config for DataONE is updated (i.e. https://dev.nceas.ucsb.edu/knb/admin?configureType=dataone), even though the allow.submitters property is changed in the global properties admin menu, (https://dev.nceas.ucsb.edu/knb/admin?configureType=properties). Maybe this needs a revisit on design?

Yes, denied.submitters should be implemented, but could that happen in a later release (ESS-DIVE hasn't requested this)?

gothub added 6 commits October 6, 2021 10:41

Update NodeModel to read service restrictions

8ba4469

Issue #1817

Check if current user is in 'allowedSubmitter' list

c43b2b4

Issue #1817

Disable 'Edit' button in MetadataView if not allowed submitter

8bcd6ef

Issue #1817

Get node capabilities from MN if standalone repo

bce99c6

Issue #1817

Get MN capabilities directly from MN if required

cb18893

Issue #1817

Merge branch 'develop' into feature-restrict-submissions-#1817

67ff3ce

gothub requested a review from laurenwalker November 10, 2021 22:11

laurenwalker closed this Dec 20, 2021

gothub and others added 6 commits January 12, 2022 10:06

Merge branch 'develop' into feature-restrict-submissions-#1817

6b3a195

Merge branch 'develop' into feature-restrict-submissions-#1817

ed68175

Allow WMTS layers in Cesium

07249f4

Relates to #1789

Merge branch 'develop' into feature-restrict-submissions-#1817

7a12fb4

Use consistent naming

23ff2d7

Issue #1817

Resolve problems with allow.submitters check

1ab2947

Issue #1817

gothub reopened this Mar 7, 2022

laurenwalker suggested changes Mar 17, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature restrict submissions #1817 #1914

Feature restrict submissions #1817 #1914

gothub commented Nov 10, 2021

laurenwalker commented Dec 20, 2021 •

edited by gothub

gothub commented Dec 27, 2021

gothub commented Mar 7, 2022

laurenwalker left a comment

laurenwalker Mar 17, 2022

laurenwalker Mar 17, 2022

laurenwalker Mar 17, 2022

laurenwalker Mar 17, 2022

gothub Mar 27, 2022

laurenwalker Mar 17, 2022

gothub Mar 27, 2022

gothub Mar 27, 2022

Feature restrict submissions #1817 #1914

Are you sure you want to change the base?

Feature restrict submissions #1817 #1914

Conversation

gothub commented Nov 10, 2021

laurenwalker commented Dec 20, 2021 • edited by gothub

Improvements needed to the warning message

This message:

Should look like:

Bug: Can still edit an existing dataset

Bug: Can click Publish

Bug: Can edit portals

Bug: When I signed out, the warning message was displayed

Bug: Sometimes the warning message was not displayed at all

gothub commented Dec 27, 2021

gothub commented Mar 7, 2022

laurenwalker left a comment

Choose a reason for hiding this comment

When AllowedSubmitters is disabled in MetacatUI and Metacat

When AllowedSubmitters is set to my ORCID

When AllowedSubmitters is set to my group

When I am not in the AllowedSubmitter list

When I am in the DeniedSubmitter list

DataONE theme testing

DataONE theme only: When I am in the AllowedSubmitter list

DataONE theme only: When I am not in the AllowedSubmitter list

DataONE theme only: When I am in the DeniedSubmitter list

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

laurenwalker commented Dec 20, 2021 •

edited by gothub