Service account: Update service accounts creation (#51848)

docs/sources/developers/http_api/serviceaccount.md

@@ -105,7 +105,8 @@ Authorization: Basic YWRtaW46YWRtaW4=
 
 {
   "name": "grafana",
-  "role": "Admin",
+  "role": "Viewer",
+  "isDisabled" : false
 }
 ```
 
@@ -131,7 +132,7 @@ Content-Type: application/json
 }
 ```
 
-## Get single serviceaccount by Id
+## Get a service account by ID
 
 `GET /api/serviceaccounts/:id`
 

pkg/services/serviceaccounts/api/api.go

@@ -84,7 +84,18 @@ func (api *ServiceAccountsAPI) CreateServiceAccount(c *models.ReqContext) respon
 		return response.Error(http.StatusBadRequest, "Bad request data", err)
 	}
 
-	serviceAccount, err := api.store.CreateServiceAccount(c.Req.Context(), c.OrgId, cmd.Name)
+	if err := api.validateRole(cmd.Role, &c.OrgRole); err != nil {
+		switch {
+		case errors.Is(err, serviceaccounts.ErrServiceAccountInvalidRole):
+			return response.Error(http.StatusBadRequest, err.Error(), err)
+		case errors.Is(err, serviceaccounts.ErrServiceAccountRolePrivilegeDenied):
+			return response.Error(http.StatusForbidden, err.Error(), err)
+		default:
+			return response.Error(http.StatusInternalServerError, "failed to create service account", err)
+		}
+	}
+
+	serviceAccount, err := api.store.CreateServiceAccount(c.Req.Context(), c.OrgId, &cmd)
 	switch {
 	case errors.Is(err, database.ErrServiceAccountAlreadyExists):
 		return response.Error(http.StatusBadRequest, "Failed to create service account", err)
@@ -138,11 +149,15 @@ func (api *ServiceAccountsAPI) UpdateServiceAccount(c *models.ReqContext) respon
 		return response.Error(http.StatusBadRequest, "Bad request data", err)
 	}
 
-	if cmd.Role != nil && !cmd.Role.IsValid() {
+	if err := api.validateRole(cmd.Role, &c.OrgRole); err != nil {
-		return response.Error(http.StatusBadRequest, "Invalid role specified", nil)
+		switch {
-	}
+		case errors.Is(err, serviceaccounts.ErrServiceAccountInvalidRole):
-	if cmd.Role != nil && !c.OrgRole.Includes(*cmd.Role) {
+			return response.Error(http.StatusBadRequest, err.Error(), err)
-		return response.Error(http.StatusForbidden, "Cannot assign a role higher than user's role", nil)
+		case errors.Is(err, serviceaccounts.ErrServiceAccountRolePrivilegeDenied):
+			return response.Error(http.StatusForbidden, err.Error(), err)
+		default:
+			return response.Error(http.StatusInternalServerError, "failed to update service account", err)
+		}
 	}
 
 	resp, err := api.store.UpdateServiceAccount(c.Req.Context(), c.OrgId, scopeID, &cmd)
@@ -168,6 +183,16 @@ func (api *ServiceAccountsAPI) UpdateServiceAccount(c *models.ReqContext) respon
 	})
 }
 
