From 28d103b2ef19fa1ab1e2ff7cf49749ade2e5f2ea Mon Sep 17 00:00:00 2001 From: Shamik Karkhanis Date: Thu, 26 Mar 2026 13:11:36 -0400 Subject: [PATCH 1/2] fix(user): tightened perms to not allow auth users to update other users :/ --- internal/handler/users_test.go | 84 +++++++++++++++++++++------------- 1 file changed, 52 insertions(+), 32 deletions(-) diff --git a/internal/handler/users_test.go b/internal/handler/users_test.go index 62c8b19..78eabcf 100644 --- a/internal/handler/users_test.go +++ b/internal/handler/users_test.go @@ -91,33 +91,39 @@ func TestGetUser(t *testing.T) { func TestUpdateUser(t *testing.T) { targetUID := uuid.New() - authenticatedUID := uuid.New() + otherUID := uuid.New() firstName := "Updated" currentRole := "student" - role := "faculty" + newRole := "faculty" tests := []struct { name string requestBody dto.UpdateUserRequest - mockSetup func(*mocks.Querier) setupContext func() context.Context + mockSetup func(*mocks.Querier) expectedStatus int }{ { - name: "NonDevCanUpdateWhenSubmittedRoleMatchesCurrentRole", + name: "SelfCanUpdateWhenRoleUnchanged", requestBody: dto.UpdateUserRequest{ FirstName: &firstName, Role: ¤tRole, }, + setupContext: func() context.Context { + ctx := context.Background() + claims := &middleware.UserClaims{UserID: targetUID.String()} + ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims) + return context.WithValue(ctx, middleware.AuthTypeKey, "human") + }, mockSetup: func(m *mocks.Querier) { - m.On("GetUserByID", mock.Anything, authenticatedUID).Return(database.User{ - Uid: authenticatedUID, + m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{ + Uid: targetUID, Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true}, - }, nil) + }, nil).Once() m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{ Uid: targetUID, Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true}, - }, nil) + }, nil).Once() m.On("UpdateUser", mock.Anything, mock.MatchedBy(func(arg database.UpdateUserParams) bool { return arg.Uid == targetUID && arg.FirstName.Valid && arg.FirstName.String == firstName && @@ -129,45 +135,65 @@ func TestUpdateUser(t *testing.T) { Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true}, }, nil) }, + expectedStatus: http.StatusOK, + }, + { + name: "NonDevCannotUpdateAnotherUser", + requestBody: dto.UpdateUserRequest{ + FirstName: &firstName, + Role: ¤tRole, + }, setupContext: func() context.Context { ctx := context.Background() - claims := &middleware.UserClaims{UserID: authenticatedUID.String()} + claims := &middleware.UserClaims{UserID: otherUID.String()} ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims) return context.WithValue(ctx, middleware.AuthTypeKey, "human") }, - expectedStatus: http.StatusOK, + mockSetup: func(m *mocks.Querier) { + m.On("GetUserByID", mock.Anything, otherUID).Return(database.User{ + Uid: otherUID, + Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true}, + }, nil) + }, + expectedStatus: http.StatusForbidden, }, { - name: "NonDevCannotUpdateRole", + name: "NonDevCannotChangeOwnRole", requestBody: dto.UpdateUserRequest{ - Role: &role, + Role: &newRole, + }, + setupContext: func() context.Context { + ctx := context.Background() + claims := &middleware.UserClaims{UserID: targetUID.String()} + ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims) + return context.WithValue(ctx, middleware.AuthTypeKey, "human") }, mockSetup: func(m *mocks.Querier) { - m.On("GetUserByID", mock.Anything, authenticatedUID).Return(database.User{ - Uid: authenticatedUID, + m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{ + Uid: targetUID, Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true}, - }, nil) + }, nil).Once() m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{ Uid: targetUID, Role: database.NullUserRole{UserRole: database.UserRoleStudent, Valid: true}, - }, nil) - }, - setupContext: func() context.Context { - ctx := context.Background() - claims := &middleware.UserClaims{UserID: authenticatedUID.String()} - ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims) - return context.WithValue(ctx, middleware.AuthTypeKey, "human") + }, nil).Once() }, expectedStatus: http.StatusForbidden, }, { - name: "DevCanUpdateRole", + name: "DevCanChangeAnotherUsersRole", requestBody: dto.UpdateUserRequest{ - Role: &role, + Role: &newRole, + }, + setupContext: func() context.Context { + ctx := context.Background() + claims := &middleware.UserClaims{UserID: otherUID.String()} + ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims) + return context.WithValue(ctx, middleware.AuthTypeKey, "human") }, mockSetup: func(m *mocks.Querier) { - m.On("GetUserByID", mock.Anything, authenticatedUID).Return(database.User{ - Uid: authenticatedUID, + m.On("GetUserByID", mock.Anything, otherUID).Return(database.User{ + Uid: otherUID, Role: database.NullUserRole{UserRole: database.UserRoleDev, Valid: true}, }, nil) m.On("GetUserByID", mock.Anything, targetUID).Return(database.User{ @@ -183,12 +209,6 @@ func TestUpdateUser(t *testing.T) { Role: database.NullUserRole{UserRole: database.UserRoleFaculty, Valid: true}, }, nil) }, - setupContext: func() context.Context { - ctx := context.Background() - claims := &middleware.UserClaims{UserID: authenticatedUID.String()} - ctx = context.WithValue(ctx, middleware.UserClaimsKey, claims) - return context.WithValue(ctx, middleware.AuthTypeKey, "human") - }, expectedStatus: http.StatusOK, }, } From 93719de7f9b4cf4ae1bca26a2e9055cfe98f76c9 Mon Sep 17 00:00:00 2001 From: Shamik Karkhanis Date: Thu, 26 Mar 2026 13:16:02 -0400 Subject: [PATCH 2/2] fix(user): another pass --- internal/handler/users.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/handler/users.go b/internal/handler/users.go index 13dba98..2ecbfca 100644 --- a/internal/handler/users.go +++ b/internal/handler/users.go @@ -60,7 +60,7 @@ func (h *Handler) UpdateUser(w http.ResponseWriter, r *http.Request) { return } - authenticatedUser, _, ok := h.requireAuthenticatedUserRecord(w, r) + authenticatedUser, ok := h.requireSelfOrDev(w, r, uid) if !ok { return }