From 00ff61cb9e59b6ec51ca59ca6aa1f79ddd165dd8 Mon Sep 17 00:00:00 2001
From: Gabriel MABILLE <gamab@users.noreply.github.com>
Date: Wed, 3 Aug 2022 11:06:06 +0200
Subject: [PATCH] RBAC: Add an additional check on UserID before fetching the
 permissions (#53002)

* RBAC: add an additional check before fetching permissions

* Nit.

* Readd removed test

* change message
---
 .../accesscontrol/database/database.go        | 22 +++++++++++++------
 .../accesscontrol/database/database_test.go   | 17 +++++++++++++-
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/pkg/services/accesscontrol/database/database.go b/pkg/services/accesscontrol/database/database.go
index 9e4d02f4b92..f056555313f 100644
--- a/pkg/services/accesscontrol/database/database.go
+++ b/pkg/services/accesscontrol/database/database.go
@@ -60,8 +60,13 @@ func (s *AccessControlStore) GetUserPermissions(ctx context.Context, query acces
 }
 
 func userRolesFilter(orgID, userID int64, roles []string) (string, []interface{}) {
-	q := `
-	WHERE role.id IN (
+	params := []interface{}{}
+	q := `WHERE role.id IN (`
+
+	// This is an additional security. We should never have permissions granted to userID 0.
+	// Only allow real users to get user/team permissions (anonymous/apikeys)
+	if userID > 0 {
+		q += `
 		SELECT ur.role_id
 		FROM user_role AS ur
 		WHERE ur.user_id = ?
@@ -69,15 +74,18 @@ func userRolesFilter(orgID, userID int64, roles []string) (string, []interface{}
 		UNION
 		SELECT tr.role_id FROM team_role as tr
 		INNER JOIN team_member as tm ON tm.team_id = tr.team_id
-		WHERE tm.user_id = ? AND tr.org_id = ?
-	`
-	params := []interface{}{userID, orgID, globalOrgID, userID, orgID}
+		WHERE tm.user_id = ? AND tr.org_id = ?`
+		params = []interface{}{userID, orgID, globalOrgID, userID, orgID}
+	}
 
 	if len(roles) != 0 {
+		if userID > 0 {
+			q += `
+			UNION`
+		}
 		q += `
-			UNION
 			SELECT br.role_id FROM builtin_role AS br
-			WHERE role IN (? ` + strings.Repeat(", ?", len(roles)-1) + `)
+			WHERE br.role IN (? ` + strings.Repeat(", ?", len(roles)-1) + `)
 		`
 		for _, role := range roles {
 			params = append(params, role)
diff --git a/pkg/services/accesscontrol/database/database_test.go b/pkg/services/accesscontrol/database/database_test.go
index 1531523dc67..de26f5b851c 100644
--- a/pkg/services/accesscontrol/database/database_test.go
+++ b/pkg/services/accesscontrol/database/database_test.go
@@ -16,6 +16,7 @@ import (
 
 type getUserPermissionsTestCase struct {
 	desc               string
+	anonymousUser      bool
 	orgID              int64
 	role               string
 	userPermissions    []string
@@ -74,6 +75,16 @@ func TestAccessControlStore_GetUserPermissions(t *testing.T) {
 			expected:           0,
 			actions:            []string{},
 		},
+		{
+			desc:               "should only get br permissions for anonymous user",
+			anonymousUser:      true,
+			orgID:              1,
+			role:               "Admin",
+			userPermissions:    []string{"1", "2", "10"},
+			teamPermissions:    []string{"100", "2"},
+			builtinPermissions: []string{"5", "6"},
+			expected:           2,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
@@ -118,9 +129,13 @@ func TestAccessControlStore_GetUserPermissions(t *testing.T) {
 				}
 			}
 
+			userID := user.ID
+			if tt.anonymousUser {
+				userID = 0
+			}
 			permissions, err := store.GetUserPermissions(context.Background(), accesscontrol.GetUserPermissionsQuery{
 				OrgID:   tt.orgID,
-				UserID:  user.ID,
+				UserID:  userID,
 				Roles:   roles,
 				Actions: tt.actions,
 			})