diff --git a/pkg/services/ngalert/api/api_alertmanager.go b/pkg/services/ngalert/api/api_alertmanager.go index b04b21cd2e5..7089671d96d 100644 --- a/pkg/services/ngalert/api/api_alertmanager.go +++ b/pkg/services/ngalert/api/api_alertmanager.go @@ -125,10 +125,6 @@ func (srv AlertmanagerSrv) RouteGetAMStatus(c *models.ReqContext) response.Respo } func (srv AlertmanagerSrv) RouteCreateSilence(c *models.ReqContext, postableSilence apimodels.PostableSilence) response.Response { - if !c.HasUserRole(models.ROLE_EDITOR) { - return ErrResp(http.StatusForbidden, errors.New("permission denied"), "") - } - am, errResp := srv.AlertmanagerFor(c.OrgId) if errResp != nil { return errResp @@ -150,10 +146,6 @@ func (srv AlertmanagerSrv) RouteCreateSilence(c *models.ReqContext, postableSile } func (srv AlertmanagerSrv) RouteDeleteAlertingConfig(c *models.ReqContext) response.Response { - if !c.HasUserRole(models.ROLE_EDITOR) { - return ErrResp(http.StatusForbidden, errors.New("permission denied"), "") - } - am, errResp := srv.AlertmanagerFor(c.OrgId) if errResp != nil { return errResp @@ -168,10 +160,6 @@ func (srv AlertmanagerSrv) RouteDeleteAlertingConfig(c *models.ReqContext) respo } func (srv AlertmanagerSrv) RouteDeleteSilence(c *models.ReqContext) response.Response { - if !c.HasUserRole(models.ROLE_EDITOR) { - return ErrResp(http.StatusForbidden, errors.New("permission denied"), "") - } - am, errResp := srv.AlertmanagerFor(c.OrgId) if errResp != nil { return errResp @@ -188,10 +176,6 @@ func (srv AlertmanagerSrv) RouteDeleteSilence(c *models.ReqContext) response.Res } func (srv AlertmanagerSrv) RouteGetAlertingConfig(c *models.ReqContext) response.Response { - if !c.HasUserRole(models.ROLE_EDITOR) { - return ErrResp(http.StatusForbidden, errors.New("permission denied"), "") - } - query := ngmodels.GetLatestAlertmanagerConfigurationQuery{OrgID: c.OrgId} if err := srv.store.GetLatestAlertmanagerConfiguration(c.Req.Context(), &query); err != nil { if errors.Is(err, store.ErrNoAlertmanagerConfiguration) { @@ -334,10 +318,6 @@ func (srv AlertmanagerSrv) RouteGetSilences(c *models.ReqContext) response.Respo } func (srv AlertmanagerSrv) RoutePostAlertingConfig(c *models.ReqContext, body apimodels.PostableUserConfig) response.Response { - if !c.HasUserRole(models.ROLE_EDITOR) { - return ErrResp(http.StatusForbidden, errors.New("permission denied"), "") - } - // Get the last known working configuration query := ngmodels.GetLatestAlertmanagerConfigurationQuery{OrgID: c.OrgId} if err := srv.store.GetLatestAlertmanagerConfiguration(c.Req.Context(), &query); err != nil { @@ -380,10 +360,6 @@ func (srv AlertmanagerSrv) RoutePostAMAlerts(_ *models.ReqContext, _ apimodels.P } func (srv AlertmanagerSrv) RoutePostTestReceivers(c *models.ReqContext, body apimodels.TestReceiversConfigBodyParams) response.Response { - if !c.HasUserRole(models.ROLE_EDITOR) { - return accessForbiddenResp() - } - if err := srv.loadSecureSettings(c.Req.Context(), c.OrgId, body.Receivers); err != nil { var unknownReceiverError UnknownReceiverError if errors.As(err, &unknownReceiverError) { diff --git a/pkg/services/ngalert/api/api_alertmanager_test.go b/pkg/services/ngalert/api/api_alertmanager_test.go index 625dfbb097d..b9f1470906c 100644 --- a/pkg/services/ngalert/api/api_alertmanager_test.go +++ b/pkg/services/ngalert/api/api_alertmanager_test.go @@ -6,6 +6,9 @@ import ( "testing" "time" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" @@ -16,8 +19,6 @@ import ( secretsManager "github.com/grafana/grafana/pkg/services/secrets/manager" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" - "github.com/prometheus/client_golang/prometheus" - "github.com/stretchr/testify/require" ) func TestContextWithTimeoutFromRequest(t *testing.T) { @@ -158,8 +159,7 @@ func TestAlertmanagerConfig(t *testing.T) { Req: &http.Request{}, }, SignedInUser: &models.SignedInUser{ - OrgRole: models.ROLE_EDITOR, - OrgId: 12, + OrgId: 12, }, } request := createAmConfigRequest(t) @@ -170,32 +170,13 @@ func TestAlertmanagerConfig(t *testing.T) { require.Contains(t, string(response.Body()), "Alertmanager does not exist for this organization") }) - t.Run("assert 403 Forbidden when applying config while not Editor", func(t *testing.T) { - rc := models.ReqContext{ - Context: &web.Context{ - Req: &http.Request{}, - }, - SignedInUser: &models.SignedInUser{ - OrgRole: models.ROLE_VIEWER, - OrgId: 1, - }, - } - request := createAmConfigRequest(t) - - response := sut.RoutePostAlertingConfig(&rc, request) - - require.Equal(t, 403, response.Status()) - require.Contains(t, string(response.Body()), "permission denied") - }) - t.Run("assert 202 when config successfully applied", func(t *testing.T) { rc := models.ReqContext{ Context: &web.Context{ Req: &http.Request{}, }, SignedInUser: &models.SignedInUser{ - OrgRole: models.ROLE_EDITOR, - OrgId: 1, + OrgId: 1, }, } request := createAmConfigRequest(t) @@ -212,8 +193,7 @@ func TestAlertmanagerConfig(t *testing.T) { Req: &http.Request{}, }, SignedInUser: &models.SignedInUser{ - OrgRole: models.ROLE_EDITOR, - OrgId: 3, // Org 3 was initialized with broken config. + OrgId: 3, // Org 3 was initialized with broken config. }, } request := createAmConfigRequest(t) diff --git a/pkg/services/ngalert/api/authorization.go b/pkg/services/ngalert/api/authorization.go index 65a19caf332..c6c7c86cc04 100644 --- a/pkg/services/ngalert/api/authorization.go +++ b/pkg/services/ngalert/api/authorization.go @@ -26,6 +26,17 @@ func (api *API) authorize(method, path string) web.Handler { authorize := acmiddleware.Middleware(api.AccessControl) var eval ac.Evaluator = nil + // Most routes follow this general authorization approach as a fallback. Exceptions are overridden directly in the below block. + var fallback web.Handler + switch method { + case http.MethodPost, http.MethodPut, http.MethodDelete: + fallback = middleware.ReqEditorRole + case http.MethodGet: + fallback = middleware.ReqSignedIn + default: + fallback = middleware.ReqSignedIn + } + switch method + path { // Alert Rules @@ -55,9 +66,11 @@ func (api *API) authorize(method, path string) web.Handler { // Grafana Rules Testing Paths case http.MethodPost + "/api/v1/rule/test/grafana": + fallback = middleware.ReqSignedIn // additional authorization is done in the request handler eval = ac.EvalPermission(ac.ActionAlertingRuleRead) case http.MethodPost + "/api/v1/eval": + fallback = middleware.ReqSignedIn // additional authorization is done in the request handler eval = ac.EvalPermission(ac.ActionAlertingRuleRead) @@ -81,6 +94,7 @@ func (api *API) authorize(method, path string) web.Handler { // Lotex Rules testing case http.MethodPost + "/api/v1/rule/test/{Recipient}": + fallback = middleware.ReqSignedIn eval = ac.EvalPermission(ac.ActionAlertingRuleExternalRead, datasources.ScopeProvider.GetResourceScope(ac.Parameter(":Recipient"))) // Alert Instances and Silences @@ -136,6 +150,7 @@ func (api *API) authorize(method, path string) web.Handler { case http.MethodDelete + "/api/alertmanager/grafana/config/api/v1/alerts": // reset alertmanager config to the default eval = ac.EvalPermission(ac.ActionAlertingNotificationsDelete) case http.MethodGet + "/api/alertmanager/grafana/config/api/v1/alerts": + fallback = middleware.ReqEditorRole eval = ac.EvalPermission(ac.ActionAlertingNotificationsRead) case http.MethodGet + "/api/alertmanager/grafana/api/v2/status": eval = ac.EvalPermission(ac.ActionAlertingNotificationsRead) @@ -143,6 +158,7 @@ func (api *API) authorize(method, path string) web.Handler { // additional authorization is done in the request handler eval = ac.EvalAny(ac.EvalPermission(ac.ActionAlertingNotificationsUpdate), ac.EvalPermission(ac.ActionAlertingNotificationsCreate), ac.EvalPermission(ac.ActionAlertingNotificationsDelete)) case http.MethodPost + "/api/alertmanager/grafana/config/api/v1/receivers/test": + fallback = middleware.ReqEditorRole eval = ac.EvalPermission(ac.ActionAlertingNotificationsRead) // External Alertmanager Paths @@ -166,7 +182,7 @@ func (api *API) authorize(method, path string) web.Handler { } if eval != nil { - return authorize(middleware.ReqSignedIn, eval) + return authorize(fallback, eval) } panic(fmt.Sprintf("no authorization handler for method [%s] of endpoint [%s]", method, path)) diff --git a/pkg/services/ngalert/api/generated_base_api_alertmanager.go b/pkg/services/ngalert/api/generated_base_api_alertmanager.go index a1f856ed0cb..69d9a06a570 100644 --- a/pkg/services/ngalert/api/generated_base_api_alertmanager.go +++ b/pkg/services/ngalert/api/generated_base_api_alertmanager.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/metrics" @@ -415,5 +416,5 @@ func (api *API) RegisterAlertmanagerApiEndpoints(srv AlertmanagerApiForkingServi m, ), ) - }) + }, middleware.ReqSignedIn) } diff --git a/pkg/services/ngalert/api/generated_base_api_configuration.go b/pkg/services/ngalert/api/generated_base_api_configuration.go index 01a7034555e..4be46dbc058 100644 --- a/pkg/services/ngalert/api/generated_base_api_configuration.go +++ b/pkg/services/ngalert/api/generated_base_api_configuration.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/metrics" @@ -87,5 +88,5 @@ func (api *API) RegisterConfigurationApiEndpoints(srv ConfigurationApiForkingSer m, ), ) - }) + }, middleware.ReqSignedIn) } diff --git a/pkg/services/ngalert/api/generated_base_api_prometheus.go b/pkg/services/ngalert/api/generated_base_api_prometheus.go index 5aa6e953113..3b6656eafba 100644 --- a/pkg/services/ngalert/api/generated_base_api_prometheus.go +++ b/pkg/services/ngalert/api/generated_base_api_prometheus.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/ngalert/metrics" ) @@ -81,5 +82,5 @@ func (api *API) RegisterPrometheusApiEndpoints(srv PrometheusApiForkingService, m, ), ) - }) + }, middleware.ReqSignedIn) } diff --git a/pkg/services/ngalert/api/generated_base_api_ruler.go b/pkg/services/ngalert/api/generated_base_api_ruler.go index 3bb765f69de..7167ebbae2e 100644 --- a/pkg/services/ngalert/api/generated_base_api_ruler.go +++ b/pkg/services/ngalert/api/generated_base_api_ruler.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/metrics" @@ -211,5 +212,5 @@ func (api *API) RegisterRulerApiEndpoints(srv RulerApiForkingService, m *metrics m, ), ) - }) + }, middleware.ReqSignedIn) } diff --git a/pkg/services/ngalert/api/generated_base_api_testing.go b/pkg/services/ngalert/api/generated_base_api_testing.go index 3e99de90d16..9f13b74dc20 100644 --- a/pkg/services/ngalert/api/generated_base_api_testing.go +++ b/pkg/services/ngalert/api/generated_base_api_testing.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" + "github.com/grafana/grafana/pkg/middleware" "github.com/grafana/grafana/pkg/models" apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions" "github.com/grafana/grafana/pkg/services/ngalert/metrics" @@ -80,5 +81,5 @@ func (api *API) RegisterTestingApiEndpoints(srv TestingApiForkingService, m *met m, ), ) - }) + }, middleware.ReqSignedIn) } diff --git a/pkg/services/ngalert/api/tooling/swagger-codegen/templates/controller-api.mustache b/pkg/services/ngalert/api/tooling/swagger-codegen/templates/controller-api.mustache index 9524445f5e8..c8686a22d00 100644 --- a/pkg/services/ngalert/api/tooling/swagger-codegen/templates/controller-api.mustache +++ b/pkg/services/ngalert/api/tooling/swagger-codegen/templates/controller-api.mustache @@ -44,6 +44,6 @@ func (api *API) Register{{classname}}Endpoints(srv {{classname}}ForkingService, m, ), ){{/operation}}{{/operations}} - }) + }, middleware.ReqSignedIn) }{{#operation}} {{/operation}}{{/operations}} \ No newline at end of file diff --git a/pkg/tests/api/alerting/api_alertmanager_test.go b/pkg/tests/api/alerting/api_alertmanager_test.go index 06f4682720d..eb1900e6c81 100644 --- a/pkg/tests/api/alerting/api_alertmanager_test.go +++ b/pkg/tests/api/alerting/api_alertmanager_test.go @@ -101,7 +101,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/config/api/v1/alerts", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "Permission denied"}`, }, { desc: "editor request should succeed", @@ -171,7 +171,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/config/api/v1/alerts", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "Permission denied"}`, }, { desc: "editor request should succeed", @@ -234,7 +234,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/api/v2/silences", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "Permission denied"}`, }, { desc: "editor request should succeed", @@ -340,7 +340,7 @@ func TestAMConfigAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/alertmanager/grafana/api/v2/silence/%s", expStatus: http.StatusForbidden, - expBody: `{"message": "permission denied"}`, + expBody: `{"message": "Permission denied"}`, }, { desc: "editor request should succeed", @@ -615,7 +615,7 @@ func TestRulerAccess(t *testing.T) { desc: "viewer request should fail", url: "http://viewer:viewer@%s/api/ruler/grafana/api/v1/rules/default", expStatus: http.StatusForbidden, - expectedResponse: `{"message": "user does not have permissions to edit the namespace: user does not have permissions to edit the namespace"}`, + expectedResponse: `{"message": "Permission denied"}`, }, { desc: "editor request should succeed",