diff --git a/docs/sources/developers/http_api/serviceaccount.md b/docs/sources/developers/http_api/serviceaccount.md index a961f042d70..cad63bd8cc8 100644 --- a/docs/sources/developers/http_api/serviceaccount.md +++ b/docs/sources/developers/http_api/serviceaccount.md @@ -138,9 +138,9 @@ Content-Type: application/json See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation. -| Action | Scope | -| -------------------- | -------------------- | -| serviceaccounts:read | serviceaccounts:id:1 | +| Action | Scope | +| -------------------- | --------------------- | +| serviceaccounts:read | serviceaccounts:id:\* | **Example Request**: @@ -179,9 +179,9 @@ Content-Type: application/json See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation. -| Action | Scope | -| --------------------- | -------------------- | -| serviceaccounts:write | serviceaccounts:id:1 | +| Action | Scope | +| --------------------- | --------------------- | +| serviceaccounts:write | serviceaccounts:id:\* | **Example Request**: @@ -227,9 +227,9 @@ Content-Type: application/json See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation. -| Action | Scope | -| -------------------- | -------------------- | -| serviceaccounts:read | serviceaccounts:id:1 | +| Action | Scope | +| -------------------- | --------------------- | +| serviceaccounts:read | serviceaccounts:id:\* | **Example Request**: @@ -267,9 +267,9 @@ Content-Type: application/json See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation. -| Action | Scope | -| --------------------- | -------------------- | -| serviceaccounts:write | serviceaccounts:id:1 | +| Action | Scope | +| --------------------- | --------------------- | +| serviceaccounts:write | serviceaccounts:id:\* | **Example Request**: @@ -306,9 +306,9 @@ Content-Type: application/json See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation. -| Action | Scope | -| --------------------- | -------------------- | -| serviceaccounts:write | serviceaccounts:id:1 | +| Action | Scope | +| --------------------- | --------------------- | +| serviceaccounts:write | serviceaccounts:id:\* | **Example Request**: @@ -332,7 +332,7 @@ Content-Type: application/json ## Revert service account token to API key -`DELETE /api/serviceaccounts/revert/:keyId` +`DELETE /api/serviceaccounts/:serviceAccountId/revert/:keyId` This operation will delete the service account and create a legacy API Key for the given `keyId`. @@ -340,14 +340,14 @@ This operation will delete the service account and create a legacy API Key for t See note in the [introduction]({{< ref "#service-account-api" >}}) for an explanation. -| Action | Scope | -| ---------------------- | ----- | -| serviceaccounts:delete | n/a | +| Action | Scope | +| ---------------------- | --------------------- | +| serviceaccounts:delete | serviceaccounts:id:\* | **Example Request**: ```http -DELETE /api/serviceaccounts/revert/glsa_VVQjot0nijQ59lun6pMZRtsdBXxnFQ9M_77c34a79 HTTP/1.1 +DELETE /api/serviceaccounts/1/revert/glsa_VVQjot0nijQ59lun6pMZRtsdBXxnFQ9M_77c34a79 HTTP/1.1 Accept: application/json Content-Type: application/json Authorization: Basic YWRtaW46YWRtaW4= diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index ba445fc1bfe..3c9fb31a114 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/pkg/services/serviceaccounts/api/api.go @@ -75,8 +75,8 @@ func (api *ServiceAccountsAPI) RegisterAPIEndpoints() { accesscontrol.EvalPermission(serviceaccounts.ActionCreate)), routing.Wrap(api.MigrateApiKeysToServiceAccounts)) serviceAccountsRoute.Post("/migrate/:keyId", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionCreate)), routing.Wrap(api.ConvertToServiceAccount)) - serviceAccountsRoute.Post("/revert/:keyId", auth(middleware.ReqOrgAdmin, - accesscontrol.EvalPermission(serviceaccounts.ActionDelete)), routing.Wrap(api.RevertApiKey)) + serviceAccountsRoute.Post("/:serviceAccountId/revert/:keyId", auth(middleware.ReqOrgAdmin, + accesscontrol.EvalPermission(serviceaccounts.ActionDelete, serviceaccounts.ScopeID)), routing.Wrap(api.RevertApiKey)) }) } @@ -307,13 +307,17 @@ func (api *ServiceAccountsAPI) ConvertToServiceAccount(ctx *models.ReqContext) r func (api *ServiceAccountsAPI) RevertApiKey(ctx *models.ReqContext) response.Response { keyId, err := strconv.ParseInt(web.Params(ctx.Req)[":keyId"], 10, 64) if err != nil { - return response.Error(http.StatusBadRequest, "Key ID is invalid", err) + return response.Error(http.StatusBadRequest, "key ID is invalid", err) + } + serviceAccountId, err := strconv.ParseInt(web.Params(ctx.Req)[":serviceAccountId"], 10, 64) + if err != nil { + return response.Error(http.StatusBadRequest, "service account ID is invalid", err) } - if err := api.store.RevertApiKey(ctx.Req.Context(), keyId); err != nil { - return response.Error(http.StatusInternalServerError, "Error reverting to API key", err) + if err := api.store.RevertApiKey(ctx.Req.Context(), serviceAccountId, keyId); err != nil { + return response.Error(http.StatusInternalServerError, "error reverting to API key", err) } - return response.Success("Reverted service account to API key") + return response.Success("reverted service account to API key") } func (api *ServiceAccountsAPI) getAccessControlMetadata(c *models.ReqContext, saIDs map[string]bool) map[string]accesscontrol.Metadata { diff --git a/pkg/services/serviceaccounts/database/database.go b/pkg/services/serviceaccounts/database/database.go index 9c7e732b921..a6c7fbb7042 100644 --- a/pkg/services/serviceaccounts/database/database.go +++ b/pkg/services/serviceaccounts/database/database.go @@ -463,7 +463,7 @@ func (s *ServiceAccountsStoreImpl) CreateServiceAccountFromApikey(ctx context.Co } // RevertApiKey converts service account token to old API key -func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, keyId int64) error { +func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, saId int64, keyId int64) error { query := models.GetApiKeyByIdQuery{ApiKeyId: keyId} if err := s.sqlStore.GetApiKeyById(ctx, &query); err != nil { return err @@ -474,6 +474,10 @@ func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, keyId int64 return fmt.Errorf("API key is not service account token") } + if *key.ServiceAccountId != saId { + return ErrServiceAccountAndTokenMismatch + } + tokens, err := s.ListTokens(ctx, key.OrgId, *key.ServiceAccountId) if err != nil { return fmt.Errorf("cannot revert token: %w", err) diff --git a/pkg/services/serviceaccounts/database/database_test.go b/pkg/services/serviceaccounts/database/database_test.go index 10b896e3d48..1815e71c97a 100644 --- a/pkg/services/serviceaccounts/database/database_test.go +++ b/pkg/services/serviceaccounts/database/database_test.go @@ -2,6 +2,7 @@ package database import ( "context" + "math/rand" "testing" "github.com/grafana/grafana/pkg/infra/kvstore" @@ -270,15 +271,22 @@ func TestStore_MigrateAllApiKeys(t *testing.T) { func TestStore_RevertApiKey(t *testing.T) { cases := []struct { - desc string - key tests.TestApiKey - expectedErr error + desc string + key tests.TestApiKey + forceMismatchServiceAccount bool + expectedErr error }{ { desc: "service account token should be reverted to api key", key: tests.TestApiKey{Name: "Test1", Role: models.ROLE_EDITOR, OrgId: 1}, expectedErr: nil, }, + { + desc: "should fail reverting to api key when the token is assigned to a different service account", + key: tests.TestApiKey{Name: "Test1", Role: models.ROLE_EDITOR, OrgId: 1}, + forceMismatchServiceAccount: true, + expectedErr: ErrServiceAccountAndTokenMismatch, + }, } for _, c := range cases { @@ -291,9 +299,23 @@ func TestStore_RevertApiKey(t *testing.T) { require.NoError(t, err) key := tests.SetupApiKey(t, db, c.key) - err = store.MigrateApiKey(context.Background(), key.OrgId, key.Id) + err = store.CreateServiceAccountFromApikey(context.Background(), key) require.NoError(t, err) - err = store.RevertApiKey(context.Background(), key.Id) + + var saId int64 + if c.forceMismatchServiceAccount { + saId = rand.Int63() + } else { + serviceAccounts, err := store.SearchOrgServiceAccounts(context.Background(), key.OrgId, "", "all", 1, 50, &models.SignedInUser{UserId: 1, OrgId: 1, Permissions: map[int64]map[string][]string{ + key.OrgId: { + "serviceaccounts:read": {"serviceaccounts:id:*"}, + }, + }}) + require.NoError(t, err) + saId = serviceAccounts.ServiceAccounts[0].Id + } + + err = store.RevertApiKey(context.Background(), saId, key.Id) if c.expectedErr != nil { require.ErrorIs(t, err, c.expectedErr) diff --git a/pkg/services/serviceaccounts/database/errors.go b/pkg/services/serviceaccounts/database/errors.go index d143e4f5a77..76560c4bc71 100644 --- a/pkg/services/serviceaccounts/database/errors.go +++ b/pkg/services/serviceaccounts/database/errors.go @@ -5,8 +5,9 @@ import ( ) var ( - ErrServiceAccountAlreadyExists = errors.New("service account already exists") - ErrServiceAccountTokenNotFound = errors.New("service account token not found") - ErrInvalidTokenExpiration = errors.New("invalid SecondsToLive value") - ErrDuplicateToken = errors.New("service account token with given name already exists in the organization") + ErrServiceAccountAlreadyExists = errors.New("service account already exists") + ErrServiceAccountTokenNotFound = errors.New("service account token not found") + ErrInvalidTokenExpiration = errors.New("invalid SecondsToLive value") + ErrDuplicateToken = errors.New("service account token with given name already exists in the organization") + ErrServiceAccountAndTokenMismatch = errors.New("API token does not belong to the given service account") ) diff --git a/pkg/services/serviceaccounts/serviceaccounts.go b/pkg/services/serviceaccounts/serviceaccounts.go index f166fadaf92..2c0c85703a7 100644 --- a/pkg/services/serviceaccounts/serviceaccounts.go +++ b/pkg/services/serviceaccounts/serviceaccounts.go @@ -26,7 +26,7 @@ type Store interface { HideApiKeysTab(ctx context.Context, orgID int64) error MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error MigrateApiKey(ctx context.Context, orgID int64, keyId int64) error - RevertApiKey(ctx context.Context, keyId int64) error + RevertApiKey(ctx context.Context, saId int64, keyId int64) error ListTokens(ctx context.Context, orgID int64, serviceAccount int64) ([]*models.ApiKey, error) DeleteServiceAccountToken(ctx context.Context, orgID, serviceAccountID, tokenID int64) error AddServiceAccountToken(ctx context.Context, serviceAccountID int64, cmd *AddServiceAccountTokenCommand) error diff --git a/pkg/services/serviceaccounts/tests/common.go b/pkg/services/serviceaccounts/tests/common.go index fcdf1418f7e..e638aa2041f 100644 --- a/pkg/services/serviceaccounts/tests/common.go +++ b/pkg/services/serviceaccounts/tests/common.go @@ -174,7 +174,7 @@ func (s *ServiceAccountsStoreMock) MigrateApiKey(ctx context.Context, orgID int6 return nil } -func (s *ServiceAccountsStoreMock) RevertApiKey(ctx context.Context, keyId int64) error { +func (s *ServiceAccountsStoreMock) RevertApiKey(ctx context.Context, saId int64, keyId int64) error { s.Calls.RevertApiKey = append(s.Calls.RevertApiKey, []interface{}{ctx}) return nil }