diff --git a/changes/issue-2133-team-maintainers-edit-delete-own-queries b/changes/issue-2133-team-maintainers-edit-delete-own-queries new file mode 100644 index 0000000000..36f7342b5a --- /dev/null +++ b/changes/issue-2133-team-maintainers-edit-delete-own-queries @@ -0,0 +1 @@ +* Team maintainers can edit and delete queries they created. diff --git a/docs/01-Using-Fleet/09-Permissions.md b/docs/01-Using-Fleet/09-Permissions.md index 17e2acba77..317cd0ffe0 100644 --- a/docs/01-Using-Fleet/09-Permissions.md +++ b/docs/01-Using-Fleet/09-Permissions.md @@ -80,3 +80,5 @@ The following table depicts various permissions levels in a 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 | | ✅ | diff --git a/server/authz/policy.rego b/server/authz/policy.rego index 0242490165..10c38999d4 100644 --- a/server/authz/policy.rego +++ b/server/authz/policy.rego @@ -255,6 +255,22 @@ allow { action == write } +# Team maintainers can create new queries +allow { + object.id == 0 # new queries have ID zero + object.type == "query" + team_role(subject, subject.teams[_].id) == maintainer + action == write +} + +# Team 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 + action == write +} + # Global admins and (team) maintainers can run any allow { object.type == "query" diff --git a/server/authz/policy_test.go b/server/authz/policy_test.go index 80d396db3e..2bc3994d1e 100644 --- a/server/authz/policy_test.go +++ b/server/authz/policy_test.go @@ -392,10 +392,10 @@ func TestAuthorizeQuery(t *testing.T) { // Team maintainer can read/write {user: teamMaintainer, object: query, action: read, allow: true}, - {user: teamMaintainer, object: query, action: write, allow: false}, + {user: teamMaintainer, object: query, action: write, allow: true}, {user: teamMaintainer, object: query, action: run, allow: true}, {user: teamMaintainer, object: observerQuery, action: read, allow: true}, - {user: teamMaintainer, object: observerQuery, action: write, allow: false}, + {user: teamMaintainer, object: observerQuery, action: write, allow: true}, {user: teamMaintainer, object: observerQuery, action: run, allow: true}, }) } @@ -484,7 +484,6 @@ func runTestCases(t *testing.T, testCases []authTestCase) { } }) } - } func TestJSONToInterfaceUser(t *testing.T) { diff --git a/server/service/service_queries.go b/server/service/service_queries.go index 3eefd83733..c951b7d233 100644 --- a/server/service/service_queries.go +++ b/server/service/service_queries.go @@ -134,7 +134,12 @@ func (svc *Service) GetQuery(ctx context.Context, id uint) (*fleet.Query, error) } func (svc *Service) NewQuery(ctx context.Context, p fleet.QueryPayload) (*fleet.Query, error) { - if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionWrite); err != nil { + user := authz.UserFromContext(ctx) + q := &fleet.Query{} + if user != nil { + q.AuthorID = ptr.Uint(user.ID) + } + if err := svc.authz.Authorize(ctx, q, fleet.ActionWrite); err != nil { return nil, err } @@ -186,7 +191,8 @@ func (svc *Service) NewQuery(ctx context.Context, p fleet.QueryPayload) (*fleet. } func (svc *Service) ModifyQuery(ctx context.Context, id uint, p fleet.QueryPayload) (*fleet.Query, error) { - if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionWrite); err != nil { + // First make sure the user can read queries + if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionRead); err != nil { return nil, err } @@ -195,6 +201,11 @@ func (svc *Service) ModifyQuery(ctx context.Context, id uint, p fleet.QueryPaylo return nil, err } + // Then we make sure they can modify them + if err := svc.authz.Authorize(ctx, query, fleet.ActionWrite); err != nil { + return nil, err + } + if p.Name != nil { query.Name = *p.Name } @@ -234,7 +245,18 @@ func (svc *Service) ModifyQuery(ctx context.Context, id uint, p fleet.QueryPaylo } func (svc *Service) DeleteQuery(ctx context.Context, name string) error { - if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionWrite); err != nil { + // First make sure the user can read queries + if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionRead); err != nil { + return err + } + + query, err := svc.ds.QueryByName(ctx, name) + if err != nil { + return err + } + + // Then we make sure they can modify them + if err := svc.authz.Authorize(ctx, query, fleet.ActionWrite); err != nil { return err } @@ -251,7 +273,8 @@ func (svc *Service) DeleteQuery(ctx context.Context, name string) error { } func (svc *Service) DeleteQueryByID(ctx context.Context, id uint) error { - if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionWrite); err != nil { + // First make sure the user can read queries + if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionRead); err != nil { return err } @@ -260,6 +283,11 @@ func (svc *Service) DeleteQueryByID(ctx context.Context, id uint) error { return errors.Wrap(err, "lookup query by ID") } + // Then we make sure they can modify them + if err := svc.authz.Authorize(ctx, query, fleet.ActionWrite); err != nil { + return err + } + if err := svc.ds.DeleteQuery(ctx, query.Name); err != nil { return errors.Wrap(err, "delete query") } @@ -273,10 +301,23 @@ func (svc *Service) DeleteQueryByID(ctx context.Context, id uint) error { } func (svc *Service) DeleteQueries(ctx context.Context, ids []uint) (uint, error) { - if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionWrite); err != nil { + // First make sure the user can read queries + if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionRead); err != nil { return 0, err } + for _, id := range ids { + query, err := svc.ds.Query(ctx, id) + if err != nil { + return 0, errors.Wrap(err, "lookup query by ID") + } + + // Then we make sure they can modify them + if err := svc.authz.Authorize(ctx, query, fleet.ActionWrite); err != nil { + return 0, err + } + } + n, err := svc.ds.DeleteQueries(ctx, ids) if err != nil { return n, err diff --git a/server/service/service_queries_test.go b/server/service/service_queries_test.go index 5f3087a81a..d012da8ada 100644 --- a/server/service/service_queries_test.go +++ b/server/service/service_queries_test.go @@ -61,8 +61,8 @@ func TestListQueries(t *testing.T) { expectedOpts: fleet.ListQueryOptions{OnlyObserverCanRun: true}, }, { - title: "team admin", - user: &fleet.User{Teams: []fleet.UserTeam{{Role: fleet.RoleAdmin}}}, + title: "team maintainer", + user: &fleet.User{Teams: []fleet.UserTeam{{Role: fleet.RoleMaintainer}}}, expectedOpts: fleet.ListQueryOptions{OnlyObserverCanRun: false}, }, } @@ -82,3 +82,121 @@ func TestListQueries(t *testing.T) { }) } } + +func TestQueryAuth(t *testing.T) { + ds := new(mock.Store) + svc := newTestService(ds, nil, nil) + authoredQueryID := uint(1) + authoredQueryName := "authored" + queryName := map[uint]string{ + authoredQueryID: authoredQueryName, + 2: "not authored", + } + teamMaintainer := &fleet.User{ID: 42, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: 1}, Role: fleet.RoleMaintainer}}} + + ds.NewQueryFunc = func(ctx context.Context, query *fleet.Query, opts ...fleet.OptionalArg) (*fleet.Query, error) { + return query, nil + } + ds.QueryByNameFunc = func(ctx context.Context, name string, opts ...fleet.OptionalArg) (*fleet.Query, error) { + if name == authoredQueryName { + return &fleet.Query{ID: 99, AuthorID: ptr.Uint(teamMaintainer.ID)}, nil + } + return &fleet.Query{ID: 8888, AuthorID: ptr.Uint(6666)}, nil + } + ds.NewActivityFunc = func(ctx context.Context, user *fleet.User, activityType string, details *map[string]interface{}) error { + return nil + } + ds.QueryFunc = func(ctx context.Context, id uint) (*fleet.Query, error) { + if id == authoredQueryID { + return &fleet.Query{ID: 99, AuthorID: ptr.Uint(teamMaintainer.ID)}, nil + } + return &fleet.Query{ID: 8888, AuthorID: ptr.Uint(6666)}, nil + } + ds.SaveQueryFunc = func(ctx context.Context, query *fleet.Query) error { + return nil + } + ds.DeleteQueryFunc = func(ctx context.Context, name string) error { + return nil + } + ds.DeleteQueriesFunc = func(ctx context.Context, ids []uint) (uint, error) { + return 0, nil + } + + var testCases = []struct { + name string + user *fleet.User + qid uint + shouldFailWrite bool + shouldFailRead bool + shouldFailNew bool + }{ + { + "global admin", + &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)}, + authoredQueryID, + false, + false, + false, + }, + { + "global maintainer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleMaintainer)}, + authoredQueryID, + false, + false, + false, + }, + { + "global observer", + &fleet.User{GlobalRole: ptr.String(fleet.RoleObserver)}, + authoredQueryID, + true, + false, + true, + }, + { + "team maintainer, author of the query", + teamMaintainer, + authoredQueryID, + false, + false, + false, + }, + { + "team maintainer, NOT author of the query", + teamMaintainer, + 2, + true, + false, + false, + }, + { + "team observer", + &fleet.User{ID: 48, Teams: []fleet.UserTeam{{Team: fleet.Team{ID: authoredQueryID}, Role: fleet.RoleObserver}}}, + 2, + true, + false, + 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.NewQuery(ctx, fleet.QueryPayload{Name: ptr.String("name"), Query: ptr.String("select 1")}) + checkAuthErr(t, tt.shouldFailNew, err) + + _, err = svc.ModifyQuery(ctx, tt.qid, fleet.QueryPayload{}) + checkAuthErr(t, tt.shouldFailWrite, err) + + err = svc.DeleteQuery(ctx, queryName[tt.qid]) + checkAuthErr(t, tt.shouldFailWrite, err) + + err = svc.DeleteQueryByID(ctx, tt.qid) + checkAuthErr(t, tt.shouldFailWrite, err) + + _, err = svc.DeleteQueries(ctx, []uint{tt.qid}) + checkAuthErr(t, tt.shouldFailWrite, err) + }) + } +}