Make roles for users mandatory (#1338)

* Make roles for users mandatory

* Remove nop migration

* Add missing test for wrong role

* Properly validate global and team roles

* Address codacy issues

* Address codacy review

* No need to check for nil

changes/issue-1292-limit-user-roles

@@ -0,0 +1 @@
+* Make sure that all the global roles are within the expected values. Fixes issue 1292.

server/datastore/datastore_app.go

@@ -2,6 +2,7 @@ package datastore
 
 import (
 	"encoding/json"
+	"github.com/fleetdm/fleet/v4/server/ptr"
 	"sort"
 	"testing"
 
@@ -60,6 +61,7 @@ func testOrgInfo(t *testing.T, ds fleet.Datastore) {
 		Password:   []byte("pass"),
 		Email:      email,
 		SSOEnabled: true,
+		GlobalRole: ptr.String(fleet.RoleAdmin),
 	}
 	_, err = ds.NewUser(u)
 	assert.Nil(t, err)

server/datastore/datastore_email_changes.go

@@ -1,6 +1,7 @@
 package datastore
 
 import (
+	"github.com/fleetdm/fleet/v4/server/ptr"
 	"testing"
 
 	"github.com/fleetdm/fleet/v4/server/fleet"
@@ -13,8 +14,9 @@ func testChangeEmail(t *testing.T, ds fleet.Datastore) {
 		t.Skip("inmem is being deprecated, test skipped")
 	}
 	user := &fleet.User{
-		Password: []byte("foobar"),
-		Email:    "bob@bob.com",
+		Password:   []byte("foobar"),
+		Email:      "bob@bob.com",
+		GlobalRole: ptr.String(fleet.RoleObserver),
 	}
 	user, err := ds.NewUser(user)
 	require.Nil(t, err)
@@ -34,8 +36,9 @@ func testChangeEmail(t *testing.T, ds fleet.Datastore) {
 	err = ds.PendingEmailChange(user.ID, "other@bob.com", "uniquetoken")
 	require.Nil(t, err)
 	otheruser, err := ds.NewUser(&fleet.User{
-		Password: []byte("supersecret"),
-		Email:    "other@bobcom",
+		Password:   []byte("supersecret"),
+		Email:      "other@bobcom",
+		GlobalRole: ptr.String(fleet.RoleObserver),
 	})
 	require.Nil(t, err)
 	_, err = ds.ConfirmPendingEmailChange(otheruser.ID, "uniquetoken")

server/datastore/datastore_invites.go

@@ -18,7 +18,6 @@ func testCreateInvite(t *testing.T, ds fleet.Datastore) {
 	}
 
 	invite := &fleet.Invite{
-
 		Email: "user@foo.com",
 		Name:  "user",
 		Token: "some_user",

server/datastore/datastore_teams.go

@@ -107,7 +107,7 @@ func testTeamUsers(t *testing.T, ds fleet.Datastore) {
 func testTeamListTeams(t *testing.T, ds fleet.Datastore) {
 	users := createTestUsers(t, ds)
 	user1 := fleet.User{Name: users[0].Name, Email: users[0].Email, ID: users[0].ID, GlobalRole: ptr.String(fleet.RoleAdmin)}
-	user2 := fleet.User{Name: users[1].Name, Email: users[1].Email, ID: users[1].ID}
+	user2 := fleet.User{Name: users[1].Name, Email: users[1].Email, ID: users[1].ID, GlobalRole: ptr.String(fleet.RoleObserver)}
 
 	team1, err := ds.NewTeam(&fleet.Team{Name: "team1"})
 	require.NoError(t, err)

server/datastore/datastore_unicode.go

@@ -1,6 +1,7 @@
 package datastore
 
 import (
+	"github.com/fleetdm/fleet/v4/server/ptr"
 	"testing"
 	"time"
 
@@ -39,7 +40,12 @@ func testUnicode(t *testing.T, ds fleet.Datastore) {
 	require.Nil(t, err)
 	assert.Equal(t, "🍌", host.Hostname)
 
-	user, err := ds.NewUser(&fleet.User{Name: "🍱", Email: "test@example.com", Password: []byte{}})
+	user, err := ds.NewUser(&fleet.User{
+		Name:       "🍱",
+		Email:      "test@example.com",
+		Password:   []byte{},
+		GlobalRole: ptr.String(fleet.RoleObserver),
+	})
 	require.Nil(t, err)
 
 	user, err = ds.UserByID(user.ID)

server/datastore/datastore_users.go

@@ -25,6 +25,7 @@ func testCreateUser(t *testing.T, ds fleet.Datastore) {
 			AdminForcedPasswordReset: tt.passwordReset,
 			Email:                    tt.email,
 			SSOEnabled:               tt.sso,
+			GlobalRole:               ptr.String(fleet.RoleObserver),
 		}
 		user, err := ds.NewUser(u)
 		assert.Nil(t, err)
@@ -68,6 +69,7 @@ func createTestUsers(t *testing.T, ds fleet.Datastore) []*fleet.User {
 			Password:                 []byte(tt.password),
 			AdminForcedPasswordReset: tt.passwordReset,
 			Email:                    tt.email,
+			GlobalRole:               ptr.String(fleet.RoleObserver),
 		}
 
 		user, err := ds.NewUser(u)
@@ -237,15 +239,15 @@ func testUserCreateWithTeams(t *testing.T, ds fleet.Datastore) {
 		Teams: []fleet.UserTeam{
 			{
 				Team: fleet.Team{ID: 6},
-				Role: "admin",
+				Role: fleet.RoleObserver,
 			},
 			{
 				Team: fleet.Team{ID: 3},
-				Role: "observer",
+				Role: fleet.RoleObserver,
 			},
 			{
 				Team: fleet.Team{ID: 9},
-				Role: "maintainer",
+				Role: fleet.RoleMaintainer,
 			},
 		},
 	}
@@ -260,7 +262,7 @@ func testUserCreateWithTeams(t *testing.T, ds fleet.Datastore) {
 	assert.Equal(t, uint(3), user.Teams[0].ID)
 	assert.Equal(t, "observer", user.Teams[0].Role)
 	assert.Equal(t, uint(6), user.Teams[1].ID)
-	assert.Equal(t, "admin", user.Teams[1].Role)
+	assert.Equal(t, "observer", user.Teams[1].Role)
 	assert.Equal(t, uint(9), user.Teams[2].ID)
 	assert.Equal(t, "maintainer", user.Teams[2].Role)
 }

server/datastore/mysql/invites.go

@@ -14,6 +14,10 @@ var inviteSearchColumns = []string{"name", "email"}
 
 // NewInvite generates a new invitation.
 func (d *Datastore) NewInvite(i *fleet.Invite) (*fleet.Invite, error) {
+	if err := fleet.ValidateRole(i.GlobalRole.Ptr(), i.Teams); err != nil {
+		return nil, err
+	}
+
 	sqlStmt := `
 	INSERT INTO invites ( invited_by, email, name, position, token, sso_enabled, global_role )
 	  VALUES ( ?, ?, ?, ?, ?, ?, ?)

server/datastore/mysql/users.go

@@ -14,6 +14,10 @@ var userSearchColumns = []string{"name", "email"}
 
 // NewUser creates a new user
 func (d *Datastore) NewUser(user *fleet.User) (*fleet.User, error) {
+	if err := fleet.ValidateRole(user.GlobalRole, user.Teams); err != nil {
+		return nil, err
+	}
+
 	sqlStatement := `
       INSERT INTO users (
       	password,
@@ -115,6 +119,10 @@ func (d *Datastore) UserByID(id uint) (*fleet.User, error) {
 }
 
 func (d *Datastore) SaveUser(user *fleet.User) error {
+	if err := fleet.ValidateRole(user.GlobalRole, user.Teams); err != nil {
+		return err
+	}
+
 	sqlStatement := `
       UPDATE users SET
       	password = ?,

server/fleet/errors.go

@@ -199,3 +199,31 @@ func (e passwordResetRequiredError) Error() string {
 func (e passwordResetRequiredError) StatusCode() int {
 	return http.StatusUnauthorized
 }
+
+// Error is a user facing error (API user). It's meant to be used for errors that are
+// related to fleet logic specifically. Other errors, such as mysql errors, shouldn't
+// be translated to this.
+type Error struct {
+	Code    int    `json:"code,omitempty"`
+	Message string `json:"message,omitempty"`
+}
+
+const (
+	// ErrNoRoleNeeded is the error number number for valid role needed
+	ErrNoRoleNeeded = 1
+)
+
+// NewError returns a fleet error with the code and message specified
+func NewError(code int, message string) error {
+	return &Error{code, message}
+}
+
+// NewErrorf returns a fleet error with the code, and message formatted
+// based on the format string and args specified
+func NewErrorf(code int, format string, args ...interface{}) error {
+	return &Error{code, fmt.Sprintf(format, args...)}
+}
+
+func (ge *Error) Error() string {
+	return ge.Message
+}

server/fleet/teams.go

@@ -143,6 +143,28 @@ func ValidGlobalRoles() []string {
 	return roles
 }
 
+// ValidateRole returns nil if the global and team roles combination is a valid
+// one within fleet, or a fleet Error otherwise.
+func ValidateRole(globalRole *string, teamUsers []UserTeam) error {
+	if globalRole == nil || *globalRole == "" {
+		if len(teamUsers) == 0 {
+			return NewError(ErrNoRoleNeeded, "either global role or team role needs to be defined")
+		}
+		for _, t := range teamUsers {
+			if !ValidTeamRole(t.Role) {
+				return NewError(ErrNoRoleNeeded, "Team roles can be observer or maintainer")
+			}
+		}
+		return nil
+	}
+
+	if !ValidGlobalRole(*globalRole) {
+		return NewError(ErrNoRoleNeeded, "GlobalRole role can only be admin, observer, or maintainer.")
+	}
+
+	return nil
+}
+
 // TeamFilter is the filtering information passed to the datastore for queries
 // that may be filtered by team.
 type TeamFilter struct {

server/service/integration_test.go

@@ -47,10 +47,10 @@ func testDoubleUserCreationErrors(t *testing.T, ds fleet.Datastore) {
 	token := getTestAdminToken(t, server)
 
 	params := fleet.UserPayload{
-		Name:     ptr.String("user1"),
-		Email:    ptr.String("email@asd.com"),
-		Password: ptr.String("pass"),
-		//Teams                    *[]UserTeam `json:"teams,omitempty"`
+		Name:       ptr.String("user1"),
+		Email:      ptr.String("email@asd.com"),
+		Password:   ptr.String("pass"),
+		GlobalRole: ptr.String(fleet.RoleObserver),
 	}
 	j, err := json.Marshal(&params)
 	assert.Nil(t, err)
@@ -72,6 +72,51 @@ func testDoubleUserCreationErrors(t *testing.T, ds fleet.Datastore) {
 	assertBodyContains(t, resp, `Error 1062: Duplicate entry 'email@asd.com'`)
 }
 
+func testUserWithoutRoleErrors(t *testing.T, ds fleet.Datastore) {
+	_, server := runServerForTestsWithDS(t, ds)
+	token := getTestAdminToken(t, server)
+
+	params := fleet.UserPayload{
+		Name:     ptr.String("user1"),
+		Email:    ptr.String("email@asd.com"),
+		Password: ptr.String("pass"),
+	}
+	j, err := json.Marshal(&params)
+	assert.Nil(t, err)
+
+	requestBody := &nopCloser{bytes.NewBuffer(j)}
+	req, _ := http.NewRequest("POST", server.URL+"/api/v1/fleet/users/admin", requestBody)
+	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token))
+	client := &http.Client{}
+	resp, err := client.Do(req)
+	require.Nil(t, err)
+	assert.Equal(t, http.StatusUnprocessableEntity, resp.StatusCode)
+	assertErrorCodeAndMessage(t, resp, fleet.ErrNoRoleNeeded, "either global role or team role needs to be defined")
+}
+
+func testUserWithWrongRoleErrors(t *testing.T, ds fleet.Datastore) {
+	_, server := runServerForTestsWithDS(t, ds)
+	token := getTestAdminToken(t, server)
+
+	params := fleet.UserPayload{
+		Name:       ptr.String("user1"),
+		Email:      ptr.String("email@asd.com"),
+		Password:   ptr.String("pass"),
+		GlobalRole: ptr.String("wrongrole"),
+	}
+	j, err := json.Marshal(&params)
+	assert.Nil(t, err)
+
+	requestBody := &nopCloser{bytes.NewBuffer(j)}
+	req, _ := http.NewRequest("POST", server.URL+"/api/v1/fleet/users/admin", requestBody)
+	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", token))
+	client := &http.Client{}
+	resp, err := client.Do(req)
+	require.Nil(t, err)
+	assert.Equal(t, http.StatusUnprocessableEntity, resp.StatusCode)
+	assertErrorCodeAndMessage(t, resp, fleet.ErrNoRoleNeeded, "GlobalRole role can only be admin, observer, or maintainer.")
+}
+
 func testUserCreationWrongTeamErrors(t *testing.T, ds fleet.Datastore) {
 	_, server := runServerForTestsWithDS(t, ds)
 	token := getTestAdminToken(t, server)
@@ -85,10 +130,11 @@ func testUserCreationWrongTeamErrors(t *testing.T, ds fleet.Datastore) {
 	}
 
 	params := fleet.UserPayload{
-		Name:     ptr.String("user1"),
-		Email:    ptr.String("email@asd.com"),
-		Password: ptr.String("pass"),
-		Teams:    &teams,
+		Name:       ptr.String("user1"),
+		Email:      ptr.String("email@asd.com"),
+		Password:   ptr.String("pass"),
+		GlobalRole: ptr.String(fleet.RoleObserver),
+		Teams:      &teams,
 	}
 	j, err := json.Marshal(&params)
 	assert.Nil(t, err)
@@ -110,6 +156,17 @@ func assertBodyContains(t *testing.T, resp *http.Response, expectedError string)
 	assert.Contains(t, bodyString, expectedError)
 }
 
+func getJSON(r *http.Response, target interface{}) error {
+	return json.NewDecoder(r.Body).Decode(target)
+}
+
+func assertErrorCodeAndMessage(t *testing.T, resp *http.Response, code int, message string) {
+	err := &fleet.Error{}
+	require.Nil(t, getJSON(resp, err))
+	assert.Equal(t, code, err.Code)
+	assert.Equal(t, message, err.Message)
+}
+
 func testAppConfigAdditionalQueriesCanBeRemoved(t *testing.T, ds fleet.Datastore) {
 	_, server := runServerForTestsWithDS(t, ds)
 	token := getTestAdminToken(t, server)
@@ -170,6 +227,8 @@ func TestSQLErrorsAreProperlyHandled(t *testing.T) {
 	mysql.RunTestsAgainstMySQL(t, []func(t *testing.T, ds fleet.Datastore){
 		testDoubleUserCreationErrors,
 		testUserCreationWrongTeamErrors,
+		testUserWithoutRoleErrors,
+		testUserWithWrongRoleErrors,
 		testAppConfigAdditionalQueriesCanBeRemoved,
 	})
 }

server/service/transport_error.go

@@ -18,6 +18,7 @@ type errorer interface {
 
 type jsonError struct {
 	Message string              `json:"message"`
+	Code    int                 `json:"code,omitempty"`
 	Errors  []map[string]string `json:"errors,omitempty"`
 }
 
@@ -128,6 +129,13 @@ func encodeError(ctx context.Context, err error, w http.ResponseWriter) {
 		}
 		w.WriteHeader(statusCode)
 		enc.Encode(je)
+	case *fleet.Error:
+		je := jsonError{
+			Message: e.Error(),
+			Code:    e.Code,
+		}
+		w.WriteHeader(http.StatusUnprocessableEntity)
+		enc.Encode(je)
 	default:
 		if fleet.IsForeignKey(errors.Cause(err)) {
 			ve := jsonError{

server/test/new_objects.go

@@ -111,11 +111,16 @@ func NewHost(t *testing.T, ds fleet.Datastore, name, ip, key, uuid string, now t
 }
 
 func NewUser(t *testing.T, ds fleet.Datastore, name, email string, admin bool) *fleet.User {
+	role := fleet.RoleObserver
+	if admin {
+		role = fleet.RoleAdmin
+	}
 	u, err := ds.NewUser(&fleet.User{
-		Password: []byte("garbage"),
-		Salt:     "garbage",
-		Name:     name,
-		Email:    email,
+		Password:   []byte("garbage"),
+		Salt:       "garbage",
+		Name:       name,
+		Email:      email,
+		GlobalRole: &role,
 	})
 
 	require.Nil(t, err)