From c6943797f94af7f793e9d4f7dad533897c6a1ef3 Mon Sep 17 00:00:00 2001 From: Eric Leijonmarck Date: Fri, 18 Feb 2022 10:43:33 +0000 Subject: [PATCH] Admin: token expiration colors and calculations (#45231) * token expiration colorign and calculations * Update public/app/features/serviceaccounts/ServiceAccountTokensTable.tsx * removed unused calculation for expiry * optional attribute * fix: typo * implement failing test :thumpsup: * tests * refactor: tests to use assertify * tiem * refactor: remote porntf * refactor: make test NOT sleep 1 sec for all builds :D Co-authored-by: J Guerreiro --- pkg/services/serviceaccounts/api/api_test.go | 18 +-- pkg/services/serviceaccounts/api/token.go | 42 +++++- .../serviceaccounts/api/token_test.go | 132 +++++++++++++++++- .../serviceaccounts/database/database.go | 4 +- .../ServiceAccountTokensTable.tsx | 70 ++++++++-- public/app/types/apiKeys.ts | 3 + 6 files changed, 234 insertions(+), 35 deletions(-) diff --git a/pkg/services/serviceaccounts/api/api_test.go b/pkg/services/serviceaccounts/api/api_test.go index b467ef6763b..bff5d52cb61 100644 --- a/pkg/services/serviceaccounts/api/api_test.go +++ b/pkg/services/serviceaccounts/api/api_test.go @@ -17,14 +17,13 @@ import ( accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/serviceaccounts" + "github.com/grafana/grafana/pkg/services/serviceaccounts/database" "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" "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" ) var ( @@ -63,7 +62,7 @@ func TestServiceAccountsAPI_DeleteServiceAccount(t *testing.T) { } serviceAccountRequestScenario(t, http.MethodDelete, serviceaccountIDPath, &testcase.user, func(httpmethod string, endpoint string, user *tests.TestUser) { createduser := tests.SetupUserServiceAccount(t, store, testcase.user) - server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), testcase.acmock, store) + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), testcase.acmock, store, database.NewServiceAccountsStore(store)) actual := requestResponse(server, httpmethod, fmt.Sprintf(endpoint, fmt.Sprint(createduser.Id))).Code require.Equal(t, testcase.expectedCode, actual) }) @@ -87,7 +86,7 @@ func TestServiceAccountsAPI_DeleteServiceAccount(t *testing.T) { } serviceAccountRequestScenario(t, http.MethodDelete, serviceaccountIDPath, &testcase.user, func(httpmethod string, endpoint string, user *tests.TestUser) { createduser := tests.SetupUserServiceAccount(t, store, testcase.user) - server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), testcase.acmock, store) + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), testcase.acmock, store, database.NewServiceAccountsStore(store)) actual := requestResponse(server, httpmethod, fmt.Sprintf(endpoint, createduser.Id)).Code require.Equal(t, testcase.expectedCode, actual) }) @@ -99,8 +98,11 @@ func serviceAccountRequestScenario(t *testing.T, httpMethod string, endpoint str fn(httpMethod, endpoint, user) } -func setupTestServer(t *testing.T, svc *tests.ServiceAccountMock, routerRegister routing.RouteRegister, acmock *accesscontrolmock.Mock, sqlStore *sqlstore.SQLStore) *web.Mux { - a := NewServiceAccountsAPI(setting.NewCfg(), svc, acmock, routerRegister, database.NewServiceAccountsStore(sqlStore), sqlStore) +func setupTestServer(t *testing.T, svc *tests.ServiceAccountMock, + routerRegister routing.RouteRegister, + acmock *accesscontrolmock.Mock, + sqlStore *sqlstore.SQLStore, saStore serviceaccounts.Store) *web.Mux { + a := NewServiceAccountsAPI(setting.NewCfg(), svc, acmock, routerRegister, saStore, sqlStore) a.RegisterAPIEndpoints(featuremgmt.WithFeatures(featuremgmt.FlagServiceAccounts)) a.cfg.ApiKeyMaxSecondsToLive = -1 // disable api key expiration @@ -190,7 +192,7 @@ func TestServiceAccountsAPI_RetrieveServiceAccount(t *testing.T) { createdUser := tests.SetupUserServiceAccount(t, store, *tc.user) scopeID = int(createdUser.Id) } - server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store) + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, database.NewServiceAccountsStore(store)) actual := requestResponse(server, httpmethod, fmt.Sprintf(endpoint, scopeID)) @@ -300,7 +302,7 @@ func TestServiceAccountsAPI_UpdateServiceAccount(t *testing.T) { createdUser := tests.SetupUserServiceAccount(t, store, *tc.user) scopeID = int(createdUser.Id) } - server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store) + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, database.NewServiceAccountsStore(store)) var rawBody io.Reader = http.NoBody if tc.body != nil { diff --git a/pkg/services/serviceaccounts/api/token.go b/pkg/services/serviceaccounts/api/token.go index 506f06afb7a..a66bcc226ff 100644 --- a/pkg/services/serviceaccounts/api/token.go +++ b/pkg/services/serviceaccounts/api/token.go @@ -16,6 +16,26 @@ import ( const failedToDeleteMsg = "Failed to delete API key" +type TokenDTO struct { + Id int64 `json:"id"` + Name string `json:"name"` + Role models.RoleType `json:"role"` + Created *time.Time `json:"created"` + Expiration *time.Time `json:"expiration"` + SecondsUntilExpiration *float64 `json:"secondsUntilExpiration"` + HasExpired bool `json:"hasExpired"` +} + +func hasExpired(expiration *int64) bool { + if expiration == nil { + return false + } + v := time.Unix(*expiration, 0) + return (v).Before(time.Now()) +} + +const sevenDaysAhead = 7 * 24 * time.Hour + func (api *ServiceAccountsAPI) ListTokens(ctx *models.ReqContext) response.Response { saID, err := strconv.ParseInt(web.Params(ctx.Req)[":serviceAccountId"], 10, 64) if err != nil { @@ -23,18 +43,28 @@ func (api *ServiceAccountsAPI) ListTokens(ctx *models.ReqContext) response.Respo } if saTokens, err := api.store.ListTokens(ctx.Req.Context(), ctx.OrgId, saID); err == nil { - result := make([]*models.ApiKeyDTO, len(saTokens)) + result := make([]*TokenDTO, len(saTokens)) for i, t := range saTokens { var expiration *time.Time = nil + var secondsUntilExpiration float64 = 0 + + 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[i] = &models.ApiKeyDTO{ - Id: t.Id, - Name: t.Name, - Role: t.Role, - Expiration: expiration, + + result[i] = &TokenDTO{ + Id: t.Id, + Name: t.Name, + Role: t.Role, + Created: &t.Created, + Expiration: expiration, + SecondsUntilExpiration: &secondsUntilExpiration, + HasExpired: isExpired, } } diff --git a/pkg/services/serviceaccounts/api/token_test.go b/pkg/services/serviceaccounts/api/token_test.go index b404b08549f..a90af8ec96f 100644 --- a/pkg/services/serviceaccounts/api/token_test.go +++ b/pkg/services/serviceaccounts/api/token_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "strings" "testing" + "time" "github.com/grafana/grafana/pkg/api/routing" "github.com/grafana/grafana/pkg/bus" @@ -17,6 +18,7 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" "github.com/grafana/grafana/pkg/services/serviceaccounts" + "github.com/grafana/grafana/pkg/services/serviceaccounts/database" "github.com/grafana/grafana/pkg/services/serviceaccounts/tests" "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/web" @@ -29,7 +31,7 @@ const ( serviceaccountIDTokensDetailPath = "/api/serviceaccounts/%v/tokens/%v" // #nosec G101 ) -func createTokenforSA(t *testing.T, keyName string, orgID int64, saID int64) *models.ApiKey { +func createTokenforSA(t *testing.T, keyName string, orgID int64, saID int64, secondsToLive int64) *models.ApiKey { key, err := apikeygen.New(orgID, keyName) require.NoError(t, err) cmd := models.AddApiKeyCommand{ @@ -37,13 +39,12 @@ func createTokenforSA(t *testing.T, keyName string, orgID int64, saID int64) *mo Role: "Viewer", OrgId: orgID, Key: key.HashedKey, - SecondsToLive: 0, + SecondsToLive: secondsToLive, ServiceAccountId: &saID, Result: &models.ApiKey{}, } err = bus.Dispatch(context.Background(), &cmd) require.NoError(t, err) - return cmd.Result } @@ -129,7 +130,7 @@ func TestServiceAccountsAPI_CreateToken(t *testing.T) { bodyString = string(b) } - server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store) + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, database.NewServiceAccountsStore(store)) actual := requestResponse(server, http.MethodPost, endpoint, strings.NewReader(bodyString)) actualCode := actual.Code @@ -215,11 +216,11 @@ func TestServiceAccountsAPI_DeleteToken(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - token := createTokenforSA(t, tc.keyName, sa.OrgId, sa.Id) + token := createTokenforSA(t, tc.keyName, sa.OrgId, sa.Id, 1) endpoint := fmt.Sprintf(serviceaccountIDTokensDetailPath, sa.Id, token.Id) bodyString := "" - server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store) + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, database.NewServiceAccountsStore(store)) actual := requestResponse(server, http.MethodDelete, endpoint, strings.NewReader(bodyString)) actualCode := actual.Code @@ -238,3 +239,122 @@ func TestServiceAccountsAPI_DeleteToken(t *testing.T) { }) } } + +type saStoreMockTokens struct { + serviceaccounts.Store + saAPIKeys []*models.ApiKey +} + +func (s *saStoreMockTokens) ListTokens(ctx context.Context, orgID, saID int64) ([]*models.ApiKey, error) { + return s.saAPIKeys, nil +} + +func TestServiceAccountsAPI_ListTokens(t *testing.T) { + store := sqlstore.InitTestDB(t) + svcmock := tests.ServiceAccountMock{} + sa := tests.SetupUserServiceAccount(t, store, tests.TestUser{Login: "sa", IsServiceAccount: true}) + + type testCreateSAToken struct { + desc string + tokens []*models.ApiKey + expectedHasExpired bool + expectedResponseBodyField string + expectedCode int + acmock *accesscontrolmock.Mock + } + + var saId int64 = 1 + var timeInFuture = time.Now().Add(time.Second * 100).Unix() + var timeInPast = time.Now().Add(-time.Second * 100).Unix() + + testCases := []testCreateSAToken{ + { + desc: "should be able to list serviceaccount with no expiration date", + tokens: []*models.ApiKey{{ + Id: 1, + OrgId: 1, + ServiceAccountId: &saId, + Expires: nil, + Name: "Test1", + }}, + acmock: tests.SetupMockAccesscontrol( + t, + func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) { + return []*accesscontrol.Permission{{Action: serviceaccounts.ActionRead, Scope: "serviceaccounts:id:1"}}, nil + }, + false, + ), + expectedHasExpired: false, + expectedResponseBodyField: "hasExpired", + expectedCode: http.StatusOK, + }, + { + desc: "should be able to list serviceaccount with secondsUntilExpiration", + tokens: []*models.ApiKey{{ + Id: 1, + OrgId: 1, + ServiceAccountId: &saId, + Expires: &timeInFuture, + Name: "Test2", + }}, + acmock: tests.SetupMockAccesscontrol( + t, + func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) { + return []*accesscontrol.Permission{{Action: serviceaccounts.ActionRead, Scope: "serviceaccounts:id:1"}}, nil + }, + false, + ), + expectedHasExpired: false, + expectedResponseBodyField: "secondsUntilExpiration", + expectedCode: http.StatusOK, + }, + { + desc: "should be able to list serviceaccount with expired token", + tokens: []*models.ApiKey{{ + Id: 1, + OrgId: 1, + ServiceAccountId: &saId, + Expires: &timeInPast, + Name: "Test3", + }}, + acmock: tests.SetupMockAccesscontrol( + t, + func(c context.Context, siu *models.SignedInUser, _ accesscontrol.Options) ([]*accesscontrol.Permission, error) { + return []*accesscontrol.Permission{{Action: serviceaccounts.ActionRead, Scope: "serviceaccounts:id:1"}}, nil + }, + false, + ), + expectedHasExpired: true, + expectedResponseBodyField: "secondsUntilExpiration", + expectedCode: http.StatusOK, + }, + } + + var requestResponse = func(server *web.Mux, httpMethod, requestpath string, requestBody io.Reader) *httptest.ResponseRecorder { + req, err := http.NewRequest(httpMethod, requestpath, requestBody) + require.NoError(t, err) + req.Header.Add("Content-Type", "application/json") + recorder := httptest.NewRecorder() + server.ServeHTTP(recorder, req) + return recorder + } + + for _, tc := range testCases { + t.Run(tc.desc, func(t *testing.T) { + endpoint := fmt.Sprintf(serviceaccountIDPath+"/tokens", sa.Id) + server := setupTestServer(t, &svcmock, routing.NewRouteRegister(), tc.acmock, store, &saStoreMockTokens{saAPIKeys: tc.tokens}) + actual := requestResponse(server, http.MethodGet, endpoint, http.NoBody) + + actualCode := actual.Code + actualBody := []map[string]interface{}{} + + _ = json.Unmarshal(actual.Body.Bytes(), &actualBody) + require.Equal(t, tc.expectedCode, actualCode, endpoint, actualBody) + + require.Equal(t, tc.expectedCode, actualCode) + require.Equal(t, tc.expectedHasExpired, actualBody[0]["hasExpired"]) + _, exists := actualBody[0][tc.expectedResponseBodyField] + require.Equal(t, exists, true) + }) + } +} diff --git a/pkg/services/serviceaccounts/database/database.go b/pkg/services/serviceaccounts/database/database.go index 35163803e48..593a23b03c3 100644 --- a/pkg/services/serviceaccounts/database/database.go +++ b/pkg/services/serviceaccounts/database/database.go @@ -128,9 +128,9 @@ func (s *ServiceAccountsStoreImpl) ListTokens(ctx context.Context, orgID int64, err := s.sqlStore.WithDbSession(ctx, func(dbSession *sqlstore.DBSession) error { var sess *xorm.Session - sess = dbSession.Limit(100, 0). + sess = dbSession. Join("inner", "user", "user.id = api_key.service_account_id"). - Where("user.org_id=? AND user.id=? AND ( expires IS NULL or expires >= ?)", orgID, serviceAccountID, time.Now().Unix()). + Where("user.org_id=? AND user.id=?", orgID, serviceAccountID). Asc("name") return sess.Find(&result) diff --git a/public/app/features/serviceaccounts/ServiceAccountTokensTable.tsx b/public/app/features/serviceaccounts/ServiceAccountTokensTable.tsx index 23dcd972391..be500c1f26e 100644 --- a/public/app/features/serviceaccounts/ServiceAccountTokensTable.tsx +++ b/public/app/features/serviceaccounts/ServiceAccountTokensTable.tsx @@ -1,5 +1,5 @@ import React, { FC } from 'react'; -import { DeleteButton, Icon, Tooltip, useTheme2 } from '@grafana/ui'; +import { DeleteButton, Icon, Tooltip, useStyles2, useTheme2 } from '@grafana/ui'; import { dateTimeFormat, GrafanaTheme2, TimeZone } from '@grafana/data'; import { ApiKey } from '../../types'; @@ -22,25 +22,19 @@ export const ServiceAccountTokensTable: FC = ({ tokens, timeZone, onDelet Name Expires + Created {tokens.map((key) => { - const isExpired = !!(key.expiration && Date.now() > new Date(key.expiration).getTime()); return ( - + {key.name} - {formatDate(timeZone, key.expiration)} - {isExpired && ( - - - - - - )} + + {formatDate(timeZone, key.created)} onDelete(key)} /> @@ -60,11 +54,61 @@ function formatDate(timeZone: TimeZone, expiration?: string): string { return dateTimeFormat(expiration, { timeZone }); } +function formatSecondsLeftUntilExpiration(secondsUntilExpiration: number): string { + const days = Math.floor(secondsUntilExpiration / (3600 * 24)); + const daysFormat = days > 1 ? `${days} days` : `${days} day`; + return `Expires in ${daysFormat}`; +} + +interface TokenExpirationProps { + timeZone: TimeZone; + token: ApiKey; +} + +const TokenExpiration = ({ timeZone, token }: TokenExpirationProps) => { + const styles = useStyles2(getStyles); + if (!token.expiration) { + return Never; + } + if (token.secondsUntilExpiration) { + return ( + + {formatSecondsLeftUntilExpiration(token.secondsUntilExpiration)} + + ); + } + if (token.hasExpired) { + return ( + + Expired + + + + + + + ); + } + return {formatDate(timeZone, token.expiration)}; +}; + const getStyles = (theme: GrafanaTheme2) => ({ - tableRow: (isExpired: boolean) => css` - color: ${isExpired ? theme.colors.text.secondary : theme.colors.text.primary}; + tableRow: (hasExpired: boolean | undefined) => css` + color: ${hasExpired ? theme.colors.text.secondary : theme.colors.text.primary}; `, tooltipContainer: css` margin-left: ${theme.spacing(1)}; `, + toolTipIcon: css` + color: ${theme.colors.error.text}; + `, + secondsUntilExpiration: css` + color: ${theme.colors.warning.text}; + `, + hasExpired: css` + color: ${theme.colors.error.text}; + `, + neverExpire: css` + color: ${theme.colors.text.secondary}; + `, }); diff --git a/public/app/types/apiKeys.ts b/public/app/types/apiKeys.ts index ec370a96521..043fb3210d2 100644 --- a/public/app/types/apiKeys.ts +++ b/public/app/types/apiKeys.ts @@ -6,6 +6,9 @@ export interface ApiKey { role: OrgRole; secondsToLive: number | null; expiration?: string; + secondsUntilExpiration?: number; + hasExpired?: boolean; + created?: string; } export interface NewApiKey {