From 05100753986f4fa89d6937bd08794b0244a47167 Mon Sep 17 00:00:00 2001 From: Tomas Touceda Date: Wed, 13 Oct 2021 12:34:59 -0300 Subject: [PATCH] Issue 2134 add team admin role (#2499) * wip * Add team admin role and tests * Revert change in invites * Update permission doc * Fix lint --- changes/issue-2134-add-team-admin-role | 1 + docs/01-Using-Fleet/09-Permissions.md | 46 +-- ee/server/service/service_teams.go | 9 + server/authz/policy.rego | 105 +++--- server/service/global_policies_test.go | 6 + .../service/service_global_schedule_test.go | 6 + server/service/service_invites_test.go | 108 ++++++ .../service/service_scheduled_queries_test.go | 4 +- server/service/service_teams_test.go | 149 +++++++++ server/service/service_users.go | 76 ++++- server/service/service_users_test.go | 310 ++++++++---------- server/service/team_policies_test.go | 12 + server/service/team_schedule_test.go | 12 + 13 files changed, 597 insertions(+), 247 deletions(-) create mode 100644 changes/issue-2134-add-team-admin-role create mode 100644 server/service/service_teams_test.go diff --git a/changes/issue-2134-add-team-admin-role b/changes/issue-2134-add-team-admin-role new file mode 100644 index 0000000000..b9827f48ea --- /dev/null +++ b/changes/issue-2134-add-team-admin-role @@ -0,0 +1 @@ +* Add Team Admin role. diff --git a/docs/01-Using-Fleet/09-Permissions.md b/docs/01-Using-Fleet/09-Permissions.md index aadc0c7075..e37fbad0f0 100644 --- a/docs/01-Using-Fleet/09-Permissions.md +++ b/docs/01-Using-Fleet/09-Permissions.md @@ -67,23 +67,29 @@ Users that are members of multiple teams can be assigned different roles for eac The following table depicts various permissions levels in a team. -| Action | Observer | Maintainer | -| ------------------------------------------------------------ | -------- | ---------- | -| Browse hosts assigned to team | ✅ | ✅ | -| Browse policies for hosts assigned to team | ✅ | ✅ | -| Filter hosts assigned to team using policies | ✅ | ✅ | -| Filter hosts assigned to team using labels | ✅ | ✅ | -| Target hosts assigned to team using labels | ✅ | ✅ | -| Run saved queries as live queries on hosts assigned to team | ✅ | ✅ | -| Run custom queries as live queries on hosts assigned to team | | ✅ | -| Enroll hosts to member team | | ✅ | -| Delete hosts belonging to member team | | ✅ | -| Edit queries they authored | | ✅ | -| Delete queries they authored | | ✅ | -| Create new team schedules | | ✅ | -| Delete team schedules | | ✅ | -| Browse global schedules | | ✅ | -| Create new team policies | | ✅ | -| Delete team policies | | ✅ | -| Browse global policies | | ✅ | - +| Action | Observer | Maintainer | Admin | +| ------------------------------------------------------------ | -------- | ---------- | ------- | +| Browse hosts assigned to team | ✅ | ✅ | ✅ | +| Browse policies for hosts assigned to team | ✅ | ✅ | ✅ | +| Filter hosts assigned to team using policies | ✅ | ✅ | ✅ | +| Filter hosts assigned to team using labels | ✅ | ✅ | ✅ | +| Target hosts assigned to team using labels | ✅ | ✅ | ✅ | +| Run saved queries as live queries on hosts assigned to team | ✅ | ✅ | ✅ | +| Run custom queries as live queries on hosts assigned to team | | ✅ | ✅ | +| Enroll hosts to member team | | ✅ | ✅ | +| Delete hosts belonging to member team | | ✅ | ✅ | +| Create saved queries | | ✅ | ✅ | +| Edit queries they authored | | ✅ | ✅ | +| Delete queries they authored | | ✅ | ✅ | +| Create new team schedules | | ✅ | ✅ | +| Delete team schedules | | ✅ | ✅ | +| Browse global schedules | | ✅ | ✅ | +| Create new team policies | | ✅ | ✅ | +| Delete team policies | | ✅ | ✅ | +| Browse global policies | | ✅ | ✅ | +| Create enroll secrets that belong to team | | | ✅ | +| Edit enroll secrets that belong to team | | | ✅ | +| Delete enroll secrets that belong to team | | | ✅ | +| Edit users assigned to team | | | ✅ | +| Remove users assigned to team | | | ✅ | +| Edit team level agent options | | | ✅ | \ No newline at end of file diff --git a/ee/server/service/service_teams.go b/ee/server/service/service_teams.go index 97b980c70c..f5710e042f 100644 --- a/ee/server/service/service_teams.go +++ b/ee/server/service/service_teams.go @@ -116,12 +116,21 @@ func (svc *Service) AddTeamUsers(ctx context.Context, teamID uint, users []fleet return nil, err } + currentUser := authz.UserFromContext(ctx) + idMap := make(map[uint]fleet.TeamUser) for _, user := range users { if !fleet.ValidTeamRole(user.Role) { return nil, fleet.NewInvalidArgumentError("users", fmt.Sprintf("%s is not a valid role for a team user", user.Role)) } idMap[user.ID] = user + fullUser, err := svc.ds.UserByID(ctx, user.ID) + if err != nil { + return nil, errors.Wrapf(err, "getting full user with id %d", user.ID) + } + if fullUser.GlobalRole != nil && currentUser.GlobalRole == nil { + return nil, errors.New("A user with a global role cannot be added to a team by a non global user.") + } } team, err := svc.ds.Team(ctx, teamID) diff --git a/server/authz/policy.rego b/server/authz/policy.rego index ca26a4ee57..d61bf728f5 100644 --- a/server/authz/policy.rego +++ b/server/authz/policy.rego @@ -56,12 +56,27 @@ allow { ## # Any logged in user can read teams (service must filter appropriately based on -# access). +# access) if the overall object is specified allow { object.type == "team" + object.id == 0 not is_null(subject) action == read } +# For specific teams, only members can read +allow { + object.type == "team" + object.id != 0 + team_role(subject, object.id) == [admin,maintainer][_] + action == read +} +# or global admins +allow { + object.type == "team" + object.id != 0 + subject.global_role == admin + action == read +} # Admin can write teams allow { @@ -70,6 +85,13 @@ allow { action == write } +# Team admin can write teams +allow { + object.type == "team" + team_role(subject, object.id) == admin + action == write +} + ## # Users ## @@ -78,7 +100,8 @@ allow { allow { object.type == "user" object.id == subject.id - action == write + object.id != 0 + action == write } # Any user can read other users @@ -92,23 +115,25 @@ allow { allow { object.type == "user" subject.global_role == admin - action == [write, write_role][_] + action == [write, write_role][_] +} + +## Team admins can create or edit new users +allow { + object.type == "user" + team_role(subject, object.teams[_].id) == admin + action == [write, write_role][_] } ## # Invites ## -# Only global admins may read/write invites +# Global admins may read/write invites allow { object.type == "invite" subject.global_role == admin - action == read -} -allow { - object.type == "invite" - subject.global_role == admin - action == write + action == [read,write][_] } ## @@ -158,10 +183,10 @@ allow { action == read } -# Team maintainers can read for appropriate teams +# Team admins and maintainers can read for appropriate teams allow { object.type == "enroll_secret" - team_role(subject, object.team_id) == maintainer + team_role(subject, object.team_id) == [admin, maintainer][_] action == read } @@ -197,22 +222,17 @@ allow { action == read } -# Allow read for matching team maintainer/observer +# Allow read for matching team admin/maintainer/observer allow { object.type == "host" - team_role(subject, object.team_id) == maintainer - action == read -} -allow { - object.type == "host" - team_role(subject, object.team_id) == observer + team_role(subject, object.team_id) == [admin, maintainer, observer][_] action == read } -# Team maintainers can write to hosts of their own team +# Team admins and maintainers can write to hosts of their own team allow { object.type == "host" - team_role(subject, object.team_id) == maintainer + team_role(subject, object.team_id) == [admin,maintainer][_] action == write } @@ -250,7 +270,7 @@ allow { action == read } -# Only admins and maintainers can write queries +# Global admins and maintainers can write queries allow { object.type == "query" subject.global_role == admin @@ -262,19 +282,19 @@ allow { action == write } -# Team maintainers can create new queries +# Team admins and maintainers can create new queries allow { object.id == 0 # new queries have ID zero object.type == "query" - team_role(subject, subject.teams[_].id) == maintainer + team_role(subject, subject.teams[_].id) == [admin, maintainer][_] action == write } -# Team maintainers can edit and delete only their own queries +# Team admins and maintainers can edit and delete only their own queries allow { object.author_id == subject.id object.type == "query" - team_role(subject, subject.teams[_].id) == maintainer + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] action == write } @@ -299,28 +319,20 @@ allow { subject.global_role == maintainer action = run_new } -# Team maintainer running a non-observers_can_run query must have the targets +# Team admin and maintainer running a non-observers_can_run query must have the targets # filtered to only teams that they maintain allow { object.type == "query" # If role is maintainer on any team - team_role(subject, subject.teams[_].id) == maintainer + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] action == run } -# Team maintainer can run a new query +# Team admin and maintainer can run a new query allow { object.type == "query" - # If role is maintainer on any team - team_role(subject, subject.teams[_].id) == maintainer - action == run_new -} - -# Team admin can run a new query -allow { - object.type == "query" - # If role is maintainer on any team - team_role(subject, subject.teams[_].id) == admin + # If role is admin or maintainer on any team + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] action == run_new } @@ -369,18 +381,19 @@ allow { action == [read, write][_] } -# Team maintainers can read global packs +# Team admins and maintainers can read global packs allow { is_null(object.team_ids) object.type == "pack" - team_role(subject, subject.teams[_].id) == maintainer + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] action == read } +# Team admins and maintainers can read their team packs allow { object.team_ids[_] == subject.teams[_].id object.type == "pack" - team_role(subject, subject.teams[_].id) == maintainer + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] action == [read, write][_] } @@ -426,21 +439,21 @@ allow { action == [read][_] } -# Team Maintainers can read and write policies +# Team admin and maintainers can read and write policies allow { not is_null(object.team_id) object.team_id == subject.teams[_].id object.type == "policy" - team_role(subject, subject.teams[_].id) == maintainer + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] action == [read, write][_] } -# Team maintainers can read global policies +# Team admin and maintainers can read global policies allow { is_null(object.team_id) object.type == "policy" - team_role(subject, subject.teams[_].id) == maintainer + team_role(subject, subject.teams[_].id) == [admin,maintainer][_] action == read } diff --git a/server/service/global_policies_test.go b/server/service/global_policies_test.go index 231c486a4a..27064762aa 100644 --- a/server/service/global_policies_test.go +++ b/server/service/global_policies_test.go @@ -51,6 +51,12 @@ func TestGlobalPoliciesAuth(t *testing.T) { true, false, }, + { + "team admin", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + true, + false, + }, { "team maintainer", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}}, diff --git a/server/service/service_global_schedule_test.go b/server/service/service_global_schedule_test.go index f6041cb9db..ce6d92be11 100644 --- a/server/service/service_global_schedule_test.go +++ b/server/service/service_global_schedule_test.go @@ -45,6 +45,12 @@ func TestGlobalScheduleAuth(t *testing.T) { true, true, }, + { + "team admin", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + true, + false, + }, { "team maintainer", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}}, diff --git a/server/service/service_invites_test.go b/server/service/service_invites_test.go index 9e1e6b688f..746ebf4847 100644 --- a/server/service/service_invites_test.go +++ b/server/service/service_invites_test.go @@ -8,12 +8,14 @@ import ( "github.com/WatchBeam/clock" "github.com/fleetdm/fleet/v4/server/authz" "github.com/fleetdm/fleet/v4/server/config" + "github.com/fleetdm/fleet/v4/server/contexts/viewer" "github.com/fleetdm/fleet/v4/server/fleet" "github.com/fleetdm/fleet/v4/server/mock" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/fleetdm/fleet/v4/server/test" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/guregu/null.v3" ) func TestInviteNewUserMock(t *testing.T) { @@ -99,3 +101,109 @@ func TestListInvites(t *testing.T) { require.Nil(t, err) assert.True(t, ms.ListInvitesFuncInvoked) } + +func TestInvitesAuth(t *testing.T) { + ds := new(mock.Store) + svc := newTestService(ds, nil, nil) + + ds.ListInvitesFunc = func(context.Context, fleet.ListOptions) ([]*fleet.Invite, error) { + return nil, nil + } + ds.DeleteInviteFunc = func(context.Context, uint) error { return nil } + ds.UserByEmailFunc = func(ctx context.Context, email string) (*fleet.User, error) { + return nil, ¬FoundError{} + } + ds.NewInviteFunc = func(ctx context.Context, i *fleet.Invite) (*fleet.Invite, error) { + return &fleet.Invite{}, nil + } + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &fleet.AppConfig{}, nil + } + var testCases = []struct { + name string + user *fleet.User + shouldFailWrite bool + shouldFailRead bool + }{ + { + "global admin", + &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}, + false, + false, + }, + { + "global maintainer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleMaintainer)}, + true, + true, + }, + { + "global observer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleObserver)}, + true, + true, + }, + { + "team admin, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + true, + true, + }, + { + "team maintainer, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}}, + true, + true, + }, + { + "team observer, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, + true, + true, + }, + { + "team maintainer, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleMaintainer}}}, + true, + true, + }, + { + "team admin, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleAdmin}}}, + true, + true, + }, + { + "team observer, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}}}, + true, + true, + }, + } + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + ctx := viewer.NewContext(context.Background(), viewer.Viewer{User: tt.user}) + + _, err := svc.InviteNewUser(ctx, fleet.InvitePayload{ + Email: ptr.String("e@mail.com"), + Name: ptr.String("name"), + Position: ptr.String("someposition"), + SSOEnabled: ptr.Bool(false), + GlobalRole: null.StringFromPtr(tt.user.GlobalRole), + Teams: []fleet.UserTeam{ + { + Team: fleet.Team{ID: 1}, + Role: fleet.RoleMaintainer, + }, + }, + }) + checkAuthErr(t, tt.shouldFailWrite, err) + + _, err = svc.ListInvites(ctx, fleet.ListOptions{}) + checkAuthErr(t, tt.shouldFailRead, err) + + err = svc.DeleteInvite(ctx, 99) + checkAuthErr(t, tt.shouldFailWrite, err) + }) + } +} diff --git a/server/service/service_scheduled_queries_test.go b/server/service/service_scheduled_queries_test.go index ed42675f34..1f96c101f9 100644 --- a/server/service/service_scheduled_queries_test.go +++ b/server/service/service_scheduled_queries_test.go @@ -48,7 +48,7 @@ func TestScheduleQueryNoName(t *testing.T) { ds.ListScheduledQueriesInPackFunc = func(ctx context.Context, id uint, opts fleet.ListOptions) ([]*fleet.ScheduledQuery, error) { // No matching query return []*fleet.ScheduledQuery{ - &fleet.ScheduledQuery{ + { Name: "froobling", }, }, nil @@ -83,7 +83,7 @@ func TestScheduleQueryNoNameMultiple(t *testing.T) { ds.ListScheduledQueriesInPackFunc = func(ctx context.Context, id uint, opts fleet.ListOptions) ([]*fleet.ScheduledQuery, error) { // No matching query return []*fleet.ScheduledQuery{ - &fleet.ScheduledQuery{ + { Name: "foobar", }, }, nil diff --git a/server/service/service_teams_test.go b/server/service/service_teams_test.go new file mode 100644 index 0000000000..346a6289b3 --- /dev/null +++ b/server/service/service_teams_test.go @@ -0,0 +1,149 @@ +package service + +import ( + "context" + "testing" + "time" + + "github.com/fleetdm/fleet/v4/server/contexts/viewer" + "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/mock" + "github.com/fleetdm/fleet/v4/server/ptr" +) + +func TestTeamAuth(t *testing.T) { + ds := new(mock.Store) + license := &fleet.LicenseInfo{Tier: fleet.TierPremium, Expiration: time.Now().Add(24 * time.Hour)} + svc := newTestService(ds, nil, nil, TestServerOpts{License: license, SkipCreateTestUsers: true}) + + ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) { + return &fleet.AppConfig{}, nil + } + ds.NewTeamFunc = func(ctx context.Context, team *fleet.Team) (*fleet.Team, error) { + return &fleet.Team{}, nil + } + ds.NewActivityFunc = func(ctx context.Context, user *fleet.User, activityType string, details *map[string]interface{}) error { + return nil + } + ds.TeamFunc = func(ctx context.Context, tid uint) (*fleet.Team, error) { + return &fleet.Team{}, nil + } + ds.SaveTeamFunc = func(ctx context.Context, team *fleet.Team) (*fleet.Team, error) { + return &fleet.Team{}, nil + } + ds.ListUsersFunc = func(ctx context.Context, opt fleet.UserListOptions) ([]*fleet.User, error) { + return nil, nil + } + ds.ListTeamsFunc = func(ctx context.Context, filter fleet.TeamFilter, opt fleet.ListOptions) ([]*fleet.Team, error) { + return nil, nil + } + ds.DeleteTeamFunc = func(ctx context.Context, tid uint) error { + return nil + } + ds.TeamEnrollSecretsFunc = func(ctx context.Context, teamID uint) ([]*fleet.EnrollSecret, error) { + return nil, nil + } + var testCases = []struct { + name string + user *fleet.User + shouldFailTeamWrite bool + shouldFailGlobalWrite bool + shouldFailRead bool + }{ + { + "global admin", + &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}, + false, + false, + false, + }, + { + "global maintainer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleMaintainer)}, + true, + true, + true, + }, + { + "global observer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleObserver)}, + true, + true, + true, + }, + { + "team admin, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + false, + true, + false, + }, + { + "team maintainer, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}}, + true, + true, + false, + }, + { + "team observer, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, + true, + true, + true, + }, + { + "team admin, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleAdmin}}}, + true, + true, + true, + }, + { + "team maintainer, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleMaintainer}}}, + true, + true, + true, + }, + { + "team observer, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}}}, + true, + true, + true, + }, + } + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + ctx := viewer.NewContext(context.Background(), viewer.Viewer{User: tt.user}) + + _, err := svc.NewTeam(ctx, fleet.TeamPayload{Name: ptr.String("name")}) + checkAuthErr(t, tt.shouldFailGlobalWrite, err) + + _, err = svc.ModifyTeam(ctx, 1, fleet.TeamPayload{Name: ptr.String("othername")}) + checkAuthErr(t, tt.shouldFailTeamWrite, err) + + _, err = svc.ModifyTeamAgentOptions(ctx, 1, nil) + checkAuthErr(t, tt.shouldFailTeamWrite, err) + + _, err = svc.AddTeamUsers(ctx, 1, []fleet.TeamUser{}) + checkAuthErr(t, tt.shouldFailTeamWrite, err) + + _, err = svc.DeleteTeamUsers(ctx, 1, []fleet.TeamUser{}) + checkAuthErr(t, tt.shouldFailTeamWrite, err) + + _, err = svc.ListTeamUsers(ctx, 1, fleet.ListOptions{}) + checkAuthErr(t, tt.shouldFailRead, err) + + _, err = svc.ListTeams(ctx, fleet.ListOptions{}) + checkAuthErr(t, false, err) // everybody can do this + + err = svc.DeleteTeam(ctx, 1) + checkAuthErr(t, tt.shouldFailTeamWrite, err) + + _, err = svc.TeamEnrollSecrets(ctx, 1) + checkAuthErr(t, tt.shouldFailRead, err) + }) + } +} diff --git a/server/service/service_users.go b/server/service/service_users.go index 2251ffd0d1..be3c203642 100644 --- a/server/service/service_users.go +++ b/server/service/service_users.go @@ -7,6 +7,7 @@ import ( "time" "github.com/fleetdm/fleet/v4/server" + "github.com/fleetdm/fleet/v4/server/authz" "github.com/fleetdm/fleet/v4/server/contexts/viewer" "github.com/fleetdm/fleet/v4/server/fleet" @@ -42,7 +43,11 @@ func (svc *Service) CreateUserFromInvite(ctx context.Context, p fleet.UserPayloa } func (svc *Service) CreateUser(ctx context.Context, p fleet.UserPayload) (*fleet.User, error) { - if err := svc.authz.Authorize(ctx, &fleet.User{}, fleet.ActionWrite); err != nil { + var teams []fleet.UserTeam + if p.Teams != nil { + teams = *p.Teams + } + if err := svc.authz.Authorize(ctx, &fleet.User{Teams: teams}, fleet.ActionWrite); err != nil { return nil, err } @@ -96,27 +101,25 @@ func (svc *Service) newUser(ctx context.Context, p fleet.UserPayload) (*fleet.Us return user, nil } -func (svc *Service) ChangeUserAdmin(ctx context.Context, id uint, isAdmin bool) (*fleet.User, error) { - // TODO remove this function - return nil, errors.New("This function is being eliminated") -} - func (svc *Service) ModifyUser(ctx context.Context, userID uint, p fleet.UserPayload) (*fleet.User, error) { - if err := svc.authz.Authorize(ctx, &fleet.User{ID: userID}, fleet.ActionWrite); err != nil { + if err := svc.authz.Authorize(ctx, &fleet.User{}, fleet.ActionRead); err != nil { return nil, err } - if p.GlobalRole != nil || p.Teams != nil { - if err := svc.authz.Authorize(ctx, &fleet.User{ID: userID}, fleet.ActionWriteRole); err != nil { - return nil, err - } - } - user, err := svc.User(ctx, userID) if err != nil { return nil, err } + if err := svc.authz.Authorize(ctx, user, fleet.ActionWrite); err != nil { + return nil, err + } + + if p.GlobalRole != nil || p.Teams != nil { + if err := svc.authz.Authorize(ctx, user, fleet.ActionWriteRole); err != nil { + return nil, err + } + } if p.Name != nil { user.Name = *p.Name } @@ -140,13 +143,22 @@ func (svc *Service) ModifyUser(ctx context.Context, userID uint, p fleet.UserPay user.SSOEnabled = *p.SSOEnabled } + currentUser := authz.UserFromContext(ctx) + if p.GlobalRole != nil && *p.GlobalRole != "" { + if currentUser.GlobalRole == nil { + return nil, errors.New("Cannot edit global role as a team member") + } + if p.Teams != nil && len(*p.Teams) > 0 { return nil, fleet.NewInvalidArgumentError("teams", "may not be specified with global_role") } user.GlobalRole = p.GlobalRole user.Teams = []fleet.UserTeam{} } else if p.Teams != nil { + if !isAdminOfTheModifiedTeams(currentUser, user.Teams, *p.Teams) { + return nil, errors.New("Cannot modify teams in that way") + } user.Teams = *p.Teams user.GlobalRole = nil } @@ -159,6 +171,44 @@ func (svc *Service) ModifyUser(ctx context.Context, userID uint, p fleet.UserPay return user, nil } +func isAdminOfTheModifiedTeams(currentUser *fleet.User, originalUserTeams, newUserTeams []fleet.UserTeam) bool { + // If the user is of the right global role, then they can modify the teams + if currentUser.GlobalRole != nil && (*currentUser.GlobalRole == fleet.RoleAdmin || *currentUser.GlobalRole == fleet.RoleMaintainer) { + return true + } + + // otherwise, gather the resulting teams + resultingTeams := make(map[uint]string) + for _, team := range newUserTeams { + resultingTeams[team.ID] = team.Role + } + + // and see which ones were removed or changed from the original + teamsAffected := make(map[uint]struct{}) + for _, team := range originalUserTeams { + if resultingTeams[team.ID] != team.Role { + teamsAffected[team.ID] = struct{}{} + } + } + + // then gather the teams the current user is admin for + currentUserTeamAdmin := make(map[uint]struct{}) + for _, team := range currentUser.Teams { + if team.Role == fleet.RoleAdmin { + currentUserTeamAdmin[team.ID] = struct{}{} + } + } + + // and let's check that the teams that were either removed or changed are also teams this user is an admin of + for teamID := range teamsAffected { + if _, ok := currentUserTeamAdmin[teamID]; !ok { + return false + } + } + + return true +} + func (svc *Service) modifyEmailAddress(ctx context.Context, user *fleet.User, email string, password *string) error { // password requirement handled in validation middleware if password != nil { diff --git a/server/service/service_users_test.go b/server/service/service_users_test.go index ddb7069db6..dffb656cf1 100644 --- a/server/service/service_users_test.go +++ b/server/service/service_users_test.go @@ -3,16 +3,16 @@ package service import ( "context" "database/sql" + "fmt" "testing" "time" "github.com/fleetdm/fleet/v4/server/contexts/viewer" "github.com/fleetdm/fleet/v4/server/datastore/mysql" "github.com/fleetdm/fleet/v4/server/fleet" + "github.com/fleetdm/fleet/v4/server/mock" "github.com/fleetdm/fleet/v4/server/ptr" "github.com/fleetdm/fleet/v4/server/test" - - "github.com/fleetdm/fleet/v4/server/mock" "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -220,170 +220,6 @@ func TestModifyAdminUserEmailPassword(t *testing.T) { } -// func TestRequestPasswordReset(t *testing.T) { -// ds, err := inmem.New(config.TestConfig()) -// require.Nil(t, err) -// createTestAppConfig(t, ds) - -// createTestUsers(t, ds) -// admin1, err := ds.User("admin1") -// assert.Nil(t, err) -// user1, err := ds.User("user1") -// assert.Nil(t, err) -// var defaultEmailFn = func(e fleet.Email) error { -// return nil -// } -// var errEmailFn = func(e fleet.Email) error { -// return errors.New("test err") -// } -// authz, err := authz.NewAuthorizer() -// require.NoError(t, err) -// svc := service{ -// ds: ds, -// config: config.TestConfig(), -// authz: authz, -// } - -// var requestPasswordResetTests = []struct { -// email string -// emailFn func(e fleet.Email) error -// wantErr error -// user *fleet.User -// vc *viewer.Viewer -// }{ -// { -// email: admin1.Email, -// emailFn: defaultEmailFn, -// user: admin1, -// vc: &viewer.Viewer{User: admin1}, -// }, -// { -// email: admin1.Email, -// emailFn: defaultEmailFn, -// user: admin1, -// vc: nil, -// }, -// { -// email: user1.Email, -// emailFn: defaultEmailFn, -// user: user1, -// vc: &viewer.Viewer{User: admin1}, -// }, -// { -// email: admin1.Email, -// emailFn: errEmailFn, -// user: user1, -// vc: nil, -// wantErr: errors.New("test err"), -// }, -// } - -// for _, tt := range requestPasswordResetTests { -// t.Run("", func(t *testing.T) { -// ctx := context.Background() -// if tt.vc != nil { -// ctx = viewer.NewContext(ctx, *tt.vc) -// } -// mailer := &mockMailService{SendEmailFn: tt.emailFn} -// svc.mailService = mailer -// serviceErr := svc.RequestPasswordReset(ctx, tt.email) -// assert.Equal(t, tt.wantErr, serviceErr) -// assert.True(t, mailer.Invoked, "email should be sent if vc is not admin") -// if serviceErr == nil { -// req, err := ds.FindPassswordResetsByUserID(tt.user.ID) -// assert.Nil(t, err) -// assert.NotEmpty(t, req, "user should have at least one password reset request") -// } -// }) -// } -// } - -// func TestCreateUserFromInvite(t *testing.T) { -// ds, _ := inmem.New(config.TestConfig()) -// svc, _ := newTestService(ds, nil, nil) -// invites := setupInvites(t, ds, []string{"admin2@example.com", "admin3@example.com"}) -// ctx := context.Background() - -// var newUserTests = []struct { -// Username *string -// Password *string -// Email *string -// NeedsPasswordReset *bool -// InviteToken *string -// wantErr error -// }{ -// { -// Username: ptr.String("admin2"), -// Password: ptr.String("foobarbaz1234!"), -// InviteToken: &invites["admin2@example.com"].Token, -// wantErr: &invalidArgumentError{invalidArgument{name: "email", reason: "missing required argument"}}, -// }, -// { -// Username: ptr.String("admin2"), -// Password: ptr.String("foobarbaz1234!"), -// Email: ptr.String("admin2@example.com"), -// wantErr: &invalidArgumentError{invalidArgument{name: "invite_token", reason: "missing required argument"}}, -// }, -// { -// Username: ptr.String("admin2"), -// Password: ptr.String("foobarbaz1234!"), -// Email: ptr.String("admin2@example.com"), -// NeedsPasswordReset: ptr.Bool(true), -// InviteToken: &invites["admin2@example.com"].Token, -// }, -// { // should return ErrNotFound because the invite is deleted -// // after a user signs up -// Username: ptr.String("admin2"), -// Password: ptr.String("foobarbaz1234!"), -// Email: ptr.String("admin2@example.com"), -// NeedsPasswordReset: ptr.Bool(true), -// InviteToken: &invites["admin2@example.com"].Token, -// wantErr: errors.New("Invite with token admin2@example.com was not found in the datastore"), -// }, -// { -// Username: ptr.String("admin3"), -// Password: ptr.String("foobarbaz1234!"), -// Email: &invites["expired"].Email, -// NeedsPasswordReset: ptr.Bool(true), -// InviteToken: &invites["expired"].Token, -// wantErr: &invalidArgumentError{{name: "invite_token", reason: "Invite token has expired."}}, -// }, -// { -// Username: ptr.String("admin3@example.com"), -// Password: ptr.String("foobarbaz1234!"), -// Email: ptr.String("admin3@example.com"), -// NeedsPasswordReset: ptr.Bool(true), -// InviteToken: &invites["admin3@example.com"].Token, -// }, -// } - -// for _, tt := range newUserTests { -// t.Run("", func(t *testing.T) { -// payload := fleet.UserPayload{ -// Username: tt.Username, -// Password: tt.Password, -// Email: tt.Email, -// InviteToken: tt.InviteToken, -// } -// user, err := svc.CreateUserFromInvite(ctx, payload) -// if tt.wantErr != nil { -// require.Error(t, err) -// assert.Equal(t, tt.wantErr.Error(), err.Error()) -// return -// } -// require.NoError(t, err) -// assert.NotZero(t, user.ID) - -// err = user.ValidatePassword(*tt.Password) -// assert.Nil(t, err) - -// err = user.ValidatePassword("different_password") -// assert.NotNil(t, err) -// }) - -// } -// } - func TestChangePassword(t *testing.T) { ds := mysql.CreateMySQLDS(t) defer ds.Close() @@ -647,3 +483,145 @@ func TestUserPasswordRequirements(t *testing.T) { }) } } + +func TestUserAuth(t *testing.T) { + ds := new(mock.Store) + svc := newTestService(ds, nil, nil) + + ds.InviteByTokenFunc = func(ctx context.Context, token string) (*fleet.Invite, error) { + return &fleet.Invite{ + Email: "some@email.com", + Token: "ABCD", + UpdateCreateTimestamps: fleet.UpdateCreateTimestamps{ + CreateTimestamp: fleet.CreateTimestamp{CreatedAt: time.Now()}, + UpdateTimestamp: fleet.UpdateTimestamp{UpdatedAt: time.Now()}}, + }, nil + } + ds.NewUserFunc = func(ctx context.Context, user *fleet.User) (*fleet.User, error) { + return &fleet.User{}, nil + } + ds.DeleteInviteFunc = func(ctx context.Context, id uint) error { + return nil + } + ds.InviteByEmailFunc = func(ctx context.Context, email string) (*fleet.Invite, error) { + return nil, fmt.Errorf("AA") + } + ds.UserByIDFunc = func(ctx context.Context, id uint) (*fleet.User, error) { + if id == 999 { + return &fleet.User{ + ID: 999, + Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}, + }, nil + } + return &fleet.User{ + ID: 888, + GlobalRole: ptr.String(fleet.RoleMaintainer), + }, nil + } + ds.SaveUserFunc = func(ctx context.Context, user *fleet.User) error { + return nil + } + + var testCases = []struct { + name string + user *fleet.User + shouldFailGlobalWrite bool + shouldFailTeamWrite bool + shouldFailRead bool + }{ + { + "global admin", + &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}, + false, + false, + false, + }, + { + "global maintainer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleMaintainer)}, + true, + true, + true, + }, + { + "global observer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleObserver)}, + true, + true, + true, + }, + { + "team admin, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + true, + false, + false, + }, + { + "team maintainer, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}}, + true, + true, + false, + }, + { + "team observer, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleObserver}}}, + true, + true, + true, + }, + { + "team maintainer, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleMaintainer}}}, + true, + true, + true, + }, + { + "team admin, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleAdmin}}}, + true, + true, + true, + }, + { + "team observer, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}}}, + true, + true, + true, + }, + } + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + ctx := viewer.NewContext(context.Background(), viewer.Viewer{User: tt.user}) + + teams := []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}} + _, err := svc.CreateUser(ctx, fleet.UserPayload{ + Name: ptr.String("Some Name"), + Email: ptr.String("some@email.com"), + Password: ptr.String("passw0rd."), + Teams: &teams, + }) + checkAuthErr(t, tt.shouldFailTeamWrite, err) + + _, err = svc.CreateUser(ctx, fleet.UserPayload{ + Name: ptr.String("Some Name"), + Email: ptr.String("some@email.com"), + Password: ptr.String("passw0rd."), + GlobalRole: ptr.String(fleet.RoleAdmin), + }) + checkAuthErr(t, tt.shouldFailGlobalWrite, err) + + _, err = svc.ModifyUser(ctx, 999, fleet.UserPayload{Teams: &teams}) + checkAuthErr(t, tt.shouldFailTeamWrite, err) + + _, err = svc.ModifyUser(ctx, 888, fleet.UserPayload{Teams: &teams}) + checkAuthErr(t, tt.shouldFailGlobalWrite, err) + + _, err = svc.ModifyUser(ctx, 888, fleet.UserPayload{GlobalRole: ptr.String(fleet.RoleMaintainer)}) + checkAuthErr(t, tt.shouldFailGlobalWrite, err) + }) + } +} diff --git a/server/service/team_policies_test.go b/server/service/team_policies_test.go index 662dcce3ea..503edcf39c 100644 --- a/server/service/team_policies_test.go +++ b/server/service/team_policies_test.go @@ -53,6 +53,12 @@ func TestTeamPoliciesAuth(t *testing.T) { true, false, }, + { + "team admin, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + false, + false, + }, { "team maintainer, belongs to team", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}}, @@ -65,6 +71,12 @@ func TestTeamPoliciesAuth(t *testing.T) { true, false, }, + { + "team admin, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleAdmin}}}, + true, + true, + }, { "team maintainer, DOES NOT belong to team", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleMaintainer}}}, diff --git a/server/service/team_schedule_test.go b/server/service/team_schedule_test.go index 1d23274d52..e34abd56a5 100644 --- a/server/service/team_schedule_test.go +++ b/server/service/team_schedule_test.go @@ -60,6 +60,12 @@ func TestTeamScheduleAuth(t *testing.T) { true, true, }, + { + "team admin, belongs to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleAdmin}}}, + false, + false, + }, { "team maintainer, belongs to team", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}}, @@ -78,6 +84,12 @@ func TestTeamScheduleAuth(t *testing.T) { true, true, }, + { + "team admin, DOES NOT belong to team", + &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleAdmin}}}, + true, + true, + }, { "team observer, DOES NOT belong to team", &fleet.User{Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 2}, Role: fleet.RoleObserver}}},