From 8afd5d54f716b55edec207d3fe250c07233ae301 Mon Sep 17 00:00:00 2001 From: J Guerreiro Date: Thu, 17 Feb 2022 12:19:58 +0000 Subject: [PATCH] Service Account detailed edit (#45404) * ServiceAccounts: Update service account route * ServiceAccounts: Update service account tests * ServiceAccounts: remove extraneous file --- log | 0 pkg/services/serviceaccounts/api/api.go | 32 ++++- pkg/services/serviceaccounts/api/api_test.go | 129 ++++++++++++++++++ .../serviceaccounts/database/database.go | 60 +++++++- .../serviceaccounts/manager/service.go | 2 +- pkg/services/serviceaccounts/models.go | 8 +- .../serviceaccounts/serviceaccounts.go | 5 +- pkg/services/serviceaccounts/tests/common.go | 22 ++- 8 files changed, 250 insertions(+), 8 deletions(-) delete mode 100644 log diff --git a/log b/log deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index 3c5ade4d439..ce1818bcb81 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/pkg/services/serviceaccounts/api/api.go @@ -71,6 +71,8 @@ func (api *ServiceAccountsAPI) RegisterAPIEndpoints( api.RouterRegister.Group("/api/serviceaccounts", func(serviceAccountsRoute routing.RouteRegister) { serviceAccountsRoute.Get("/", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionRead, serviceaccounts.ScopeAll)), routing.Wrap(api.ListServiceAccounts)) serviceAccountsRoute.Get("/:serviceAccountId", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionRead, serviceaccounts.ScopeID)), routing.Wrap(api.RetrieveServiceAccount)) + serviceAccountsRoute.Patch("/:serviceAccountId", auth(middleware.ReqOrgAdmin, + accesscontrol.EvalPermission(serviceaccounts.ActionWrite, serviceaccounts.ScopeID)), routing.Wrap(api.updateServiceAccount)) serviceAccountsRoute.Delete("/:serviceAccountId", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionDelete, serviceaccounts.ScopeID)), routing.Wrap(api.DeleteServiceAccount)) serviceAccountsRoute.Post("/upgradeall", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionCreate)), routing.Wrap(api.UpgradeServiceAccounts)) serviceAccountsRoute.Post("/convert/:keyId", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionCreate, serviceaccounts.ScopeID)), routing.Wrap(api.ConvertToServiceAccount)) @@ -86,7 +88,7 @@ func (api *ServiceAccountsAPI) RegisterAPIEndpoints( // POST /api/serviceaccounts func (api *ServiceAccountsAPI) CreateServiceAccount(c *models.ReqContext) response.Response { - cmd := serviceaccounts.CreateServiceaccountForm{} + cmd := serviceaccounts.CreateServiceAccountForm{} if err := web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "Bad request data", err) } @@ -189,3 +191,31 @@ func (api *ServiceAccountsAPI) RetrieveServiceAccount(ctx *models.ReqContext) re } return response.JSON(http.StatusOK, serviceAccount) } + +func (api *ServiceAccountsAPI) updateServiceAccount(c *models.ReqContext) response.Response { + scopeID, err := strconv.ParseInt(web.Params(c.Req)[":serviceAccountId"], 10, 64) + if err != nil { + return response.Error(http.StatusBadRequest, "serviceAccountId is invalid", err) + } + + cmd := &serviceaccounts.UpdateServiceAccountForm{} + if err := web.Bind(c.Req, &cmd); err != nil { + 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) + } + + resp, err := api.store.UpdateServiceAccount(c.Req.Context(), c.OrgId, scopeID, cmd) + if err != nil { + switch { + case errors.Is(err, serviceaccounts.ErrServiceAccountNotFound): + return response.Error(http.StatusNotFound, "Failed to retrieve service account", err) + default: + return response.Error(http.StatusInternalServerError, "Failed update service account", err) + } + } + + return response.JSON(http.StatusOK, resp) +} diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index 84172674f3e..b467ef6763b 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/pkg/services/serviceaccounts/api/api_test.go @@ -1,9 +1,11 @@ package api import ( + "bytes" "context" "encoding/json" "fmt" + "io" "net/http" "net/http/httptest" "testing" @@ -19,6 +21,7 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/web" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/services/serviceaccounts/database" @@ -205,3 +208,129 @@ func TestServiceAccountsAPI_RetrieveServiceAccount(t *testing.T) { }) } } + +func newString(s string) *string { + return &s +} + +func TestServiceAccountsAPI_UpdateServiceAccount(t *testing.T) { + store := sqlstore.InitTestDB(t) + svcmock := tests.ServiceAccountMock{} + type testUpdateSATestCase struct { + desc string + user *tests.TestUser + expectedCode int + acmock *accesscontrolmock.Mock + body *serviceaccounts.UpdateServiceAccountForm + Id int + } + + role := models.ROLE_ADMIN + var invalidRole models.RoleType = "InvalidRole" + testCases := []testUpdateSATestCase{ + { + desc: "should be ok to update serviceaccount with permissions", + user: &tests.TestUser{Login: "servicetest1@admin", IsServiceAccount: true, Role: "Editor", Name: "Unaltered"}, + body: &serviceaccounts.UpdateServiceAccountForm{Name: newString("New Name"), Role: &role}, + acmock: tests.SetupMockAccesscontrol( + t, + func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) { + return []*accesscontrol.Permission{{Action: serviceaccounts.ActionWrite, Scope: serviceaccounts.ScopeAll}}, nil + }, + false, + ), + expectedCode: http.StatusOK, + }, + { + desc: "bad request when invalid role", + user: &tests.TestUser{Login: "servicetest3@admin", IsServiceAccount: true, Role: "Invalid", Name: "Unaltered"}, + body: &serviceaccounts.UpdateServiceAccountForm{Name: newString("NameB"), Role: &invalidRole}, + acmock: tests.SetupMockAccesscontrol( + t, + func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) { + return []*accesscontrol.Permission{{Action: serviceaccounts.ActionWrite, Scope: serviceaccounts.ScopeAll}}, nil + }, + false, + ), + expectedCode: http.StatusBadRequest, + }, + { + desc: "should be forbidden to update serviceaccount if no permissions", + user: &tests.TestUser{Login: "servicetest2@admin", IsServiceAccount: true}, + body: nil, + acmock: tests.SetupMockAccesscontrol( + t, + func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) { + return []*accesscontrol.Permission{}, nil + }, + false, + ), + expectedCode: http.StatusForbidden, + }, + { + desc: "should be not found when the user doesnt exist", + user: nil, + body: nil, + Id: 12, + acmock: tests.SetupMockAccesscontrol( + t, + func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) { + return []*accesscontrol.Permission{{Action: serviceaccounts.ActionWrite, Scope: serviceaccounts.ScopeAll}}, nil + }, + false, + ), + expectedCode: http.StatusNotFound, + }, + } + + 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 + } + + 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) { + scopeID := tc.Id + if tc.user != nil { + createdUser := tests.SetupUserServiceAccount(t, store, *tc.user) + scopeID = int(createdUser.Id) + } + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store) + + var rawBody io.Reader = http.NoBody + if tc.body != nil { + body, err := json.Marshal(tc.body) + require.NoError(t, err) + rawBody = bytes.NewReader(body) + } + + actual := requestResponse(server, httpmethod, fmt.Sprintf(endpoint, scopeID), rawBody) + + actualCode := actual.Code + require.Equal(t, tc.expectedCode, actualCode) + + if actualCode == http.StatusOK { + actualBody := map[string]interface{}{} + err := json.Unmarshal(actual.Body.Bytes(), &actualBody) + require.NoError(t, err) + assert.Equal(t, scopeID, int(actualBody["id"].(float64))) + assert.Equal(t, string(*tc.body.Role), actualBody["role"].(string)) + assert.Equal(t, *tc.body.Name, actualBody["name"].(string)) + assert.Equal(t, tc.user.Login, actualBody["login"].(string)) + + // Ensure the user was updated in DB + query := models.GetOrgUsersQuery{UserID: int64(scopeID), OrgId: 1, IsServiceAccount: true} + err = store.GetOrgUsers(context.Background(), &query) + require.NoError(t, err) + require.Equal(t, *tc.body.Name, query.Result[0].Name) + require.Equal(t, string(*tc.body.Role), query.Result[0].Role) + } + }) + }) + } +} diff --git a/pkg/services/serviceaccounts/database/database.go b/pkg/services/serviceaccounts/database/database.go index e6f1191a584..35163803e48 100644 --- a/pkg/services/serviceaccounts/database/database.go +++ b/pkg/services/serviceaccounts/database/database.go @@ -25,7 +25,7 @@ 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, sa *serviceaccounts.CreateServiceAccountForm) (saDTO *serviceaccounts.ServiceAccountDTO, err error) { // create a new service account - "user" with empty permissions generatedLogin := "Service-Account-" + uuid.New().String() cmd := models.CreateUserCommand{ @@ -185,6 +185,64 @@ func (s *ServiceAccountsStoreImpl) RetrieveServiceAccount(ctx context.Context, o return saProfile, err } +func (s *ServiceAccountsStoreImpl) UpdateServiceAccount(ctx context.Context, + orgID, serviceAccountID int64, + saForm *serviceaccounts.UpdateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { + updatedUser := &models.OrgUserDTO{} + + err := s.sqlStore.WithTransactionalDbSession(ctx, func(sess *sqlstore.DBSession) error { + query := models.GetOrgUsersQuery{UserID: serviceAccountID, OrgId: orgID, IsServiceAccount: true} + if err := s.sqlStore.GetOrgUsers(ctx, &query); err != nil { + return err + } + if len(query.Result) != 1 { + return serviceaccounts.ErrServiceAccountNotFound + } + + updatedUser = query.Result[0] + + if saForm.Name == nil && saForm.Role == nil { + return nil + } + + updateTime := time.Now() + if saForm.Role != nil { + var orgUser models.OrgUser + orgUser.Role = *saForm.Role + orgUser.Updated = updateTime + + if _, err := sess.ID(orgUser.Id).Update(&orgUser); err != nil { + return err + } + + updatedUser.Role = string(*saForm.Role) + } + + if saForm.Name != nil { + user := models.User{ + Name: *saForm.Name, + Updated: updateTime, + } + + if _, err := sess.ID(serviceAccountID).Update(&user); err != nil { + return err + } + + updatedUser.Name = *saForm.Name + } + + return nil + }) + + return &serviceaccounts.ServiceAccountDTO{ + Id: updatedUser.UserId, + Name: updatedUser.Name, + Login: updatedUser.Login, + Role: updatedUser.Role, + OrgId: updatedUser.OrgId, + }, err +} + func contains(s []int64, e int64) bool { for _, a := range s { if a == e { diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index 02a8b8f0154..e74aaa5a02c 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/pkg/services/serviceaccounts/manager/service.go @@ -49,7 +49,7 @@ 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, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { if !sa.features.IsEnabled(featuremgmt.FlagServiceAccounts) { sa.log.Debug(ServiceAccountFeatureToggleNotFound) return nil, nil diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index d5577e2ecc7..81ad0337b4d 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/pkg/services/serviceaccounts/models.go @@ -3,6 +3,7 @@ package serviceaccounts import ( "time" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol" ) @@ -22,7 +23,12 @@ type ServiceAccount struct { Id int64 } -type CreateServiceaccountForm struct { +type UpdateServiceAccountForm struct { + Name *string `json:"name"` + Role *models.RoleType `json:"role"` +} + +type CreateServiceAccountForm struct { OrgID int64 `json:"-"` Name string `json:"name" binding:"Required"` } diff --git a/pkg/services/serviceaccounts/serviceaccounts.go b/pkg/services/serviceaccounts/serviceaccounts.go index 80b2d258e1f..3aa6d895169 100644 --- a/pkg/services/serviceaccounts/serviceaccounts.go +++ b/pkg/services/serviceaccounts/serviceaccounts.go @@ -8,13 +8,14 @@ import ( // this should reflect the api type Service interface { - CreateServiceAccount(ctx context.Context, saForm *CreateServiceaccountForm) (*ServiceAccountDTO, error) + CreateServiceAccount(ctx context.Context, saForm *CreateServiceAccountForm) (*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, saForm *CreateServiceAccountForm) (*ServiceAccountDTO, error) ListServiceAccounts(ctx context.Context, orgID, serviceAccountID int64) ([]*ServiceAccountDTO, error) + UpdateServiceAccount(ctx context.Context, orgID, serviceAccountID int64, saForm *UpdateServiceAccountForm) (*ServiceAccountDTO, error) RetrieveServiceAccount(ctx context.Context, orgID, serviceAccountID int64) (*ServiceAccountProfileDTO, error) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error UpgradeServiceAccounts(ctx context.Context) error diff --git a/pkg/services/serviceaccounts/tests/common.go b/pkg/services/serviceaccounts/tests/common.go index 2262438e022..17e9f16d9b2 100644 --- a/pkg/services/serviceaccounts/tests/common.go +++ b/pkg/services/serviceaccounts/tests/common.go @@ -13,14 +13,23 @@ import ( ) type TestUser struct { + Name string + Role string Login string IsServiceAccount bool } func SetupUserServiceAccount(t *testing.T, sqlStore *sqlstore.SQLStore, testUser TestUser) *models.User { + role := string(models.ROLE_VIEWER) + if testUser.Role != "" { + role = testUser.Role + } + u1, err := sqlStore.CreateUser(context.Background(), models.CreateUserCommand{ Login: testUser.Login, IsServiceAccount: testUser.IsServiceAccount, + DefaultOrgRole: role, + Name: testUser.Name, }) require.NoError(t, err) return u1 @@ -29,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, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { return nil, nil } @@ -65,13 +74,14 @@ type Calls struct { UpgradeServiceAccounts []interface{} ConvertServiceAccounts []interface{} ListTokens []interface{} + UpdateServiceAccount []interface{} } 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, cmd *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, cmd}) return nil, nil @@ -106,3 +116,11 @@ func (s *ServiceAccountsStoreMock) RetrieveServiceAccount(ctx context.Context, o s.Calls.RetrieveServiceAccount = append(s.Calls.RetrieveServiceAccount, []interface{}{ctx, orgID, serviceAccountID}) return nil, nil } + +func (s *ServiceAccountsStoreMock) UpdateServiceAccount(ctx context.Context, + orgID, serviceAccountID int64, + saForm *serviceaccounts.UpdateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) { + s.Calls.UpdateServiceAccount = append(s.Calls.UpdateServiceAccount, []interface{}{ctx, orgID, serviceAccountID, saForm}) + + return nil, nil +}