diff --git a/changes/issue-1292-limit-user-roles b/changes/issue-1292-limit-user-roles new file mode 100644 index 0000000000..273c4f6a68 --- /dev/null +++ b/changes/issue-1292-limit-user-roles @@ -0,0 +1 @@ +* Make sure that all the global roles are within the expected values. Fixes issue 1292. \ No newline at end of file diff --git a/server/datastore/datastore_app.go b/server/datastore/datastore_app.go index 0e533f9c94..517a4cda35 100644 --- a/server/datastore/datastore_app.go +++ b/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) diff --git a/server/datastore/datastore_email_changes.go b/server/datastore/datastore_email_changes.go index 9f5b784e01..7c13faff1f 100644 --- a/server/datastore/datastore_email_changes.go +++ b/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") diff --git a/server/datastore/datastore_invites.go b/server/datastore/datastore_invites.go index 6a1b90c6b3..1a69899518 100644 --- a/server/datastore/datastore_invites.go +++ b/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", diff --git a/server/datastore/datastore_teams.go b/server/datastore/datastore_teams.go index 91d8efe814..f45a5eb13d 100644 --- a/server/datastore/datastore_teams.go +++ b/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) diff --git a/server/datastore/datastore_unicode.go b/server/datastore/datastore_unicode.go index 1ca70d2a43..3e702e1244 100644 --- a/server/datastore/datastore_unicode.go +++ b/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) diff --git a/server/datastore/datastore_users.go b/server/datastore/datastore_users.go index 4ef8737603..ad4fb1fbd5 100644 --- a/server/datastore/datastore_users.go +++ b/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) } diff --git a/server/datastore/mysql/invites.go b/server/datastore/mysql/invites.go index 96e07be98a..a3b33afc86 100644 --- a/server/datastore/mysql/invites.go +++ b/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 ( ?, ?, ?, ?, ?, ?, ?) diff --git a/server/datastore/mysql/users.go b/server/datastore/mysql/users.go index 679f57586a..fc4f9e6e39 100644 --- a/server/datastore/mysql/users.go +++ b/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 = ?, diff --git a/server/fleet/errors.go b/server/fleet/errors.go index b4f3374208..01d7aba39d 100644 --- a/server/fleet/errors.go +++ b/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 +} diff --git a/server/fleet/teams.go b/server/fleet/teams.go index bdbbdf8bc7..6c398c1c82 100644 --- a/server/fleet/teams.go +++ b/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 { diff --git a/server/service/integration_test.go b/server/service/integration_test.go index 3c11c25f2a..6e1a947dd7 100644 --- a/server/service/integration_test.go +++ b/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(¶ms) 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(¶ms) + 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(¶ms) + 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(¶ms) 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, }) } diff --git a/server/service/transport_error.go b/server/service/transport_error.go index 7d088aa67a..212f8e8f1d 100644 --- a/server/service/transport_error.go +++ b/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{ diff --git a/server/test/new_objects.go b/server/test/new_objects.go index 8ea008f2e3..ff6b008383 100644 --- a/server/test/new_objects.go +++ b/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)