Service Account detailed edit (#45404)

* ServiceAccounts: Update service account route

* ServiceAccounts: Update service account tests

* ServiceAccounts: remove extraneous file

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)
+}

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)
+				}
+			})
+		})
+	}
+}

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 {

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

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"`
 }

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

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
+}