Refactoring and fixes in user authorization

- Simplify/fix logic for authorization
- Rename/refactor for clarity
- Add tests for auth related code

server/contexts/viewer/viewer.go

@@ -30,15 +30,6 @@ type Viewer struct {
 	Session *kolide.Session
 }
 
-// IsAdmin indicates whether or not the current user can perform administrative
-// actions.
-func (v Viewer) IsAdmin() bool {
-	if v.User != nil {
-		return v.User.Admin && v.User.Enabled
-	}
-	return false
-}
-
 // UserID is a helper that enables quick access to the user ID of the current
 // user.
 func (v Viewer) UserID() uint {
@@ -54,7 +45,7 @@ func (v Viewer) Username() string {
 	if v.User != nil {
 		return v.User.Username
 	}
-	return "none"
+	return ""
 }
 
 // FullName is a helper that enables quick access to the full name of the
@@ -63,7 +54,7 @@ func (v Viewer) FullName() string {
 	if v.User != nil {
 		return v.User.Name
 	}
-	return "none"
+	return ""
 }
 
 // SessionID returns the current user's session ID
@@ -74,6 +65,12 @@ func (v Viewer) SessionID() uint {
 	return 0
 }
 
+// IsUserID returns true if the given user id the same as the user which is
+// represented by this ViewerContext
+func (v Viewer) IsUserID(id uint) bool {
+	return v.UserID() == id
+}
+
 // IsLoggedIn determines whether or not the current VC is attached to a user
 // account
 func (v Viewer) IsLoggedIn() bool {
@@ -101,10 +98,13 @@ func (v Viewer) CanPerformActions() bool {
 	return false
 }
 
-// IsUserID returns true if the given user id the same as the user which is
-// represented by this ViewerContext
-func (v Viewer) IsUserID(id uint) bool {
-	return v.UserID() == id
+// CanPerformAdminActions indicates whether or not the current user can perform
+// administrative actions.
+func (v Viewer) CanPerformAdminActions() bool {
+	if v.User != nil {
+		return v.CanPerformActions() && v.User.Admin
+	}
+	return false
 }
 
 // CanPerformReadActionOnUser returns a bool indicating the current user's
@@ -120,10 +120,17 @@ func (v Viewer) CanPerformReadActionOnUser(uid uint) bool {
 // ability to perform write actions on the given user
 func (v Viewer) CanPerformWriteActionOnUser(uid uint) bool {
 	if v.User != nil {
-		// By not requiring v.CanPerformActions() here, we allow the
-		// user to update their password when they are in the forced
-		// password reset state.
-		return v.IsUserID(uid) || v.IsAdmin()
+		return (v.IsLoggedIn() && v.IsUserID(uid)) || v.CanPerformAdminActions()
+	}
+	return false
+}
+
+// CanPerformPasswordReset returns a bool indicating the current user's
+// ability to perform a password reset (in the case they have been required by
+// the admin).
+func (v Viewer) CanPerformPasswordReset() bool {
+	if v.User != nil {
+		return v.IsLoggedIn() && v.User.AdminForcedPasswordReset
 	}
 	return false
 }

server/contexts/viewer/viewer_test.go

@@ -0,0 +1,212 @@
+package viewer
+
+import (
+	"context"
+	"testing"
+
+	"github.com/kolide/fleet/server/kolide"
+	"github.com/stretchr/testify/assert"
+)
+
+var (
+	// Weird states
+	nilViewer       = Viewer{}
+	noSessionViewer = Viewer{
+		User: &kolide.User{
+			ID:       41,
+			Name:     "No Session",
+			Username: "nosession",
+			Enabled:  true,
+		},
+	}
+
+	// Regular users
+	userViewer = Viewer{
+		User: &kolide.User{
+			ID:       45,
+			Name:     "Regular User",
+			Username: "user",
+			Admin:    false,
+			Enabled:  true,
+		},
+		Session: &kolide.Session{
+			ID:     4,
+			UserID: 45,
+		},
+	}
+	disabledUserViewer = Viewer{
+		User: &kolide.User{
+			ID:       46,
+			Name:     "Disabled Regular User",
+			Username: "disabled_user",
+			Admin:    false,
+			Enabled:  false,
+		},
+		Session: &kolide.Session{
+			ID:     5,
+			UserID: 46,
+		},
+	}
+
+	needsPasswordResetUserViewer = Viewer{
+		User: &kolide.User{
+			ID:                       47,
+			Name:                     "Regular User Needs Password Reset",
+			Username:                 "reset_user",
+			Admin:                    false,
+			Enabled:                  true,
+			AdminForcedPasswordReset: true,
+		},
+		Session: &kolide.Session{
+			ID:     6,
+			UserID: 47,
+		},
+	}
+
+	// Admin users
+	adminViewer = Viewer{
+		User: &kolide.User{
+			ID:       42,
+			Name:     "The Admin",
+			Username: "admin",
+			Admin:    true,
+			Enabled:  true,
+		},
+		Session: &kolide.Session{
+			ID:     1,
+			UserID: 42,
+		},
+	}
+	disabledAdminViewer = Viewer{
+		User: &kolide.User{
+			ID:       43,
+			Name:     "The Disabled Admin",
+			Username: "disabled_admin",
+			Admin:    true,
+			Enabled:  false,
+		},
+		Session: &kolide.Session{
+			ID:     2,
+			UserID: 43,
+		},
+	}
+	needsPasswordResetAdminViewer = Viewer{
+		User: &kolide.User{
+			ID:                       44,
+			Name:                     "The Admin Requires Password Reset",
+			Username:                 "reset_admin",
+			Admin:                    true,
+			Enabled:                  true,
+			AdminForcedPasswordReset: true,
+		},
+		Session: &kolide.Session{
+			ID:     3,
+			UserID: 44,
+		},
+	}
+)
+
+func TestContext(t *testing.T) {
+	ctx := NewContext(context.Background(), userViewer)
+	v, ok := FromContext(ctx)
+	assert.True(t, ok)
+	assert.Equal(t, userViewer, v)
+}
+
+func TestIsUserID(t *testing.T) {
+	assert.True(t, adminViewer.IsUserID(42))
+	assert.False(t, adminViewer.IsUserID(7))
+	assert.True(t, userViewer.IsUserID(45))
+	assert.True(t, disabledUserViewer.IsUserID(46))
+}
+
+func TestIsLoggedIn(t *testing.T) {
+	assert.Equal(t, false, nilViewer.IsLoggedIn())
+	assert.Equal(t, false, noSessionViewer.IsLoggedIn())
+
+	assert.Equal(t, true, userViewer.IsLoggedIn())
+	assert.Equal(t, false, disabledUserViewer.IsLoggedIn())
+	assert.Equal(t, true, needsPasswordResetUserViewer.IsLoggedIn())
+
+	assert.Equal(t, true, adminViewer.IsLoggedIn())
+	assert.Equal(t, false, disabledAdminViewer.IsLoggedIn())
+	assert.Equal(t, true, needsPasswordResetAdminViewer.IsLoggedIn())
+}
+
+func TestCanPerformActions(t *testing.T) {
+	assert.Equal(t, false, nilViewer.CanPerformActions())
+	assert.Equal(t, false, noSessionViewer.CanPerformActions())
+
+	assert.Equal(t, true, userViewer.CanPerformActions())
+	assert.Equal(t, false, disabledUserViewer.CanPerformActions())
+	assert.Equal(t, false, needsPasswordResetUserViewer.CanPerformActions())
+
+	assert.Equal(t, true, adminViewer.CanPerformActions())
+	assert.Equal(t, false, disabledAdminViewer.CanPerformActions())
+	assert.Equal(t, false, needsPasswordResetAdminViewer.CanPerformActions())
+}
+
+func TestCanPerformAdminActions(t *testing.T) {
+	assert.Equal(t, false, nilViewer.CanPerformAdminActions())
+	assert.Equal(t, false, noSessionViewer.CanPerformAdminActions())
+
+	assert.Equal(t, false, userViewer.CanPerformAdminActions())
+	assert.Equal(t, false, disabledUserViewer.CanPerformAdminActions())
+	assert.Equal(t, false, needsPasswordResetUserViewer.CanPerformAdminActions())
+
+	assert.Equal(t, true, adminViewer.CanPerformAdminActions())
+	assert.Equal(t, false, disabledAdminViewer.CanPerformAdminActions())
+	assert.Equal(t, false, needsPasswordResetAdminViewer.CanPerformAdminActions())
+}
+
+func TestCanPerformReadActionOnUser(t *testing.T) {
+	assert.Equal(t, false, nilViewer.CanPerformReadActionOnUser(1))
+	assert.Equal(t, false, noSessionViewer.CanPerformReadActionOnUser(1))
+
+	assert.Equal(t, true, userViewer.CanPerformReadActionOnUser(1))
+	assert.Equal(t, true, userViewer.CanPerformReadActionOnUser(userViewer.User.ID))
+	assert.Equal(t, false, disabledUserViewer.CanPerformReadActionOnUser(1))
+	assert.Equal(t, false, disabledUserViewer.CanPerformReadActionOnUser(disabledUserViewer.User.ID))
+	assert.Equal(t, false, needsPasswordResetUserViewer.CanPerformReadActionOnUser(1))
+	assert.Equal(t, true, needsPasswordResetUserViewer.CanPerformReadActionOnUser(needsPasswordResetUserViewer.User.ID))
+
+	assert.Equal(t, true, adminViewer.CanPerformReadActionOnUser(1))
+	assert.Equal(t, true, adminViewer.CanPerformReadActionOnUser(adminViewer.User.ID))
+	assert.Equal(t, false, disabledAdminViewer.CanPerformReadActionOnUser(1))
+	assert.Equal(t, false, disabledAdminViewer.CanPerformReadActionOnUser(disabledAdminViewer.User.ID))
+	assert.Equal(t, false, needsPasswordResetAdminViewer.CanPerformReadActionOnUser(1))
+	assert.Equal(t, true, needsPasswordResetAdminViewer.CanPerformReadActionOnUser(needsPasswordResetAdminViewer.User.ID))
+}
+
+func TestCanPerformWriteActionOnUser(t *testing.T) {
+	assert.Equal(t, false, nilViewer.CanPerformWriteActionOnUser(1))
+	assert.Equal(t, false, noSessionViewer.CanPerformWriteActionOnUser(1))
+
+	assert.Equal(t, false, userViewer.CanPerformWriteActionOnUser(1))
+	assert.Equal(t, true, userViewer.CanPerformWriteActionOnUser(userViewer.User.ID))
+	assert.Equal(t, false, disabledUserViewer.CanPerformWriteActionOnUser(1))
+	assert.Equal(t, false, disabledUserViewer.CanPerformWriteActionOnUser(disabledUserViewer.User.ID))
+	assert.Equal(t, false, needsPasswordResetUserViewer.CanPerformWriteActionOnUser(1))
+	assert.Equal(t, true, needsPasswordResetUserViewer.CanPerformWriteActionOnUser(needsPasswordResetUserViewer.User.ID))
+
+	assert.Equal(t, true, adminViewer.CanPerformWriteActionOnUser(1))
+	assert.Equal(t, true, adminViewer.CanPerformWriteActionOnUser(adminViewer.User.ID))
+	assert.Equal(t, false, disabledAdminViewer.CanPerformWriteActionOnUser(1))
+	assert.Equal(t, false, disabledAdminViewer.CanPerformWriteActionOnUser(disabledAdminViewer.User.ID))
+	assert.Equal(t, false, needsPasswordResetAdminViewer.CanPerformWriteActionOnUser(1))
+	assert.Equal(t, true, needsPasswordResetAdminViewer.CanPerformWriteActionOnUser(needsPasswordResetAdminViewer.User.ID))
+}
+
+func TestCanPerformPasswordReset(t *testing.T) {
+	assert.Equal(t, false, nilViewer.CanPerformPasswordReset())
+	assert.Equal(t, false, noSessionViewer.CanPerformPasswordReset())
+
+	assert.Equal(t, false, userViewer.CanPerformPasswordReset())
+	assert.Equal(t, false, disabledUserViewer.CanPerformPasswordReset())
+	assert.Equal(t, true, needsPasswordResetUserViewer.CanPerformPasswordReset())
+
+	assert.Equal(t, false, adminViewer.CanPerformPasswordReset())
+	assert.Equal(t, false, disabledAdminViewer.CanPerformPasswordReset())
+	assert.Equal(t, true, needsPasswordResetAdminViewer.CanPerformPasswordReset())
+
+}

server/service/endpoint_appconfig.go

@@ -36,7 +36,7 @@ func makeGetAppConfigEndpoint(svc kolide.Service) endpoint.Endpoint {
 		var smtpSettings *kolide.SMTPSettingsPayload
 		var ssoSettings *kolide.SSOSettingsPayload
 		// only admin can see smtp settings
-		if vc.IsAdmin() {
+		if vc.CanPerformAdminActions() {
 			smtpSettings = smtpSettingsFromAppConfig(config)
 			if smtpSettings.SMTPPassword != nil {
 				*smtpSettings.SMTPPassword = "********"

server/service/endpoint_middleware.go

@@ -126,7 +126,7 @@ func mustBeAdmin(next endpoint.Endpoint) endpoint.Endpoint {
 		if !ok {
 			return nil, errNoContext
 		}
-		if !vc.IsAdmin() {
+		if !vc.CanPerformAdminActions() {
 			return nil, permissionError{message: "must be an admin"}
 		}
 		return next(ctx, request)
@@ -174,6 +174,19 @@ func canModifyUser(next endpoint.Endpoint) endpoint.Endpoint {
 	}
 }
 
+func canPerformPasswordReset(next endpoint.Endpoint) endpoint.Endpoint {
+	return func(ctx context.Context, request interface{}) (interface{}, error) {
+		vc, ok := viewer.FromContext(ctx)
+		if !ok {
+			return nil, errNoContext
+		}
+		if !vc.CanPerformPasswordReset() {
+			return nil, permissionError{message: "cannot reset password"}
+		}
+		return next(ctx, request)
+	}
+}
+
 type permission int
 
 const (

server/service/handler.go

@@ -128,7 +128,7 @@ func MakeKolideServerEndpoints(svc kolide.Service, jwtKey string) KolideEndpoint
 		RequirePasswordReset: authenticatedUser(jwtKey, svc, mustBeAdmin(makeRequirePasswordResetEndpoint(svc))),
 		// PerformRequiredPasswordReset needs only to authenticate the
 		// logged in user
-		PerformRequiredPasswordReset:          authenticatedUser(jwtKey, svc, makePerformRequiredPasswordResetEndpoint(svc)),
+		PerformRequiredPasswordReset:          authenticatedUser(jwtKey, svc, canPerformPasswordReset(makePerformRequiredPasswordResetEndpoint(svc))),
 		GetSessionsForUserInfo:                authenticatedUser(jwtKey, svc, canReadUser(makeGetInfoAboutSessionsForUserEndpoint(svc))),
 		DeleteSessionsForUser:                 authenticatedUser(jwtKey, svc, canModifyUser(makeDeleteSessionsForUserEndpoint(svc))),
 		GetSessionInfo:                        authenticatedUser(jwtKey, svc, mustBeAdmin(makeGetInfoAboutSessionEndpoint(svc))),

server/service/handler_test.go

@@ -223,7 +223,7 @@ func TestModifyUserPermissions(t *testing.T) {
 	)
 	ms := new(mock.Store)
 	ms.SessionByKeyFunc = func(key string) (*kolide.Session, error) {
-		return &kolide.Session{AccessedAt: time.Now(), UserID: uid}, nil
+		return &kolide.Session{AccessedAt: time.Now(), UserID: uid, ID: 1}, nil
 	}
 	ms.DestroySessionFunc = func(session *kolide.Session) error {
 		return nil

server/service/validation_users.go

@@ -108,7 +108,7 @@ func passwordRequiredForEmailChange(ctx context.Context, uid uint, invalid *inva
 		return true
 	}
 	// if an admin is changing another users email no password needed
-	if vc.IsAdmin() {
+	if vc.CanPerformAdminActions() {
 		return false
 	}
 	// should never get here because a non admin can't change the email of another