diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index a2660525a1a..0df95ce25a3 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/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): - return response.Error(http.StatusBadRequest, "Failed to create role with the provided name", err) + case errors.Is(err, &database.ErrSAInvalidName{}): + 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, - Message: "Service account created", - } - return response.JSON(http.StatusCreated, sa) + + return response.JSON(http.StatusCreated, serviceAccount) } func (api *ServiceAccountsAPI) DeleteServiceAccount(ctx *models.ReqContext) response.Response { diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index 37687b32147..ab9156d4ffe 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/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) diff --git a/pkg/services/serviceaccounts/api/token_test.go b/pkg/services/serviceaccounts/api/token_test.go index 959d7fc0e9a..6bac8f667af 100644 --- a/pkg/services/serviceaccounts/api/token_test.go +++ b/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) diff --git a/pkg/services/serviceaccounts/database/database.go b/pkg/services/serviceaccounts/database/database.go index 8a4115542ad..2245a000458 100644 --- a/pkg/services/serviceaccounts/database/database.go +++ b/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) { - // create a new service account - "user" with empty permissions - generatedLogin := "Service-Account-" + uuid.New().String() +func (s *ServiceAccountsStoreImpl) CreateServiceAccount(ctx context.Context, orgID int64, name string) (saDTO *serviceaccounts.ServiceAccountDTO, err error) { + generatedLogin := "sa-" + strings.ToLower(name) + generatedLogin = strings.ReplaceAll(generatedLogin, " ", "-") cmd := models.CreateUserCommand{ Login: generatedLogin, - Name: sa.Name, - OrgId: sa.OrgID, + OrgId: 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, diff --git a/pkg/services/serviceaccounts/database/database_test.go b/pkg/services/serviceaccounts/database/database_test.go index dcf74047c7d..ea1600d46eb 100644 --- a/pkg/services/serviceaccounts/database/database_test.go +++ b/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 diff --git a/pkg/services/serviceaccounts/database/errors.go b/pkg/services/serviceaccounts/database/errors.go index 09eb550c4a3..3cd555480a6 100644 --- a/pkg/services/serviceaccounts/database/errors.go +++ b/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 { } diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index f5b066cad2b..455e0bd7c0a 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/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 { diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index 4a5fc81e347..2f6a34099dd 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/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"` diff --git a/pkg/services/serviceaccounts/serviceaccounts.go b/pkg/services/serviceaccounts/serviceaccounts.go index cedc29a84d1..bdf9e65125f 100644 --- a/pkg/services/serviceaccounts/serviceaccounts.go +++ b/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) diff --git a/pkg/services/serviceaccounts/tests/common.go b/pkg/services/serviceaccounts/tests/common.go index 4b2a3731a19..e4363554aa9 100644 --- a/pkg/services/serviceaccounts/tests/common.go +++ b/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 } diff --git a/pkg/services/sqlstore/mockstore/mockstore.go b/pkg/services/sqlstore/mockstore/mockstore.go index 2490035c0df..256798905e4 100644 --- a/pkg/services/sqlstore/mockstore/mockstore.go +++ b/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 } diff --git a/pkg/services/sqlstore/store.go b/pkg/services/sqlstore/store.go index 2e2dde3e988..9e16be97c1c 100644 --- a/pkg/services/sqlstore/store.go +++ b/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 diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index 394648217eb..e6a2349d494 100644 --- a/pkg/services/sqlstore/user.go +++ b/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{