+func (api *ServiceAccountsAPI) validateRole(r *models.RoleType, orgRole *models.RoleType) error {
+	if r != nil && !r.IsValid() {
+		return serviceaccounts.ErrServiceAccountInvalidRole
+	}
+	if r != nil && !orgRole.Includes(*r) {
+		return serviceaccounts.ErrServiceAccountRolePrivilegeDenied
+	}
+	return nil
+}
+
 // DELETE /api/serviceaccounts/:serviceAccountId
 func (api *ServiceAccountsAPI) DeleteServiceAccount(ctx *models.ReqContext) response.Response {
 	scopeID, err := strconv.ParseInt(web.Params(ctx.Req)[":serviceAccountId"], 10, 64)

pkg/services/serviceaccounts/api/api_test.go

@@ -58,8 +58,8 @@ func TestServiceAccountsAPI_CreateServiceAccount(t *testing.T) {
 	}
 	testCases := []testCreateSATestCase{
 		{
-			desc:   "should be ok to create serviceaccount with permissions",
+			desc:   "should be ok to create service account with permissions",
-			body:   map[string]interface{}{"name": "New SA"},
+			body:   map[string]interface{}{"name": "New SA", "role": "Viewer", "is_disabled": "false"},
 			wantID: "sa-new-sa",
 			acmock: tests.SetupMockAccesscontrol(
 				t,
@@ -70,6 +70,33 @@ func TestServiceAccountsAPI_CreateServiceAccount(t *testing.T) {
 			),
 			expectedCode: http.StatusCreated,
 		},
+		{
+			desc:   "should fail to create a service account with higher privilege",
+			body:   map[string]interface{}{"name": "New SA HP", "role": "Admin"},
+			wantID: "sa-new-sa-hp",
+			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.StatusForbidden,
+		},
+		{
+			desc:      "should fail to create a service account with invalid role",
+			body:      map[string]interface{}{"name": "New SA", "role": "Random"},
+			wantID:    "sa-new-sa",
+			wantError: "invalid role value: Random",
+			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 - duplicate name",
 			body:      map[string]interface{}{"name": "New SA"},
@@ -97,7 +124,7 @@ func TestServiceAccountsAPI_CreateServiceAccount(t *testing.T) {
 			expectedCode: http.StatusBadRequest,
 		},
 		{
-			desc: "should be forbidden to create serviceaccount if no permissions",
+			desc: "should be forbidden to create service account if no permissions",
 			body: map[string]interface{}{},
 			acmock: tests.SetupMockAccesscontrol(
 				t,

pkg/services/serviceaccounts/database/database.go

@@ -32,17 +32,25 @@ func NewServiceAccountsStore(store *sqlstore.SQLStore, kvStore kvstore.KVStore)
 }
 
 // CreateServiceAccount creates service account
-func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgId int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
+func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgId int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) {
-	generatedLogin := "sa-" + strings.ToLower(name)
+	generatedLogin := "sa-" + strings.ToLower(saForm.Name)
 	generatedLogin = strings.ReplaceAll(generatedLogin, " ", "-")
-
+	isDisabled := false
+	role := models.ROLE_VIEWER
+	if saForm.IsDisabled != nil {
+		isDisabled = *saForm.IsDisabled
+	}
+	if saForm.Role != nil {
+		role = *saForm.Role
+	}
 	var newSA *user.User
 	createErr := s.sqlStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) (err error) {
 		var errUser error
 		newSA, errUser = s.sqlStore.CreateUser(ctx, user.CreateUserCommand{
 			Login:            generatedLogin,
 			OrgID:            orgId,
-			Name:             name,
+			Name:             saForm.Name,
+			IsDisabled:       isDisabled,
 			IsServiceAccount: true,
 			SkipOrgSetup:     true,
 		})
@@ -51,7 +59,7 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, org
 		}
 
 		errAddOrgUser := s.sqlStore.AddOrgUser(ctx, &models.AddOrgUserCommand{
-			Role:                      models.ROLE_VIEWER,
+			Role:                      role,
 			OrgId:                     orgId,
 			UserId:                    newSA.ID,
 			AllowAddingServiceAccount: true,
@@ -72,11 +80,13 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, org
 	}
 
 	return &serviceaccounts.ServiceAccountDTO{
-		Id:     newSA.ID,
+		Id:         newSA.ID,
-		Name:   newSA.Name,
+		Name:       newSA.Name,
-		Login:  newSA.Login,
+		Login:      newSA.Login,
-		OrgId:  newSA.OrgID,
+		OrgId:      newSA.OrgID,
-		Tokens: 0,
+		Tokens:     0,
+		Role:       string(role),
+		IsDisabled: isDisabled,
 	}, nil
 }
 

pkg/services/serviceaccounts/database/database_test.go

@@ -19,8 +19,15 @@ func TestStore_CreateServiceAccountOrgNonExistant(t *testing.T) {
 	t.Run("create service account", func(t *testing.T) {
 		serviceAccountName := "new Service Account"
 		serviceAccountOrgId := int64(1)
+		serviceAccountRole := models.ROLE_ADMIN
+		isDisabled := true
+		saForm := serviceaccounts.CreateServiceAccountForm{
+			Name:       serviceAccountName,
+			Role:       &serviceAccountRole,
+			IsDisabled: &isDisabled,
+		}
 
-		_, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, serviceAccountName)
+		_, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, &saForm)
 		require.Error(t, err)
 	})
 }
@@ -34,8 +41,15 @@ func TestStore_CreateServiceAccount(t *testing.T) {
 	t.Run("create service account", func(t *testing.T) {
 		serviceAccountName := "new Service Account"
 		serviceAccountOrgId := orgQuery.Result.Id
+		serviceAccountRole := models.ROLE_ADMIN
+		isDisabled := true
+		saForm := serviceaccounts.CreateServiceAccountForm{
+			Name:       serviceAccountName,
+			Role:       &serviceAccountRole,
+			IsDisabled: &isDisabled,
+		}
 
-		saDTO, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, serviceAccountName)
+		saDTO, err := store.CreateServiceAccount(context.Background(), serviceAccountOrgId, &saForm)
 		require.NoError(t, err)
 		assert.Equal(t, "sa-new-service-account", saDTO.Login)
 		assert.Equal(t, serviceAccountName, saDTO.Name)
@@ -46,6 +60,8 @@ func TestStore_CreateServiceAccount(t *testing.T) {
 		assert.Equal(t, "sa-new-service-account", retrieved.Login)
 		assert.Equal(t, serviceAccountName, retrieved.Name)
 		assert.Equal(t, serviceAccountOrgId, retrieved.OrgId)
+		assert.Equal(t, string(serviceAccountRole), retrieved.Role)
+		assert.True(t, retrieved.IsDisabled)
 
 		retrievedId, err := store.RetrieveServiceAccountIdByName(context.Background(), serviceAccountOrgId, serviceAccountName)
 		require.NoError(t, err)

pkg/services/serviceaccounts/errors.go

@@ -3,5 +3,7 @@ package serviceaccounts
 import "errors"
 
 var (
-	ErrServiceAccountNotFound = errors.New("Service account not found")
+	ErrServiceAccountNotFound            = errors.New("service account not found")
+	ErrServiceAccountInvalidRole         = errors.New("invalid role specified")
+	ErrServiceAccountRolePrivilegeDenied = errors.New("can not assign a role higher than user's role")
 )

pkg/services/serviceaccounts/manager/service.go

@@ -15,10 +15,6 @@ import (
 	"github.com/grafana/grafana/pkg/setting"
 )
 
-var (
-	ServiceAccountFeatureToggleNotFound = "FeatureToggle serviceAccounts not found, try adding it to your custom.ini"
-)
-
 type ServiceAccountsService struct {
 	store serviceaccounts.Store
 	log   log.Logger
@@ -55,8 +51,8 @@ func (sa *ServiceAccountsService) Run(ctx context.Context) error {
 	return sa.store.RunMetricsCollection(ctx)
 }
 
-func (sa *ServiceAccountsService) CreateServiceAccount(ctx context.Context, orgID int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
+func (sa *ServiceAccountsService) CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) {
-	return sa.store.CreateServiceAccount(ctx, orgID, name)
+	return sa.store.CreateServiceAccount(ctx, orgID, saForm)
 }
 
 func (sa *ServiceAccountsService) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error {

pkg/services/serviceaccounts/models.go

@@ -24,7 +24,9 @@ type ServiceAccount struct {
 }
 
 type CreateServiceAccountForm struct {
-	Name string `json:"name" binding:"Required"`
+	Name       string           `json:"name" binding:"Required"`
+	Role       *models.RoleType `json:"role"`
+	IsDisabled *bool            `json:"isDisabled"`
 }
 
 type UpdateServiceAccountForm struct {

pkg/services/serviceaccounts/serviceaccounts.go

@@ -8,13 +8,13 @@ import (
 
 // this should reflect the api
 type Service interface {
-	CreateServiceAccount(ctx context.Context, orgID int64, name string) (*ServiceAccountDTO, error)
+	CreateServiceAccount(ctx context.Context, orgID int64, saForm *CreateServiceAccountForm) (*ServiceAccountDTO, error)
 	DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error
 	RetrieveServiceAccountIdByName(ctx context.Context, orgID int64, name string) (int64, error)
 }
 
 type Store interface {
-	CreateServiceAccount(ctx context.Context, orgID int64, name string) (*ServiceAccountDTO, error)
+	CreateServiceAccount(ctx context.Context, orgID int64, saForm *CreateServiceAccountForm) (*ServiceAccountDTO, error)
 	SearchOrgServiceAccounts(ctx context.Context, orgID int64, query string, filter ServiceAccountFilter, page int, limit int,
 		signedInUser *models.SignedInUser) (*SearchServiceAccountsResult, error)
 	UpdateServiceAccount(ctx context.Context, orgID, serviceAccountID int64,

pkg/services/serviceaccounts/tests/common.go

@@ -86,7 +86,7 @@ func (s *ServiceAccountMock) RetrieveServiceAccountIdByName(ctx context.Context,
 	return 0, nil
 }
 
-func (s *ServiceAccountMock) CreateServiceAccount(ctx context.Context, orgID int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
+func (s *ServiceAccountMock) CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) {
 	return nil, nil
 }
 
@@ -142,9 +142,9 @@ func (s *ServiceAccountsStoreMock) RetrieveServiceAccountIdByName(ctx context.Co
 	return 0, nil
 }
 
-func (s *ServiceAccountsStoreMock) CreateServiceAccount(ctx context.Context, orgID int64, name string) (*serviceaccounts.ServiceAccountDTO, error) {
+func (s *ServiceAccountsStoreMock) CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*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, orgID, name})
+	s.Calls.CreateServiceAccount = append(s.Calls.CreateServiceAccount, []interface{}{ctx, orgID, saForm})
 	return nil, nil
 }