From 896eeb65a9242378a8688ffb60888ffb4f0e08fa Mon Sep 17 00:00:00 2001 From: Yuriy Tseretyan Date: Wed, 14 Sep 2022 09:30:58 -0400 Subject: [PATCH] Alerting: Fix alerting evaluation to use proper permissions (#55127) * access control to log user name if it does not have permissions * update ngalert Evaluator to accept user instead of creating a pseudo one * update alerting eval (rule\query testing) API to provide the real user to the Evaluator * update scheduler to create a pseudo user with proper permissions --- .../accesscontrol/acimpl/accesscontrol.go | 2 +- pkg/services/ngalert/api/api_testing.go | 4 +- pkg/services/ngalert/api/api_testing_test.go | 14 ++-- pkg/services/ngalert/eval/eval.go | 22 +++--- pkg/services/ngalert/eval/evaluator_mock.go | 67 ++++++++++++------- pkg/services/ngalert/schedule/schedule.go | 19 +++++- 6 files changed, 81 insertions(+), 47 deletions(-) diff --git a/pkg/services/accesscontrol/acimpl/accesscontrol.go b/pkg/services/accesscontrol/acimpl/accesscontrol.go index cbfd9d0aa0e..caac5b873c6 100644 --- a/pkg/services/accesscontrol/acimpl/accesscontrol.go +++ b/pkg/services/accesscontrol/acimpl/accesscontrol.go @@ -34,7 +34,7 @@ func (a *AccessControl) Evaluate(ctx context.Context, user *user.SignedInUser, e metrics.MAccessEvaluationCount.Inc() if !verifyPermissions(user) { - a.log.Warn("no permissions set for user", "userID", user.UserID, "orgID", user.OrgID) + a.log.Warn("no permissions set for user", "userID", user.UserID, "orgID", user.OrgID, "login", user.Login) return false, nil } // Test evaluation without scope resolver first, this will prevent 403 for wildcard scopes when resource does not exist diff --git a/pkg/services/ngalert/api/api_testing.go b/pkg/services/ngalert/api/api_testing.go index 7d5b121feb6..85a136f76d2 100644 --- a/pkg/services/ngalert/api/api_testing.go +++ b/pkg/services/ngalert/api/api_testing.go @@ -53,7 +53,7 @@ func (srv TestingApiSrv) RouteTestGrafanaRuleConfig(c *models.ReqContext, body a now = timeNow() } - evalResults := srv.evaluator.ConditionEval(c.Req.Context(), evalCond, now) + evalResults := srv.evaluator.ConditionEval(c.Req.Context(), c.SignedInUser, evalCond, now) frame := evalResults.AsDataFrame() return response.JSONStreaming(http.StatusOK, util.DynMap{ @@ -115,7 +115,7 @@ func (srv TestingApiSrv) RouteEvalQueries(c *models.ReqContext, cmd apimodels.Ev return ErrResp(http.StatusBadRequest, err, "invalid queries or expressions") } - evalResults, err := srv.evaluator.QueriesAndExpressionsEval(c.Req.Context(), c.SignedInUser.OrgID, cmd.Data, now) + evalResults, err := srv.evaluator.QueriesAndExpressionsEval(c.Req.Context(), c.SignedInUser, cmd.Data, now) if err != nil { return ErrResp(http.StatusBadRequest, err, "Failed to evaluate queries and expressions") } diff --git a/pkg/services/ngalert/api/api_testing_test.go b/pkg/services/ngalert/api/api_testing_test.go index 11682c9abbd..7df02986af8 100644 --- a/pkg/services/ngalert/api/api_testing_test.go +++ b/pkg/services/ngalert/api/api_testing_test.go @@ -70,7 +70,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { evaluator := &eval.FakeEvaluator{} var result []eval.Result - evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything).Return(result) + evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result) srv := createTestingApiSrv(ds, ac, evaluator) @@ -85,7 +85,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { require.Equal(t, http.StatusOK, response.Status()) - evaluator.AssertCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything) + evaluator.AssertCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything, mock.Anything) }) }) @@ -110,7 +110,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { evaluator := &eval.FakeEvaluator{} var result []eval.Result - evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything).Return(result) + evaluator.EXPECT().ConditionEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result) srv := createTestingApiSrv(ds, ac, evaluator) @@ -124,7 +124,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { }) require.Equal(t, http.StatusUnauthorized, response.Status()) - evaluator.AssertNotCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything) + evaluator.AssertNotCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything, mock.Anything) rc.IsSignedIn = true @@ -139,7 +139,7 @@ func TestRouteTestGrafanaRuleConfig(t *testing.T) { require.Equal(t, http.StatusOK, response.Status()) - evaluator.AssertCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything) + evaluator.AssertCalled(t, "ConditionEval", mock.Anything, mock.Anything, mock.Anything, mock.Anything) }) }) } @@ -198,7 +198,7 @@ func TestRouteEvalQueries(t *testing.T) { }, }, } - evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything).Return(result, nil) + evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result, nil) srv := createTestingApiSrv(ds, ac, evaluator) @@ -241,7 +241,7 @@ func TestRouteEvalQueries(t *testing.T) { }, }, } - evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything).Return(result, nil) + evaluator.EXPECT().QueriesAndExpressionsEval(mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(result, nil) srv := createTestingApiSrv(ds, ac, evaluator) diff --git a/pkg/services/ngalert/eval/eval.go b/pkg/services/ngalert/eval/eval.go index d0ca5cd180c..4a847c3c791 100644 --- a/pkg/services/ngalert/eval/eval.go +++ b/pkg/services/ngalert/eval/eval.go @@ -16,7 +16,6 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/ngalert/models" - "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" @@ -27,9 +26,9 @@ import ( //go:generate mockery --name Evaluator --structname FakeEvaluator --inpackage --filename evaluator_mock.go --with-expecter type Evaluator interface { // ConditionEval executes conditions and evaluates the result. - ConditionEval(ctx context.Context, condition models.Condition, now time.Time) Results + ConditionEval(ctx context.Context, user *user.SignedInUser, condition models.Condition, now time.Time) Results // QueriesAndExpressionsEval executes queries and expressions and returns the result. - QueriesAndExpressionsEval(ctx context.Context, orgID int64, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) + QueriesAndExpressionsEval(ctx context.Context, user *user.SignedInUser, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) } type evaluatorImpl struct { @@ -151,7 +150,7 @@ func (s State) String() string { // AlertExecCtx is the context provided for executing an alert condition. type AlertExecCtx struct { - OrgID int64 + User *user.SignedInUser ExpressionsEnabled bool Log log.Logger @@ -161,7 +160,7 @@ type AlertExecCtx struct { // getExprRequest validates the condition, gets the datasource information and creates an expr.Request from it. func getExprRequest(ctx AlertExecCtx, data []models.AlertQuery, now time.Time, dsCacheService datasources.CacheService) (*expr.Request, error) { req := &expr.Request{ - OrgId: ctx.OrgID, + OrgId: ctx.User.OrgID, Headers: map[string]string{ // Some data sources check this in query method as sometimes alerting needs special considerations. "FromAlert": "true", @@ -191,10 +190,7 @@ func getExprRequest(ctx AlertExecCtx, data []models.AlertQuery, now time.Time, d if expr.IsDataSource(q.DatasourceUID) { ds = expr.DataSourceModel() } else { - ds, err = dsCacheService.GetDatasourceByUID(ctx.Ctx, q.DatasourceUID, &user.SignedInUser{ - OrgID: ctx.OrgID, - OrgRole: org.RoleAdmin, // Get DS as admin for service, API calls (test/post) must check permissions based on user. - }, true) + ds, err = dsCacheService.GetDatasourceByUID(ctx.Ctx, q.DatasourceUID, ctx.User, true) if err != nil { return nil, err } @@ -552,8 +548,8 @@ func (evalResults Results) AsDataFrame() data.Frame { } // ConditionEval executes conditions and evaluates the result. -func (e *evaluatorImpl) ConditionEval(ctx context.Context, condition models.Condition, now time.Time) Results { - execResp, err := e.QueriesAndExpressionsEval(ctx, condition.OrgID, condition.Data, now) +func (e *evaluatorImpl) ConditionEval(ctx context.Context, user *user.SignedInUser, condition models.Condition, now time.Time) Results { + execResp, err := e.QueriesAndExpressionsEval(ctx, user, condition.Data, now) var execResults ExecutionResults if err != nil { execResults = ExecutionResults{Error: err} @@ -564,11 +560,11 @@ func (e *evaluatorImpl) ConditionEval(ctx context.Context, condition models.Cond } // QueriesAndExpressionsEval executes queries and expressions and returns the result. -func (e *evaluatorImpl) QueriesAndExpressionsEval(ctx context.Context, orgID int64, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) { +func (e *evaluatorImpl) QueriesAndExpressionsEval(ctx context.Context, user *user.SignedInUser, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) { alertCtx, cancelFn := context.WithTimeout(ctx, e.cfg.UnifiedAlerting.EvaluationTimeout) defer cancelFn() - alertExecCtx := AlertExecCtx{OrgID: orgID, Ctx: alertCtx, ExpressionsEnabled: e.cfg.ExpressionsEnabled, Log: e.log} + alertExecCtx := AlertExecCtx{User: user, Ctx: alertCtx, ExpressionsEnabled: e.cfg.ExpressionsEnabled, Log: e.log} execResult, err := executeQueriesAndExpressions(alertExecCtx, data, now, e.expressionService, e.dataSourceCache) if err != nil { diff --git a/pkg/services/ngalert/eval/evaluator_mock.go b/pkg/services/ngalert/eval/evaluator_mock.go index 568cd8c99a3..bb3f7d3d1f0 100644 --- a/pkg/services/ngalert/eval/evaluator_mock.go +++ b/pkg/services/ngalert/eval/evaluator_mock.go @@ -1,16 +1,19 @@ -// Code generated by mockery v2.10.0. DO NOT EDIT. +// Code generated by mockery v2.14.0. DO NOT EDIT. package eval import ( - "context" + context "context" backend "github.com/grafana/grafana-plugin-sdk-go/backend" + mock "github.com/stretchr/testify/mock" models "github.com/grafana/grafana/pkg/services/ngalert/models" time "time" + + user "github.com/grafana/grafana/pkg/services/user" ) // FakeEvaluator is an autogenerated mock type for the Evaluator type @@ -26,13 +29,13 @@ func (_m *FakeEvaluator) EXPECT() *FakeEvaluator_Expecter { return &FakeEvaluator_Expecter{mock: &_m.Mock} } -// ConditionEval provides a mock function with given fields: condition, now -func (_m *FakeEvaluator) ConditionEval(ctx context.Context, condition models.Condition, now time.Time) Results { - ret := _m.Called(condition, now) +// ConditionEval provides a mock function with given fields: ctx, _a1, condition, now +func (_m *FakeEvaluator) ConditionEval(ctx context.Context, _a1 *user.SignedInUser, condition models.Condition, now time.Time) Results { + ret := _m.Called(ctx, _a1, condition, now) var r0 Results - if rf, ok := ret.Get(0).(func(models.Condition, time.Time) Results); ok { - r0 = rf(condition, now) + if rf, ok := ret.Get(0).(func(context.Context, *user.SignedInUser, models.Condition, time.Time) Results); ok { + r0 = rf(ctx, _a1, condition, now) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(Results) @@ -48,15 +51,17 @@ type FakeEvaluator_ConditionEval_Call struct { } // ConditionEval is a helper method to define mock.On call +// - ctx context.Context +// - _a1 *user.SignedInUser // - condition models.Condition // - now time.Time -func (_e *FakeEvaluator_Expecter) ConditionEval(condition interface{}, now interface{}) *FakeEvaluator_ConditionEval_Call { - return &FakeEvaluator_ConditionEval_Call{Call: _e.mock.On("ConditionEval", condition, now)} +func (_e *FakeEvaluator_Expecter) ConditionEval(ctx interface{}, _a1 interface{}, condition interface{}, now interface{}) *FakeEvaluator_ConditionEval_Call { + return &FakeEvaluator_ConditionEval_Call{Call: _e.mock.On("ConditionEval", ctx, _a1, condition, now)} } -func (_c *FakeEvaluator_ConditionEval_Call) Run(run func(condition models.Condition, now time.Time)) *FakeEvaluator_ConditionEval_Call { +func (_c *FakeEvaluator_ConditionEval_Call) Run(run func(ctx context.Context, _a1 *user.SignedInUser, condition models.Condition, now time.Time)) *FakeEvaluator_ConditionEval_Call { _c.Call.Run(func(args mock.Arguments) { - run(args[0].(models.Condition), args[1].(time.Time)) + run(args[0].(context.Context), args[1].(*user.SignedInUser), args[2].(models.Condition), args[3].(time.Time)) }) return _c } @@ -66,13 +71,13 @@ func (_c *FakeEvaluator_ConditionEval_Call) Return(_a0 Results) *FakeEvaluator_C return _c } -// QueriesAndExpressionsEval provides a mock function with given fields: orgID, data, now -func (_m *FakeEvaluator) QueriesAndExpressionsEval(ctx context.Context, orgID int64, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) { - ret := _m.Called(orgID, data, now) +// QueriesAndExpressionsEval provides a mock function with given fields: ctx, _a1, data, now +func (_m *FakeEvaluator) QueriesAndExpressionsEval(ctx context.Context, _a1 *user.SignedInUser, data []models.AlertQuery, now time.Time) (*backend.QueryDataResponse, error) { + ret := _m.Called(ctx, _a1, data, now) var r0 *backend.QueryDataResponse - if rf, ok := ret.Get(0).(func(int64, []models.AlertQuery, time.Time) *backend.QueryDataResponse); ok { - r0 = rf(orgID, data, now) + if rf, ok := ret.Get(0).(func(context.Context, *user.SignedInUser, []models.AlertQuery, time.Time) *backend.QueryDataResponse); ok { + r0 = rf(ctx, _a1, data, now) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(*backend.QueryDataResponse) @@ -80,8 +85,8 @@ func (_m *FakeEvaluator) QueriesAndExpressionsEval(ctx context.Context, orgID in } var r1 error - if rf, ok := ret.Get(1).(func(int64, []models.AlertQuery, time.Time) error); ok { - r1 = rf(orgID, data, now) + if rf, ok := ret.Get(1).(func(context.Context, *user.SignedInUser, []models.AlertQuery, time.Time) error); ok { + r1 = rf(ctx, _a1, data, now) } else { r1 = ret.Error(1) } @@ -95,16 +100,17 @@ type FakeEvaluator_QueriesAndExpressionsEval_Call struct { } // QueriesAndExpressionsEval is a helper method to define mock.On call -// - orgID int64 +// - ctx context.Context +// - _a1 *user.SignedInUser // - data []models.AlertQuery // - now time.Time -func (_e *FakeEvaluator_Expecter) QueriesAndExpressionsEval(orgID interface{}, data interface{}, now interface{}) *FakeEvaluator_QueriesAndExpressionsEval_Call { - return &FakeEvaluator_QueriesAndExpressionsEval_Call{Call: _e.mock.On("QueriesAndExpressionsEval", orgID, data, now)} +func (_e *FakeEvaluator_Expecter) QueriesAndExpressionsEval(ctx interface{}, _a1 interface{}, data interface{}, now interface{}) *FakeEvaluator_QueriesAndExpressionsEval_Call { + return &FakeEvaluator_QueriesAndExpressionsEval_Call{Call: _e.mock.On("QueriesAndExpressionsEval", ctx, _a1, data, now)} } -func (_c *FakeEvaluator_QueriesAndExpressionsEval_Call) Run(run func(orgID int64, data []models.AlertQuery, now time.Time)) *FakeEvaluator_QueriesAndExpressionsEval_Call { +func (_c *FakeEvaluator_QueriesAndExpressionsEval_Call) Run(run func(ctx context.Context, _a1 *user.SignedInUser, data []models.AlertQuery, now time.Time)) *FakeEvaluator_QueriesAndExpressionsEval_Call { _c.Call.Run(func(args mock.Arguments) { - run(args[0].(int64), args[1].([]models.AlertQuery), args[2].(time.Time)) + run(args[0].(context.Context), args[1].(*user.SignedInUser), args[2].([]models.AlertQuery), args[3].(time.Time)) }) return _c } @@ -113,3 +119,18 @@ func (_c *FakeEvaluator_QueriesAndExpressionsEval_Call) Return(_a0 *backend.Quer _c.Call.Return(_a0, _a1) return _c } + +type mockConstructorTestingTNewFakeEvaluator interface { + mock.TestingT + Cleanup(func()) +} + +// NewFakeEvaluator creates a new instance of FakeEvaluator. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +func NewFakeEvaluator(t mockConstructorTestingTNewFakeEvaluator) *FakeEvaluator { + mock := &FakeEvaluator{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/services/ngalert/schedule/schedule.go b/pkg/services/ngalert/schedule/schedule.go index 5928071c171..7df6cc15740 100644 --- a/pkg/services/ngalert/schedule/schedule.go +++ b/pkg/services/ngalert/schedule/schedule.go @@ -11,12 +11,15 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/alerting" + "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/eval" "github.com/grafana/grafana/pkg/services/ngalert/metrics" ngmodels "github.com/grafana/grafana/pkg/services/ngalert/models" "github.com/grafana/grafana/pkg/services/ngalert/state" "github.com/grafana/grafana/pkg/services/ngalert/store" + "github.com/grafana/grafana/pkg/services/org" + "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/benbjohnson/clock" @@ -331,7 +334,21 @@ func (sch *schedule) ruleRoutine(grafanaCtx context.Context, key ngmodels.AlertR logger := logger.New("version", e.rule.Version, "attempt", attempt, "now", e.scheduledAt) start := sch.clock.Now() - results := sch.evaluator.ConditionEval(ctx, e.rule.GetEvalCondition(), e.scheduledAt) + schedulerUser := &user.SignedInUser{ + UserID: -1, + Login: "grafana_scheduler", + OrgID: e.rule.OrgID, + OrgRole: org.RoleAdmin, + Permissions: map[int64]map[string][]string{ + e.rule.OrgID: { + datasources.ActionQuery: []string{ + datasources.ScopeAll, + }, + }, + }, + } + + results := sch.evaluator.ConditionEval(ctx, schedulerUser, e.rule.GetEvalCondition(), e.scheduledAt) dur := sch.clock.Now().Sub(start) evalTotal.Inc() evalDuration.Observe(dur.Seconds())