informatici
OH2-253 | Reorganize user related endpoints
See OH2-253. Paired with https://github.com/informatici/openhospital-core/pull/1406.
Hi @mwithi ! Could we merge this ?
Hi @SteveGT96 not yet. We requested review from @dbmalkovsky and @SilverD3, and me also I need to test it, but I will try to do it soon.
Will this PR interfer with #464 ?
Hi @mwithi ! Could we merge this ?
Hi @SteveGT96 not yet. We requested review from @dbmalkovsky and @SilverD3, and me also I need to test it, but I will try to do it soon.
Will this PR interfer with #464 ?
Certainly !
I've resolved all comments submitted by @dbmalkovsky ! He could review if it's OK, or not.
Hello @SteveGT96, please can you resolve the conflicts after #464 ?
Done !
I'm testing and it seems that PUT /users and POST /users/settings are not working
I'm testing and it seems that
PUT /usersandPOST /users/settingsare not working
What is "PUT /users" for ? Endpoints to update user are "PUT /users/{username}" and "PUT /users/me". I didn't change "POST /users/groups"(changes on it come from #464).
What is "PUT /users" for ? Endpoints to update user are "PUT /users/{username}" and "PUT /users/me". I didn't change "POST /users/groups"(changes on it come from #464).
Sorry, my bad, I had a dirty environment due to other developments, let me test again.
Maybe I'm wrong, but I logged as doctor:doctor (from demo data) and I was able to change the admin password with PUT /users/{username} endpoint... also, before the same endpoint had a boolean param to specify if the update is concerning the password or not. Please check
debugging, it seems that whenever I log as doctor, then the DTO has userName=admin, so you have not to compare the two (forged request) but the logged user.
Maybe I'm wrong, but I logged as
doctor:doctor(from demo data) and I was able to change theadminpassword withPUT /users/{username}endpoint... also, before the same endpoint had a boolean param to specify if the update is concerning the password or not. Please check
The endpoint "PUT /users/{username}":
- used to update user(logged in user should have users.update and users.read - may be users.update is suffiscient). That means that even the logged in user cannot use this endpoint if it has not the 'users.update && users.read' privilleges.
- when the password in the payload is not empty, other prop are ignored and only the the password is updated(with that we don't need to pass anymore 'updatePassword=true').
PUT /users/me:
- Used to to update the logged in user
- Same as for the previous endpoint, when the field password in the payload is not empty, then only the password is updated.
Actually in the security config we have:
.requestMatchers("/users/me").authenticated() .requestMatchers(HttpMethod.PUT, "/users/{username}").access("hasAuthority('users.update') && hasAuthority('users.read')")
I'm going to add the aboved description in the endpoint documentation.
Should we change that behaviour ?
Should we change that behaviour ?
The above descriptions mean that users.update permission should be given only to an admin user?
Should we change that behaviour ?
The above descriptions means that
users.updatepermission should be given only to an admin user?
Correct ! Other users could update their profile/password using "PUT /users/me"
Correct ! Other users could update their profile/password using "PUT /users/me"
I understand now, so we need to change the demo data in core, could you do that (using same issue code)?
Correct ! Other users could update their profile/password using "PUT /users/me"
I understand now, so we need to change the demo data in core, could you do that (using same issue code)?
Sure ! Im also going to keep only hasAuthority('users.update') (not hasAuthority('users.update') && hasAuthority('users.read')).
Do you agree ?
Is it ok that any user can read other users settings with GET /users/settings/{id}?
POST /users/settings adding a new config for the logged user (doctor) returns Duplicate entry 'doctor' for key 'USS_US_ID_A'.
Why USS_US_ID_A is set as unique in step_96? Should a user have more the one setting?
With POST /users/groups/{group_code}/permissions/{id} I was able to reassign myself (doctor) the just removed permission (113)
Is it ok that any user can read other users settings with
GET /users/settings/{id}?
No. Let me fix it !
Is it ok that any user can read other users settings with
GET /users/settings/{id}?
Resolved: Logged in user can update only its own setting
POST /users/settingsadding a new config for the logged user (doctor) returnsDuplicate entry 'doctor' for key 'USS_US_ID_A'.Why
USS_US_ID_Ais set as unique instep_96? Should a user have more the one setting?
Resolved
With
POST /users/groups/{group_code}/permissions/{id}I was able to reassign myself (doctor) the just removed permission (113)
Resolved: Only user with 'grouppermission.create' permission can call POST /users/groups/{group_code}/permissions/{id}.
The following refactoring could be nice:
users/groups/{username} -> [/users/{username}/groups => UserController] /user/groups -> [/groups => GroupController] users/groups/{group_code}/permissions -> [/groups/{id}/permissions => GroupController]
@mwithi what do you think ?
The following refactoring could be nice:
users/groups/{username} -> [/users/{username}/groups => UserController] /user/groups -> [/groups => GroupController] users/groups/{group_code}/permissions -> [/groups/{id}/permissions => GroupController]
@mwithi what do you think ?
I suggest to open a discussion on Slack to address this topic, because, of course, I think we should make some refactoring concerning user resources too. Actually user resources are in package menu which is not really suitable.
Isn't it a good idea to return more comprehensive error messages ?
When I call GET /permissions/userGroupCode/{code} I just get 204 response with no content