Alerting: update authorization logic to use proper legacy roles when fine-grained access is disabled (#46931)

* require legacy Editor for post, put, delete endpoints
* require user to be signed in on group level because handler that checks that user has role Editor does not check it is signed in

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) {

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,7 +159,6 @@ func TestAlertmanagerConfig(t *testing.T) {
 				Req: &http.Request{},
 			},
 			SignedInUser: &models.SignedInUser{
-				OrgRole: models.ROLE_EDITOR,
 				OrgId: 12,
 			},
 		}
@@ -170,31 +170,12 @@ 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,
 			},
 		}
@@ -212,7 +193,6 @@ func TestAlertmanagerConfig(t *testing.T) {
 				Req: &http.Request{},
 			},
 			SignedInUser: &models.SignedInUser{
-				OrgRole: models.ROLE_EDITOR,
 				OrgId: 3, // Org 3 was initialized with broken config.
 			},
 		}

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))

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)
 }

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)
 }

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)
 }

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)
 }

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)
 }

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}}

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",