Separate API key store from SA token store (#45862)

* ServiceAccounts: Fix token-apikey cross deletion

* ServiceAccounts: separate API key store and service account token store

* ServiceAccounts: hide service account tokens from API Keys page

* ServiceAccounts: uppercase statement

* ServiceAccounts: fix and add new tests for SAT store

* ServiceAccounts: remove service account ID from add API key

* ServiceAccounts: clear up errors

pkg/services/serviceaccounts/api/api.go

@@ -1,7 +1,6 @@
 package api
 
 import (
-	"context"
 	"errors"
 	"net/http"
 	"strconv"
@@ -20,19 +19,12 @@ import (
 	"github.com/grafana/grafana/pkg/web"
 )
 
-type APIKeyStore interface {
-	AddAPIKey(ctx context.Context, cmd *models.AddApiKeyCommand) error
-	GetApiKeyById(ctx context.Context, query *models.GetApiKeyByIdQuery) error
-	DeleteApiKey(ctx context.Context, cmd *models.DeleteApiKeyCommand) error
-}
-
 type ServiceAccountsAPI struct {
 	cfg            *setting.Cfg
 	service        serviceaccounts.Service
 	accesscontrol  accesscontrol.AccessControl
 	RouterRegister routing.RouteRegister
 	store          serviceaccounts.Store
-	apiKeyStore    APIKeyStore
 	log            log.Logger
 }
 
@@ -47,7 +39,6 @@ func NewServiceAccountsAPI(
 	accesscontrol accesscontrol.AccessControl,
 	routerRegister routing.RouteRegister,
 	store serviceaccounts.Store,
-	apiKeyStore APIKeyStore,
 ) *ServiceAccountsAPI {
 	return &ServiceAccountsAPI{
 		cfg:            cfg,
@@ -55,7 +46,6 @@ func NewServiceAccountsAPI(
 		accesscontrol:  accesscontrol,
 		RouterRegister: routerRegister,
 		store:          store,
-		apiKeyStore:    apiKeyStore,
 		log:            log.New("serviceaccounts.api"),
 	}
 }

pkg/services/serviceaccounts/api/api_test.go

@@ -102,7 +102,7 @@ func setupTestServer(t *testing.T, svc *tests.ServiceAccountMock,
 	routerRegister routing.RouteRegister,
 	acmock *accesscontrolmock.Mock,
 	sqlStore *sqlstore.SQLStore, saStore serviceaccounts.Store) *web.Mux {
-	a := NewServiceAccountsAPI(setting.NewCfg(), svc, acmock, routerRegister, saStore, sqlStore)
+	a := NewServiceAccountsAPI(setting.NewCfg(), svc, acmock, routerRegister, saStore)
 	a.RegisterAPIEndpoints(featuremgmt.WithFeatures(featuremgmt.FlagServiceAccounts))
 
 	a.cfg.ApiKeyMaxSecondsToLive = -1 // disable api key expiration

pkg/services/serviceaccounts/api/token.go

@@ -97,7 +97,6 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon
 	}
 
 	// Force affected service account to be the one referenced in the URL
-	cmd.ServiceAccountId = &saID
 	cmd.OrgId = c.OrgId
 
 	if !cmd.Role.IsValid() {
@@ -120,7 +119,7 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon
 
 	cmd.Key = newKeyInfo.HashedKey
 
-	if err := api.apiKeyStore.AddAPIKey(c.Req.Context(), &cmd); err != nil {
+	if err := api.store.AddServiceAccountToken(c.Req.Context(), saID, &cmd); err != nil {
 		if errors.Is(err, models.ErrInvalidApiKeyExpiration) {
 			return response.Error(http.StatusBadRequest, err.Error(), nil)
 		}
@@ -143,7 +142,7 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon
 func (api *ServiceAccountsAPI) DeleteToken(c *models.ReqContext) response.Response {
 	saID, err := strconv.ParseInt(web.Params(c.Req)[":serviceAccountId"], 10, 64)
 	if err != nil {
-		return response.Error(http.StatusBadRequest, "serviceAccountId is invalid", err)
+		return response.Error(http.StatusBadRequest, "Service Account ID is invalid", err)
 	}
 
 	// confirm service account exists
@@ -158,29 +157,10 @@ func (api *ServiceAccountsAPI) DeleteToken(c *models.ReqContext) response.Respon
 
 	tokenID, err := strconv.ParseInt(web.Params(c.Req)[":tokenId"], 10, 64)
 	if err != nil {
-		return response.Error(http.StatusBadRequest, "serviceAccountId is invalid", err)
+		return response.Error(http.StatusBadRequest, "Token ID is invalid", err)
 	}
 
-	// confirm API key belongs to service account. TODO: refactor get & delete to single call
-	cmdGet := &models.GetApiKeyByIdQuery{ApiKeyId: tokenID}
-	if err = api.apiKeyStore.GetApiKeyById(c.Req.Context(), cmdGet); err != nil {
-		status := http.StatusNotFound
-		if err != nil && !errors.Is(err, models.ErrApiKeyNotFound) {
-			status = http.StatusInternalServerError
-		} else {
-			err = models.ErrApiKeyNotFound
-		}
-
-		return response.Error(status, failedToDeleteMsg, err)
-	}
-
-	// verify service account ID matches the URL
-	if *cmdGet.Result.ServiceAccountId != saID {
-		return response.Error(http.StatusNotFound, failedToDeleteMsg, err)
-	}
-
-	cmdDel := &models.DeleteApiKeyCommand{Id: tokenID, OrgId: c.OrgId}
-	if err = api.apiKeyStore.DeleteApiKey(c.Req.Context(), cmdDel); err != nil {
+	if err = api.store.DeleteServiceAccountToken(c.Req.Context(), c.OrgId, saID, tokenID); err != nil {
 		status := http.StatusNotFound
 		if err != nil && !errors.Is(err, models.ErrApiKeyNotFound) {
 			status = http.StatusInternalServerError

pkg/services/serviceaccounts/api/token_test.go

@@ -12,7 +12,6 @@ import (
 	"time"
 
 	"github.com/grafana/grafana/pkg/api/routing"
-	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/components/apikeygen"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
@@ -31,19 +30,20 @@ const (
 	serviceaccountIDTokensDetailPath = "/api/serviceaccounts/%v/tokens/%v" // #nosec G101
 )
 
-func createTokenforSA(t *testing.T, keyName string, orgID int64, saID int64, secondsToLive int64) *models.ApiKey {
+func createTokenforSA(t *testing.T, store serviceaccounts.Store, keyName string, orgID int64, saID int64, secondsToLive int64) *models.ApiKey {
 	key, err := apikeygen.New(orgID, keyName)
 	require.NoError(t, err)
+
 	cmd := models.AddApiKeyCommand{
-		Name:             keyName,
-		Role:             "Viewer",
-		OrgId:            orgID,
-		Key:              key.HashedKey,
-		SecondsToLive:    secondsToLive,
-		ServiceAccountId: &saID,
-		Result:           &models.ApiKey{},
+		Name:          keyName,
+		Role:          "Viewer",
+		OrgId:         orgID,
+		Key:           key.HashedKey,
+		SecondsToLive: secondsToLive,
+		Result:        &models.ApiKey{},
 	}
-	err = bus.Dispatch(context.Background(), &cmd)
+
+	err = store.AddServiceAccountToken(context.Background(), saID, &cmd)
 	require.NoError(t, err)
 	return cmd.Result
 }
@@ -156,7 +156,8 @@ func TestServiceAccountsAPI_CreateToken(t *testing.T) {
 
 func TestServiceAccountsAPI_DeleteToken(t *testing.T) {
 	store := sqlstore.InitTestDB(t)
-	svcmock := tests.ServiceAccountMock{}
+	svcMock := &tests.ServiceAccountMock{}
+	saStore := database.NewServiceAccountsStore(store)
 	sa := tests.SetupUserServiceAccount(t, store, tests.TestUser{Login: "sa", IsServiceAccount: true})
 
 	type testCreateSAToken struct {
@@ -216,11 +217,11 @@ func TestServiceAccountsAPI_DeleteToken(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			token := createTokenforSA(t, tc.keyName, sa.OrgId, sa.Id, 1)
+			token := createTokenforSA(t, saStore, tc.keyName, sa.OrgId, sa.Id, 1)
 
 			endpoint := fmt.Sprintf(serviceaccountIDTokensDetailPath, sa.Id, token.Id)
 			bodyString := ""
-			server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, database.NewServiceAccountsStore(store))
+			server := setupTestServer(t, svcMock, routing.NewRouteRegister(), tc.acmock, store, saStore)
 			actual := requestResponse(server, http.MethodDelete, endpoint, strings.NewReader(bodyString))
 
 			actualCode := actual.Code

pkg/services/serviceaccounts/database/errors.go

@@ -0,0 +1,41 @@
+package database
+
+import (
+	"fmt"
+
+	"github.com/grafana/grafana/pkg/models"
+)
+
+type ErrMisingSAToken struct {
+}
+
+func (e *ErrMisingSAToken) Error() string {
+	return "service account token not found"
+}
+
+func (e *ErrMisingSAToken) Unwrap() error {
+	return models.ErrApiKeyNotFound
+}
+
+type ErrInvalidExpirationSAToken struct {
+}
+
+func (e *ErrInvalidExpirationSAToken) Error() string {
+	return "service account token not found"
+}
+
+func (e *ErrInvalidExpirationSAToken) Unwrap() error {
+	return models.ErrInvalidApiKeyExpiration
+}
+
+type ErrDuplicateSAToken struct {
+	name string
+}
+
+func (e *ErrDuplicateSAToken) Error() string {
+	return fmt.Sprintf("service account token %s already exists", e.name)
+}
+
+func (e *ErrDuplicateSAToken) Unwrap() error {
+	return models.ErrDuplicateApiKey
+}

pkg/services/serviceaccounts/database/token_store.go

@@ -0,0 +1,63 @@
+package database
+
+import (
+	"context"
+	"time"
+
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/sqlstore"
+)
+
+func (s *ServiceAccountsStoreImpl) AddServiceAccountToken(ctx context.Context, saID int64, cmd *models.AddApiKeyCommand) error {
+	return s.sqlStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error {
+		key := models.ApiKey{OrgId: cmd.OrgId, Name: cmd.Name}
+		exists, _ := sess.Get(&key)
+		if exists {
+			return &ErrDuplicateSAToken{cmd.Name}
+		}
+
+		updated := time.Now()
+		var expires *int64 = nil
+		if cmd.SecondsToLive > 0 {
+			v := updated.Add(time.Second * time.Duration(cmd.SecondsToLive)).Unix()
+			expires = &v
+		} else if cmd.SecondsToLive < 0 {
+			return &ErrInvalidExpirationSAToken{}
+		}
+
+		t := models.ApiKey{
+			OrgId:            cmd.OrgId,
+			Name:             cmd.Name,
+			Role:             cmd.Role,
+			Key:              cmd.Key,
+			Created:          updated,
+			Updated:          updated,
+			Expires:          expires,
+			ServiceAccountId: &saID,
+		}
+
+		if _, err := sess.Insert(&t); err != nil {
+			return err
+		}
+		cmd.Result = &t
+		return nil
+	})
+}
+
+func (s *ServiceAccountsStoreImpl) DeleteServiceAccountToken(ctx context.Context, orgID, serviceAccountID, tokenID int64) error {
+	rawSQL := "DELETE FROM api_key WHERE id=? and org_id=? and service_account_id=?"
+
+	return s.sqlStore.WithDbSession(ctx, func(sess *sqlstore.DBSession) error {
+		result, err := sess.Exec(rawSQL, tokenID, orgID, serviceAccountID)
+		if err != nil {
+			return err
+		}
+		n, err := result.RowsAffected()
+		if err != nil {
+			return err
+		} else if n == 0 {
+			return &ErrMisingSAToken{}
+		}
+		return nil
+	})
+}

pkg/services/serviceaccounts/database/token_store_test.go

@@ -0,0 +1,115 @@
+package database
+
+import (
+	"context"
+	"testing"
+
+	"github.com/grafana/grafana/pkg/components/apikeygen"
+	"github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/services/serviceaccounts/tests"
+	"github.com/stretchr/testify/require"
+)
+
+func TestStore_AddServiceAccountToken(t *testing.T) {
+	userToCreate := tests.TestUser{Login: "servicetestwithTeam@admin", IsServiceAccount: true}
+	db, store := setupTestDatabase(t)
+	user := tests.SetupUserServiceAccount(t, db, userToCreate)
+
+	type testCasesAdd struct {
+		secondsToLive int64
+		desc          string
+	}
+
+	testCases := []testCasesAdd{{-10, "invalid"}, {0, "no expiry"}, {10, "valid"}}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			keyName := t.Name()
+			key, err := apikeygen.New(user.OrgId, keyName)
+			require.NoError(t, err)
+
+			cmd := models.AddApiKeyCommand{
+				Name:          keyName,
+				Role:          "Viewer",
+				OrgId:         user.OrgId,
+				Key:           key.HashedKey,
+				SecondsToLive: tc.secondsToLive,
+				Result:        &models.ApiKey{},
+			}
+
+			err = store.AddServiceAccountToken(context.Background(), user.Id, &cmd)
+			if tc.secondsToLive < 0 {
+				require.Error(t, err)
+				return
+			}
+
+			require.NoError(t, err)
+			newKey := cmd.Result
+			require.Equal(t, t.Name(), newKey.Name)
+
+			// Verify against DB
+			keys, errT := store.ListTokens(context.Background(), user.OrgId, user.Id)
+
+			require.NoError(t, errT)
+
+			found := false
+			for _, k := range keys {
+				if k.Name == keyName {
+					found = true
+					require.Equal(t, key.HashedKey, newKey.Key)
+					if tc.secondsToLive == 0 {
+						require.Nil(t, k.Expires)
+					} else {
+						require.NotNil(t, k.Expires)
+					}
+				}
+			}
+
+			require.True(t, found, "Key not found")
+		})
+	}
+}
+
+func TestStore_DeleteServiceAccountToken(t *testing.T) {
+	userToCreate := tests.TestUser{Login: "servicetestwithTeam@admin", IsServiceAccount: true}
+	db, store := setupTestDatabase(t)
+	user := tests.SetupUserServiceAccount(t, db, userToCreate)
+
+	keyName := t.Name()
+	key, err := apikeygen.New(user.OrgId, keyName)
+	require.NoError(t, err)
+
+	cmd := models.AddApiKeyCommand{
+		Name:          keyName,
+		Role:          "Viewer",
+		OrgId:         user.OrgId,
+		Key:           key.HashedKey,
+		SecondsToLive: 0,
+		Result:        &models.ApiKey{},
+	}
+
+	err = store.AddServiceAccountToken(context.Background(), user.Id, &cmd)
+	require.NoError(t, err)
+	newKey := cmd.Result
+
+	// Delete key from wrong service account
+	err = store.DeleteServiceAccountToken(context.Background(), user.OrgId, user.Id+2, newKey.Id)
+	require.Error(t, err)
+
+	// Delete key from wrong org
+	err = store.DeleteServiceAccountToken(context.Background(), user.OrgId+2, user.Id, newKey.Id)
+	require.Error(t, err)
+
+	err = store.DeleteServiceAccountToken(context.Background(), user.OrgId, user.Id, newKey.Id)
+	require.NoError(t, err)
+
+	// Verify against DB
+	keys, errT := store.ListTokens(context.Background(), user.OrgId, user.Id)
+	require.NoError(t, errT)
+
+	for _, k := range keys {
+		if k.Name == keyName {
+			require.Fail(t, "Key not deleted")
+		}
+	}
+}

pkg/services/serviceaccounts/manager/service.go

@@ -43,7 +43,7 @@ func ProvideServiceAccountsService(
 		}
 	}
 
-	serviceaccountsAPI := api.NewServiceAccountsAPI(cfg, s, ac, routeRegister, s.store, store)
+	serviceaccountsAPI := api.NewServiceAccountsAPI(cfg, s, ac, routeRegister, s.store)
 	serviceaccountsAPI.RegisterAPIEndpoints(features)
 
 	return s, nil

pkg/services/serviceaccounts/serviceaccounts.go

@@ -21,4 +21,6 @@ type Store interface {
 	UpgradeServiceAccounts(ctx context.Context) error
 	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
 }

pkg/services/serviceaccounts/tests/common.go

@@ -67,14 +67,16 @@ func SetupMockAccesscontrol(t *testing.T,
 var _ serviceaccounts.Store = new(ServiceAccountsStoreMock)
 
 type Calls struct {
-	CreateServiceAccount   []interface{}
-	ListServiceAccounts    []interface{}
-	RetrieveServiceAccount []interface{}
-	DeleteServiceAccount   []interface{}
-	UpgradeServiceAccounts []interface{}
-	ConvertServiceAccounts []interface{}
-	ListTokens             []interface{}
-	UpdateServiceAccount   []interface{}
+	CreateServiceAccount      []interface{}
+	ListServiceAccounts       []interface{}
+	RetrieveServiceAccount    []interface{}
+	DeleteServiceAccount      []interface{}
+	UpgradeServiceAccounts    []interface{}
+	ConvertServiceAccounts    []interface{}
+	ListTokens                []interface{}
+	DeleteServiceAccountToken []interface{}
+	UpdateServiceAccount      []interface{}
+	AddServiceAccountToken    []interface{}
 }
 
 type ServiceAccountsStoreMock struct {
@@ -124,3 +126,13 @@ func (s *ServiceAccountsStoreMock) UpdateServiceAccount(ctx context.Context,
 
 	return nil, nil
 }
+
+func (s *ServiceAccountsStoreMock) DeleteServiceAccountToken(ctx context.Context, orgID, serviceAccountID, tokenID int64) error {
+	s.Calls.DeleteServiceAccountToken = append(s.Calls.DeleteServiceAccountToken, []interface{}{ctx, orgID, serviceAccountID, tokenID})
+	return nil
+}
+
+func (s *ServiceAccountsStoreMock) AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *models.AddApiKeyCommand) error {
+	s.Calls.AddServiceAccountToken = append(s.Calls.AddServiceAccountToken, []interface{}{ctx, cmd})
+	return nil
+}