Service Accounts: Display name to ID (#46258)

* ServiceAccounts: modernize SA creation interface

* ServiceAccounts: improve service account ID generation

* ServiceAccounts: remove unused method

* ServiceAccounts: Make SA ID display name dependent

* ServiceAccounts: Add tests for Service Account creation

* trim trailing whitespace

* Update pkg/services/serviceaccounts/api/api.go

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

* Update pkg/services/serviceaccounts/api/api.go

Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>

pkg/services/serviceaccounts/api/api.go

@@ -15,6 +15,7 @@ import (
 	acmiddleware "github.com/grafana/grafana/pkg/services/accesscontrol/middleware"
 	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
+	"github.com/grafana/grafana/pkg/services/serviceaccounts/database"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/web"
 )
@@ -28,11 +29,6 @@ type ServiceAccountsAPI struct {
 	log            log.Logger
 }
 
-type serviceAccountIdDTO struct {
-	Id      int64  `json:"id"`
-	Message string `json:"message"`
-}
-
 func NewServiceAccountsAPI(
 	cfg *setting.Cfg,
 	service serviceaccounts.Service,
@@ -81,24 +77,23 @@ func (api *ServiceAccountsAPI) RegisterAPIEndpoints(
 
 // POST /api/serviceaccounts
 func (api *ServiceAccountsAPI) CreateServiceAccount(c *models.ReqContext) response.Response {
-	cmd := serviceaccounts.CreateServiceAccountForm{}
+	type createServiceAccountForm struct {
+		Name string `json:"name" binding:"Required"`
+	}
+	cmd := createServiceAccountForm{}
 	if err := web.Bind(c.Req, &cmd); err != nil {
 		return response.Error(http.StatusBadRequest, "Bad request data", err)
 	}
-	cmd.OrgID = c.OrgId
 
-	user, err := api.service.CreateServiceAccount(c.Req.Context(), &cmd)
+	serviceAccount, err := api.store.CreateServiceAccount(c.Req.Context(), c.OrgId, cmd.Name)
 	switch {
-	case errors.Is(err, serviceaccounts.ErrServiceAccountNotFound):
+	case errors.Is(err, &database.ErrSAInvalidName{}):
-		return response.Error(http.StatusBadRequest, "Failed to create role with the provided name", err)
+		return response.Error(http.StatusBadRequest, "Invalid service account name", err)
 	case err != nil:
 		return response.Error(http.StatusInternalServerError, "Failed to create service account", err)
 	}
-	sa := &serviceAccountIdDTO{
+
-		Id:      user.Id,
+	return response.JSON(http.StatusCreated, serviceAccount)
-		Message: "Service account created",
-	}
-	return response.JSON(http.StatusCreated, sa)
 }
 
 func (api *ServiceAccountsAPI) DeleteServiceAccount(ctx *models.ReqContext) response.Response {

pkg/services/serviceaccounts/api/api_test.go

@@ -27,9 +27,126 @@ import (
 )
 
 var (
-	serviceaccountIDPath = "/api/serviceaccounts/%v"
+	serviceAccountPath   = "/api/serviceaccounts/"
+	serviceAccountIDPath = serviceAccountPath + "%v"
 )
 
+func TestServiceAccountsAPI_CreateServiceAccount(t *testing.T) {
+	store := sqlstore.InitTestDB(t)
+	svcmock := tests.ServiceAccountMock{}
+
+	autoAssignOrg := setting.AutoAssignOrg
+	setting.AutoAssignOrg = true
+	defer func() {
+		setting.AutoAssignOrg = autoAssignOrg
+	}()
+
+	orgCmd := &models.CreateOrgCommand{Name: "Some Test Org"}
+	err := sqlstore.CreateOrg(context.Background(), orgCmd)
+	require.Nil(t, err)
+
+	type testCreateSATestCase struct {
+		desc         string
+		body         map[string]interface{}
+		expectedCode int
+		wantID       string
+		wantError    string
+		acmock       *accesscontrolmock.Mock
+	}
+	testCases := []testCreateSATestCase{
+		{
+			desc:   "should be ok to create serviceaccount with permissions",
+			body:   map[string]interface{}{"name": "New SA"},
+			wantID: "sa-new-sa",
+			acmock: tests.SetupMockAccesscontrol(
+				t,
+				func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) {
+					return []*accesscontrol.Permission{{Action: serviceaccounts.ActionCreate}}, nil
+				},
+				false,
+			),
+			expectedCode: http.StatusCreated,
+		},
+		{
+			desc:      "not ok - duplicate name",
+			body:      map[string]interface{}{"name": "New SA"},
+			wantError: "service account name already in use",
+			acmock: tests.SetupMockAccesscontrol(
+				t,
+				func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) {
+					return []*accesscontrol.Permission{{Action: serviceaccounts.ActionCreate}}, nil
+				},
+				false,
+			),
+			expectedCode: http.StatusBadRequest,
+		},
+		{
+			desc:      "not ok - missing name",
+			body:      map[string]interface{}{},
+			wantError: "required value Name must not be empty",
+			acmock: tests.SetupMockAccesscontrol(
+				t,
+				func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) {
+					return []*accesscontrol.Permission{{Action: serviceaccounts.ActionCreate}}, nil
+				},
+				false,
+			),
+			expectedCode: http.StatusBadRequest,
+		},
+		{
+			desc: "should be forbidden to create serviceaccount if no permissions",
+			body: map[string]interface{}{},
+			acmock: tests.SetupMockAccesscontrol(
+				t,
+				func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) {
+					return []*accesscontrol.Permission{}, nil
+				},
+				false,
+			),
+			expectedCode: http.StatusForbidden,
+		},
+	}
+
+	var requestResponse = func(server *web.Mux, httpMethod, requestpath string, body io.Reader) *httptest.ResponseRecorder {
+		req, err := http.NewRequest(httpMethod, requestpath, body)
+		req.Header.Add("Content-Type", "application/json")
+		require.NoError(t, err)
+		recorder := httptest.NewRecorder()
+		server.ServeHTTP(recorder, req)
+		return recorder
+	}
+
+	testUser := &tests.TestUser{}
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			serviceAccountRequestScenario(t, http.MethodPost, serviceAccountPath, testUser, func(httpmethod string, endpoint string, user *tests.TestUser) {
+				server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, database.NewServiceAccountsStore(store))
+				marshalled, err := json.Marshal(tc.body)
+				require.NoError(t, err)
+
+				ioReader := bytes.NewReader(marshalled)
+
+				actual := requestResponse(server, httpmethod, endpoint, ioReader)
+
+				actualCode := actual.Code
+				actualBody := map[string]interface{}{}
+
+				err = json.Unmarshal(actual.Body.Bytes(), &actualBody)
+				require.NoError(t, err)
+				require.Equal(t, tc.expectedCode, actualCode, actualBody)
+
+				if actualCode == http.StatusCreated {
+					assert.NotEmpty(t, actualBody["id"])
+					assert.Equal(t, tc.body["name"], actualBody["name"].(string))
+					assert.Equal(t, tc.wantID, actualBody["login"].(string))
+				} else if actualCode == http.StatusBadRequest {
+					assert.Contains(t, tc.wantError, actualBody["error"].(string))
+				}
+			})
+		})
+	}
+}
+
 // test the accesscontrol endpoints
 // with permissions and without permissions
 func TestServiceAccountsAPI_DeleteServiceAccount(t *testing.T) {
@@ -60,7 +177,7 @@ func TestServiceAccountsAPI_DeleteServiceAccount(t *testing.T) {
 			),
 			expectedCode: http.StatusOK,
 		}
