From 5bc4f56c7967e50d958e087c9bdbecfc4ed3ee4e Mon Sep 17 00:00:00 2001 From: linoman <2051016+linoman@users.noreply.github.com> Date: Thu, 9 Nov 2023 17:45:46 +0100 Subject: [PATCH] IAM: Protect external service accounts frontend list page (#77834) * Add `isExternal` property to frontend model * Remove enabled and token buttons for external SA * Replace trash icon for lock icon for external SA * Block the role picker for external SA * Filter SA list using the external filter * Add only external filter at backend --------- Co-authored-by: Gabriel MABILLE --- pkg/services/serviceaccounts/api/api.go | 8 +++ pkg/services/serviceaccounts/models.go | 4 +- pkg/services/serviceaccounts/proxy/service.go | 6 +- .../serviceaccounts/proxy/service_test.go | 6 +- public/api-enterprise-spec.json | 4 +- public/api-merged.json | 4 +- .../ServiceAccountsListPage.tsx | 17 +++-- .../components/ServiceAccountsListItem.tsx | 65 ++++++++++++------- .../features/serviceaccounts/state/actions.ts | 2 + public/app/types/serviceaccount.ts | 2 + public/openapi3.json | 4 +- 11 files changed, 80 insertions(+), 42 deletions(-) diff --git a/pkg/services/serviceaccounts/api/api.go b/pkg/services/serviceaccounts/api/api.go index bf9d8ed3b54..4932c83a061 100644 --- a/pkg/services/serviceaccounts/api/api.go +++ b/pkg/services/serviceaccounts/api/api.go @@ -12,6 +12,7 @@ import ( "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/auth/identity" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" + "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/serviceaccounts" "github.com/grafana/grafana/pkg/setting" @@ -27,6 +28,7 @@ type ServiceAccountsAPI struct { RouterRegister routing.RouteRegister log log.Logger permissionService accesscontrol.ServiceAccountPermissionsService + isExternalSAEnabled bool } func NewServiceAccountsAPI( @@ -36,6 +38,7 @@ func NewServiceAccountsAPI( accesscontrolService accesscontrol.Service, routerRegister routing.RouteRegister, permissionService accesscontrol.ServiceAccountPermissionsService, + features *featuremgmt.FeatureManager, ) *ServiceAccountsAPI { return &ServiceAccountsAPI{ cfg: cfg, @@ -45,6 +48,7 @@ func NewServiceAccountsAPI( RouterRegister: routerRegister, log: log.New("serviceaccounts.api"), permissionService: permissionService, + isExternalSAEnabled: features.IsEnabled(featuremgmt.FlagExternalServiceAccounts) || features.IsEnabled(featuremgmt.FlagExternalServiceAuth), } } @@ -265,10 +269,14 @@ func (api *ServiceAccountsAPI) SearchOrgServiceAccountsWithPaging(c *contextmode // its okay that it fails, it is only filtering that might be weird, but to safe quard against any weird incoming query param onlyWithExpiredTokens := c.QueryBool("expiredTokens") onlyDisabled := c.QueryBool("disabled") + onlyExternal := c.QueryBool("external") filter := serviceaccounts.FilterIncludeAll if onlyWithExpiredTokens { filter = serviceaccounts.FilterOnlyExpiredTokens } + if api.isExternalSAEnabled && onlyExternal { + filter = serviceaccounts.FilterOnlyExternal + } if onlyDisabled { filter = serviceaccounts.FilterOnlyDisabled } diff --git a/pkg/services/serviceaccounts/models.go b/pkg/services/serviceaccounts/models.go index d126fe57f96..90b74ee6634 100644 --- a/pkg/services/serviceaccounts/models.go +++ b/pkg/services/serviceaccounts/models.go @@ -85,7 +85,7 @@ type ServiceAccountDTO struct { // example: false IsDisabled bool `json:"isDisabled" xorm:"is_disabled"` // example: false - IsManaged bool `json:"isManaged,omitempty" xorm:"-"` + IsExternal bool `json:"isExternal,omitempty" xorm:"-"` // example: Viewer Role string `json:"role" xorm:"role"` // example: 0 @@ -157,7 +157,7 @@ type ServiceAccountProfileDTO struct { // example: [] Teams []string `json:"teams" xorm:"-"` // example: false - IsManaged bool `json:"isManaged,omitempty" xorm:"-"` + IsExternal bool `json:"isExternal,omitempty" xorm:"-"` Tokens int64 `json:"tokens,omitempty"` AccessControl map[string]bool `json:"accessControl,omitempty" xorm:"-"` diff --git a/pkg/services/serviceaccounts/proxy/service.go b/pkg/services/serviceaccounts/proxy/service.go index 875c4a3f48f..4aa593ebaa3 100644 --- a/pkg/services/serviceaccounts/proxy/service.go +++ b/pkg/services/serviceaccounts/proxy/service.go @@ -41,7 +41,7 @@ func ProvideServiceAccountsProxy( isProxyEnabled: features.IsEnabled(featuremgmt.FlagExternalServiceAccounts) || features.IsEnabled(featuremgmt.FlagExternalServiceAuth), } - serviceaccountsAPI := api.NewServiceAccountsAPI(cfg, s, ac, accesscontrolService, routeRegister, permissionService) + serviceaccountsAPI := api.NewServiceAccountsAPI(cfg, s, ac, accesscontrolService, routeRegister, permissionService, features) serviceaccountsAPI.RegisterAPIEndpoints() return s, nil @@ -138,7 +138,7 @@ func (s *ServiceAccountsProxy) RetrieveServiceAccount(ctx context.Context, orgID } if s.isProxyEnabled { - sa.IsManaged = isExternalServiceAccount(sa.Login) + sa.IsExternal = isExternalServiceAccount(sa.Login) } return sa, nil @@ -175,7 +175,7 @@ func (s *ServiceAccountsProxy) SearchOrgServiceAccounts(ctx context.Context, que if s.isProxyEnabled { for i := range sa.ServiceAccounts { - sa.ServiceAccounts[i].IsManaged = isExternalServiceAccount(sa.ServiceAccounts[i].Login) + sa.ServiceAccounts[i].IsExternal = isExternalServiceAccount(sa.ServiceAccounts[i].Login) } } return sa, nil diff --git a/pkg/services/serviceaccounts/proxy/service_test.go b/pkg/services/serviceaccounts/proxy/service_test.go index b5e6b5cce62..db0325a2172 100644 --- a/pkg/services/serviceaccounts/proxy/service_test.go +++ b/pkg/services/serviceaccounts/proxy/service_test.go @@ -146,7 +146,7 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { serviceMock.ExpectedServiceAccountProfile = tc.expectedServiceAccount sa, err := svc.RetrieveServiceAccount(context.Background(), testOrgId, testServiceAccountId) assert.NoError(t, err, tc.description) - assert.Equal(t, tc.expectedIsExternal, sa.IsManaged, tc.description) + assert.Equal(t, tc.expectedIsExternal, sa.IsExternal, tc.description) }) } }) @@ -164,8 +164,8 @@ func TestProvideServiceAccount_crudServiceAccount(t *testing.T) { res, err := svc.SearchOrgServiceAccounts(context.Background(), &serviceaccounts.SearchOrgServiceAccountsQuery{OrgID: 1}) require.Len(t, res.ServiceAccounts, 2) require.NoError(t, err) - require.False(t, res.ServiceAccounts[0].IsManaged) - require.True(t, res.ServiceAccounts[1].IsManaged) + require.False(t, res.ServiceAccounts[0].IsExternal) + require.True(t, res.ServiceAccounts[1].IsExternal) }) t.Run("should update service account", func(t *testing.T) { diff --git a/public/api-enterprise-spec.json b/public/api-enterprise-spec.json index 94b9bd14f3e..a9bc2297374 100644 --- a/public/api-enterprise-spec.json +++ b/public/api-enterprise-spec.json @@ -6767,7 +6767,7 @@ "type": "boolean", "example": false }, - "isManaged": { + "isExternal": { "type": "boolean", "example": false }, @@ -6822,7 +6822,7 @@ "type": "boolean", "example": false }, - "isManaged": { + "isExternal": { "type": "boolean", "example": false }, diff --git a/public/api-merged.json b/public/api-merged.json index 034764e06dc..6f410199d4d 100644 --- a/public/api-merged.json +++ b/public/api-merged.json @@ -18652,7 +18652,7 @@ "type": "boolean", "example": false }, - "isManaged": { + "isExternal": { "type": "boolean", "example": false }, @@ -18707,7 +18707,7 @@ "type": "boolean", "example": false }, - "isManaged": { + "isExternal": { "type": "boolean", "example": false }, diff --git a/public/app/features/serviceaccounts/ServiceAccountsListPage.tsx b/public/app/features/serviceaccounts/ServiceAccountsListPage.tsx index d6aedc26be6..79411995351 100644 --- a/public/app/features/serviceaccounts/ServiceAccountsListPage.tsx +++ b/public/app/features/serviceaccounts/ServiceAccountsListPage.tsx @@ -17,6 +17,7 @@ import { import EmptyListCTA from 'app/core/components/EmptyListCTA/EmptyListCTA'; import { Page } from 'app/core/components/Page/Page'; import PageLoader from 'app/core/components/PageLoader/PageLoader'; +import config from 'app/core/config'; import { contextSrv } from 'app/core/core'; import { StoreState, ServiceAccountDTO, AccessControlAction, ServiceAccountStateFilter } from 'app/types'; @@ -56,6 +57,16 @@ const mapDispatchToProps = { const connector = connect(mapStateToProps, mapDispatchToProps); +const availableFilters = [ + { label: 'All', value: ServiceAccountStateFilter.All }, + { label: 'With expired tokens', value: ServiceAccountStateFilter.WithExpiredTokens }, + { label: 'Disabled', value: ServiceAccountStateFilter.Disabled }, +]; + +if (config.featureToggles.externalServiceAccounts || config.featureToggles.externalServiceAuth) { + availableFilters.push({ label: 'Managed', value: ServiceAccountStateFilter.External }); +} + export const ServiceAccountsListPageUnconnected = ({ page, changePage, @@ -191,11 +202,7 @@ export const ServiceAccountsListPageUnconnected = ({ /> onRoleChange(newRole, serviceAccount)} /> @@ -112,32 +112,48 @@ const ServiceAccountListItem = memo( - - {contextSrv.hasPermission(AccessControlAction.ServiceAccountsWrite) && !serviceAccount.tokens && ( - - )} - {contextSrv.hasPermissionInMetadata(AccessControlAction.ServiceAccountsWrite, serviceAccount) && - (serviceAccount.isDisabled ? ( - - ) : ( - - ))} - {contextSrv.hasPermissionInMetadata(AccessControlAction.ServiceAccountsDelete, serviceAccount) && ( + )} + {contextSrv.hasPermissionInMetadata(AccessControlAction.ServiceAccountsWrite, serviceAccount) && + (serviceAccount.isDisabled ? ( + + ) : ( + + ))} + {contextSrv.hasPermissionInMetadata(AccessControlAction.ServiceAccountsDelete, serviceAccount) && ( + onRemoveButtonClick(serviceAccount)} + tooltip={`Delete service account ${serviceAccount.name}`} + /> + )} + + )} + {serviceAccount.isExternal && ( + onRemoveButtonClick(serviceAccount)} - tooltip={`Delete service account ${serviceAccount.name}`} + tooltip={`This is a managed service account and cannot be modified.`} /> - )} - + + )} ); @@ -174,6 +190,9 @@ const getStyles = (theme: GrafanaTheme2) => { color: ${theme.colors.text.secondary}; } `, + actionButton: css({ + minWidth: 85, + }), }; }; diff --git a/public/app/features/serviceaccounts/state/actions.ts b/public/app/features/serviceaccounts/state/actions.ts index 9a9433a24f1..51a31906d35 100644 --- a/public/app/features/serviceaccounts/state/actions.ts +++ b/public/app/features/serviceaccounts/state/actions.ts @@ -114,6 +114,8 @@ const getStateFilter = (value: ServiceAccountStateFilter) => { return '&expiredTokens=true'; case ServiceAccountStateFilter.Disabled: return '&disabled=true'; + case ServiceAccountStateFilter.External: + return '&external=true'; default: return ''; } diff --git a/public/app/types/serviceaccount.ts b/public/app/types/serviceaccount.ts index b14751e122f..8a3d4a8ef9a 100644 --- a/public/app/types/serviceaccount.ts +++ b/public/app/types/serviceaccount.ts @@ -34,6 +34,7 @@ export interface ServiceAccountDTO extends WithAccessControlMetadata { avatarUrl?: string; createdAt: string; isDisabled: boolean; + isExternal?: boolean; teams: string[]; role: OrgRole; roles?: Role[]; @@ -60,6 +61,7 @@ export interface ServiceAccountProfileState { export enum ServiceAccountStateFilter { All = 'All', WithExpiredTokens = 'WithExpiredTokens', + External = 'External', Disabled = 'Disabled', } diff --git a/public/openapi3.json b/public/openapi3.json index c43e7814b3f..bb3c4006ccf 100644 --- a/public/openapi3.json +++ b/public/openapi3.json @@ -9554,7 +9554,7 @@ "example": false, "type": "boolean" }, - "isManaged": { + "isExternal": { "example": false, "type": "boolean" }, @@ -9609,7 +9609,7 @@ "example": false, "type": "boolean" }, - "isManaged": { + "isExternal": { "example": false, "type": "boolean" },