Alerting: AlertingProxy to elevate permissions for request forwarded to data proxy when RBAC enabled (#53620)

pkg/services/ngalert/api/api.go

@@ -91,6 +91,7 @@ func (api *API) RegisterAPIEndpoints(m *metrics.API) {
 	logger := log.New("ngalert.api")
 	proxy := &AlertingProxy{
 		DataProxy: api.DataProxy,
+		ac:        api.AccessControl,
 	}
 
 	// Register endpoints for proxying to Alertmanager-compatible backends.

pkg/services/ngalert/api/util.go

@@ -17,10 +17,12 @@ import (
 
 	"github.com/grafana/grafana/pkg/api/response"
 	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/accesscontrol"
 	"github.com/grafana/grafana/pkg/services/datasourceproxy"
 	"github.com/grafana/grafana/pkg/services/datasources"
 	apimodels "github.com/grafana/grafana/pkg/services/ngalert/api/tooling/definitions"
 	ngmodels "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/web"
 )
@@ -69,19 +71,35 @@ func (w *safeMacaronWrapper) CloseNotify() <-chan bool {
 	return make(chan bool)
 }
 
-// replacedResponseWriter overwrites the underlying responsewriter used by a *models.ReqContext.
+// createProxyContext creates a new request context that is provided down to the data source proxy.
-// It's ugly because it needs to replace a value behind a few nested pointers.
+// The request context
-func replacedResponseWriter(ctx *models.ReqContext) (*models.ReqContext, *response.NormalResponse) {
+// 1. overwrites the underlying response writer used by a *models.ReqContext because AlertingProxy needs to intercept
-	resp := response.CreateNormalResponse(make(http.Header), nil, 0)
+// the response from the data source to analyze it and probably change
+// 2. elevates the current user permissions to Editor if both conditions are met: RBAC is enabled, user does not have Editor role.
+// This is needed to bypass the plugin authorization, which still relies on the legacy roles.
+// This elevation can be considered safe because all upstream calls are protected by the RBAC on web request router level.
+func (p *AlertingProxy) createProxyContext(ctx *models.ReqContext, request *http.Request, response *response.NormalResponse) *models.ReqContext {
 	cpy := *ctx
 	cpyMCtx := *cpy.Context
-	cpyMCtx.Resp = web.NewResponseWriter(ctx.Req.Method, &safeMacaronWrapper{resp})
+	cpyMCtx.Resp = web.NewResponseWriter(ctx.Req.Method, &safeMacaronWrapper{response})
 	cpy.Context = &cpyMCtx
-	return &cpy, resp
+	cpy.Req = request
+
+	// If RBAC is enabled, the actions are checked upstream and if the user gets here then it is allowed to do an action against a datasource.
+	// Some data sources require legacy Editor role in order to perform mutating operations. In this case, we elevate permissions for the context that we
+	// will provide downstream.
+	// TODO (yuri) remove this after RBAC for plugins is implemented
+	if !p.ac.IsDisabled() && !ctx.SignedInUser.HasRole(org.RoleEditor) {
+		newUser := *ctx.SignedInUser
+		newUser.OrgRole = org.RoleEditor
+		cpy.SignedInUser = &newUser
+	}
+	return &cpy
 }
 
 type AlertingProxy struct {
 	DataProxy *datasourceproxy.DataSourceProxyService
+	ac        accesscontrol.AccessControl
 }
 
 // withReq proxies a different request
@@ -100,8 +118,9 @@ func (p *AlertingProxy) withReq(
 	for h, v := range headers {
 		req.Header.Add(h, v)
 	}
-	newCtx, resp := replacedResponseWriter(ctx)
+	// this response will be populated by the response from the datasource
-	newCtx.Req = req
+	resp := response.CreateNormalResponse(make(http.Header), nil, 0)
+	proxyContext := p.createProxyContext(ctx, req, resp)
 
 	datasourceID := web.Params(ctx.Req)[":DatasourceID"]
 	if datasourceID != "" {
@@ -109,14 +128,13 @@ func (p *AlertingProxy) withReq(
 		if err != nil {
 			return ErrResp(http.StatusBadRequest, err, "DatasourceID is invalid")
 		}
-
+		p.DataProxy.ProxyDatasourceRequestWithID(proxyContext, recipient)
-		p.DataProxy.ProxyDatasourceRequestWithID(newCtx, recipient)
 	} else {
 		datasourceUID := web.Params(ctx.Req)[":DatasourceUID"]
 		if datasourceUID == "" {
 			return ErrResp(http.StatusBadRequest, err, "DatasourceUID is empty")
 		}
-		p.DataProxy.ProxyDatasourceRequestWithUID(newCtx, datasourceUID)
+		p.DataProxy.ProxyDatasourceRequestWithUID(proxyContext, datasourceUID)
 	}
 
 	status := resp.Status()

pkg/services/ngalert/api/util_test.go

@@ -1,9 +1,21 @@
 package api
 
 import (
+	"math/rand"
+	"net/http"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/api/response"
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/models"
+	accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
+	"github.com/grafana/grafana/pkg/services/org"
+	"github.com/grafana/grafana/pkg/services/user"
+	"github.com/grafana/grafana/pkg/util"
+	"github.com/grafana/grafana/pkg/web"
 )
 
 func TestToMacaronPath(t *testing.T) {
@@ -25,3 +37,124 @@ func TestToMacaronPath(t *testing.T) {
 		assert.Equal(t, tc.expectedOutputPath, outputPath)
 	}
 }
+
+func TestAlertingProxy_createProxyContext(t *testing.T) {
+	ctx := &models.ReqContext{
+		Context: &web.Context{
+			Router: web.NewRouter(),
+			Req:    &http.Request{},
+		},
+		SignedInUser:          &user.SignedInUser{},
+		UserToken:             &models.UserToken{},
+		IsSignedIn:            rand.Int63()%2 == 1,
+		IsRenderCall:          rand.Int63()%2 == 1,
+		AllowAnonymous:        rand.Int63()%2 == 1,
+		SkipCache:             rand.Int63()%2 == 1,
+		Logger:                log.New("test"),
+		RequestNonce:          util.GenerateShortUID(),
+		IsPublicDashboardView: rand.Int63()%2 == 1,
+	}
+
+	t.Run("should create a copy of request context", func(t *testing.T) {
+		for _, mock := range []*accesscontrolmock.Mock{
+			accesscontrolmock.New(), accesscontrolmock.New().WithDisabled(),
+		} {
+			proxy := AlertingProxy{
+				DataProxy: nil,
+				ac:        mock,
+			}
+
+			req := &http.Request{}
+			resp := &response.NormalResponse{}
+
+			newCtx := proxy.createProxyContext(ctx, req, resp)
+
+			require.NotEqual(t, ctx, newCtx)
+			require.Equal(t, ctx.UserToken, newCtx.UserToken)
+			require.Equal(t, ctx.IsSignedIn, newCtx.IsSignedIn)
+			require.Equal(t, ctx.IsRenderCall, newCtx.IsRenderCall)
+			require.Equal(t, ctx.AllowAnonymous, newCtx.AllowAnonymous)
+			require.Equal(t, ctx.SkipCache, newCtx.SkipCache)
+			require.Equal(t, ctx.Logger, newCtx.Logger)
+			require.Equal(t, ctx.RequestNonce, newCtx.RequestNonce)
+			require.Equal(t, ctx.IsPublicDashboardView, newCtx.IsPublicDashboardView)
+		}
+	})
+	t.Run("should overwrite response writer", func(t *testing.T) {
+		proxy := AlertingProxy{
+			DataProxy: nil,
+			ac:        accesscontrolmock.New(),
+		}
+
+		req := &http.Request{}
+		resp := &response.NormalResponse{}
+
+		newCtx := proxy.createProxyContext(ctx, req, resp)
+
+		require.NotEqual(t, ctx.Context.Resp, newCtx.Context.Resp)
+		require.Equal(t, ctx.Context.Router, newCtx.Context.Router)
+		require.Equal(t, ctx.Context.Req, newCtx.Context.Req)
+
+		require.NotEqual(t, 123, resp.Status())
+		newCtx.Context.Resp.WriteHeader(123)
+		require.Equal(t, 123, resp.Status())
+	})
+	t.Run("if access control is enabled", func(t *testing.T) {
+		t.Run("should elevate permissions to Editor for Viewer", func(t *testing.T) {
+			proxy := AlertingProxy{
+				DataProxy: nil,
+				ac:        accesscontrolmock.New(),
+			}
+
+			req := &http.Request{}
+			resp := &response.NormalResponse{}
+
+			viewerCtx := *ctx
+			viewerCtx.SignedInUser = &user.SignedInUser{
+				OrgRole: org.RoleViewer,
+			}
+
+			newCtx := proxy.createProxyContext(&viewerCtx, req, resp)
+			require.NotEqual(t, viewerCtx.SignedInUser, newCtx.SignedInUser)
+			require.Truef(t, newCtx.SignedInUser.HasRole(org.RoleEditor), "user of the proxy request should have at least Editor role but has %s", newCtx.SignedInUser.OrgRole)
+		})
+		t.Run("should not alter user if it is Editor", func(t *testing.T) {
+			proxy := AlertingProxy{
+				DataProxy: nil,
+				ac:        accesscontrolmock.New(),
+			}
+
+			req := &http.Request{}
+			resp := &response.NormalResponse{}
+
+			for _, roleType := range []org.RoleType{org.RoleEditor, org.RoleAdmin} {
+				roleCtx := *ctx
+				roleCtx.SignedInUser = &user.SignedInUser{
+					OrgRole: roleType,
+				}
+				newCtx := proxy.createProxyContext(&roleCtx, req, resp)
+				require.Equalf(t, roleCtx.SignedInUser, newCtx.SignedInUser, "user should not be altered if role is %s", roleType)
+			}
+		})
+	})
+	t.Run("if access control is disabled", func(t *testing.T) {
+		t.Run("should not alter user", func(t *testing.T) {
+			proxy := AlertingProxy{
+				DataProxy: nil,
+				ac:        accesscontrolmock.New().WithDisabled(),
+			}
+
+			req := &http.Request{}
+			resp := &response.NormalResponse{}
+
+			for _, roleType := range []org.RoleType{org.RoleViewer, org.RoleEditor, org.RoleAdmin} {
+				roleCtx := *ctx
+				roleCtx.SignedInUser = &user.SignedInUser{
+					OrgRole: roleType,
+				}
+				newCtx := proxy.createProxyContext(&roleCtx, req, resp)
+				require.Equalf(t, roleCtx.SignedInUser, newCtx.SignedInUser, "user should not be altered if access control is disabled and role is %s", roleType)
+			}
+		})
+	})
+}