From 5eaba5b5b2e26fc794a7d714a6f6dcf8616bf414 Mon Sep 17 00:00:00 2001 From: Vardan Torosyan Date: Thu, 7 Jul 2022 17:32:56 +0100 Subject: [PATCH] Service account: Update service accounts creation (#51848) --- .../developers/http_api/serviceaccount.md | 5 ++- pkg/services/serviceaccounts/api/api.go | 37 ++++++++++++++++--- pkg/services/serviceaccounts/api/api_test.go | 33 +++++++++++++++-- .../serviceaccounts/database/database.go | 30 ++++++++++----- .../serviceaccounts/database/database_test.go | 20 +++++++++- pkg/services/serviceaccounts/errors.go | 4 +- .../serviceaccounts/manager/service.go | 8 +--- pkg/services/serviceaccounts/models.go | 4 +- .../serviceaccounts/serviceaccounts.go | 4 +- pkg/services/serviceaccounts/tests/common.go | 6 +-- 10 files changed, 115 insertions(+), 36 deletions(-) diff --git a/docs/sources/developers/http_api/serviceaccount.md b/docs/sources/developers/http_api/serviceaccount.md index 0e12e8e285e..a1adc243bc7 100644 --- a/docs/sources/developers/http_api/serviceaccount.md +++ b/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` diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index ce185eae024..c926e4ce5ba 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/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() { - 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) + 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 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) diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index 2f63ea3aa74..2623440d866 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/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", - body: map[string]interface{}{"name": "New SA"}, + desc: "should be ok to create service account with permissions", + 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, diff --git a/pkg/services/serviceaccounts/database/database.go b/pkg/services/serviceaccounts/database/database.go index f6391889e42..5fc89b7ddf3 100644 --- a/pkg/services/serviceaccounts/database/database.go +++ b/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) { - generatedLogin := "sa-" + strings.ToLower(name) +func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgId int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { + 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, - Name: newSA.Name, - Login: newSA.Login, - OrgId: newSA.OrgID, - Tokens: 0, + Id: newSA.ID, + Name: newSA.Name, + Login: newSA.Login, + OrgId: newSA.OrgID, + Tokens: 0, + Role: string(role), + IsDisabled: isDisabled, }, nil } diff --git a/pkg/services/serviceaccounts/database/database_test.go b/pkg/services/serviceaccounts/database/database_test.go index d7d292df951..1c0c0f3b899 100644 --- a/pkg/services/serviceaccounts/database/database_test.go +++ b/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) diff --git a/pkg/services/serviceaccounts/errors.go b/pkg/services/serviceaccounts/errors.go index 8538928be31..2ea8853d995 100644 --- a/pkg/services/serviceaccounts/errors.go +++ b/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") ) diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index cab12c094b8..7c578c30931 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/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) { - return sa.store.CreateServiceAccount(ctx, orgID, name) +func (sa *ServiceAccountsService) CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { + return sa.store.CreateServiceAccount(ctx, orgID, saForm) } func (sa *ServiceAccountsService) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error { diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index 884e69a7910..39d78c4cf12 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/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 { diff --git a/pkg/services/serviceaccounts/serviceaccounts.go b/pkg/services/serviceaccounts/serviceaccounts.go index 68bf94bd560..f166fadaf92 100644 --- a/pkg/services/serviceaccounts/serviceaccounts.go +++ b/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, diff --git a/pkg/services/serviceaccounts/tests/common.go b/pkg/services/serviceaccounts/tests/common.go index f695b2f6349..fcdf1418f7e 100644 --- a/pkg/services/serviceaccounts/tests/common.go +++ b/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 }