diff --git a/pkg/api/api.go b/pkg/api/api.go index bc663b6939f..f54e5ee7a94 100644 --- a/pkg/api/api.go +++ b/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) { diff --git a/pkg/api/apikey.go b/pkg/api/apikey.go index a282ee2f257..15599f037f6 100644 --- a/pkg/api/apikey.go +++ b/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 { diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index 7c177329a26..d4da2814925 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/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 { diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index a5ed51dcde1..c017b3dee0d 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/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, diff --git a/pkg/services/serviceaccounts/api/token.go b/pkg/services/serviceaccounts/api/token.go index 787d06fdadb..dec1d498280 100644 --- a/pkg/services/serviceaccounts/api/token.go +++ b/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) diff --git a/pkg/services/serviceaccounts/api/token_test.go b/pkg/services/serviceaccounts/api/token_test.go index 38eb95de0bb..7f04cc8d1bb 100644 --- a/pkg/services/serviceaccounts/api/token_test.go +++ b/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, diff --git a/pkg/services/serviceaccounts/database/stats_test.go b/pkg/services/serviceaccounts/database/stats_test.go index 067bf756d9d..55405509c4b 100644 --- a/pkg/services/serviceaccounts/database/stats_test.go +++ b/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, diff --git a/pkg/services/serviceaccounts/database/token_store.go b/pkg/services/serviceaccounts/database/token_store.go index e69317e55c0..320bc2f1ed5 100644 --- a/pkg/services/serviceaccounts/database/token_store.go +++ b/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, diff --git a/pkg/services/serviceaccounts/database/token_store_test.go b/pkg/services/serviceaccounts/database/token_store_test.go index 2e24caa9c37..1560e19ffcd 100644 --- a/pkg/services/serviceaccounts/database/token_store_test.go +++ b/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, diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index f350f259a90..ffc354ce7f0 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/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"` diff --git a/pkg/services/serviceaccounts/serviceaccounts.go b/pkg/services/serviceaccounts/serviceaccounts.go index fc4c14ad869..632aa42a928 100644 --- a/pkg/services/serviceaccounts/serviceaccounts.go +++ b/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) } diff --git a/pkg/services/serviceaccounts/tests/common.go b/pkg/services/serviceaccounts/tests/common.go index b89a602dded..67c30d20a10 100644 --- a/pkg/services/serviceaccounts/tests/common.go +++ b/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 } diff --git a/public/app/features/serviceaccounts/CreateServiceAccountTokenModal.tsx b/public/app/features/serviceaccounts/CreateServiceAccountTokenModal.tsx index 996ee89962e..5cc322dc664 100644 --- a/public/app/features/serviceaccounts/CreateServiceAccountTokenModal.tsx +++ b/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), }) } diff --git a/public/app/features/serviceaccounts/ServiceAccountPage.tsx b/public/app/features/serviceaccounts/ServiceAccountPage.tsx index e8aaa35b572..66e59e0cc83 100644 --- a/public/app/features/serviceaccounts/ServiceAccountPage.tsx +++ b/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); }; diff --git a/public/app/features/serviceaccounts/state/actions.ts b/public/app/features/serviceaccounts/state/actions.ts index 0e1a6d750c7..48bfe6f1c2c 100644 --- a/public/app/features/serviceaccounts/state/actions.ts +++ b/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 { export function createServiceAccountToken( saID: number, - token: ApiKey, + token: ServiceAccountToken, onTokenCreated: (key: string) => void ): ThunkResult { return async (dispatch) => {