-		serviceAccountRequestScenario(t, http.MethodDelete, serviceaccountIDPath, &testcase.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
+		serviceAccountRequestScenario(t, http.MethodDelete, serviceAccountIDPath, &testcase.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
 			createduser := tests.SetupUserServiceAccount(t, store, testcase.user)
 			server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), testcase.acmock, store, database.NewServiceAccountsStore(store))
 			actual := requestResponse(server, httpmethod, fmt.Sprintf(endpoint, fmt.Sprint(createduser.Id))).Code
@@ -84,7 +201,7 @@ func TestServiceAccountsAPI_DeleteServiceAccount(t *testing.T) {
 			),
 			expectedCode: http.StatusForbidden,
 		}
-		serviceAccountRequestScenario(t, http.MethodDelete, serviceaccountIDPath, &testcase.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
+		serviceAccountRequestScenario(t, http.MethodDelete, serviceAccountIDPath, &testcase.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
 			createduser := tests.SetupUserServiceAccount(t, store, testcase.user)
 			server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), testcase.acmock, store, database.NewServiceAccountsStore(store))
 			actual := requestResponse(server, httpmethod, fmt.Sprintf(endpoint, createduser.Id)).Code
@@ -186,7 +303,7 @@ func TestServiceAccountsAPI_RetrieveServiceAccount(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			serviceAccountRequestScenario(t, http.MethodGet, serviceaccountIDPath, tc.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
+			serviceAccountRequestScenario(t, http.MethodGet, serviceAccountIDPath, tc.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
 				scopeID := tc.Id
 				if tc.user != nil {
 					createdUser := tests.SetupUserServiceAccount(t, store, *tc.user)
@@ -296,7 +413,7 @@ func TestServiceAccountsAPI_UpdateServiceAccount(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			serviceAccountRequestScenario(t, http.MethodPatch, serviceaccountIDPath, tc.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
+			serviceAccountRequestScenario(t, http.MethodPatch, serviceAccountIDPath, tc.user, func(httpmethod string, endpoint string, user *tests.TestUser) {
 				scopeID := tc.Id
 				if tc.user != nil {
 					createdUser := tests.SetupUserServiceAccount(t, store, *tc.user)

pkg/services/serviceaccounts/api/token_test.go

@@ -342,7 +342,7 @@ func TestServiceAccountsAPI_ListTokens(t *testing.T) {
 
 	for _, tc := range testCases {
 		t.Run(tc.desc, func(t *testing.T) {
-			endpoint := fmt.Sprintf(serviceaccountIDPath+"/tokens", sa.Id)
+			endpoint := fmt.Sprintf(serviceAccountIDPath+"/tokens", sa.Id)
 			server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, &saStoreMockTokens{saAPIKeys: tc.tokens})
 			actual := requestResponse(server, http.MethodGet, endpoint, http.NoBody)
 

pkg/services/serviceaccounts/database/database.go

@@ -3,11 +3,11 @@ package database
 //nolint:goimports
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"time"
 
-	"github.com/google/uuid"
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/accesscontrol"
@@ -28,19 +28,24 @@ func NewServiceAccountsStore(store *sqlstore.SQLStore) *ServiceAccountsStoreImpl
 	}
 }
 
-func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, sa *serviceaccounts.CreateServiceAccountForm) (saDTO *serviceaccounts.ServiceAccountDTO, err error) {
+func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgID int64, name string) (saDTO *serviceaccounts.ServiceAccountDTO, err error) {
-	// create a new service account - "user" with empty permissions
+	generatedLogin := "sa-" + strings.ToLower(name)
-	generatedLogin := "Service-Account-" + uuid.New().String()
+	generatedLogin = strings.ReplaceAll(generatedLogin, " ", "-")
 	cmd := models.CreateUserCommand{
 		Login:            generatedLogin,
-		Name:             sa.Name,
+		OrgId:            orgID,
-		OrgId:            sa.OrgID,
+		Name:             name,
 		IsServiceAccount: true,
 	}
+
 	newuser, err := s.sqlStore.CreateUser(ctx, cmd)
 	if err != nil {
-		return nil, fmt.Errorf("failed to create user: %v", err)
+		if errors.Is(err, models.ErrUserAlreadyExists) {
+			return nil, &ErrSAInvalidName{}
+		}
+		return nil, fmt.Errorf("failed to create service account: %w", err)
 	}
+
 	return &serviceaccounts.ServiceAccountDTO{
 		Id:     newuser.Id,
 		Name:   newuser.Name,

pkg/services/serviceaccounts/database/database_test.go

@@ -8,9 +8,33 @@ import (
 	"github.com/grafana/grafana/pkg/services/serviceaccounts"
 	"github.com/grafana/grafana/pkg/services/serviceaccounts/tests"
 	"github.com/grafana/grafana/pkg/services/sqlstore"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
+func TestStore_CreateServiceAccount(t *testing.T) {
+	sqlStore, store := setupTestDatabase(t)
+	t.Run("create service account", func(t *testing.T) {
+		saDTO, err := store.CreateServiceAccount(context.Background(), 1, "new Service Account")
+		require.NoError(t, err)
+		assert.Equal(t, "sa-new-service-account", saDTO.Login)
+		assert.Equal(t, "new Service Account", saDTO.Name)
+		assert.Equal(t, 0, int(saDTO.Tokens))
+
+		query := models.GetUserByIdQuery{Id: saDTO.Id}
+		err = sqlStore.GetUserById(context.Background(), &query)
+		retrieved := query.Result
+		require.NoError(t, err)
+		assert.Equal(t, "sa-new-service-account", retrieved.Login)
+		assert.Equal(t, "new Service Account", retrieved.Name)
+		assert.Equal(t, "sa-new-service-account", retrieved.Email)
+		assert.Equal(t, "", retrieved.Password)
+		assert.Equal(t, 1, int(retrieved.OrgId))
+		assert.Len(t, retrieved.Salt, 10)
+		assert.Equal(t, true, retrieved.IsServiceAccount)
+	})
+}
+
 func TestStore_DeleteServiceAccount(t *testing.T) {
 	cases := []struct {
 		desc        string

pkg/services/serviceaccounts/database/errors.go

@@ -6,6 +6,17 @@ import (
 	"github.com/grafana/grafana/pkg/models"
 )
 
+type ErrSAInvalidName struct {
+}
+
+func (e *ErrSAInvalidName) Error() string {
+	return "service account name already in use"
+}
+
+func (e *ErrSAInvalidName) Unwrap() error {
+	return models.ErrUserAlreadyExists
+}
+
 type ErrMisingSAToken struct {
 }
 

pkg/services/serviceaccounts/manager/service.go

@@ -49,12 +49,12 @@ func ProvideServiceAccountsService(
 	return s, nil
 }
 
-func (sa *ServiceAccountsService) CreateServiceAccount(ctx context.Context, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) {
+func (sa *ServiceAccountsService) CreateServiceAccount(ctx context.Context, orgID int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
 	if !sa.features.IsEnabled(featuremgmt.FlagServiceAccounts) {
 		sa.log.Debug(ServiceAccountFeatureToggleNotFound)
 		return nil, nil
 	}
-	return sa.store.CreateServiceAccount(ctx, saForm)
+	return sa.store.CreateServiceAccount(ctx, orgID, name)
 }
 
 func (sa *ServiceAccountsService) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error {

pkg/services/serviceaccounts/models.go

@@ -29,11 +29,6 @@ type UpdateServiceAccountForm struct {
 	IsDisabled *bool            `json:"isDisabled"`
 }
 
-type CreateServiceAccountForm struct {
-	OrgID int64  `json:"-"`
-	Name  string `json:"name" binding:"Required"`
-}
-
 type ServiceAccountDTO struct {
 	Id            int64           `json:"id" xorm:"user_id"`
 	Name          string          `json:"name" xorm:"name"`

pkg/services/serviceaccounts/serviceaccounts.go

@@ -8,12 +8,12 @@ import (
 
 // this should reflect the api
 type Service interface {
-	CreateServiceAccount(ctx context.Context, saForm *CreateServiceAccountForm) (*ServiceAccountDTO, error)
+	CreateServiceAccount(ctx context.Context, orgID int64, name string) (*ServiceAccountDTO, error)
 	DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error
 }
 
 type Store interface {
-	CreateServiceAccount(ctx context.Context, saForm *CreateServiceAccountForm) (*ServiceAccountDTO, error)
+	CreateServiceAccount(ctx context.Context, orgID int64, name string) (*ServiceAccountDTO, error)
 	ListServiceAccounts(ctx context.Context, orgID, serviceAccountID int64) ([]*ServiceAccountDTO, error)
 	SearchOrgServiceAccounts(ctx context.Context, query *models.SearchOrgUsersQuery) ([]*ServiceAccountDTO, error)
 	UpdateServiceAccount(ctx context.Context, orgID, serviceAccountID int64, saForm *UpdateServiceAccountForm) (*ServiceAccountProfileDTO, error)

pkg/services/serviceaccounts/tests/common.go

@@ -38,7 +38,7 @@ func SetupUserServiceAccount(t *testing.T, sqlStore *sqlstore.SQLStore, testUser
 // create mock for serviceaccountservice
 type ServiceAccountMock struct{}
 
-func (s *ServiceAccountMock) CreateServiceAccount(ctx context.Context, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) {
+func (s *ServiceAccountMock) CreateServiceAccount(ctx context.Context, orgID int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
 	return nil, nil
 }
 
@@ -84,9 +84,9 @@ type ServiceAccountsStoreMock struct {
 	Calls Calls
 }
 
-func (s *ServiceAccountsStoreMock) CreateServiceAccount(ctx context.Context, cmd *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) {
+func (s *ServiceAccountsStoreMock) CreateServiceAccount(ctx context.Context, orgID int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
 	// now we can test that the mock has these calls when we call the function
-	s.Calls.CreateServiceAccount = append(s.Calls.CreateServiceAccount, []interface{}{ctx, cmd})
+	s.Calls.CreateServiceAccount = append(s.Calls.CreateServiceAccount, []interface{}{ctx, orgID, name})
 	return nil, nil
 }
 

pkg/services/sqlstore/mockstore/mockstore.go

@@ -136,10 +136,6 @@ func (m *SQLStoreMock) DeleteOldLoginAttempts(ctx context.Context, cmd *models.D
 	return m.ExpectedError
 }
 
-func (m *SQLStoreMock) CloneUserToServiceAccount(ctx context.Context, siUser *models.SignedInUser) (*models.User, error) {
-	return nil, m.ExpectedError
-}
-
 func (m *SQLStoreMock) CreateServiceAccountForApikey(ctx context.Context, orgId int64, keyname string, role models.RoleType) (*models.User, error) {
 	return nil, m.ExpectedError
 }

pkg/services/sqlstore/store.go

@@ -28,7 +28,6 @@ type Store interface {
 	GetOrgByNameHandler(ctx context.Context, query *models.GetOrgByNameQuery) error
 	CreateLoginAttempt(ctx context.Context, cmd *models.CreateLoginAttemptCommand) error
 	DeleteOldLoginAttempts(ctx context.Context, cmd *models.DeleteOldLoginAttemptsCommand) error
-	CloneUserToServiceAccount(ctx context.Context, siUser *models.SignedInUser) (*models.User, error)
 	CreateServiceAccountForApikey(ctx context.Context, orgId int64, keyname string, role models.RoleType) (*models.User, error)
 	CreateUser(ctx context.Context, cmd models.CreateUserCommand) (*models.User, error)
 	GetUserById(ctx context.Context, query *models.GetUserByIdQuery) error

pkg/services/sqlstore/user.go

@@ -8,8 +8,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/google/uuid"
-
 	"github.com/grafana/grafana/pkg/bus"
 	"github.com/grafana/grafana/pkg/events"
 	"github.com/grafana/grafana/pkg/models"
@@ -186,25 +184,6 @@ func (ss *SQLStore) createUser(ctx context.Context, sess *DBSession, args userCr
 	return user, nil
 }
 
-func (ss *SQLStore) CloneUserToServiceAccount(ctx context.Context, siUser *models.SignedInUser) (*models.User, error) {
-	cmd := models.CreateUserCommand{
-		Login:            "Service-Account-" + uuid.New().String(),
-		Email:            uuid.New().String(),
-		Password:         "Password-" + uuid.New().String(),
-		Name:             siUser.Name + "-Service-Account-" + uuid.New().String(),
-		OrgId:            siUser.OrgId,
-		IsServiceAccount: true,
-	}
-
-	newuser, err := ss.CreateUser(ctx, cmd)
-	if err != nil {
-		ss.log.Warn("user not cloned", "err", err)
-		return nil, fmt.Errorf("failed to create user: %w", err)
-	}
-
-	return newuser, err
-}
-
 func (ss *SQLStore) CreateServiceAccountForApikey(ctx context.Context, orgId int64, keyname string, role models.RoleType) (*models.User, error) {
 	prefix := "Service-Account-Autogen-"
 	cmd := models.CreateUserCommand{