From 578a9780f2bf495a0b41f0103063cb04bc6430c2 Mon Sep 17 00:00:00 2001
From: Michal Nicpon <39177923+michalnicp@users.noreply.github.com>
Date: Tue, 8 Feb 2022 09:47:48 -0700
Subject: [PATCH] apply queries spec endpoint missing authorization check
 (#4068)

* do authorization check when updating existing query
---
 ...e-3953-apply-queries-missing-authorization |  1 +
 cmd/fleetctl/apply_test.go                    |  3 +++
 server/service/queries.go                     | 23 +++++++++++++++----
 server/service/queries_test.go                |  9 +++-----
 4 files changed, 25 insertions(+), 11 deletions(-)
 create mode 100644 changes/issue-3953-apply-queries-missing-authorization

diff --git a/changes/issue-3953-apply-queries-missing-authorization b/changes/issue-3953-apply-queries-missing-authorization
new file mode 100644
index 000000000..81bb44960
--- /dev/null
+++ b/changes/issue-3953-apply-queries-missing-authorization
@@ -0,0 +1 @@
+* Check that user is authorized to update query in update query specs endpoint
diff --git a/cmd/fleetctl/apply_test.go b/cmd/fleetctl/apply_test.go
index d317acea5..b93ea8aa2 100644
--- a/cmd/fleetctl/apply_test.go
+++ b/cmd/fleetctl/apply_test.go
@@ -423,6 +423,9 @@ func TestApplyQueries(t *testing.T) {
 	_, ds := runServerWithMockedDS(t)
 
 	var appliedQueries []*fleet.Query
+	ds.QueryByNameFunc = func(ctx context.Context, name string, opts ...fleet.OptionalArg) (*fleet.Query, error) {
+		return nil, sql.ErrNoRows
+	}
 	ds.ApplyQueriesFunc = func(ctx context.Context, authorID uint, queries []*fleet.Query) error {
 		appliedQueries = queries
 		return nil
diff --git a/server/service/queries.go b/server/service/queries.go
index 16b5b9ee4..37495640e 100644
--- a/server/service/queries.go
+++ b/server/service/queries.go
@@ -2,6 +2,8 @@ package service
 
 import (
 	"context"
+	"database/sql"
+	"errors"
 	"fmt"
 
 	"github.com/fleetdm/fleet/v4/server/authz"
@@ -459,15 +461,11 @@ func applyQuerySpecsEndpoint(ctx context.Context, request interface{}, svc fleet
 }
 
 func (svc *Service) ApplyQuerySpecs(ctx context.Context, specs []*fleet.QuerySpec) error {
+	// check that the user can create queries
 	if err := svc.authz.Authorize(ctx, &fleet.Query{}, fleet.ActionWrite); err != nil {
 		return err
 	}
 
-	vc, ok := viewer.FromContext(ctx)
-	if !ok {
-		return ctxerr.New(ctx, "user must be authenticated to apply queries")
-	}
-
 	queries := []*fleet.Query{}
 	for _, spec := range specs {
 		queries = append(queries, queryFromSpec(spec))
@@ -479,6 +477,21 @@ func (svc *Service) ApplyQuerySpecs(ctx context.Context, specs []*fleet.QuerySpe
 				message: fmt.Sprintf("query payload verification: %s", err),
 			})
 		}
+
+		// check that the user can update the query if it already exists
+		query, err := svc.ds.QueryByName(ctx, query.Name)
+		if err != nil && !errors.Is(err, sql.ErrNoRows) {
+			return err
+		} else if err == nil {
+			if err := svc.authz.Authorize(ctx, query, fleet.ActionWrite); err != nil {
+				return err
+			}
+		}
+	}
+
+	vc, ok := viewer.FromContext(ctx)
+	if !ok {
+		return ctxerr.New(ctx, "user must be authenticated to apply queries")
 	}
 
 	err := svc.ds.ApplyQueries(ctx, vc.UserID(), queries)
diff --git a/server/service/queries_test.go b/server/service/queries_test.go
index ce9c768d7..80bbafca6 100644
--- a/server/service/queries_test.go
+++ b/server/service/queries_test.go
@@ -211,15 +211,12 @@ func TestQueryAuth(t *testing.T) {
 			_, err = svc.ListQueries(ctx, fleet.ListOptions{})
 			checkAuthErr(t, tt.shouldFailRead, err)
 
-			// TODO(mna): the authorization seems wrong here - ApplyQuerySpecs never loads the queries it
-			// receives, so it just validates that the user has Write access to Queries in general, not
-			// that it can update those provided in the specs in particular. Here, a team maintainer can
-			// end up updating a query they did not author. See https://github.com/fleetdm/fleet/issues/3953
-			//err = svc.ApplyQuerySpecs(ctx, []*fleet.QuerySpec{{Name: queryName[tt.qid], Query: "SELECT 1"}})
-			//checkAuthErr(t, tt.shouldFailWrite, err)
+			err = svc.ApplyQuerySpecs(ctx, []*fleet.QuerySpec{{Name: queryName[tt.qid], Query: "SELECT 1"}})
+			checkAuthErr(t, tt.shouldFailWrite, err)
 
 			_, err = svc.GetQuerySpecs(ctx)
 			checkAuthErr(t, tt.shouldFailRead, err)
+
 			_, err = svc.GetQuerySpec(ctx, queryName[tt.qid])
 			checkAuthErr(t, tt.shouldFailRead, err)
 		})