Skip to content

Commit 6201956

Browse files
shamikkarkhanisshamikkarkhanis
committed
fix(user): relaxed user update permissions
1 parent b19662e commit 6201956

2 files changed

Lines changed: 144 additions & 4 deletions

File tree

‎internal/handler/users.go‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ func (h *Handler) GetUser(w http.ResponseWriter, r *http.Request) {
4040

4141
// UpdateUser updates a user's profile
4242
// @Summary Update user
43-
// @Description Updates a user's profile. Users can only update their own profile.
43+
// @Description Updates a user's profile. Only role changes require the caller to have the dev role.
4444
// @Tags users
4545
// @Accept json
4646
// @Produce json
@@ -60,7 +60,7 @@ func (h *Handler) UpdateUser(w http.ResponseWriter, r *http.Request) {
6060
return
6161
}
6262

63-
authenticatedUser, ok := h.requireSelfOrDev(w, r, uid)
63+
authenticatedUser, _, ok := h.requireAuthenticatedUserRecord(w, r)
6464
if !ok {
6565
return
6666
}
@@ -71,13 +71,23 @@ func (h *Handler) UpdateUser(w http.ResponseWriter, r *http.Request) {
7171
return
7272
}
7373

74+
targetUser, err := h.queries.GetUserByID(r.Context(), uid)
75+
if err != nil {
76+
h.handleDBError(w, err)
77+
return
78+
}
79+
7480
var role database.NullUserRole
7581
if req.Role != nil {
76-
if !authenticatedUser.Role.Valid || authenticatedUser.Role.UserRole != database.UserRoleDev {
82+
requestedRole := database.UserRole(*req.Role)
83+
roleChanged := !targetUser.Role.Valid || targetUser.Role.UserRole != requestedRole
84+
if roleChanged && (!authenticatedUser.Role.Valid || authenticatedUser.Role.UserRole != database.UserRoleDev) {
7785
h.respondError(w, http.StatusForbidden, "Only dev may update user roles")
7886
return
7987
}
80-
role = database.NullUserRole{UserRole: database.UserRole(*req.Role), Valid: true}
88+
if roleChanged {
89+
role = database.NullUserRole{UserRole: requestedRole, Valid: true}
90+
}
8191
}
8292

8393
user, err := h.queries.UpdateUser(r.Context(), database.UpdateUserParams{

‎internal/handler/users_test.go‎

Lines changed: 130 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package handler_test
22

33
import (
4+
"bytes"
5+
"context"
6+
"encoding/json"
47
"fmt"
58
"net/http"
69
"net/http/httptest"
@@ -9,7 +12,9 @@ import (
912
"github.com/capyrpi/api/internal/config"
1013
"github.com/capyrpi/api/internal/database"
1114
"github.com/capyrpi/api/internal/database/mocks"
15+
"github.com/capyrpi/api/internal/dto"
1216
"github.com/capyrpi/api/internal/handler"
17+
"github.com/capyrpi/api/internal/middleware"
1318
"github.com/go-chi/chi/v5"
1419
"github.com/google/uuid"
1520
"github.com/jackc/pgx/v5"
@@ -83,3 +88,128 @@ func TestGetUser(t *testing.T) {
8388
})
8489
}
8590
}
91+
92+
func TestUpdateUser(t *testing.T) {
93+
targetUID := uuid.New()
94+
authenticatedUID := uuid.New()
95+
firstName := "Updated"
96+
currentRole := "student"
97+
role := "faculty"
98+
99+
tests := []struct {
100+
name string
101+
requestBody dto.UpdateUserRequest
102+
mockSetup func(*mocks.Querier)
103+
setupContext func() context.Context
104+
expectedStatus int
105+
}{
106+
{
107+
name: "NonDevCanUpdateWhenSubmittedRoleMatchesCurrentRole",
108+
requestBody: dto.UpdateUserRequest{
109+
FirstName: &firstName,
110+
Role: &currentRole,
111+
},
112+
mockSetup: func(m *mocks.Querier) {
113+
m.On("GetUserByID", mock.Anything, authenticatedUID).Return(database.User{
114+
Uid: authenticatedUID,
115+
Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true},
116+
}, nil)
117+
m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{
118+
Uid: targetUID,
119+
Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true},
120+
}, nil)
121+
m.On("UpdateUser", mock.Anything, mock.MatchedBy(func(arg database.UpdateUserParams) bool {
122+
return arg.Uid == targetUID &&
123+
arg.FirstName.Valid && arg.FirstName.String == firstName &&
124+
!arg.Role.Valid
125+
})).Return(database.User{
126+
Uid: targetUID,
127+
FirstName: firstName,
128+
LastName: "Doe",
129+
Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true},
130+
}, nil)
131+
},
132+
setupContext: func() context.Context {
133+
ctx := context.Background()
134+
claims := &middleware.UserClaims{UserID: authenticatedUID.String()}
135+
ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims)
136+
return context.WithValue(ctx, middleware.AuthTypeKey, "human")
137+
},
138+
expectedStatus: http.StatusOK,
139+
},
140+
{
141+
name: "NonDevCannotUpdateRole",
142+
requestBody: dto.UpdateUserRequest{
143+
Role: &role,
144+
},
145+
mockSetup: func(m *mocks.Querier) {
146+
m.On("GetUserByID", mock.Anything, authenticatedUID).Return(database.User{
147+
Uid: authenticatedUID,
148+
Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true},
149+
}, nil)
150+
m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{
151+
Uid: targetUID,
152+
Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true},
153+
}, nil)
154+
},
155+
setupContext: func() context.Context {
156+
ctx := context.Background()
157+
claims := &middleware.UserClaims{UserID: authenticatedUID.String()}
158+
ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims)
159+
return context.WithValue(ctx, middleware.AuthTypeKey, "human")
160+
},
161+
expectedStatus: http.StatusForbidden,
162+
},
163+
{
164+
name: "DevCanUpdateRole",
165+
requestBody: dto.UpdateUserRequest{
166+
Role: &role,
167+
},
168+
mockSetup: func(m *mocks.Querier) {
169+
m.On("GetUserByID", mock.Anything, authenticatedUID).Return(database.User{
170+
Uid: authenticatedUID,
171+
Role: database.NullUserRole{UserRole: database.UserRoleDev, Valid: true},
172+
}, nil)
173+
m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{
174+
Uid: targetUID,
175+
Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true},
176+
}, nil)
177+
m.On("UpdateUser", mock.Anything, mock.MatchedBy(func(arg database.UpdateUserParams) bool {
178+
return arg.Uid == targetUID &&
179+
arg.Role.Valid &&
180+
arg.Role.UserRole == database.UserRoleFaculty
181+
})).Return(database.User{
182+
Uid: targetUID,
183+
Role: database.NullUserRole{UserRole: database.UserRoleFaculty, Valid: true},
184+
}, nil)
185+
},
186+
setupContext: func() context.Context {
187+
ctx := context.Background()
188+
claims := &middleware.UserClaims{UserID: authenticatedUID.String()}
189+
ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims)
190+
return context.WithValue(ctx, middleware.AuthTypeKey, "human")
191+
},
192+
expectedStatus: http.StatusOK,
193+
},
194+
}
195+
196+
for _, tt := range tests {
197+
t.Run(tt.name, func(t *testing.T) {
198+
mockQueries := mocks.NewQuerier(t)
199+
tt.mockSetup(mockQueries)
200+
201+
h := handler.New(mockQueries, &config.Config{})
202+
r := chi.NewRouter()
203+
r.Put("/users/{uid}", h.UpdateUser)
204+
205+
body, _ := json.Marshal(tt.requestBody)
206+
req := httptest.NewRequest(http.MethodPut, fmt.Sprintf("/users/%s", targetUID), bytes.NewBuffer(body))
207+
req = req.WithContext(tt.setupContext())
208+
rr := httptest.NewRecorder()
209+
210+
r.ServeHTTP(rr, req)
211+
212+
assert.Equal(t, tt.expectedStatus, rr.Code)
213+
})
214+
}
215+
}

0 commit comments

Comments
 (0)