Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Update global role + fixed issue with GUI custom role edition #10612

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

luddaniel
Copy link
Contributor

What this PR does / why we need it:

  • Global role can now be updated via API
  • Customised role can now be edited via GUI

PR is in Draft mode waiting for an answer. Guide to Create Global Role and to Delete a Global Role suggest to use $API_TOKEN. As a matter of fact you don't need it. Question is : Should we add SuperAdmin authorization on create, update and delete of a Global Role ? I would say yes but I want your opinion.

Which issue(s) this PR closes:

Special notes for your reviewer:

I removed a comment @todo update permissionModificationTime here. as it is handled later/deeper here : DvObject savedDvObject = dvObjectService.updatePermissionIndexTime(dvObject);

Demos:

  • Global role can now be updated via API
Update.Role.via.API.mp4
  • Customised role can now be edited via GUI
Edit.custom.role.GUI.mp4

Suggestions on how to test this:
Play around roles and permissions.
Ex :
roles.json

{
   "alias":"sys1",
   "name":"Restricted System Role",
   "description":"A person who may only add datasets.",
   "permissions":[
      "AddDataset"
   ]
}

Create a new global role :
curl -H 'Content-Type: application/json' -X POST "http://localhost:8080/api/admin/roles" --upload-file roles.json
Change roles.json :

{
   "alias":"sys1",
   "name":"Restricted System Role 23",
   "description":"A person who may only add datasets.",
   "permissions":[
      "AddDataset"
   ]
}

Update Role (Try to change name) :
curl -H 'Content-Type: application/json' -X PUT "http://localhost:8080/api/admin/roles/15" --upload-file roles.json
OK

Try to update Curator role (change permissions) :

{
   "alias":"curator",
   "name":"Curator",
   "permissions":[
      "ViewUnpublishedDataverse",
      "ViewUnpublishedDataset",
      "DownloadFile",
      "EditDataset",
      "ManageDatasetPermissions",
      "ManageFilePermissions",
      "PublishDataset",
      "DeleteDatasetDraft"
   ],
   "description":"For datasets, a person who can edit License + Terms, edit Permissions, and publish datasets."
}

curl -H 'Content-Type: application/json' -X PUT "http://localhost:8080/api/admin/roles/7" --upload-file roles.json
OK

@coveralls
Copy link

Coverage Status

coverage: 20.57% (-0.003%) from 20.573%
when pulling c29a9af on Recherche-Data-Gouv:8808-10575-update-global-role
into 3c55c3f on IQSS:develop.

@coveralls
Copy link

Coverage Status

coverage: 20.57% (-0.003%) from 20.573%
when pulling 7d55ae1 on Recherche-Data-Gouv:8808-10575-update-global-role
into 3c55c3f on IQSS:develop.

@pdurbin
Copy link
Member

pdurbin commented Jun 5, 2024

PR is in Draft mode waiting for an answer. Guide to Create Global Role and to Delete a Global Role suggest to use $API_TOKEN. As a matter of fact you don't need it. Question is : Should we add SuperAdmin authorization on create, update and delete of a Global Role ? I would say yes but I want your opinion.

These are both under /api/admin so I think it's ok to leave them as non-superuser because /api/admin should be blocked.

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just leaving a little comment for now. Overall, this looks great!


Update a global role in the Dataverse installation. The data PUTed are assumed to be a role JSON. ::

POST http://$SERVER/api/admin/roles
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be PUT instead of POST? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No doubt in my mind : PUT to update, copy paste from the web :

  • POST requests create child resources at a server defined URI. POST is also used as general processing operation
  • PUT requests create or replace the resource at the client defined URI

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, but I find the docs confusing. "data PUTed" followed by POST.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right ! I'll change POST http://$SERVER/api/admin/roles

Copy link
Contributor

@poikilotherm poikilotherm Jun 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

REST good practices: 1, 2, 3

  • POST is for creation of new resources
  • PUT is for updates / replacements in an idempotent way (complete object is provided/required). Will create new object if not existing, too
  • PATCH is for partial updates / modifications

As this is about "updating" a global role, this should use a "PUT" request and the docs should note the requirement of a complete object and the inability to update the role partially.

Co-authored-by: Oliver Bertuch <poikilotherm@users.noreply.github.com>
@coveralls
Copy link

Coverage Status

coverage: 20.571% (-0.002%) from 20.573%
when pulling 9d0004d on Recherche-Data-Gouv:8808-10575-update-global-role
into 3c55c3f on IQSS:develop.

@luddaniel luddaniel force-pushed the 8808-10575-update-global-role branch from 7a864e8 to 52d72d3 Compare June 12, 2024 08:13
@luddaniel
Copy link
Contributor Author

@pdurbin @poikilotherm Guide is updated (sorry for the force-push fixing git bad manipulation).

@coveralls
Copy link

Coverage Status

coverage: 20.571% (-0.003%) from 20.574%
when pulling 1b115dc on Recherche-Data-Gouv:8808-10575-update-global-role
into 5bf6b6d on IQSS:develop.

@coveralls
Copy link

Coverage Status

coverage: 20.571% (-0.003%) from 20.574%
when pulling 1b115dc on Recherche-Data-Gouv:8808-10575-update-global-role
into 5bf6b6d on IQSS:develop.

@coveralls
Copy link

Coverage Status

coverage: 20.571% (-0.003%) from 20.574%
when pulling 1b115dc on Recherche-Data-Gouv:8808-10575-update-global-role
into 5bf6b6d on IQSS:develop.

@pdurbin pdurbin added the Champion: pdurbin Championed by @pdurbin for inclusion in the next release label Jul 19, 2024
@pdurbin
Copy link
Member

pdurbin commented Jul 19, 2024

@gwendoux suggested this for 6.4 and I agree if would be nice.

@luddaniel the plan is to not require superuser right? The API endpoints are safe under /api/admin. Can you please merge the latest from develop and mark this pull request as non-draft if you're ready? Thanks!

@luddaniel luddaniel marked this pull request as ready for review July 22, 2024 08:32
@pdurbin pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Sep 11, 2024
@cmbz cmbz added the FY25 Sprint 6 FY25 Sprint 6 label Sep 11, 2024
@pdurbin pdurbin removed the Champion: pdurbin Championed by @pdurbin for inclusion in the next release label Sep 12, 2024
@pdurbin pdurbin self-assigned this Sep 16, 2024
@@ -1010,6 +1010,22 @@ public Response createNewBuiltinRole(RoleDTO roleDto) {
actionLogSvc.log(alr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Above, at return ok(json(rolesSvc.save(roleDto.asRole()))); I'm getting the following error when trying to set up Dataverse for the first time:

{"status":"ERROR","message":"Exception thrown from bean: jakarta.ejb.EJBTransactionRolledbackException: Exception thrown from bean: java.lang.NullPointerException: Cannot invoke \"edu.harvard.iq.dataverse.DvObject.isInstanceofDataverse()\" because \"definitionPoint\" is null"}

Specifically, I'm doing this:

rm -rf docker-dev-volumes

mvn -Pct clean package docker:run

@luddaniel can you replicate this?

@pdurbin pdurbin added the Status: Needs Input Applied to issues in need of input from someone currently unavailable label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FY25 Sprint 6 FY25 Sprint 6 Size: 3 A percentage of a sprint. 2.1 hours. Status: Needs Input Applied to issues in need of input from someone currently unavailable
Projects
Status: In Review 🔎
Status: 🚧 Dev by Recherche Data Gouv
6 participants