Prevent non-admin users from modifying other users

An incorrect authorization check allowed non-admin users to modify the details of other users. We now enforce the appropriate authorization so that unprivileged users can only modify their own details.

Thanks to 'Quikke' for the report.

server/mock/datastore.go

@@ -12,16 +12,17 @@ package mock
 //go:generate mockimpl -o datastore_scheduled_queries.go "s *ScheduledQueryStore" "kolide.ScheduledQueryStore"
 //go:generate mockimpl -o datastore_queries.go "s *QueryStore" "kolide.QueryStore"
 //go:generate mockimpl -o datastore_campaigns.go "s *CampaignStore" "kolide.CampaignStore"
+//go:generate mockimpl -o datastore_sessions.go "s *SessionStore" "kolide.SessionStore"
 
 import "github.com/kolide/fleet/server/kolide"
 
 var _ kolide.Datastore = (*Store)(nil)
 
 type Store struct {
-	kolide.SessionStore
 	kolide.PasswordResetStore
 	kolide.YARAStore
 	kolide.TargetStore
+	SessionStore
 	CampaignStore
 	ScheduledQueryStore
 	OsqueryOptionsStore

server/mock/datastore_sessions.go

@@ -0,0 +1,79 @@
+// Automatically generated by mockimpl. DO NOT EDIT!
+
+package mock
+
+import "github.com/kolide/fleet/server/kolide"
+
+var _ kolide.SessionStore = (*SessionStore)(nil)
+
+type SessionByKeyFunc func(key string) (*kolide.Session, error)
+
+type SessionByIDFunc func(id uint) (*kolide.Session, error)
+
+type ListSessionsForUserFunc func(id uint) ([]*kolide.Session, error)
+
+type NewSessionFunc func(session *kolide.Session) (*kolide.Session, error)
+
+type DestroySessionFunc func(session *kolide.Session) error
+
+type DestroyAllSessionsForUserFunc func(id uint) error
+
+type MarkSessionAccessedFunc func(session *kolide.Session) error
+
+type SessionStore struct {
+	SessionByKeyFunc        SessionByKeyFunc
+	SessionByKeyFuncInvoked bool
+
+	SessionByIDFunc        SessionByIDFunc
+	SessionByIDFuncInvoked bool
+
+	ListSessionsForUserFunc        ListSessionsForUserFunc
+	ListSessionsForUserFuncInvoked bool
+
+	NewSessionFunc        NewSessionFunc
+	NewSessionFuncInvoked bool
+
+	DestroySessionFunc        DestroySessionFunc
+	DestroySessionFuncInvoked bool
+
+	DestroyAllSessionsForUserFunc        DestroyAllSessionsForUserFunc
+	DestroyAllSessionsForUserFuncInvoked bool
+
+	MarkSessionAccessedFunc        MarkSessionAccessedFunc
+	MarkSessionAccessedFuncInvoked bool
+}
+
+func (s *SessionStore) SessionByKey(key string) (*kolide.Session, error) {
+	s.SessionByKeyFuncInvoked = true
+	return s.SessionByKeyFunc(key)
+}
+
+func (s *SessionStore) SessionByID(id uint) (*kolide.Session, error) {
+	s.SessionByIDFuncInvoked = true
+	return s.SessionByIDFunc(id)
+}
+
+func (s *SessionStore) ListSessionsForUser(id uint) ([]*kolide.Session, error) {
+	s.ListSessionsForUserFuncInvoked = true
+	return s.ListSessionsForUserFunc(id)
+}
+
+func (s *SessionStore) NewSession(session *kolide.Session) (*kolide.Session, error) {
+	s.NewSessionFuncInvoked = true
+	return s.NewSessionFunc(session)
+}
+
+func (s *SessionStore) DestroySession(session *kolide.Session) error {
+	s.DestroySessionFuncInvoked = true
+	return s.DestroySessionFunc(session)
+}
+
+func (s *SessionStore) DestroyAllSessionsForUser(id uint) error {
+	s.DestroyAllSessionsForUserFuncInvoked = true
+	return s.DestroyAllSessionsForUserFunc(id)
+}
+
+func (s *SessionStore) MarkSessionAccessed(session *kolide.Session) error {
+	s.MarkSessionAccessedFuncInvoked = true
+	return s.MarkSessionAccessedFunc(session)
+}

server/service/handler.go

@@ -122,7 +122,7 @@ func MakeKolideServerEndpoints(svc kolide.Service, jwtKey string) KolideEndpoint
 		ChangePassword:       authenticatedUser(jwtKey, svc, canPerformActions(makeChangePasswordEndpoint(svc))),
 		GetUser:              authenticatedUser(jwtKey, svc, canReadUser(makeGetUserEndpoint(svc))),
 		ListUsers:            authenticatedUser(jwtKey, svc, canPerformActions(makeListUsersEndpoint(svc))),
-		ModifyUser:           authenticatedUser(jwtKey, svc, canPerformActions(makeModifyUserEndpoint(svc))),
+		ModifyUser:           authenticatedUser(jwtKey, svc, canModifyUser(makeModifyUserEndpoint(svc))),
 		AdminUser:            authenticatedUser(jwtKey, svc, mustBeAdmin(makeAdminUserEndpoint(svc))),
 		EnableUser:           authenticatedUser(jwtKey, svc, mustBeAdmin(makeEnableUserEndpoint(svc))),
 		RequirePasswordReset: authenticatedUser(jwtKey, svc, mustBeAdmin(makeRequirePasswordResetEndpoint(svc))),

server/service/handler_test.go

@@ -1,13 +1,19 @@
 package service
 
 import (
+	"bytes"
+	"errors"
 	"fmt"
 	"net/http/httptest"
 	"testing"
+	"time"
 
+	"github.com/go-kit/kit/log"
 	"github.com/gorilla/mux"
 	"github.com/kolide/fleet/server/config"
 	"github.com/kolide/fleet/server/datastore/inmem"
+	"github.com/kolide/fleet/server/kolide"
+	"github.com/kolide/fleet/server/mock"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -209,3 +215,112 @@ func TestAPIRoutes(t *testing.T) {
 		})
 	}
 }
