Access control: service account role check (#47710)

* forbid setting role higher than user's role

* change response code

* can assign API key permissions to non-admin users

* add: assign viewer role directly upon creation

* refactor: add AddSATcommand infavor of AddAPIkey

* refactor: frontend fixes for ServiceAccountToken

Co-authored-by: eleijonmarck <eric.leijonmarck@gmail.com>

pkg/api/api.go

@@ -279,7 +279,7 @@ func (hs *HTTPServer) registerRoutes() {
 			keysRoute.Get("/", authorize(reqOrgAdmin, ac.EvalPermission(ac.ActionAPIKeyRead, ac.ScopeAPIKeysAll)), routing.Wrap(hs.GetAPIKeys))
 			keysRoute.Post("/", authorize(reqOrgAdmin, ac.EvalPermission(ac.ActionAPIKeyCreate)), quota("api_key"), routing.Wrap(hs.AddAPIKey))
 			keysRoute.Delete("/:id", authorize(reqOrgAdmin, ac.EvalPermission(ac.ActionAPIKeyDelete, apikeyIDScope)), routing.Wrap(hs.DeleteAPIKey))
-		}, reqOrgAdmin)
+		})
 
 		// Preferences
 		apiRoute.Group("/preferences", func(prefRoute routing.RouteRegister) {

pkg/api/apikey.go

@@ -70,6 +70,9 @@ func (hs *HTTPServer) AddAPIKey(c *models.ReqContext) response.Response {
 	if !cmd.Role.IsValid() {
 		return response.Error(400, "Invalid role specified", nil)
 	}
+	if !c.OrgRole.Includes(cmd.Role) {
+		return response.Error(http.StatusForbidden, "Cannot assign a role higher than user's role", nil)
+	}
 
 	if hs.Cfg.ApiKeyMaxSecondsToLive != -1 {
 		if cmd.SecondsToLive == 0 {

pkg/services/serviceaccounts/api/api.go

@@ -187,6 +187,9 @@ func (api *ServiceAccountsAPI) updateServiceAccount(c *models.ReqContext) respon
 	if cmd.Role != nil && !cmd.Role.IsValid() {
 		return response.Error(http.StatusBadRequest, "Invalid role specified", nil)
 	}
+	if cmd.Role != nil && !c.OrgRole.Includes(*cmd.Role) {
+		return response.Error(http.StatusForbidden, "Cannot assign a role higher than user's role", nil)
+	}
 
 	resp, err := api.store.UpdateServiceAccount(c.Req.Context(), c.OrgId, scopeID, &cmd)
 	if err != nil {

pkg/services/serviceaccounts/api/api_test.go

@@ -227,7 +227,7 @@ func setupTestServer(t *testing.T, svc *tests.ServiceAccountMock,
 	m := web.New()
 	signedUser := &models.SignedInUser{
 		OrgId:   1,
-		OrgRole: models.ROLE_ADMIN,
+		OrgRole: models.ROLE_VIEWER,
 	}
 
 	m.Use(func(c *web.Context) {
@@ -344,13 +344,14 @@ func TestServiceAccountsAPI_UpdateServiceAccount(t *testing.T) {
 		Id           int
 	}
 
-	role := models.ROLE_ADMIN
+	viewerRole := models.ROLE_VIEWER
+	editorRole := models.ROLE_EDITOR
 	var invalidRole models.RoleType = "InvalidRole"
 	testCases := []testUpdateSATestCase{
 		{
 			desc: "should be ok to update serviceaccount with permissions",
-			user: &tests.TestUser{Login: "servicetest1@admin", IsServiceAccount: true, Role: "Editor", Name: "Unaltered"},
-			body: &serviceaccounts.UpdateServiceAccountForm{Name: newString("New Name"), Role: &role},
+			user: &tests.TestUser{Login: "servicetest1@admin", IsServiceAccount: true, Role: "Viewer", Name: "Unaltered"},
+			body: &serviceaccounts.UpdateServiceAccountForm{Name: newString("New Name"), Role: &viewerRole},
 			acmock: tests.SetupMockAccesscontrol(
 				t,
 				func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) {
@@ -360,6 +361,19 @@ func TestServiceAccountsAPI_UpdateServiceAccount(t *testing.T) {
 			),
 			expectedCode: http.StatusOK,
 		},
+		{
+			desc: "should be forbidden to set role higher than user's role",
+			user: &tests.TestUser{Login: "servicetest2@admin", IsServiceAccount: true, Role: "Viewer", Name: "Unaltered 2"},
+			body: &serviceaccounts.UpdateServiceAccountForm{Name: newString("New Name 2"), Role: &editorRole},
+			acmock: tests.SetupMockAccesscontrol(
+				t,
+				func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) {
+					return []*accesscontrol.Permission{{Action: serviceaccounts.ActionWrite, Scope: serviceaccounts.ScopeAll}}, nil
+				},
+				false,
+			),
+			expectedCode: http.StatusForbidden,
+		},
 		{
 			desc: "bad request when invalid role",
 			user: &tests.TestUser{Login: "servicetest3@admin", IsServiceAccount: true, Role: "Invalid", Name: "Unaltered"},
@@ -375,7 +389,7 @@ func TestServiceAccountsAPI_UpdateServiceAccount(t *testing.T) {
 		},
 		{
 			desc: "should be forbidden to update serviceaccount if no permissions",
-			user: &tests.TestUser{Login: "servicetest2@admin", IsServiceAccount: true},
+			user: &tests.TestUser{Login: "servicetest4@admin", IsServiceAccount: true},
 			body: nil,
 			acmock: tests.SetupMockAccesscontrol(
 				t,

pkg/services/serviceaccounts/api/token.go

@@ -17,13 +17,12 @@ import (
 const failedToDeleteMsg = "Failed to delete API key"
 
 type TokenDTO struct {
-	Id                     int64           `json:"id"`
-	Name                   string          `json:"name"`
-	Role                   models.RoleType `json:"role"`
-	Created                *time.Time      `json:"created"`
-	Expiration             *time.Time      `json:"expiration"`
-	SecondsUntilExpiration *float64        `json:"secondsUntilExpiration"`
-	HasExpired             bool            `json:"hasExpired"`
+	Id                     int64      `json:"id"`
+	Name                   string     `json:"name"`
+	Created                *time.Time `json:"created"`
+	Expiration             *time.Time `json:"expiration"`
+	SecondsUntilExpiration *float64   `json:"secondsUntilExpiration"`
+	HasExpired             bool       `json:"hasExpired"`
 }
 
 func hasExpired(expiration *int64) bool {
@@ -60,7 +59,6 @@ func (api *ServiceAccountsAPI) ListTokens(ctx *models.ReqContext) response.Respo
 			result[i] = &TokenDTO{
 				Id:                     t.Id,
 				Name:                   t.Name,
-				Role:                   t.Role,
 				Created:                &t.Created,
 				Expiration:             expiration,
 				SecondsUntilExpiration: &secondsUntilExpiration,
@@ -91,7 +89,7 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon
 		}
 	}
 
-	cmd := models.AddApiKeyCommand{}
+	cmd := serviceaccounts.AddServiceAccountTokenCommand{}
 	if err := web.Bind(c.Req, &cmd); err != nil {
 		return response.Error(http.StatusBadRequest, "Bad request data", err)
 	}
@@ -99,10 +97,6 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon
 	// Force affected service account to be the one referenced in the URL
 	cmd.OrgId = c.OrgId
 
-	if !cmd.Role.IsValid() {
-		return response.Error(http.StatusBadRequest, "Invalid role specified", nil)
-	}
-
 	if api.cfg.ApiKeyMaxSecondsToLive != -1 {
 		if cmd.SecondsToLive == 0 {
 			return response.Error(http.StatusBadRequest, "Number of seconds before expiration should be set", nil)

pkg/services/serviceaccounts/api/token_test.go

@@ -34,9 +34,8 @@ func createTokenforSA(t *testing.T, store serviceaccounts.Store, keyName string,
 	key, err := apikeygen.New(orgID, keyName)
 	require.NoError(t, err)
 
-	cmd := models.AddApiKeyCommand{
+	cmd := serviceaccounts.AddServiceAccountTokenCommand{
 		Name:          keyName,
-		Role:          "Viewer",
 		OrgId:         orgID,
 		Key:           key.HashedKey,
 		SecondsToLive: secondsToLive,

pkg/services/serviceaccounts/database/stats_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/grafana/grafana/pkg/components/apikeygen"
 	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/serviceaccounts"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts/tests"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -20,9 +21,8 @@ func TestStore_UsageStats(t *testing.T) {
 	key, err := apikeygen.New(sa.OrgId, keyName)
 	require.NoError(t, err)
 
-	cmd := models.AddApiKeyCommand{
+	cmd := serviceaccounts.AddServiceAccountTokenCommand{
 		Name:          keyName,
-		Role:          "Viewer",
 		OrgId:         sa.OrgId,
 		Key:           key.HashedKey,
 		SecondsToLive: 0,

pkg/services/serviceaccounts/database/token_store.go

@@ -5,10 +5,11 @@ import (
 	"time"
 
 	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/serviceaccounts"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
 )
 
-func (s *ServiceAccountsStoreImpl) AddServiceAccountToken(ctx context.Context, saID int64, cmd *models.AddApiKeyCommand) error {
+func (s *ServiceAccountsStoreImpl) AddServiceAccountToken(ctx context.Context, saID int64, cmd *serviceaccounts.AddServiceAccountTokenCommand) error {
 	return s.sqlStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
 		key := models.ApiKey{OrgId: cmd.OrgId, Name: cmd.Name}
 		exists, _ := sess.Get(&key)
@@ -28,7 +29,7 @@ func (s *ServiceAccountsStoreImpl) AddServiceAccountToken(ctx context.Context, s
 		t := models.ApiKey{
 			OrgId:            cmd.OrgId,
 			Name:             cmd.Name,
-			Role:             cmd.Role,
+			Role:             models.ROLE_VIEWER,
 			Key:              cmd.Key,
 			Created:          updated,
 			Updated:          updated,

pkg/services/serviceaccounts/database/token_store_test.go

@@ -6,6 +6,7 @@ import (
 
 	"github.com/grafana/grafana/pkg/components/apikeygen"
 	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/serviceaccounts"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts/tests"
 	"github.com/stretchr/testify/require"
 )
@@ -28,9 +29,8 @@ func TestStore_AddServiceAccountToken(t *testing.T) {
 			key, err := apikeygen.New(user.OrgId, keyName)
 			require.NoError(t, err)
 
-			cmd := models.AddApiKeyCommand{
+			cmd := serviceaccounts.AddServiceAccountTokenCommand{
 				Name:          keyName,
-				Role:          "Viewer",
 				OrgId:         user.OrgId,
 				Key:           key.HashedKey,
 				SecondsToLive: tc.secondsToLive,
@@ -79,9 +79,8 @@ func TestStore_DeleteServiceAccountToken(t *testing.T) {
 	key, err := apikeygen.New(user.OrgId, keyName)
 	require.NoError(t, err)
 
-	cmd := models.AddApiKeyCommand{
+	cmd := serviceaccounts.AddServiceAccountTokenCommand{
 		Name:          keyName,
-		Role:          "Viewer",
 		OrgId:         user.OrgId,
 		Key:           key.HashedKey,
 		SecondsToLive: 0,

pkg/services/serviceaccounts/models.go

@@ -40,6 +40,15 @@ type ServiceAccountDTO struct {
 	AvatarUrl     string          `json:"avatarUrl"`
 	AccessControl map[string]bool `json:"accessControl,omitempty"`
 }
+
+type AddServiceAccountTokenCommand struct {
+	Name          string         `json:"name" binding:"Required"`
+	OrgId         int64          `json:"-"`
+	Key           string         `json:"-"`
+	SecondsToLive int64          `json:"secondsToLive"`
+	Result        *models.ApiKey `json:"-"`
+}
+
 type SearchServiceAccountsResult struct {
 	TotalCount      int64                `json:"totalCount"`
 	ServiceAccounts []*ServiceAccountDTO `json:"serviceAccounts"`

pkg/services/serviceaccounts/serviceaccounts.go

@@ -26,6 +26,6 @@ type Store interface {
 	ConvertToServiceAccounts(ctx context.Context, keys []int64) error
 	ListTokens(ctx context.Context, orgID int64, serviceAccount int64) ([]*models.ApiKey, error)
 	DeleteServiceAccountToken(ctx context.Context, orgID, serviceAccountID, tokenID int64) error
-	AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *models.AddApiKeyCommand) error
+	AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *AddServiceAccountTokenCommand) error
 	GetUsageMetrics(ctx context.Context) (map[string]interface{}, error)
 }

pkg/services/serviceaccounts/tests/common.go

@@ -151,7 +151,7 @@ func (s *ServiceAccountsStoreMock) DeleteServiceAccountToken(ctx context.Context
 	return nil
 }
 
-func (s *ServiceAccountsStoreMock) AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *models.AddApiKeyCommand) error {
+func (s *ServiceAccountsStoreMock) AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *serviceaccounts.AddServiceAccountTokenCommand) error {
 	s.Calls.AddServiceAccountToken = append(s.Calls.AddServiceAccountToken, []interface{}{ctx, cmd})
 	return nil
 }

public/app/features/serviceaccounts/CreateServiceAccountTokenModal.tsx

@@ -15,17 +15,21 @@ import {
   RadioButtonGroup,
   useStyles2,
 } from '@grafana/ui';
-import { ApiKey, OrgRole } from 'app/types';
 
 const EXPIRATION_OPTIONS = [
   { label: 'No expiration', value: false },
   { label: 'Set expiration date', value: true },
 ];
 
+export type ServiceAccountToken = {
+  name: string;
+  secondsToLive: number;
+};
+
 interface CreateTokenModalProps {
   isOpen: boolean;
   token: string;
-  onCreateToken: (token: ApiKey) => void;
+  onCreateToken: (token: ServiceAccountToken) => void;
   onClose: () => void;
 }
 
@@ -95,7 +99,6 @@ export const CreateTokenModal = ({ isOpen, token, onCreateToken, onClose }: Crea
             onClick={() =>
               onCreateToken({
                 name: newTokenName,
-                role: OrgRole.Viewer,
                 secondsToLive: getSecondsToLive(newTokenExpirationDate),
               })
             }

public/app/features/serviceaccounts/ServiceAccountPage.tsx

@@ -17,7 +17,7 @@ import {
 import { ServiceAccountTokensTable } from './ServiceAccountTokensTable';
 import { getTimeZone, NavModel } from '@grafana/data';
 import { Button } from '@grafana/ui';
-import { CreateTokenModal } from './CreateServiceAccountTokenModal';
+import { CreateTokenModal, ServiceAccountToken } from './CreateServiceAccountTokenModal';
 import { contextSrv } from 'app/core/core';
 
 interface OwnProps extends GrafanaRouteComponentProps<{ id: string }> {
@@ -86,7 +86,7 @@ const ServiceAccountPageUnconnected = ({
     deleteServiceAccountToken(parseInt(match.params.id, 10), key.id!);
   };
 
-  const onCreateToken = (token: ApiKey) => {
+  const onCreateToken = (token: ServiceAccountToken) => {
     createServiceAccountToken(serviceAccount.id, token, setNewToken);
   };
 

public/app/features/serviceaccounts/state/actions.ts

@@ -1,4 +1,4 @@
-import { ApiKey, ServiceAccountDTO, ThunkResult, ServiceAccountFilter } from '../../../types';
+import { ServiceAccountDTO, ThunkResult, ServiceAccountFilter } from '../../../types';
 import { getBackendSrv, locationService } from '@grafana/runtime';
 import {
   acOptionsLoaded,
@@ -16,6 +16,7 @@ import {
 import { accessControlQueryParam } from 'app/core/utils/accessControl';
 import { fetchBuiltinRoles, fetchRoleOptions } from 'app/core/components/RolePicker/api';
 import { debounce } from 'lodash';
+import { ServiceAccountToken } from '../CreateServiceAccountTokenModal';
 
 const BASE_URL = `/api/serviceaccounts`;
 
@@ -55,7 +56,7 @@ export function loadServiceAccount(saID: number): ThunkResult<void> {
 
 export function createServiceAccountToken(
   saID: number,
-  token: ApiKey,
+  token: ServiceAccountToken,
   onTokenCreated: (key: string) => void
 ): ThunkResult<void> {
   return async (dispatch) => {