From ad4b05323199b31938e4f3d2f5210c04dcf44725 Mon Sep 17 00:00:00 2001 From: Eric Leijonmarck Date: Fri, 3 Mar 2023 16:12:34 +0000 Subject: [PATCH] API keys: Remove state hideAPIkeys and refactor interface to IsDisabled (#64018) * remove state and refactor interface to IsDisabled * update docs and span * Update pkg/services/apikey/apikey.go Co-authored-by: Alexander Zobnin --------- Co-authored-by: Alexander Zobnin --- docs/sources/administration/api-keys/index.md | 11 --- pkg/services/apikey/apikey.go | 3 +- pkg/services/apikey/apikeyimpl/apikey.go | 11 ++- pkg/services/apikey/apikeytest/fake.go | 8 +- pkg/services/navtree/navtreeimpl/admin.go | 8 +- pkg/services/serviceaccounts/api/api.go | 11 --- .../serviceaccounts/database/store.go | 7 -- .../serviceaccounts/manager/service.go | 7 -- .../serviceaccounts/manager/service_test.go | 5 -- pkg/services/serviceaccounts/manager/store.go | 12 --- .../features/api-keys/APIKeysMigratedCard.tsx | 52 ------------- .../features/api-keys/ApiKeysController.tsx | 21 ------ .../features/api-keys/ApiKeysPage.test.tsx | 2 - public/app/features/api-keys/ApiKeysPage.tsx | 73 +++++++------------ .../api-keys/MigrateToServiceAccountsCard.tsx | 66 +++++++++++------ public/app/features/api-keys/state/actions.ts | 6 -- 16 files changed, 87 insertions(+), 216 deletions(-) delete mode 100644 public/app/features/api-keys/APIKeysMigratedCard.tsx delete mode 100644 public/app/features/api-keys/ApiKeysController.tsx diff --git a/docs/sources/administration/api-keys/index.md b/docs/sources/administration/api-keys/index.md index 1882c542c0f..f3168b7fb96 100644 --- a/docs/sources/administration/api-keys/index.md +++ b/docs/sources/administration/api-keys/index.md @@ -69,14 +69,3 @@ You can choose to migrate a single API key or all API keys. Note that when you m 1. Sign in to Grafana, hover your cursor over **Configuration** (the gear icon), and click **API Keys**. 1. Find the API Key you want to migrate. 1. Click **Migrate to service account**. - -### Revert service account token to API key - -**Note:** This is undesired operation and should be used only in emergency situations. - -It is possible to convert back service account token to API key. You can use the [Revert service account token to API key HTTP API]({{< relref "../../developers/http_api/create-api-tokens-for-org/#how-to-create-a-new-organization-and-an-api-token" >}}) for that. - -**The revert will perform the following actions:** - -1. Convert the given service account token back to API key -1. Delete the service account associated with the given key. **Make sure there are no other tokens associated with the service account, otherwise they all will be deleted.** diff --git a/pkg/services/apikey/apikey.go b/pkg/services/apikey/apikey.go index ccdd3454ace..cde6c9fd097 100644 --- a/pkg/services/apikey/apikey.go +++ b/pkg/services/apikey/apikey.go @@ -7,11 +7,12 @@ import ( type Service interface { GetAPIKeys(ctx context.Context, query *GetApiKeysQuery) error GetAllAPIKeys(ctx context.Context, orgID int64) ([]*APIKey, error) - CountAPIKeys(ctx context.Context, orgID int64) (int64, error) DeleteApiKey(ctx context.Context, cmd *DeleteCommand) error AddAPIKey(ctx context.Context, cmd *AddCommand) error GetApiKeyById(ctx context.Context, query *GetByIDQuery) error GetApiKeyByName(ctx context.Context, query *GetByNameQuery) error GetAPIKeyByHash(ctx context.Context, hash string) (*APIKey, error) UpdateAPIKeyLastUsedDate(ctx context.Context, tokenID int64) error + // IsDisabled returns true if the API key is not available for use. + IsDisabled(ctx context.Context, orgID int64) (bool, error) } diff --git a/pkg/services/apikey/apikeyimpl/apikey.go b/pkg/services/apikey/apikeyimpl/apikey.go index 1472662628e..e5f6ea2f098 100644 --- a/pkg/services/apikey/apikeyimpl/apikey.go +++ b/pkg/services/apikey/apikeyimpl/apikey.go @@ -68,8 +68,15 @@ func (s *Service) AddAPIKey(ctx context.Context, cmd *apikey.AddCommand) error { func (s *Service) UpdateAPIKeyLastUsedDate(ctx context.Context, tokenID int64) error { return s.store.UpdateAPIKeyLastUsedDate(ctx, tokenID) } -func (s *Service) CountAPIKeys(ctx context.Context, orgID int64) (int64, error) { - return s.store.CountAPIKeys(ctx, orgID) + +// IsDisabled returns true if the apikey service is disabled for the given org. +// This is the case if the org has no apikeys. +func (s *Service) IsDisabled(ctx context.Context, orgID int64) (bool, error) { + apikeys, err := s.store.CountAPIKeys(ctx, orgID) + if err != nil { + return false, err + } + return apikeys == 0, nil } func readQuotaConfig(cfg *setting.Cfg) (*quota.Map, error) { diff --git a/pkg/services/apikey/apikeytest/fake.go b/pkg/services/apikey/apikeytest/fake.go index 00f14b326f6..c03aa16c040 100644 --- a/pkg/services/apikey/apikeytest/fake.go +++ b/pkg/services/apikey/apikeytest/fake.go @@ -8,7 +8,7 @@ import ( type Service struct { ExpectedError error - ExpectedCount int64 + ExpectedBool bool ExpectedAPIKeys []*apikey.APIKey ExpectedAPIKey *apikey.APIKey } @@ -20,9 +20,6 @@ func (s *Service) GetAPIKeys(ctx context.Context, query *apikey.GetApiKeysQuery) func (s *Service) GetAllAPIKeys(ctx context.Context, orgID int64) ([]*apikey.APIKey, error) { return s.ExpectedAPIKeys, s.ExpectedError } -func (s *Service) CountAPIKeys(ctx context.Context, orgID int64) (int64, error) { - return s.ExpectedCount, s.ExpectedError -} func (s *Service) GetApiKeyById(ctx context.Context, query *apikey.GetByIDQuery) error { query.Result = s.ExpectedAPIKey return s.ExpectedError @@ -44,3 +41,6 @@ func (s *Service) AddAPIKey(ctx context.Context, cmd *apikey.AddCommand) error { func (s *Service) UpdateAPIKeyLastUsedDate(ctx context.Context, tokenID int64) error { return s.ExpectedError } +func (s *Service) IsDisabled(ctx context.Context, orgID int64) (bool, error) { + return s.ExpectedBool, s.ExpectedError +} diff --git a/pkg/services/navtree/navtreeimpl/admin.go b/pkg/services/navtree/navtreeimpl/admin.go index 531c63233b1..0fd2df0146f 100644 --- a/pkg/services/navtree/navtreeimpl/admin.go +++ b/pkg/services/navtree/navtreeimpl/admin.go @@ -79,15 +79,11 @@ func (s *ServiceImpl) getOrgAdminNode(c *contextmodel.ReqContext) (*navtree.NavL }) } - hideApiKeys, _, _ := s.kvStore.Get(c.Req.Context(), c.OrgID, "serviceaccounts", "hideApiKeys") - apiKeys, err := s.apiKeyService.CountAPIKeys(c.Req.Context(), c.OrgID) + disabled, err := s.apiKeyService.IsDisabled(c.Req.Context(), c.OrgID) if err != nil { return nil, err } - - // Hide API keys if the global setting is set or if the org setting is set and there are no API keys - apiKeysHidden := hideApiKeys == "1" && apiKeys == 0 - if hasAccess(ac.ReqOrgAdmin, ac.ApiKeyAccessEvaluator) && !apiKeysHidden { + if hasAccess(ac.ReqOrgAdmin, ac.ApiKeyAccessEvaluator) && !disabled { configNodes = append(configNodes, &navtree.NavLink{ Text: "API keys", Id: "apikeys", diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index dba8f532d09..e1544f31e25 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/pkg/services/serviceaccounts/api/api.go @@ -40,7 +40,6 @@ type service interface { SearchOrgServiceAccounts(ctx context.Context, query *serviceaccounts.SearchOrgServiceAccountsQuery) (*serviceaccounts.SearchOrgServiceAccountsResult, error) ListTokens(ctx context.Context, query *serviceaccounts.GetSATokensQuery) ([]apikey.APIKey, error) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error - HideApiKeysTab(ctx context.Context, orgID int64) error MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error MigrateApiKey(ctx context.Context, orgID int64, keyId int64) error // Service account tokens @@ -86,8 +85,6 @@ func (api *ServiceAccountsAPI) RegisterAPIEndpoints() { accesscontrol.EvalPermission(serviceaccounts.ActionWrite, serviceaccounts.ScopeID)), routing.Wrap(api.CreateToken)) serviceAccountsRoute.Delete("/:serviceAccountId/tokens/:tokenId", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionWrite, serviceaccounts.ScopeID)), routing.Wrap(api.DeleteToken)) - serviceAccountsRoute.Post("/hideApiKeys", auth(middleware.ReqOrgAdmin, - accesscontrol.EvalPermission(serviceaccounts.ActionCreate)), routing.Wrap(api.HideApiKeysTab)) serviceAccountsRoute.Post("/migrate", auth(middleware.ReqOrgAdmin, accesscontrol.EvalPermission(serviceaccounts.ActionCreate)), routing.Wrap(api.MigrateApiKeysToServiceAccounts)) serviceAccountsRoute.Post("/migrate/:keyId", auth(middleware.ReqOrgAdmin, @@ -357,14 +354,6 @@ func (api *ServiceAccountsAPI) SearchOrgServiceAccountsWithPaging(c *contextmode return response.JSON(http.StatusOK, serviceAccountSearch) } -// POST /api/serviceaccounts/hideapikeys -func (api *ServiceAccountsAPI) HideApiKeysTab(ctx *contextmodel.ReqContext) response.Response { - if err := api.service.HideApiKeysTab(ctx.Req.Context(), ctx.OrgID); err != nil { - return response.Error(http.StatusInternalServerError, "Internal server error", err) - } - return response.Success("API keys hidden") -} - // POST /api/serviceaccounts/migrate func (api *ServiceAccountsAPI) MigrateApiKeysToServiceAccounts(ctx *contextmodel.ReqContext) response.Response { if err := api.service.MigrateApiKeysToServiceAccounts(ctx.Req.Context(), ctx.OrgID); err != nil { diff --git a/pkg/services/serviceaccounts/database/store.go b/pkg/services/serviceaccounts/database/store.go index 4f5d74203bd..5679e87ff12 100644 --- a/pkg/services/serviceaccounts/database/store.go +++ b/pkg/services/serviceaccounts/database/store.go @@ -364,13 +364,6 @@ func (s *ServiceAccountsStoreImpl) SearchOrgServiceAccounts(ctx context.Context, return searchResult, nil } -func (s *ServiceAccountsStoreImpl) HideApiKeysTab(ctx context.Context, orgId int64) error { - if err := s.kvStore.Set(ctx, orgId, "serviceaccounts", "hideApiKeys", "1"); err != nil { - s.log.Error("Failed to hide API keys tab", err) - } - return nil -} - func (s *ServiceAccountsStoreImpl) MigrateApiKeysToServiceAccounts(ctx context.Context, orgId int64) error { basicKeys, err := s.apiKeyService.GetAllAPIKeys(ctx, orgId) if err != nil { diff --git a/pkg/services/serviceaccounts/manager/service.go b/pkg/services/serviceaccounts/manager/service.go index cd152e7cc81..a415617fd6a 100644 --- a/pkg/services/serviceaccounts/manager/service.go +++ b/pkg/services/serviceaccounts/manager/service.go @@ -226,13 +226,6 @@ func (sa *ServiceAccountsService) DeleteServiceAccountToken(ctx context.Context, return sa.store.DeleteServiceAccountToken(ctx, orgID, serviceAccountID, tokenID) } -func (sa *ServiceAccountsService) HideApiKeysTab(ctx context.Context, orgID int64) error { - if err := validOrgID(orgID); err != nil { - return err - } - return sa.store.HideApiKeysTab(ctx, orgID) -} - func (sa *ServiceAccountsService) MigrateApiKey(ctx context.Context, orgID, keyID int64) error { if err := validOrgID(orgID); err != nil { return err diff --git a/pkg/services/serviceaccounts/manager/service_test.go b/pkg/services/serviceaccounts/manager/service_test.go index 9d7f806385c..3b182dc7ab4 100644 --- a/pkg/services/serviceaccounts/manager/service_test.go +++ b/pkg/services/serviceaccounts/manager/service_test.go @@ -59,11 +59,6 @@ func (f *FakeServiceAccountStore) DeleteServiceAccount(ctx context.Context, orgI return f.ExpectedError } -// HideApiKeysTab is a fake hiding the api keys tab. -func (f *FakeServiceAccountStore) HideApiKeysTab(ctx context.Context, orgID int64) error { - return f.ExpectedError -} - // MigrateApiKeysToServiceAccounts is a fake migrating api keys to service accounts. func (f *FakeServiceAccountStore) MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error { return f.ExpectedError diff --git a/pkg/services/serviceaccounts/manager/store.go b/pkg/services/serviceaccounts/manager/store.go index 46893811e11..fd7182c9844 100644 --- a/pkg/services/serviceaccounts/manager/store.go +++ b/pkg/services/serviceaccounts/manager/store.go @@ -7,17 +7,6 @@ import ( "github.com/grafana/grafana/pkg/services/serviceaccounts" ) -/* -Store is the database store for service accounts. - -migration from apikeys to service accounts: -HideApiKeyTab is used to hide the api key tab in the UI. -MigrateApiKeysToServiceAccounts migrates all API keys to service accounts. -MigrateApiKey migrates a single API key to a service account. - -// only used for interal api calls -RevertApiKey reverts a single service account to an API key. -*/ type store interface { CreateServiceAccount(ctx context.Context, orgID int64, saForm *serviceaccounts.CreateServiceAccountForm) (*serviceaccounts.ServiceAccountDTO, error) SearchOrgServiceAccounts(ctx context.Context, query *serviceaccounts.SearchOrgServiceAccountsQuery) (*serviceaccounts.SearchOrgServiceAccountsResult, error) @@ -26,7 +15,6 @@ type store interface { RetrieveServiceAccount(ctx context.Context, orgID, serviceAccountID int64) (*serviceaccounts.ServiceAccountProfileDTO, error) RetrieveServiceAccountIdByName(ctx context.Context, orgID int64, name string) (int64, error) DeleteServiceAccount(ctx context.Context, orgID, serviceAccountID int64) error - HideApiKeysTab(ctx context.Context, orgID int64) error MigrateApiKeysToServiceAccounts(ctx context.Context, orgID int64) error MigrateApiKey(ctx context.Context, orgID int64, keyId int64) error ListTokens(ctx context.Context, query *serviceaccounts.GetSATokensQuery) ([]apikey.APIKey, error) diff --git a/public/app/features/api-keys/APIKeysMigratedCard.tsx b/public/app/features/api-keys/APIKeysMigratedCard.tsx deleted file mode 100644 index a6cd305c6e7..00000000000 --- a/public/app/features/api-keys/APIKeysMigratedCard.tsx +++ /dev/null @@ -1,52 +0,0 @@ -import { css } from '@emotion/css'; -import React, { useState } from 'react'; - -import { GrafanaTheme2 } from '@grafana/data'; -import { Alert, ConfirmModal, useStyles2, Button } from '@grafana/ui'; - -interface Props { - onHideApiKeys: () => void; - apikeys: number; -} - -export const APIKeysMigratedCard = ({ onHideApiKeys, apikeys }: Props): JSX.Element => { - const [isModalOpen, setIsModalOpen] = useState(false); - const styles = useStyles2(getStyles); - - return ( - -
- Migrated API keys are safe and continue working as they used to. You can find them inside the respective service - account. -
-
- - setIsModalOpen(false)} - confirmButtonVariant="primary" - /> - View service accounts page -
- - ); -}; - -export const getStyles = (theme: GrafanaTheme2) => ({ - text: css` - margin-bottom: ${theme.spacing(2)}; - `, - actionRow: css` - display: flex; - align-items: center; - `, - actionButton: css` - margin-right: ${theme.spacing(2)}; - `, -}); diff --git a/public/app/features/api-keys/ApiKeysController.tsx b/public/app/features/api-keys/ApiKeysController.tsx deleted file mode 100644 index 6b1c65c3b6e..00000000000 --- a/public/app/features/api-keys/ApiKeysController.tsx +++ /dev/null @@ -1,21 +0,0 @@ -import { FC, useCallback, useState } from 'react'; - -interface Api { - isAdding: boolean; - toggleIsAdding: () => void; -} - -interface Props { - children: (props: Api) => JSX.Element; -} - -export const ApiKeysController: FC = ({ children }) => { - // FIXME(eleijonmarck): could not remove state from this component - // as component cannot render properly without it - const [isAdding, setIsAdding] = useState(false); - const toggleIsAdding = useCallback(() => { - setIsAdding(!isAdding); - }, [isAdding]); - - return children({ isAdding, toggleIsAdding }); -}; diff --git a/public/app/features/api-keys/ApiKeysPage.test.tsx b/public/app/features/api-keys/ApiKeysPage.test.tsx index f4369e75487..2e6e688df96 100644 --- a/public/app/features/api-keys/ApiKeysPage.test.tsx +++ b/public/app/features/api-keys/ApiKeysPage.test.tsx @@ -29,7 +29,6 @@ const setup = (propOverrides: Partial) => { const migrateAllMock = jest.fn(); const toggleIncludeExpiredMock = jest.fn(); const setSearchQueryMock = mockToolkitActionCreator(setSearchQuery); - const hideApiKeysMock = jest.fn(); const props: Props = { apiKeys: [] as ApiKey[], searchQuery: '', @@ -39,7 +38,6 @@ const setup = (propOverrides: Partial) => { setSearchQuery: setSearchQueryMock, migrateApiKey: migrateApiKeyMock, migrateAll: migrateAllMock, - hideApiKeys: hideApiKeysMock, apiKeysCount: 0, timeZone: 'utc', includeExpired: false, diff --git a/public/app/features/api-keys/ApiKeysPage.tsx b/public/app/features/api-keys/ApiKeysPage.tsx index 012441f2990..e6828b24d7b 100644 --- a/public/app/features/api-keys/ApiKeysPage.tsx +++ b/public/app/features/api-keys/ApiKeysPage.tsx @@ -9,19 +9,10 @@ import { contextSrv } from 'app/core/core'; import { getTimeZone } from 'app/features/profile/state/selectors'; import { AccessControlAction, ApiKey, StoreState } from 'app/types'; -import { APIKeysMigratedCard } from './APIKeysMigratedCard'; import { ApiKeysActionBar } from './ApiKeysActionBar'; -import { ApiKeysController } from './ApiKeysController'; import { ApiKeysTable } from './ApiKeysTable'; import { MigrateToServiceAccountsCard } from './MigrateToServiceAccountsCard'; -import { - deleteApiKey, - migrateApiKey, - migrateAll, - loadApiKeys, - toggleIncludeExpired, - hideApiKeys, -} from './state/actions'; +import { deleteApiKey, migrateApiKey, migrateAll, loadApiKeys, toggleIncludeExpired } from './state/actions'; import { setSearchQuery } from './state/reducers'; import { getApiKeys, getApiKeysCount, getIncludeExpired, getIncludeExpiredDisabled } from './state/selectors'; @@ -51,7 +42,6 @@ const mapDispatchToProps = { migrateAll, setSearchQuery, toggleIncludeExpired, - hideApiKeys, }; const connector = connect(mapStateToProps, mapDispatchToProps); @@ -97,9 +87,9 @@ export class ApiKeysPageUnconnected extends PureComponent { this.props.toggleIncludeExpired(); }; - onHideApiKeys = async () => { + onMigrateApiKeys = async () => { try { - await this.props.hideApiKeys(); + this.onMigrateAll(); let serviceAccountsUrl = '/org/serviceaccounts'; locationService.push(serviceAccountsUrl); window.location.reload(); @@ -128,42 +118,33 @@ export class ApiKeysPageUnconnected extends PureComponent { ); } + const showTable = apiKeysCount > 0; return ( - - {({}) => { - const showTable = apiKeysCount > 0; - return ( - <> - {apiKeysCount !== 0 && } - {apiKeysCount === 0 && ( - - )} - {showTable ? ( - - ) : null} - {showTable ? ( - - - - - - - ) : null} - - ); - }} - + <> + + {showTable ? ( + + ) : null} + {showTable ? ( + + + + + + + ) : null} + ); diff --git a/public/app/features/api-keys/MigrateToServiceAccountsCard.tsx b/public/app/features/api-keys/MigrateToServiceAccountsCard.tsx index b76e470e913..0f1d7e107b2 100644 --- a/public/app/features/api-keys/MigrateToServiceAccountsCard.tsx +++ b/public/app/features/api-keys/MigrateToServiceAccountsCard.tsx @@ -6,10 +6,11 @@ import { Alert, Button, ConfirmModal, useStyles2 } from '@grafana/ui'; interface Props { onMigrate: () => void; + apikeysCount: number; disabled?: boolean; } -export const MigrateToServiceAccountsCard = ({ onMigrate, disabled }: Props): JSX.Element => { +export const MigrateToServiceAccountsCard = ({ onMigrate, apikeysCount, disabled }: Props): JSX.Element => { const [isModalOpen, setIsModalOpen] = useState(false); const styles = useStyles2(getStyles); @@ -23,30 +24,49 @@ export const MigrateToServiceAccountsCard = ({ onMigrate, disabled }: Props): JS Find out more about the migration here. ); - const migrationBoxDesc = Are you sure you want to migrate all API keys to service accounts? {docsLink}; + const migrationBoxDesc = ( + + Are you sure you want to migrate all API keys to service accounts? {docsLink} +
This hides the API keys tab.
+ + ); return ( - -
- We will soon deprecate API keys. Each API key will be migrated into a service account with a token and will - continue to work as they were. We encourage you to migrate your API keys to service accounts now. {docsLink} -
-
- - setIsModalOpen(false)} - confirmVariant="primary" - confirmButtonVariant="primary" - /> -
- + <> + {apikeysCount > 0 && ( + +
+ We will soon deprecate API keys. Each API key will be migrated into a service account with a token and will + continue to work as they were. We encourage you to migrate your API keys to service accounts now. {docsLink} +
+
+ + setIsModalOpen(false)} + confirmVariant="primary" + confirmButtonVariant="primary" + /> +
+ + )} + {apikeysCount === 0 && ( + <> + +
+ No API keys were found. If you reload the browser, this tab will be not available. If any API keys are + created, this tab will appear again. +
+ + + )} + ); }; diff --git a/public/app/features/api-keys/state/actions.ts b/public/app/features/api-keys/state/actions.ts index 65f90b415cf..220fb770dd5 100644 --- a/public/app/features/api-keys/state/actions.ts +++ b/public/app/features/api-keys/state/actions.ts @@ -42,12 +42,6 @@ export function migrateAll(): ThunkResult { }; } -export function hideApiKeys(): ThunkResult { - return async (dispatch) => { - await getBackendSrv().post('/api/serviceaccounts/hideApiKeys'); - }; -} - export function toggleIncludeExpired(): ThunkResult { return (dispatch) => { dispatch(includeExpiredToggled());