-
Notifications
You must be signed in to change notification settings - Fork 490
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
base: develop
Are you sure you want to change the base?
API Update global role + fixed issue with GUI custom role edition #10612
Conversation
These are both under |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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? 🤔
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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>
7a864e8
to
52d72d3
Compare
@pdurbin @poikilotherm Guide is updated (sorry for the force-push fixing git bad manipulation). |
@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! |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/CreateRoleCommand.java
Outdated
Show resolved
Hide resolved
@@ -1010,6 +1010,22 @@ public Response createNewBuiltinRole(RoleDTO roleDto) { | |||
actionLogSvc.log(alr); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pdurbin I can replicate. Starting investigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setup-all.sh
creates roles before creating the root dataverse. Causing issue while creating a role
owner = dataverseService.findByAlias("root");
IndexResponse indexDefinitionPointResult = indexDefinitionPoint(owner);
Resulting in "definitionPoint" is null"
Fixed in my last commit.
2024/09/25: Moving to On Hold for now. |
Yep, someone needs to fix the problem I described at #10612 (comment) before this PR can move forward. In short, it doesn't work with a new installation that gets freshly bootstrapped, I believe. |
@cmbz @scolapasta I looked at 3acd516 (didn't run it) and it looks like a good fix. I'm ready to move this to "ready for QA" if you are. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the latest commit from @luddaniel and it looks good. Thanks! Approved.
@luddaniel sorry, sorry, sorry, can you please resolve merge conflicts? ❤️ |
What this PR does / why we need it:
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:
Update.Role.via.API.mp4
Edit.custom.role.GUI.mp4
Suggestions on how to test this:
Play around roles and permissions.
Ex :
roles.json
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 :
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) :
curl -H 'Content-Type: application/json' -X PUT "http://localhost:8080/api/admin/roles/7" --upload-file roles.json
OK