+
+func TestModifyUserPermissions(t *testing.T) {
+	var (
+		admin, enabled bool
+		uid            uint
+	)
+	ms := new(mock.Store)
+	ms.SessionByKeyFunc = func(key string) (*kolide.Session, error) {
+		return &kolide.Session{AccessedAt: time.Now(), UserID: uid}, nil
+	}
+	ms.DestroySessionFunc = func(session *kolide.Session) error {
+		return nil
+	}
+	ms.MarkSessionAccessedFunc = func(session *kolide.Session) error {
+		return nil
+	}
+	ms.UserByIDFunc = func(id uint) (*kolide.User, error) {
+		return &kolide.User{ID: id, Enabled: enabled, Admin: admin}, nil
+	}
+	ms.SaveUserFunc = func(u *kolide.User) error {
+		// Return an error so that the endpoint returns
+		return errors.New("foo")
+	}
+
+	svc, err := newTestService(ms, nil)
+	assert.Nil(t, err)
+
+	handler := MakeHandler(svc, "CHANGEME", log.NewNopLogger())
+
+	testCases := []struct {
+		ActingUserID      uint
+		ActingUserAdmin   bool
+		ActingUserEnabled bool
+		TargetUserID      uint
+		Authorized        bool
+	}{
+		// Disabled regular user
+		{
+			ActingUserID:      1,
+			ActingUserAdmin:   false,
+			ActingUserEnabled: false,
+			TargetUserID:      1,
+			Authorized:        false,
+		},
+		// Enabled regular user acting on self
+		{
+			ActingUserID:      1,
+			ActingUserAdmin:   false,
+			ActingUserEnabled: true,
+			TargetUserID:      1,
+			Authorized:        true,
+		},
+		// Enabled regular user acting on other
+		{
+			ActingUserID:      2,
+			ActingUserAdmin:   false,
+			ActingUserEnabled: true,
+			TargetUserID:      1,
+			Authorized:        false,
+		},
+		// Disabled admin user
+		{
+			ActingUserID:      1,
+			ActingUserAdmin:   true,
+			ActingUserEnabled: false,
+			TargetUserID:      1,
+			Authorized:        false,
+		},
+		// Enabled admin user acting on self
+		{
+			ActingUserID:      1,
+			ActingUserAdmin:   true,
+			ActingUserEnabled: true,
+			TargetUserID:      1,
+			Authorized:        true,
+		},
+		// Enabled admin user acting on other
+		{
+			ActingUserID:      2,
+			ActingUserAdmin:   true,
+			ActingUserEnabled: true,
+			TargetUserID:      1,
+			Authorized:        true,
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run("", func(t *testing.T) {
+			// Set user params
+			uid = tt.ActingUserID
+			admin, enabled = tt.ActingUserAdmin, tt.ActingUserEnabled
+
+			recorder := httptest.NewRecorder()
+			path := fmt.Sprintf("/api/v1/kolide/users/%d", tt.TargetUserID)
+			request := httptest.NewRequest("PATCH", path, bytes.NewBufferString("{}"))
+			// Bearer token generated with session key CHANGEME on jwt.io
+			request.Header.Add("Authorization", "Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzZXNzaW9uX2tleSI6ImZsb29wIn0.ukCPTFvgSJrXbHH2QeAMx3EKwoMh1OmhP3xXxy5I-Wk")
+
+			handler.ServeHTTP(recorder, request)
+			if tt.Authorized {
+				assert.NotEqual(t, 403, recorder.Code)
+			} else {
+				assert.Equal(t, 403, recorder.Code)
+			}
+
+		})
+	}
+
+}