Access Control: refactor permission evaluator to be more flexible (#35996)

* add a more flexible way to create permissions

* update interface for accesscontrol to use new eval interface

* use new eval interface

* update middleware to use new eval interface

* remove evaluator function and move metrics to service

* add tests for accesscontrol middleware

* Remove failed function from interface and update inejct to create a new
evaluator

* Change name

* Support Several sopes for a permission


* use evaluator and update fakeAccessControl

* Implement String that will return string representation of permissions
for an evaluator

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

Co-authored-by: Emil Tullstedt <emil.tullstedt@grafana.com>

pkg/services/accesscontrol/middleware/middleware.go

@@ -1,68 +1,50 @@
 package middleware
 
 import (
-	"bytes"
 	"fmt"
 	"net/http"
-	"text/template"
 	"time"
 
-	"github.com/grafana/grafana/pkg/util"
-
-	macaron "gopkg.in/macaron.v1"
+	"gopkg.in/macaron.v1"
 
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
+	"github.com/grafana/grafana/pkg/util"
 )
 
-func Middleware(ac accesscontrol.AccessControl) func(macaron.Handler, string, ...string) macaron.Handler {
-	return func(fallback macaron.Handler, permission string, scopes ...string) macaron.Handler {
+func Middleware(ac accesscontrol.AccessControl) func(macaron.Handler, accesscontrol.Evaluator) macaron.Handler {
+	return func(fallback macaron.Handler, evaluator accesscontrol.Evaluator) macaron.Handler {
 		if ac.IsDisabled() {
 			return fallback
 		}
 
 		return func(c *models.ReqContext) {
-			// We need this otherwise templated scopes get initialized only once, during the first call
-			runtimeScope := make([]string, len(scopes))
-			for i, scope := range scopes {
-				var buf bytes.Buffer
-
-				tmpl, err := template.New("scope").Parse(scope)
-				if err != nil {
-					c.JsonApiErr(http.StatusInternalServerError, "Internal server error", err)
-					return
-				}
-				err = tmpl.Execute(&buf, c.AllParams())
-				if err != nil {
-					c.JsonApiErr(http.StatusInternalServerError, "Internal server error", err)
-					return
-				}
-				runtimeScope[i] = buf.String()
-			}
-
-			hasAccess, err := ac.Evaluate(c.Req.Context(), c.SignedInUser, permission, runtimeScope...)
+			injected, err := evaluator.Inject(c.AllParams())
 			if err != nil {
-				Deny(c, permission, runtimeScope, err)
+				c.JsonApiErr(http.StatusInternalServerError, "Internal server error", err)
 				return
 			}
-			if !hasAccess {
-				Deny(c, permission, runtimeScope, nil)
+
+			hasAccess, err := ac.Evaluate(c.Req.Context(), c.SignedInUser, injected)
+			if !hasAccess || err != nil {
+				Deny(c, injected, err)
 				return
 			}
 		}
 	}
 }
 
-func Deny(c *models.ReqContext, permission string, scopes []string, err error) {
+func Deny(c *models.ReqContext, evaluator accesscontrol.Evaluator, err error) {
 	id := newID()
 	if err != nil {
 		c.Logger.Error("Error from access control system", "error", err, "accessErrorID", id)
 	} else {
-		c.Logger.Info("Access denied",
+		c.Logger.Info(
+			"Access denied",
 			"userID", c.UserId,
-			"permission", permission,
-			"scopes", scopes,
-			"accessErrorID", id)
+			"accessErrorID", id,
+			"permissions", evaluator.String(),
+		)
 	}
 
 	// If the user triggers an error in the access control system, we

pkg/services/accesscontrol/middleware/middleware_test.go

@@ -0,0 +1,121 @@
+package middleware
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+
+	"gopkg.in/macaron.v1"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/grafana/grafana/pkg/infra/log"
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/accesscontrol"
+)
+
+type middlewareTestCase struct {
+	desc           string
+	expectFallback bool
+	expectEndpoint bool
+	evaluator      accesscontrol.Evaluator
+	ac             accesscontrol.AccessControl
+}
+
+func TestMiddleware(t *testing.T) {
+	tests := []middlewareTestCase{
+		{
+			desc:           "should use fallback if access control is disabled",
+			ac:             fakeAccessControl{isDisabled: true},
+			expectFallback: true,
+			expectEndpoint: true,
+		},
+		{
+			desc: "should pass middleware for correct permissions",
+			ac: fakeAccessControl{
+				isDisabled:  false,
+				permissions: []*accesscontrol.Permission{{Action: "users:read", Scope: "users:*"}},
+			},
+			evaluator:      accesscontrol.EvalPermission("users:read", "users:*"),
+			expectFallback: false,
+			expectEndpoint: true,
+		},
+		{
+			desc: "should not reach endpoint when missing permissions",
+			ac: fakeAccessControl{
+				isDisabled:  false,
+				permissions: []*accesscontrol.Permission{{Action: "users:read", Scope: "users:1"}},
+			},
+			evaluator:      accesscontrol.EvalPermission("users:read", "users:*"),
+			expectFallback: false,
+			expectEndpoint: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			fallbackCalled := false
+			fallback := func(c *models.ReqContext) {
+				fallbackCalled = true
+			}
+
+			server := macaron.New()
+			server.UseMiddleware(macaron.Renderer("../../public/views", "[[", "]]"))
+
+			server.Use(contextProvider())
+			server.Use(Middleware(test.ac)(fallback, test.evaluator))
+
+			endpointCalled := false
+			server.Get("/", func(c *models.ReqContext) {
+				endpointCalled = true
+			})
+
+			request, err := http.NewRequest(http.MethodGet, "/", nil)
+			assert.NoError(t, err)
+			recorder := httptest.NewRecorder()
+
+			server.ServeHTTP(recorder, request)
+
+			assert.Equal(t, test.expectFallback, fallbackCalled)
+			assert.Equal(t, test.expectEndpoint, endpointCalled)
+		})
+	}
+}
+
+func contextProvider() macaron.Handler {
+	return func(c *macaron.Context) {
+		reqCtx := &models.ReqContext{
+			Context:      c,
+			Logger:       log.New(""),
+			SignedInUser: &models.SignedInUser{},
+			IsSignedIn:   true,
+			SkipCache:    true,
+		}
+		c.Map(reqCtx)
+	}
+}
+
+var _ accesscontrol.AccessControl = new(fakeAccessControl)
+
+type fakeAccessControl struct {
+	isDisabled  bool
+	permissions []*accesscontrol.Permission
+}
+
+func (f fakeAccessControl) Evaluate(ctx context.Context, user *models.SignedInUser, evaluator accesscontrol.Evaluator) (bool, error) {
+	permissions, _ := f.GetUserPermissions(ctx, user)
+	return evaluator.Evaluate(accesscontrol.GroupScopesByAction(permissions))
+}
+
+func (f fakeAccessControl) GetUserPermissions(ctx context.Context, user *models.SignedInUser) ([]*accesscontrol.Permission, error) {
+	return f.permissions, nil
+}
+
+func (f fakeAccessControl) IsDisabled() bool {
+	return f.isDisabled
+}
+
+func (f fakeAccessControl) DeclareFixedRoles(registration ...accesscontrol.RoleRegistration) error {
+	return nil
+}