From f3e3828279ee078108a410ba6617839d0ba1baeb Mon Sep 17 00:00:00 2001 From: Jguer Date: Thu, 16 Jun 2022 10:11:22 +0000 Subject: [PATCH] ServiceAccounts: Fix errors returning API key and simplify conditions (#50885) * ServiceAccounts: Fix naming API key->Service account token * simplify redundant elses * Apply suggestions from code review Co-authored-by: Eric Leijonmarck Co-authored-by: Alexander Zobnin Co-authored-by: Eric Leijonmarck Co-authored-by: Alexander Zobnin --- pkg/services/serviceaccounts/api/api.go | 18 +++--- pkg/services/serviceaccounts/api/token.go | 57 ++++++++++--------- .../serviceaccounts/database/database.go | 3 +- 3 files changed, 41 insertions(+), 37 deletions(-) diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index 3b5f8401d9c..23876d6cf20 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/pkg/services/serviceaccounts/api/api.go @@ -256,11 +256,11 @@ func (api *ServiceAccountsAPI) HideApiKeysTab(ctx *models.ReqContext) response.R // POST /api/serviceaccounts/migrate func (api *ServiceAccountsAPI) MigrateApiKeysToServiceAccounts(ctx *models.ReqContext) response.Response { - if err := api.store.MigrateApiKeysToServiceAccounts(ctx.Req.Context(), ctx.OrgId); err == nil { - return response.Success("API keys migrated to service accounts") - } else { + if err := api.store.MigrateApiKeysToServiceAccounts(ctx.Req.Context(), ctx.OrgId); err != nil { return response.Error(http.StatusInternalServerError, "Internal server error", err) } + + return response.Success("API keys migrated to service accounts") } // POST /api/serviceaccounts/migrate/:keyId @@ -269,11 +269,12 @@ func (api *ServiceAccountsAPI) ConvertToServiceAccount(ctx *models.ReqContext) r if err != nil { return response.Error(http.StatusBadRequest, "Key ID is invalid", err) } - if err := api.store.MigrateApiKey(ctx.Req.Context(), ctx.OrgId, keyId); err == nil { - return response.Success("Service accounts converted") - } else { + + if err := api.store.MigrateApiKey(ctx.Req.Context(), ctx.OrgId, keyId); err != nil { return response.Error(http.StatusInternalServerError, "Error converting API key", err) } + + return response.Success("Service accounts migrated") } // POST /api/serviceaccounts/revert/:keyId @@ -282,10 +283,11 @@ func (api *ServiceAccountsAPI) RevertApiKey(ctx *models.ReqContext) response.Res if err != nil { return response.Error(http.StatusBadRequest, "Key ID is invalid", err) } + if err := api.store.RevertApiKey(ctx.Req.Context(), keyId); err != nil { - return response.Error(http.StatusInternalServerError, "Error reverting API key", err) + return response.Error(http.StatusInternalServerError, "Error reverting to API key", err) } - return response.Success("API key reverted") + 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/api/token.go b/pkg/services/serviceaccounts/api/token.go index 6ea860c1a14..d1c24151cdd 100644 --- a/pkg/services/serviceaccounts/api/token.go +++ b/pkg/services/serviceaccounts/api/token.go @@ -15,7 +15,7 @@ import ( ) const ( - failedToDeleteMsg = "Failed to delete API key" + failedToDeleteMsg = "Failed to delete service account token" ServiceID = "sa" ) @@ -45,38 +45,39 @@ func (api *ServiceAccountsAPI) ListTokens(ctx *models.ReqContext) response.Respo return response.Error(http.StatusBadRequest, "Service Account ID is invalid", err) } - if saTokens, err := api.store.ListTokens(ctx.Req.Context(), ctx.OrgId, saID); err == nil { - result := make([]*TokenDTO, len(saTokens)) - for i, t := range saTokens { - var expiration *time.Time = nil - var secondsUntilExpiration float64 = 0 + saTokens, err := api.store.ListTokens(ctx.Req.Context(), ctx.OrgId, saID) + if err != nil { + return response.Error(http.StatusInternalServerError, "Internal server error", err) + } - isExpired := hasExpired(t.Expires) - if t.Expires != nil { - v := time.Unix(*t.Expires, 0) - expiration = &v - if !isExpired && (*expiration).Before(time.Now().Add(sevenDaysAhead)) { - secondsUntilExpiration = time.Until(*expiration).Seconds() - } - } + result := make([]*TokenDTO, len(saTokens)) + for i, t := range saTokens { + var expiration *time.Time = nil + var secondsUntilExpiration float64 = 0 - result[i] = &TokenDTO{ - Id: t.Id, - Name: t.Name, - Created: &t.Created, - Expiration: expiration, - SecondsUntilExpiration: &secondsUntilExpiration, - HasExpired: isExpired, + isExpired := hasExpired(t.Expires) + if t.Expires != nil { + v := time.Unix(*t.Expires, 0) + expiration = &v + if !isExpired && (*expiration).Before(time.Now().Add(sevenDaysAhead)) { + secondsUntilExpiration = time.Until(*expiration).Seconds() } } - return response.JSON(http.StatusOK, result) - } else { - return response.Error(http.StatusInternalServerError, "Internal server error", err) + result[i] = &TokenDTO{ + Id: t.Id, + Name: t.Name, + Created: &t.Created, + Expiration: expiration, + SecondsUntilExpiration: &secondsUntilExpiration, + HasExpired: isExpired, + } } + + return response.JSON(http.StatusOK, result) } -// CreateNewToken adds an API key to a service account +// CreateNewToken adds a token to a service account // POST /api/serviceaccounts/:serviceAccountId/tokens func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Response { saID, err := strconv.ParseInt(web.Params(c.Req)[":serviceAccountId"], 10, 64) @@ -113,7 +114,7 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon newKeyInfo, err := apikeygenprefix.New(ServiceID) if err != nil { - return response.Error(http.StatusInternalServerError, "Generating API key failed", err) + return response.Error(http.StatusInternalServerError, "Generating service account token failed", err) } cmd.Key = newKeyInfo.HashedKey @@ -125,7 +126,7 @@ func (api *ServiceAccountsAPI) CreateToken(c *models.ReqContext) response.Respon if errors.Is(err, models.ErrDuplicateApiKey) { return response.Error(http.StatusConflict, err.Error(), nil) } - return response.Error(http.StatusInternalServerError, "Failed to add API Key", err) + return response.Error(http.StatusInternalServerError, "Failed to add service account token", err) } result := &dtos.NewApiKeyResult{ @@ -171,5 +172,5 @@ func (api *ServiceAccountsAPI) DeleteToken(c *models.ReqContext) response.Respon return response.Error(status, failedToDeleteMsg, err) } - return response.Success("API key deleted") + return response.Success("Service account token deleted") } diff --git a/pkg/services/serviceaccounts/database/database.go b/pkg/services/serviceaccounts/database/database.go index 90f0398924c..019247a7472 100644 --- a/pkg/services/serviceaccounts/database/database.go +++ b/pkg/services/serviceaccounts/database/database.go @@ -471,8 +471,9 @@ func (s *ServiceAccountsStoreImpl) RevertApiKey(ctx context.Context, keyId int64 } return nil }) + if err != nil { - return fmt.Errorf("cannot revert API key: %w", err) + return fmt.Errorf("cannot revert to API key: %w", err) } return nil }