diff --git a/changes/15597-observer-query-filter b/changes/15597-observer-query-filter new file mode 100644 index 0000000000..3c1fa48b54 --- /dev/null +++ b/changes/15597-observer-query-filter @@ -0,0 +1 @@ +- team and global observers can now see all respective queries regardless of the observerCanRun value \ No newline at end of file diff --git a/server/datastore/mysql/queries.go b/server/datastore/mysql/queries.go index 097f269e4c..a399cffdbb 100644 --- a/server/datastore/mysql/queries.go +++ b/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 = ?" diff --git a/server/datastore/mysql/queries_test.go b/server/datastore/mysql/queries_test.go index d3a2440870..ed748ca849 100644 --- a/server/datastore/mysql/queries_test.go +++ b/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) - } diff --git a/server/fleet/app.go b/server/fleet/app.go index 930c2bf310..a3f2cc122d 100644 --- a/server/fleet/app.go +++ b/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 { diff --git a/server/service/queries.go b/server/service/queries.go index f59c4d46d2..3730d96487 100644 --- a/server/service/queries.go +++ b/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 //////////////////////////////////////////////////////////////////////////////// diff --git a/server/service/queries_test.go b/server/service/queries_test.go index e7580c57f1..944a2fe146 100644 --- a/server/service/queries_test.go +++ b/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", diff --git a/server/service/team_policies_test.go b/server/service/team_policies_test.go index 0adc05db36..e6079f1101 100644 --- a/server/service/team_policies_test.go +++ b/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