EVEREST-1799 get groups claim and validate permissions

Author: recharte

internal/server/handlers/rbac/backup_storage_test.go

@@ -96,7 +96,7 @@ func TestRBAC_BackupStorage(t *testing.T) {
 			},
 		}
 
-		ctx := context.WithValue(context.Background(), common.UserCtxKey, "test-user")
+		ctx := context.WithValue(context.Background(), common.UserCtxKey, rbac.User{Subject: "test-user"})
 		for _, tc := range testCases {
 			t.Run(tc.desc, func(t *testing.T) {
 				t.Parallel()
@@ -174,7 +174,7 @@ func TestRBAC_BackupStorage(t *testing.T) {
 			},
 		}
 
-		ctx := context.WithValue(context.Background(), common.UserCtxKey, "test-user")
+		ctx := context.WithValue(context.Background(), common.UserCtxKey, rbac.User{Subject: "test-user"})
 		for _, tc := range testCases {
 			t.Run(tc.desc, func(t *testing.T) {
 				t.Parallel()
@@ -216,10 +216,10 @@ func newConfigMapPolicy(policy string) *corev1.ConfigMap {
 	}
 }
 
-func testUserGetter(ctx context.Context) (string, error) {
-	user, ok := ctx.Value(common.UserCtxKey).(string)
+func testUserGetter(ctx context.Context) (rbac.User, error) {
+	user, ok := ctx.Value(common.UserCtxKey).(rbac.User)
 	if !ok {
-		return "", errors.New("user not found in context")
+		return rbac.User{}, errors.New("user not found in context")
 	}
 	return user, nil
 }

internal/server/handlers/rbac/handler.go

@@ -21,7 +21,7 @@ type rbacHandler struct {
 	enforcer   casbin.IEnforcer
 	log        *zap.SugaredLogger
 	next       handlers.Handler
-	userGetter func(ctx context.Context) (string, error)
+	userGetter func(ctx context.Context) (rbac.User, error)
 }
 
 // New returns a new RBAC handler.
@@ -55,17 +55,23 @@ func (h *rbacHandler) enforce(
 	action,
 	object string,
 ) error {
-	subject, err := h.userGetter(ctx)
+	user, err := h.userGetter(ctx)
 	if err != nil {
 		return err
 	}
-	ok, err := h.enforcer.Enforce(subject, resource, action, object)
-	if err != nil {
-		return fmt.Errorf("enforce error: %w", err)
-	}
-	if !ok {
-		h.log.Warnf("Permission denied: [%s %s %s %s]", subject, resource, action, object)
-		return ErrInsufficientPermissions
+
+	// User is allowed to perform the operation if the user's subjects or any
+	// of its groups have the required permission.
+	for _, sub := range append([]string{user.Subject}, user.Groups...) {
+		ok, err := h.enforcer.Enforce(sub, resource, action, object)
+		if err != nil {
+			return fmt.Errorf("enforce error: %w", err)
+		}
+		if ok {
+			return nil
+		}
 	}
-	return nil
+
+	h.log.Warnf("Permission denied: [%s %s %s %s]", user.Subject, resource, action, object)
+	return ErrInsufficientPermissions
 }

internal/server/handlers/rbac/kubernetes.go

@@ -3,7 +3,6 @@ package rbac
 
 import (
 	"context"
-	"errors"
 	"fmt"
 
 	"github.com/AlekSi/pointer"
@@ -24,15 +23,39 @@ func (h *rbacHandler) GetUserPermissions(ctx context.Context) (*api.UserPermissi
 	if err != nil {
 		return nil, err
 	}
-	perms, err := h.enforcer.GetImplicitPermissionsForUser(user)
-	if err != nil {
-		return nil, fmt.Errorf("failed to GetImplicitPermissionsForUser: %w", err)
+
+	// Let's use a map to deduplicate the permissions after resolving all roles
+	permsMap := make(map[[4]string]struct{})
+
+	// Get permissions for the user and the groups it belongs to
+	for _, sub := range append([]string{user.Subject}, user.Groups...) {
+		perms, err := h.enforcer.GetImplicitPermissionsForUser(sub)
+		if err != nil {
+			return nil, fmt.Errorf("failed to GetImplicitPermissionsForUser: %w", err)
+		}
+
+		// GetImplicitPermissionsForUser returns all policies assigned to the
+		// user/group directly as well as the policies assigned to the roles
+		// the user/group has. We need to resolve all roles for the user.
+		for _, perm := range perms {
+			if len(perm) != 4 {
+				// This should never happen, but let's be safe
+				return nil, fmt.Errorf("invalid permission")
+			}
+
+			// We don't want to expose the groups or roles in the permissions
+			// so we replace them with the user
+			permsMap[[4]string{user.Subject, perm[1], perm[2], perm[3]}] = struct{}{}
+		}
 	}
 
-	if err := h.resolveRoles(user, perms); err != nil {
-		return nil, err
+	// Convert the map back to a slice
+	result := make([][]string, len(permsMap))
+	i := 0
+	for k := range permsMap {
+		result[i] = []string(k[:])
+		i++
 	}
-	result := pointer.To(perms)
 
 	nextRes, err := h.next.GetUserPermissions(ctx)
 	if err != nil {
@@ -45,25 +68,8 @@ func (h *rbacHandler) GetUserPermissions(ctx context.Context) (*api.UserPermissi
 	}
 
 	res := &api.UserPermissions{
-		Permissions: result,
+		Permissions: pointer.To(result),
 		Enabled:     enabled,
 	}
 	return res, nil
 }
-
-// For a given set of `permissions` for a `user`, this function
-// will resolve all roles for the user.
-func (h *rbacHandler) resolveRoles(user string, permissions [][]string) error {
-	userRoles, err := h.enforcer.GetRolesForUser(user)
-	if err != nil {
-		return errors.Join(err, errors.New("cannot get user roles"))
-	}
-	for _, role := range userRoles {
-		for i, perm := range permissions {
-			if perm[0] == role {
-				permissions[i][0] = user
-			}
-		}
-	}
-	return nil
-}

internal/server/handlers/rbac/kubernetes_test.go

@@ -0,0 +1,150 @@
+package rbac
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/percona/everest/api"
+	"github.com/percona/everest/internal/server/handlers"
+	"github.com/percona/everest/pkg/common"
+	"github.com/percona/everest/pkg/rbac"
+)
+
+func TestRBAC_Kubernetes(t *testing.T) {
+	t.Parallel()
+
+	data := func() *handlers.MockHandler {
+		next := handlers.MockHandler{}
+		next.On("GetUserPermissions",
+			mock.Anything,
+		).Return(
+			&api.UserPermissions{
+				Enabled: true,
+			},
+			nil,
+		)
+		return &next
+	}
+
+	t.Run("GetUserPermissions", func(t *testing.T) {
+		t.Parallel()
+
+		testCases := []struct {
+			desc     string
+			user     rbac.User
+			policy   string
+			outPerms [][]string
+		}{
+			{
+				desc: "default admin permissions",
+				user: rbac.User{
+					Subject: "test-user",
+				},
+				policy: newPolicy(
+					"g, test-user, role:admin",
+				),
+				outPerms: [][]string{
+					{"test-user", "monitoring-instances", "*", "*/*"},
+					{"test-user", "database-cluster-backups", "*", "*/*"},
+					{"test-user", "database-cluster-restores", "*", "*/*"},
+					{"test-user", "database-clusters", "*", "*/*"},
+					{"test-user", "database-cluster-credentials", "*", "*/*"},
+					{"test-user", "database-engines", "*", "*/*"},
+					{"test-user", "namespaces", "*", "*"},
+					{"test-user", "backup-storages", "*", "*/*"},
+				},
+			},
+			{
+				desc: "permissions from different roles are merged",
+				user: rbac.User{
+					Subject: "test-user",
+				},
+				policy: newPolicy(
+					"p, test-user, database-clusters, *, */*",
+					"p, role:creater, database-clusters, create, */*",
+					"p, role:reader, database-clusters, read, */*",
+					"p, role:updater, database-clusters, update, */*",
+					"p, role:deleter, database-clusters, delete, */*",
+					"g, test-user, role:creater",
+					"g, test-user, role:reader",
+					"g, test-user, role:updater",
+					"g, another-user, role:deleter",
+				),
+				outPerms: [][]string{
+					{"test-user", "database-clusters", "*", "*/*"},
+					{"test-user", "database-clusters", "create", "*/*"},
+					{"test-user", "database-clusters", "read", "*/*"},
+					{"test-user", "database-clusters", "update", "*/*"},
+				},
+			},
+			{
+				desc: "permissions from different groups are merged",
+				user: rbac.User{
+					Subject: "test-user",
+					Groups:  []string{"test-group-1", "test-group-2"},
+				},
+				policy: newPolicy(
+					"p, test-user, database-clusters, read, */*",
+					"p, test-group-1, database-clusters, create, */*",
+					"p, test-group-2, database-clusters, update, */*",
+					"p, test-group-3, database-clusters, delete, */*",
+				),
+				outPerms: [][]string{
+					{"test-user", "database-clusters", "read", "*/*"},
+					{"test-user", "database-clusters", "create", "*/*"},
+					{"test-user", "database-clusters", "update", "*/*"},
+				},
+			},
+			{
+				desc: "duplicate permissions are removed",
+				user: rbac.User{
+					Subject: "test-user",
+				},
+				policy: newPolicy(
+					"p, test-user, database-clusters, *, */*",
+					"p, role:test, database-clusters, *, */*",
+					"g, test-user, role:test",
+				),
+				outPerms: [][]string{
+					{"test-user", "database-clusters", "*", "*/*"},
+				},
+			},
+			{
+				desc: "no policy",
+				user: rbac.User{
+					Subject: "test-user",
+				},
+				policy:   newPolicy(),
+				outPerms: [][]string{},
+			},
+		}
+
+		for _, tc := range testCases {
+			ctx := context.WithValue(context.Background(), common.UserCtxKey, tc.user)
+			t.Run(tc.desc, func(t *testing.T) {
+				t.Parallel()
+				k8sMock := newConfigMapMock(tc.policy)
+				enf, err := rbac.NewEnforcer(ctx, k8sMock, zap.NewNop().Sugar())
+				require.NoError(t, err)
+				next := data()
+
+				h := &rbacHandler{
+					next:       next,
+					log:        zap.NewNop().Sugar(),
+					enforcer:   enf,
+					userGetter: testUserGetter,
+				}
+
+				perms, err := h.GetUserPermissions(ctx)
+				require.NoError(t, err)
+				assert.True(t, perms.Enabled)
+				assert.ElementsMatch(t, tc.outPerms, *perms.Permissions)
+			})
+		}
+	})
+}

pkg/rbac/rbac.go

@@ -68,6 +68,11 @@ const (
 	rbacEnabledValueTrue = "true"
 )
 
+type User struct {
+	Subject string
+	Groups  []string
+}
+
 // Setup a new informer that watches our RBAC ConfigMap.
 // This informer reloads the policy whenever the ConfigMap is updated.
 func refreshEnforcerInBackground(
@@ -178,31 +183,59 @@ func NewEnforcer(ctx context.Context, kubeClient kubernetes.KubernetesConnector,
 }
 
 // GetUser extracts the user from the JWT token in the context.
-func GetUser(ctx context.Context) (string, error) {
+func GetUser(ctx context.Context) (User, error) {
 	token, ok := ctx.Value(common.UserCtxKey).(*jwt.Token)
 	if !ok {
-		return "", errors.New("failed to get token from context")
+		return User{}, errors.New("failed to get token from context")
 	}
 
 	claims, ok := token.Claims.(jwt.MapClaims) // by default claims is of type `jwt.MapClaims`
 	if !ok {
-		return "", errors.New("failed to get claims from token")
+		return User{}, errors.New("failed to get claims from token")
 	}
 
 	subject, err := claims.GetSubject()
 	if err != nil {
-		return "", errors.Join(err, errors.New("failed to get subject from claims"))
+		return User{}, errors.Join(err, errors.New("failed to get subject from claims"))
 	}
 
 	issuer, err := claims.GetIssuer()
 	if err != nil {
-		return "", errors.Join(err, errors.New("failed to get issuer from claims"))
+		return User{}, errors.Join(err, errors.New("failed to get issuer from claims"))
 	}
 
 	if issuer == session.SessionManagerClaimsIssuer {
-		return strings.Split(subject, ":")[0], nil
+		subject = strings.Split(subject, ":")[0]
+	}
+
+	groups := getScopeValues(claims, []string{"groups"})
+	return User{Subject: subject, Groups: groups}, nil
+}
+
+func getScopeValues(claims jwt.MapClaims, scopes []string) []string {
+	groups := []string{}
+	for i := range scopes {
+		scopeIf, ok := claims[scopes[i]]
+		if !ok {
+			continue
+		}
+
+		switch val := scopeIf.(type) {
+		case []interface{}:
+			for _, groupIf := range val {
+				group, ok := groupIf.(string)
+				if ok {
+					groups = append(groups, group)
+				}
+			}
+		case []string:
+			groups = append(groups, val...)
+		case string:
+			groups = append(groups, val)
+		}
 	}
-	return subject, nil
+
+	return groups
 }
 
 func loadAdminPolicy(enf casbin.IEnforcer) error {

ui/apps/everest/src/App.tsx

@@ -72,7 +72,7 @@ const App = () => {
               oidcConfig={{
                 ...configs?.oidc,
                 redirectUri: `${window.location.protocol}//${window.location.host}/login-callback`,
-                scope: 'openid profile email',
+                scope: 'openid profile email groups',
                 responseType: 'code',
                 autoSignIn: false,
                 automaticSilentRenew: false,