fix(coderd): improve password update logic #15210
Conversation
coderd/users.go
Outdated
| admin, err := api.Database.GetUserByID(ctx, apiKey.UserID) | ||
| if err != nil { | ||
| httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ | ||
| Message: "Internal error fetching user.", | ||
| Detail: err.Error(), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
This isn't necessary, you can check the RBAC roles of the user from httpmw.UserParam directly.
There was a problem hiding this comment.
From what I tested the UserParam returns the user from the URL and not the user authenticated - did I missed something ?
What I try to achieve here is to know if the user doing the request is an admin or not.
There was a problem hiding this comment.
Ah, good point. I think there are still some subtleties here:
httpmw.UserParamwill still use the authenticated request user (requestor) from the request context to fetch the required user. If the requestor does not have the permission to read the user, this will fail.- This may not take into account custom RBAC roles or an organization admin role
I think the reason for this check is to allow user administrators to reset a user's password without having to specify their existing password.
A better check might be something along the lines of apiKey.UserID != user.ID && api.Authorize(r, policy.ActionUpdate, user.
There was a problem hiding this comment.
Hey @johnstcn as we discussed I changed a bit the logic to verify the user associated to the APIKey to the user we try to update - also validated the behavior for Auditor (it works as expected.)
| @@ -1133,7 +1159,7 @@ func TestUpdateUserPassword(t *testing.T) { | |||
| require.Equal(t, int32(http.StatusNotFound), auditor.AuditLogs()[numLogs-1].StatusCode) | |||
| }) | |||
|
|
|||
| t.Run("AdminCanUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { | |||
| t.Run("AdminCantUpdateOwnPasswordWithoutOldPassword", func(t *testing.T) { | |||
There was a problem hiding this comment.
I changed this test to reflect the logic change here.
IMO even an admin should not be able to change its own password without giving the old password - also the UI currently forces users (admin too) to have the old password.
Working on #15202
The main change is to fetch the user doing the action to verify if it should be able to change the password if there's no old_password set.