Issue 2133 team maintainer can edit delete queries (#2256)

* wip

* Team maintainers can edit and delete queries they authored

* Update documentation

* Fix test

changes/issue-2133-team-maintainers-edit-delete-own-queries

@@ -0,0 +1 @@
+* Team maintainers can edit and delete queries they created.

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                                 |          | ✅         |

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"

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) {

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

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)
+		})
+	}
+}