15597 Observer list query filter (#15653)

changes/15597-observer-query-filter

@@ -0,0 +1 @@
+- team and global observers can now see all respective queries regardless of the observerCanRun value

server/datastore/mysql/queries.go

@@ -489,10 +489,6 @@ func (ds *Datastore) ListQueries(ctx context.Context, opt fleet.ListQueryOptions
 	args := []interface{}{false, fleet.AggregatedStatsTypeScheduledQuery}
 	whereClauses := "WHERE saved = true"
 
-	if opt.OnlyObserverCanRun {
-		whereClauses += " AND q.observer_can_run=true"
-	}
-
 	if opt.TeamID != nil {
 		args = append(args, *opt.TeamID)
 		whereClauses += " AND team_id = ?"

server/datastore/mysql/queries_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"math"
+	"math/rand"
 	"sort"
 	"testing"
 
@@ -29,7 +30,6 @@ func TestQueries(t *testing.T) {
 		{"List", testQueriesList},
 		{"LoadPacksForQueries", testQueriesLoadPacksForQueries},
 		{"DuplicateNew", testQueriesDuplicateNew},
-		{"ListFiltersObservers", testQueriesListFiltersObservers},
 		{"ObserverCanRunQuery", testObserverCanRunQuery},
 		{"ListQueriesFiltersByTeamID", testListQueriesFiltersByTeamID},
 		{"ListQueriesFiltersByIsScheduled", testListQueriesFiltersByIsScheduled},
@@ -177,7 +177,7 @@ func testQueriesDelete(t *testing.T, ds *Datastore) {
 	assert.NotEqual(t, query.ID, 0)
 	err = ds.UpdateLiveQueryStats(
 		context.Background(), query.ID, []*fleet.LiveQueryStats{
-			&fleet.LiveQueryStats{
+			{
 				HostID: hostID,
 			},
 		},
@@ -353,12 +353,13 @@ func testQueriesList(t *testing.T, ds *Datastore) {
 
 	for i := 0; i < 10; i++ {
 		_, err := ds.NewQuery(context.Background(), &fleet.Query{
-			Name:        fmt.Sprintf("name%02d", i),
-			Query:       fmt.Sprintf("query%02d", i),
-			Saved:       true,
-			AuthorID:    &user.ID,
-			DiscardData: true,
-			Logging:     fleet.LoggingSnapshot,
+			Name:           fmt.Sprintf("name%02d", i),
+			Query:          fmt.Sprintf("query%02d", i),
+			Saved:          true,
+			AuthorID:       &user.ID,
+			DiscardData:    true,
+			ObserverCanRun: rand.Intn(2) == 0, //nolint:gosec
+			Logging:        fleet.LoggingSnapshot,
 		})
 		require.Nil(t, err)
 	}
@@ -574,43 +575,6 @@ func testQueriesDuplicateNew(t *testing.T, ds *Datastore) {
 	require.Contains(t, err.Error(), "already exists")
 }
 
-func testQueriesListFiltersObservers(t *testing.T, ds *Datastore) {
-	_, err := ds.NewQuery(context.Background(), &fleet.Query{
-		Name:    "query1",
-		Query:   "select 1;",
-		Saved:   true,
-		Logging: fleet.LoggingSnapshot,
-	})
-	require.NoError(t, err)
-	_, err = ds.NewQuery(context.Background(), &fleet.Query{
-		Name:    "query2",
-		Query:   "select 1;",
-		Saved:   true,
-		Logging: fleet.LoggingSnapshot,
-	})
-	require.NoError(t, err)
-	query3, err := ds.NewQuery(context.Background(), &fleet.Query{
-		Name:           "query3",
-		Query:          "select 1;",
-		Saved:          true,
-		ObserverCanRun: true,
-		Logging:        fleet.LoggingSnapshot,
-	})
-	require.NoError(t, err)
-
-	queries, err := ds.ListQueries(context.Background(), fleet.ListQueryOptions{})
-	require.NoError(t, err)
-	require.Len(t, queries, 3)
-
-	queries, err = ds.ListQueries(
-		context.Background(),
-		fleet.ListQueryOptions{OnlyObserverCanRun: true, ListOptions: fleet.ListOptions{PerPage: 1}},
-	)
-	require.NoError(t, err)
-	require.Len(t, queries, 1)
-	require.Equal(t, query3.ID, queries[0].ID)
-}
-
 func testObserverCanRunQuery(t *testing.T, ds *Datastore) {
 	_, err := ds.NewQuery(context.Background(), &fleet.Query{
 		Name:           "canRunTrue",
@@ -1057,5 +1021,4 @@ func testIsSavedQuery(t *testing.T, ds *Datastore) {
 	// error case
 	_, err = ds.IsSavedQuery(context.Background(), math.MaxUint)
 	require.Error(t, err)
-
 }

server/fleet/app.go

@@ -967,8 +967,7 @@ type ListQueryOptions struct {
 	// team.
 	TeamID *uint
 	// IsScheduled filters queries that are meant to run at a set interval.
-	IsScheduled        *bool
-	OnlyObserverCanRun bool
+	IsScheduled *bool
 }
 
 type ListActivitiesOptions struct {

server/service/queries.go

@@ -96,14 +96,10 @@ func (svc *Service) ListQueries(ctx context.Context, opt fleet.ListOptions, team
 		return nil, err
 	}
 
-	user := authz.UserFromContext(ctx)
-	onlyShowObserverCanRun := onlyShowObserverCanRunQueries(user, teamID)
-
 	queries, err := svc.ds.ListQueries(ctx, fleet.ListQueryOptions{
-		ListOptions:        opt,
-		OnlyObserverCanRun: onlyShowObserverCanRun,
-		TeamID:             teamID,
-		IsScheduled:        scheduled,
+		ListOptions: opt,
+		TeamID:      teamID,
+		IsScheduled: scheduled,
 	})
 	if err != nil {
 		return nil, err
@@ -112,19 +108,6 @@ func (svc *Service) ListQueries(ctx context.Context, opt fleet.ListOptions, team
 	return queries, nil
 }
 
-func onlyShowObserverCanRunQueries(user *fleet.User, teamID *uint) bool {
-	if user.GlobalRole != nil && *user.GlobalRole == fleet.RoleObserver {
-		// Return false here because Global Observers should be able to access all queries via API.
-		// However, the UI will only show queries that have "observer can run" set to true.
-		// See the user permissions matrix: https://fleetdm.com/docs/using-fleet/manage-access#user-permissions
-		return false
-	}
-
-	return teamID != nil && user.TeamMembership(func(ut fleet.UserTeam) bool {
-		return ut.Role == fleet.RoleObserver
-	})[*teamID]
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 // Query Reports
 ////////////////////////////////////////////////////////////////////////////////

server/service/queries_test.go

@@ -12,108 +12,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestFilterQueriesForObserver(t *testing.T) {
-	t.Run("global role", func(t *testing.T) {
-		require.False(t, onlyShowObserverCanRunQueries(&fleet.User{
-			GlobalRole: ptr.String(fleet.RoleObserver),
-		}, nil))
-
-		require.False(t, onlyShowObserverCanRunQueries(&fleet.User{
-			GlobalRole: ptr.String(fleet.RoleObserverPlus),
-		}, nil))
-
-		require.False(t, onlyShowObserverCanRunQueries(&fleet.User{
-			GlobalRole: ptr.String(fleet.RoleMaintainer),
-		}, nil))
-
-		require.False(t, onlyShowObserverCanRunQueries(&fleet.User{
-			GlobalRole: ptr.String(fleet.RoleAdmin),
-		}, nil))
-	})
-
-	t.Run("user belongs to one or more teams", func(t *testing.T) {
-		require.True(t, onlyShowObserverCanRunQueries(&fleet.User{Teams: []fleet.UserTeam{{
-			Role: fleet.RoleObserver,
-			Team: fleet.Team{ID: 1},
-		}}}, ptr.Uint(1)))
-
-		require.True(t, onlyShowObserverCanRunQueries(&fleet.User{Teams: []fleet.UserTeam{
-			{
-				Role: fleet.RoleObserver,
-				Team: fleet.Team{ID: 1},
-			},
-			{
-				Role: fleet.RoleObserver,
-				Team: fleet.Team{ID: 2},
-			},
-		}}, ptr.Uint(2)))
-
-		require.True(t, onlyShowObserverCanRunQueries(&fleet.User{Teams: []fleet.UserTeam{
-			{
-				Role: fleet.RoleObserver,
-				Team: fleet.Team{ID: 1},
-			},
-			{
-				Role: fleet.RoleMaintainer,
-				Team: fleet.Team{ID: 2},
-			},
-		}}, ptr.Uint(1)))
-
-		require.False(t, onlyShowObserverCanRunQueries(&fleet.User{Teams: []fleet.UserTeam{
-			{
-				Role: fleet.RoleObserver,
-				Team: fleet.Team{ID: 1},
-			},
-			{
-				Role: fleet.RoleMaintainer,
-				Team: fleet.Team{ID: 2},
-			},
-		}}, ptr.Uint(2)))
-	})
-}
-
-func TestListQueries(t *testing.T) {
-	ds := new(mock.Store)
-	svc, ctx := newTestService(t, ds, nil, nil)
-
-	cases := [...]struct {
-		title        string
-		user         *fleet.User
-		expectedOpts fleet.ListQueryOptions
-	}{
-		{
-			title:        "global admin",
-			user:         &fleet.User{GlobalRole: ptr.String(fleet.RoleAdmin)},
-			expectedOpts: fleet.ListQueryOptions{OnlyObserverCanRun: false},
-		},
-		{
-			title:        "global observer",
-			user:         &fleet.User{GlobalRole: ptr.String(fleet.RoleObserver)},
-			expectedOpts: fleet.ListQueryOptions{OnlyObserverCanRun: false},
-		},
-		{
-			title:        "team maintainer",
-			user:         &fleet.User{Teams: []fleet.UserTeam{{Role: fleet.RoleMaintainer}}},
-			expectedOpts: fleet.ListQueryOptions{OnlyObserverCanRun: false},
-		},
-	}
-
-	var calledWithOpts fleet.ListQueryOptions
-	ds.ListQueriesFunc = func(ctx context.Context, opt fleet.ListQueryOptions) ([]*fleet.Query, error) {
-		calledWithOpts = opt
-		return []*fleet.Query{}, nil
-	}
-
-	for _, tt := range cases {
-		t.Run(tt.title, func(t *testing.T) {
-			viewerCtx := viewer.NewContext(ctx, viewer.Viewer{User: tt.user})
-			_, err := svc.ListQueries(viewerCtx, fleet.ListOptions{}, nil, nil)
-			require.NoError(t, err)
-			assert.Equal(t, tt.expectedOpts, calledWithOpts)
-		})
-	}
-}
-
 func TestQueryPayloadValidationCreate(t *testing.T) {
 	ds := new(mock.Store)
 	ds.NewQueryFunc = func(ctx context.Context, query *fleet.Query, opts ...fleet.OptionalArg) (*fleet.Query, error) {
@@ -356,6 +254,12 @@ func TestQueryAuth(t *testing.T) {
 		ID:   1,
 		Name: "Foobar",
 	}
+
+	team2 := fleet.Team{
+		ID:   2,
+		Name: "Barfoo",
+	}
+
 	teamAdmin := &fleet.User{
 		ID: 42,
 		Teams: []fleet.UserTeam{
@@ -411,17 +315,31 @@ func TestQueryAuth(t *testing.T) {
 		Name:   "team query",
 		TeamID: ptr.Uint(team.ID),
 	}
+	team2Query := fleet.Query{
+		ID:     77,
+		Name:   "team2 query",
+		TeamID: ptr.Uint(team2.ID),
+	}
 	queriesMap := map[uint]fleet.Query{
 		globalQuery.ID: globalQuery,
 		teamQuery.ID:   teamQuery,
+		team2Query.ID:  team2Query,
 	}
 
 	ds.TeamFunc = func(ctx context.Context, tid uint) (*fleet.Team, error) {
-		return &team, nil
+		if tid == team.ID {
+			return &team, nil
+		} else if tid == team2.ID {
+			return &team2, nil
+		}
+		return nil, newNotFoundError()
 	}
+
 	ds.TeamByNameFunc = func(ctx context.Context, name string) (*fleet.Team, error) {
 		if name == team.Name {
 			return &team, nil
+		} else if name == team2.Name {
+			return &team2, nil
 		}
 		return nil, newNotFoundError()
 	}
@@ -433,6 +351,8 @@ func TestQueryAuth(t *testing.T) {
 			return &globalQuery, nil
 		} else if teamID != nil && *teamID == team.ID && name == "team query" {
 			return &teamQuery, nil
+		} else if teamID != nil && *teamID == team2.ID && name == "team2 query" {
+			return &team2Query, nil
 		}
 		return nil, newNotFoundError()
 	}
@@ -444,6 +364,8 @@ func TestQueryAuth(t *testing.T) {
 			return &globalQuery, nil
 		} else if id == 88 {
 			return &teamQuery, nil
+		} else if id == 77 {
+			return &team2Query, nil
 		}
 		return nil, newNotFoundError()
 	}
@@ -572,6 +494,14 @@ func TestQueryAuth(t *testing.T) {
 			false,
 			false,
 		},
+		{
+			"team admin and team2 query",
+			teamAdmin,
+			team2Query.ID,
+			true,
+			true,
+			true,
+		},
 		{
 			"team maintainer and global query",
 			teamMaintainer,
@@ -588,6 +518,14 @@ func TestQueryAuth(t *testing.T) {
 			false,
 			false,
 		},
+		{
+			"team maintainer and team2 query",
+			teamMaintainer,
+			team2Query.ID,
+			true,
+			true,
+			true,
+		},
 		{
 			"team observer and global query",
 			teamObserver,
@@ -604,6 +542,14 @@ func TestQueryAuth(t *testing.T) {
 			false,
 			true,
 		},
+		{
+			"team observer and team2 query",
+			teamObserver,
+			team2Query.ID,
+			true,
+			true,
+			true,
+		},
 		{
 			"team observer+ and global query",
 			teamObserverPlus,
@@ -620,6 +566,14 @@ func TestQueryAuth(t *testing.T) {
 			false,
 			true,
 		},
+		{
+			"team observer+ and team2 query",
+			teamObserverPlus,
+			team2Query.ID,
+			true,
+			true,
+			true,
+		},
 		{
 			"team gitops and global query",
 			teamGitOps,
@@ -636,6 +590,14 @@ func TestQueryAuth(t *testing.T) {
 			true,
 			false,
 		},
+		{
+			"team gitops and team2 query",
+			teamGitOps,
+			team2Query.ID,
+			true,
+			true,
+			true,
+		},
 	}
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
@@ -672,9 +634,12 @@ func TestQueryAuth(t *testing.T) {
 			checkAuthErr(t, tt.shouldFailRead, err)
 
 			teamName := ""
-			if query.TeamID != nil {
+			if query.TeamID != nil && *query.TeamID == team.ID {
 				teamName = team.Name
+			} else if query.TeamID != nil && *query.TeamID == team2.ID {
+				teamName = team2.Name
 			}
+
 			err = svc.ApplyQuerySpecs(ctx, []*fleet.QuerySpec{{
 				Name:     query.Name,
 				Query:    "SELECT 1",

server/service/team_policies_test.go

@@ -174,6 +174,7 @@ func TestTeamPoliciesAuth(t *testing.T) {
 }
 
 func checkAuthErr(t *testing.T, shouldFail bool, err error) {
+	t.Helper()
 	if shouldFail {
 		require.Error(t, err)
 		var forbiddenError *authz.Forbidden