From 52220b2470c666e517dc7908379d0db7d84d91e2 Mon Sep 17 00:00:00 2001 From: Ieva Date: Tue, 5 Oct 2021 14:54:26 +0100 Subject: [PATCH] AccessControl: frontend changes for adding FGAC to licensing (#39484) * refactor licenseURL function to use context and export permission evaluation fction * remove provisioning file * refactor licenseURL to take in a bool to avoid circular dependencies * remove function for appending nav link, as it was only used once and move the function to create admin node * better argument names * create a function for permission checking * extend permission checking when displaying server stats * enable the use of enterprise access control actions when evaluating permissions * import ordering * move licensing FGAC action definitions to models package to allow access from oss * move evaluatePermissions for routes to context serve * change permission evaluator to take in more permissions * move licensing FGAC actions again to appease wire * avoid index out of bounds issue in case no children are passed in when creating server admin node * simplify syntax for permission checking Co-authored-by: Alex Khomenko * update loading state for server stats * linting * more linting * fix test * fix a frontend test * update "licensing.reports:read" action naming * UI doesn't allow reading only licensing reports and not the rest of licensing info Co-authored-by: Alex Khomenko --- pkg/api/frontendsettings.go | 15 ++++++------- pkg/api/frontendsettings_test.go | 17 +++++++-------- pkg/api/index.go | 13 +++--------- pkg/api/navlinks/navlinks.go | 20 ++++++++++++++++++ pkg/models/licensing.go | 2 +- .../backendplugin/manager/manager_test.go | 3 +-- pkg/services/accesscontrol/models.go | 12 +++++++++++ pkg/services/licensing/oss.go | 6 +++--- public/app/core/services/context_srv.ts | 21 ++++++++++++++++++- .../app/features/admin/ServerStats.test.tsx | 5 +++++ public/app/features/admin/ServerStats.tsx | 15 +++++++------ public/app/routes/routes.tsx | 20 +++--------------- 12 files changed, 92 insertions(+), 57 deletions(-) create mode 100644 pkg/api/navlinks/navlinks.go diff --git a/pkg/api/frontendsettings.go b/pkg/api/frontendsettings.go index 82b5568d201..450cbc3c556 100644 --- a/pkg/api/frontendsettings.go +++ b/pkg/api/frontendsettings.go @@ -4,16 +4,15 @@ import ( "errors" "strconv" - "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/tsdb/grafanads" - - "github.com/grafana/grafana/pkg/components/simplejson" - "github.com/grafana/grafana/pkg/util" - "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/components/simplejson" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" + "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/setting" + "github.com/grafana/grafana/pkg/tsdb/grafanads" + "github.com/grafana/grafana/pkg/util" ) func (hs *HTTPServer) getFSDataSources(c *models.ReqContext, enabledPlugins *plugins.EnabledPlugins) (map[string]interface{}, error) { @@ -196,6 +195,8 @@ func (hs *HTTPServer) getFrontendSettingsMap(c *models.ReqContext) (map[string]i buildstamp = 0 } + hasAccess := accesscontrol.HasAccess(hs.AccessControl, c) + jsonObj := map[string]interface{}{ "defaultDatasource": defaultDS, "datasources": dataSources, @@ -247,7 +248,7 @@ func (hs *HTTPServer) getFrontendSettingsMap(c *models.ReqContext) (map[string]i "hasValidLicense": hs.License.HasValidLicense(), "expiry": hs.License.Expiry(), "stateInfo": hs.License.StateInfo(), - "licenseUrl": hs.License.LicenseURL(c.SignedInUser), + "licenseUrl": hs.License.LicenseURL(hasAccess(accesscontrol.ReqGrafanaAdmin, accesscontrol.LicensingPageReaderAccess)), "edition": hs.License.Edition(), }, "featureToggles": hs.Cfg.FeatureToggles, diff --git a/pkg/api/frontendsettings_test.go b/pkg/api/frontendsettings_test.go index 11c858be3a8..1c46421c80d 100644 --- a/pkg/api/frontendsettings_test.go +++ b/pkg/api/frontendsettings_test.go @@ -8,19 +8,15 @@ import ( "testing" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - "github.com/grafana/grafana/pkg/plugins/manager" - "github.com/grafana/grafana/pkg/services/rendering" - - "github.com/grafana/grafana/pkg/services/licensing" - - "github.com/grafana/grafana/pkg/bus" - "github.com/grafana/grafana/pkg/services/sqlstore" - "gopkg.in/macaron.v1" + "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/plugins/manager" + accesscontrolmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock" + "github.com/grafana/grafana/pkg/services/licensing" + "github.com/grafana/grafana/pkg/services/rendering" + "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/setting" ) @@ -54,6 +50,7 @@ func setupTestEnvironment(t *testing.T, cfg *setting.Cfg) (*macaron.Macaron, *HT RenderService: r, SQLStore: sqlStore, PluginManager: pm, + AccessControl: accesscontrolmock.New().WithDisabled(), } m := macaron.New() diff --git a/pkg/api/index.go b/pkg/api/index.go index d59510c29af..014555785e1 100644 --- a/pkg/api/index.go +++ b/pkg/api/index.go @@ -6,6 +6,7 @@ import ( "strings" "github.com/grafana/grafana/pkg/api/dtos" + "github.com/grafana/grafana/pkg/api/navlinks" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" ac "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -347,16 +348,8 @@ func (hs *HTTPServer) getNavTree(c *models.ReqContext, hasEditPerm bool) ([]*dto adminNavLinks := hs.buildAdminNavLinks(c) if len(adminNavLinks) > 0 { - navTree = append(navTree, &dtos.NavLink{ - Text: "Server Admin", - SubTitle: "Manage all users and orgs", - HideFromTabs: true, - Id: "admin", - Icon: "shield", - Url: adminNavLinks[0].Url, - SortWeight: dtos.WeightAdmin, - Children: adminNavLinks, - }) + serverAdminNode := navlinks.GetServerAdminNode(adminNavLinks) + navTree = append(navTree, serverAdminNode) } helpVersion := fmt.Sprintf(`%s v%s (%s)`, setting.ApplicationName, setting.BuildVersion, setting.BuildCommit) diff --git a/pkg/api/navlinks/navlinks.go b/pkg/api/navlinks/navlinks.go new file mode 100644 index 00000000000..3ebe6cdee26 --- /dev/null +++ b/pkg/api/navlinks/navlinks.go @@ -0,0 +1,20 @@ +package navlinks + +import "github.com/grafana/grafana/pkg/api/dtos" + +func GetServerAdminNode(children []*dtos.NavLink) *dtos.NavLink { + url := "" + if len(children) > 0 { + url = children[0].Url + } + return &dtos.NavLink{ + Text: "Server Admin", + SubTitle: "Manage all users and orgs", + HideFromTabs: true, + Id: "admin", + Icon: "shield", + Url: url, + SortWeight: dtos.WeightAdmin, + Children: children, + } +} diff --git a/pkg/models/licensing.go b/pkg/models/licensing.go index 90b3f78f778..7a5abe324a9 100644 --- a/pkg/models/licensing.go +++ b/pkg/models/licensing.go @@ -16,7 +16,7 @@ type Licensing interface { // Used to build content delivery URL ContentDeliveryPrefix() string - LicenseURL(user *SignedInUser) string + LicenseURL(showAdminLicensingPage bool) string StateInfo() string } diff --git a/pkg/plugins/backendplugin/manager/manager_test.go b/pkg/plugins/backendplugin/manager/manager_test.go index 196164900e4..74ac2661dd1 100644 --- a/pkg/plugins/backendplugin/manager/manager_test.go +++ b/pkg/plugins/backendplugin/manager/manager_test.go @@ -13,7 +13,6 @@ import ( "github.com/grafana/grafana-aws-sdk/pkg/awsds" "github.com/grafana/grafana-plugin-sdk-go/backend" "github.com/grafana/grafana/pkg/infra/log" - "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins/backendplugin" "github.com/grafana/grafana/pkg/setting" "github.com/stretchr/testify/require" @@ -493,7 +492,7 @@ func (t *testLicensingService) ContentDeliveryPrefix() string { return "" } -func (t *testLicensingService) LicenseURL(user *models.SignedInUser) string { +func (t *testLicensingService) LicenseURL(showAdminLicensingPage bool) string { return "" } diff --git a/pkg/services/accesscontrol/models.go b/pkg/services/accesscontrol/models.go index b882375a085..b1ceb37f4ee 100644 --- a/pkg/services/accesscontrol/models.go +++ b/pkg/services/accesscontrol/models.go @@ -197,8 +197,20 @@ const ( // Settings scope ScopeSettingsAll = "settings:*" + + // Licensing related actions + ActionLicensingRead = "licensing:read" + ActionLicensingUpdate = "licensing:update" + ActionLicensingDelete = "licensing:delete" + ActionLicensingReportsRead = "licensing.reports:read" ) const RoleGrafanaAdmin = "Grafana Admin" const FixedRolePrefix = "fixed:" + +// LicensingPageReaderAccess defines permissions that grant access to the licensing and stats page +var LicensingPageReaderAccess = EvalAny( + EvalPermission(ActionLicensingRead), + EvalPermission(ActionServerStatsRead), +) diff --git a/pkg/services/licensing/oss.go b/pkg/services/licensing/oss.go index 4c68cd68a3e..4781554a94b 100644 --- a/pkg/services/licensing/oss.go +++ b/pkg/services/licensing/oss.go @@ -36,8 +36,8 @@ func (*OSSLicensingService) ContentDeliveryPrefix() string { return "grafana-oss" } -func (l *OSSLicensingService) LicenseURL(user *models.SignedInUser) string { - if user.IsGrafanaAdmin { +func (l *OSSLicensingService) LicenseURL(showAdminLicensingPage bool) string { + if showAdminLicensingPage { return l.Cfg.AppSubURL + "/admin/upgrading" } @@ -59,7 +59,7 @@ func ProvideService(cfg *setting.Cfg, hooksService *hooks.HooksService) *OSSLice node.Children = append(node.Children, &dtos.NavLink{ Text: "Stats and license", Id: "upgrading", - Url: l.LicenseURL(req.SignedInUser), + Url: l.LicenseURL(req.IsGrafanaAdmin), Icon: "unlock", }) } diff --git a/public/app/core/services/context_srv.ts b/public/app/core/services/context_srv.ts index fb82101a77a..90a8e811f18 100644 --- a/public/app/core/services/context_srv.ts +++ b/public/app/core/services/context_srv.ts @@ -89,7 +89,7 @@ export class ContextSrv { } isGrafanaVisible() { - return !!(document.visibilityState === undefined || document.visibilityState === 'visible'); + return document.visibilityState === undefined || document.visibilityState === 'visible'; } // checks whether the passed interval is longer than the configured minimum refresh rate @@ -113,6 +113,25 @@ export class ContextSrv { } return (this.isEditor || config.viewersCanEdit) && config.exploreEnabled; } + + hasAccess(action: string, fallBack: boolean) { + if (!config.featureToggles['accesscontrol']) { + return fallBack; + } + return this.hasPermission(action); + } + + // evaluates access control permissions, granting access if the user has any of them; uses fallback if access control is disabled + evaluatePermission(fallback: () => string[], actions: string[]) { + if (!config.featureToggles['accesscontrol']) { + return fallback(); + } + if (actions.some((action) => this.hasPermission(action))) { + return []; + } + // Hack to reject when user does not have permission + return ['Reject']; + } } let contextSrv = new ContextSrv(); diff --git a/public/app/features/admin/ServerStats.test.tsx b/public/app/features/admin/ServerStats.test.tsx index cfe42db210e..e302440cb5e 100644 --- a/public/app/features/admin/ServerStats.test.tsx +++ b/public/app/features/admin/ServerStats.test.tsx @@ -26,6 +26,11 @@ const stats: ServerStat = { jest.mock('./state/apis', () => ({ getServerStats: async () => stats, })); +jest.mock('../../core/services/context_srv', () => ({ + contextSrv: { + hasAccess: () => true, + }, +})); describe('ServerStats', () => { it('Should render page with stats', async () => { diff --git a/public/app/features/admin/ServerStats.tsx b/public/app/features/admin/ServerStats.tsx index 5f4529ec912..d34077afc7e 100644 --- a/public/app/features/admin/ServerStats.tsx +++ b/public/app/features/admin/ServerStats.tsx @@ -9,17 +9,20 @@ import { Loader } from '../plugins/admin/components/Loader'; export const ServerStats = () => { const [stats, setStats] = useState(null); - const [isLoading, setIsLoading] = useState(true); + const [isLoading, setIsLoading] = useState(false); const styles = useStyles2(getStyles); useEffect(() => { - getServerStats().then((stats) => { - setStats(stats); - setIsLoading(false); - }); + if (contextSrv.hasAccess(AccessControlAction.ActionServerStatsRead, contextSrv.isGrafanaAdmin)) { + setIsLoading(true); + getServerStats().then((stats) => { + setStats(stats); + setIsLoading(false); + }); + } }, []); - if (!contextSrv.hasPermission(AccessControlAction.ActionServerStatsRead)) { + if (!contextSrv.hasAccess(AccessControlAction.ActionServerStatsRead, contextSrv.isGrafanaAdmin)) { return null; } diff --git a/public/app/routes/routes.tsx b/public/app/routes/routes.tsx index 2ccab1855e9..bc85a765316 100644 --- a/public/app/routes/routes.tsx +++ b/public/app/routes/routes.tsx @@ -139,10 +139,9 @@ export function getAppRoutes(): RouteDescriptor[] { path: '/explore', pageClass: 'page-explore', roles: () => - evaluatePermission( - () => (config.viewersCanEdit ? [] : ['Editor', 'Admin']), - AccessControlAction.DataSourcesExplore - ), + contextSrv.evaluatePermission(() => (config.viewersCanEdit ? [] : ['Editor', 'Admin']), [ + AccessControlAction.DataSourcesExplore, + ]), component: SafeDynamicImport(() => import(/* webpackChunkName: "explore" */ 'app/features/explore/Wrapper')), }, { @@ -526,16 +525,3 @@ export function getAppRoutes(): RouteDescriptor[] { // ...playlistRoutes, ]; } - -// evaluates access control permission, using fallback if access control is disabled -const evaluatePermission = (fallback: () => string[], action: AccessControlAction): string[] => { - if (!config.featureToggles['accesscontrol']) { - return fallback(); - } - if (contextSrv.hasPermission(action)) { - return []; - } else { - // Hack to reject when user does not have permission - return ['Reject']; - } -};