From 18f5f763a968037d9557468e4f828a1a0a8757e2 Mon Sep 17 00:00:00 2001 From: mikkancso Date: Fri, 6 Jan 2023 09:11:27 +0100 Subject: [PATCH] Connections: Align permissions for Connections page (#60725) * protect /connection url paths with permissions These permissions match the original ones at /datasources and /plugins * add Connections section to navtree only if user has permissions This commit works only when the easystart plugin is not present. I'll see what I can do when it is present in the next commit(s). * update datasources page permissions The datasources page have Explore buttons on datasource entries, therefore it makes sense to show this page for those, who can't edit or create datasources but have explore permissions. This applies for the traditional Editor role. * DataSourcesList: link to edit page only if has right to write If the user doesn't have rights to write datasources, then it's better to not create a link from cards to the edit page. This way they won't see the configuration of the data sources either, which is a desirable outcome. Also, I moved the query for DataSourcesExplore permission out from the DataSourcesListView component in the DataSourcesList component, next to the other permission queries - for the sake of consistency. * fix permissions for connect data This way it matches the permissions of the "Plugins" page. * fix applinks test --- pkg/api/api.go | 8 ++ pkg/services/datasources/accesscontrol.go | 15 ++-- .../navtree/navtreeimpl/applinks_test.go | 15 ++-- pkg/services/navtree/navtreeimpl/navtree.go | 83 +++++++++++-------- .../components/DataSourcesList.test.tsx | 17 +--- .../components/DataSourcesList.tsx | 20 ++++- .../pages/DataSourcesListPage.test.tsx | 51 ++++++++++-- 7 files changed, 136 insertions(+), 73 deletions(-) diff --git a/pkg/api/api.go b/pkg/api/api.go index 2036b617b2d..943a6156ebd 100644 --- a/pkg/api/api.go +++ b/pkg/api/api.go @@ -132,6 +132,14 @@ func (hs *HTTPServer) registerRoutes() { r.Get("/plugins/:id/", middleware.CanAdminPlugins(hs.Cfg), hs.Index) r.Get("/plugins/:id/edit", middleware.CanAdminPlugins(hs.Cfg), hs.Index) // deprecated r.Get("/plugins/:id/page/:page", middleware.CanAdminPlugins(hs.Cfg), hs.Index) + + r.Get("/connections/your-connections/datasources", authorize(reqOrgAdmin, datasources.ConfigurationPageAccess), hs.Index) + r.Get("/connections/your-connections/datasources/new", authorize(reqOrgAdmin, datasources.NewPageAccess), hs.Index) + r.Get("/connections/your-connections/datasources/edit/*", authorize(reqOrgAdmin, datasources.EditPageAccess), hs.Index) + r.Get("/connections/connect-data", middleware.CanAdminPlugins(hs.Cfg), hs.Index) + r.Get("/connections/connect-data/datasources/:id", middleware.CanAdminPlugins(hs.Cfg), hs.Index) + r.Get("/connections/connect-data/datasources/:id/page/:page", middleware.CanAdminPlugins(hs.Cfg), hs.Index) + // App Root Page appPluginIDScope := plugins.ScopeProvider.GetResourceScope(ac.Parameter(":id")) r.Get("/a/:id/*", authorize(reqSignedIn, ac.EvalPermission(plugins.ActionAppAccess, appPluginIDScope)), hs.Index) diff --git a/pkg/services/datasources/accesscontrol.go b/pkg/services/datasources/accesscontrol.go index 4b16ab1479c..aae4773e18a 100644 --- a/pkg/services/datasources/accesscontrol.go +++ b/pkg/services/datasources/accesscontrol.go @@ -24,12 +24,15 @@ var ( var ( // ConfigurationPageAccess is used to protect the "Configure > Data sources" tab access - ConfigurationPageAccess = accesscontrol.EvalAll( - accesscontrol.EvalPermission(ActionRead), - accesscontrol.EvalAny( - accesscontrol.EvalPermission(ActionCreate), - accesscontrol.EvalPermission(ActionDelete), - accesscontrol.EvalPermission(ActionWrite), + ConfigurationPageAccess = accesscontrol.EvalAny( + accesscontrol.EvalPermission(accesscontrol.ActionDatasourcesExplore), + accesscontrol.EvalAll( + accesscontrol.EvalPermission(ActionRead), + accesscontrol.EvalAny( + accesscontrol.EvalPermission(ActionCreate), + accesscontrol.EvalPermission(ActionDelete), + accesscontrol.EvalPermission(ActionWrite), + ), ), ) diff --git a/pkg/services/navtree/navtreeimpl/applinks_test.go b/pkg/services/navtree/navtreeimpl/applinks_test.go index f0ed89967ba..ba32c38c9b4 100644 --- a/pkg/services/navtree/navtreeimpl/applinks_test.go +++ b/pkg/services/navtree/navtreeimpl/applinks_test.go @@ -25,6 +25,7 @@ func TestAddAppLinks(t *testing.T) { reqCtx := &models.ReqContext{SignedInUser: &user.SignedInUser{}, Context: &web.Context{Req: httpReq}} permissions := []ac.Permission{ {Action: plugins.ActionAppAccess, Scope: "*"}, + {Action: plugins.ActionInstall, Scope: "*"}, } testApp1 := plugins.PluginDTO{ @@ -290,18 +291,20 @@ func TestAddAppLinks(t *testing.T) { treeRoot.AddSection(service.buildDataConnectionsNavLink(reqCtx)) connectionsNode := treeRoot.FindById("connections") require.Equal(t, "Connections", connectionsNode.Text) - require.Equal(t, "Connect data", connectionsNode.Children[1].Text) - require.Equal(t, "connections-connect-data", connectionsNode.Children[1].Id) // Original "Connect data" page - require.Equal(t, "", connectionsNode.Children[1].PluginID) + + connectDataNode := connectionsNode.Children[0] + require.Equal(t, "Connect data", connectDataNode.Text) + require.Equal(t, "connections-connect-data", connectDataNode.Id) // Original "Connect data" page + require.Equal(t, "", connectDataNode.PluginID) err := service.addAppLinks(&treeRoot, reqCtx) // Check if the standalone plugin page appears under the section where we registered it require.NoError(t, err) require.Equal(t, "Connections", connectionsNode.Text) - require.Equal(t, "Connect data", connectionsNode.Children[1].Text) - require.Equal(t, "standalone-plugin-page-/connections/connect-data", connectionsNode.Children[1].Id) // Overridden "Connect data" page - require.Equal(t, "test-app3", connectionsNode.Children[1].PluginID) + require.Equal(t, "Connect data", connectDataNode.Text) + require.Equal(t, "standalone-plugin-page-/connections/connect-data", connectDataNode.Id) // Overridden "Connect data" page + require.Equal(t, "test-app3", connectDataNode.PluginID) // Check if the standalone plugin page does not appear under the app section anymore // (Also checking if the Default Page got removed) diff --git a/pkg/services/navtree/navtreeimpl/navtree.go b/pkg/services/navtree/navtreeimpl/navtree.go index bef8730c2a7..73df6e82de1 100644 --- a/pkg/services/navtree/navtreeimpl/navtree.go +++ b/pkg/services/navtree/navtreeimpl/navtree.go @@ -12,6 +12,7 @@ import ( ac "github.com/grafana/grafana/pkg/services/accesscontrol" "github.com/grafana/grafana/pkg/services/apikey" "github.com/grafana/grafana/pkg/services/dashboards" + "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/navtree" "github.com/grafana/grafana/pkg/services/org" @@ -155,7 +156,9 @@ func (s *ServiceImpl) GetNavTree(c *models.ReqContext, hasEditPerm bool, prefs * } if s.features.IsEnabled(featuremgmt.FlagDataConnectionsConsole) { - treeRoot.AddSection(s.buildDataConnectionsNavLink(c)) + if connectionsSection := s.buildDataConnectionsNavLink(c); connectionsSection != nil { + treeRoot.AddSection(connectionsSection) + } } if s.features.IsEnabled(featuremgmt.FlagLivePipeline) { @@ -558,45 +561,55 @@ func (s *ServiceImpl) buildAlertNavLinks(c *models.ReqContext, hasEditPerm bool) } func (s *ServiceImpl) buildDataConnectionsNavLink(c *models.ReqContext) *navtree.NavLink { + hasAccess := ac.HasAccess(s.accessControl, c) + var children []*navtree.NavLink var navLink *navtree.NavLink baseUrl := s.cfg.AppSubURL + "/connections" - // Your connections - children = append(children, &navtree.NavLink{ - Id: "connections-your-connections", - Text: "Your connections", - SubTitle: "Manage your existing connections", - Url: baseUrl + "/your-connections", - // Datasources - Children: []*navtree.NavLink{{ - Id: "connections-your-connections-datasources", - Text: "Data sources", - SubTitle: "View and manage your connected data source connections", - Url: baseUrl + "/your-connections/datasources", - }}, - }) - - // Connect data - children = append(children, &navtree.NavLink{ - Id: "connections-connect-data", - Text: "Connect data", - SubTitle: "Browse and create new connections", - Url: s.cfg.AppSubURL + "/connections/connect-data", - Children: []*navtree.NavLink{}, - }) - - // Connections (main) - navLink = &navtree.NavLink{ - Text: "Connections", - Icon: "adjust-circle", - Id: "connections", - Url: baseUrl, - Children: children, - Section: navtree.NavSectionCore, - SortWeight: navtree.WeightDataConnections, + if hasAccess(ac.ReqOrgAdmin, datasources.ConfigurationPageAccess) { + // Your connections + children = append(children, &navtree.NavLink{ + Id: "connections-your-connections", + Text: "Your connections", + SubTitle: "Manage your existing connections", + Url: baseUrl + "/your-connections", + // Datasources + Children: []*navtree.NavLink{{ + Id: "connections-your-connections-datasources", + Text: "Data sources", + SubTitle: "View and manage your connected data source connections", + Url: baseUrl + "/your-connections/datasources", + }}, + }) } - return navLink + // Connect data + // FIXME: while we don't have a permissions for listing plugins the legacy check has to stay as a default + if plugins.ReqCanAdminPlugins(s.cfg)(c) || hasAccess(plugins.ReqCanAdminPlugins(s.cfg), plugins.AdminAccessEvaluator) { + children = append(children, &navtree.NavLink{ + Id: "connections-connect-data", + Text: "Connect data", + SubTitle: "Browse and create new connections", + Url: s.cfg.AppSubURL + "/connections/connect-data", + Children: []*navtree.NavLink{}, + }) + } + + if len(children) > 0 { + // Connections (main) + navLink = &navtree.NavLink{ + Text: "Connections", + Icon: "adjust-circle", + Id: "connections", + Url: baseUrl, + Children: children, + Section: navtree.NavSectionCore, + SortWeight: navtree.WeightDataConnections, + } + + return navLink + } + return nil } diff --git a/public/app/features/datasources/components/DataSourcesList.test.tsx b/public/app/features/datasources/components/DataSourcesList.test.tsx index bb6cf117858..36db8204a1d 100644 --- a/public/app/features/datasources/components/DataSourcesList.test.tsx +++ b/public/app/features/datasources/components/DataSourcesList.test.tsx @@ -2,15 +2,12 @@ import { render, screen } from '@testing-library/react'; import React from 'react'; import { Provider } from 'react-redux'; -import { contextSrv } from 'app/core/services/context_srv'; import { configureStore } from 'app/store/configureStore'; import { getMockDataSources } from '../__mocks__'; import { DataSourcesListView } from './DataSourcesList'; -jest.mock('app/core/services/context_srv'); - const setup = () => { const store = configureStore(); @@ -21,16 +18,14 @@ const setup = () => { dataSourcesCount={3} isLoading={false} hasCreateRights={true} + hasWriteRights={true} + hasExploreRights={true} /> ); }; describe('', () => { - beforeEach(() => { - (contextSrv.hasPermission as jest.Mock) = jest.fn().mockReturnValue(true); - }); - it('should render action bar', async () => { setup(); @@ -53,12 +48,4 @@ describe('', () => { expect(await screen.findByRole('heading', { name: 'dataSource-0' })).toBeInTheDocument(); expect(await screen.findByRole('link', { name: 'dataSource-0' })).toBeInTheDocument(); }); - - it('should not render Explore button if user has no permissions', async () => { - (contextSrv.hasPermission as jest.Mock) = jest.fn().mockReturnValue(false); - setup(); - - expect(await screen.findAllByRole('link', { name: 'Build a Dashboard' })).toHaveLength(3); - expect(screen.queryAllByRole('link', { name: 'Explore' })).toHaveLength(0); - }); }); diff --git a/public/app/features/datasources/components/DataSourcesList.tsx b/public/app/features/datasources/components/DataSourcesList.tsx index cc41d384036..281d2e7dd67 100644 --- a/public/app/features/datasources/components/DataSourcesList.tsx +++ b/public/app/features/datasources/components/DataSourcesList.tsx @@ -21,6 +21,8 @@ export function DataSourcesList() { const dataSourcesCount = useSelector(({ dataSources }: StoreState) => getDataSourcesCount(dataSources)); const hasFetched = useSelector(({ dataSources }: StoreState) => dataSources.hasFetched); const hasCreateRights = contextSrv.hasPermission(AccessControlAction.DataSourcesCreate); + const hasWriteRights = contextSrv.hasPermission(AccessControlAction.DataSourcesWrite); + const hasExploreRights = contextSrv.hasPermission(AccessControlAction.DataSourcesExplore); return ( ); } @@ -37,12 +41,20 @@ export type ViewProps = { dataSourcesCount: number; isLoading: boolean; hasCreateRights: boolean; + hasWriteRights: boolean; + hasExploreRights: boolean; }; -export function DataSourcesListView({ dataSources, dataSourcesCount, isLoading, hasCreateRights }: ViewProps) { +export function DataSourcesListView({ + dataSources, + dataSourcesCount, + isLoading, + hasCreateRights, + hasWriteRights, + hasExploreRights, +}: ViewProps) { const styles = useStyles2(getStyles); const dataSourcesRoutes = useDataSourcesRoutes(); - const canExploreDataSources = contextSrv.hasPermission(AccessControlAction.DataSourcesExplore); if (isLoading) { return ; @@ -75,7 +87,7 @@ export function DataSourcesListView({ dataSources, dataSourcesCount, isLoading, const dsLink = config.appSubUrl + dataSourcesRoutes.Edit.replace(/:uid/gi, dataSource.uid); return (
  • - + {dataSource.name} @@ -91,7 +103,7 @@ export function DataSourcesListView({ dataSources, dataSourcesCount, isLoading, Build a Dashboard - {canExploreDataSources && ( + {hasExploreRights && ( { expect(await screen.findByRole('link', { name: 'Add data source' })).toBeInTheDocument(); }); - it('should disable the "Add data source" button if user has no permissions', async () => { - (contextSrv.hasPermission as jest.Mock) = jest.fn().mockReturnValue(false); + describe('when user has no permissions', () => { + beforeEach(() => { + (contextSrv.hasPermission as jest.Mock) = jest.fn().mockReturnValue(false); + }); + + it('should disable the "Add data source" button if user has no permissions', async () => { + setup({ isSortAscending: true }); + + expect(await screen.findByRole('heading', { name: 'Configuration' })).toBeInTheDocument(); + expect(await screen.findByRole('link', { name: 'Documentation' })).toBeInTheDocument(); + expect(await screen.findByRole('link', { name: 'Support' })).toBeInTheDocument(); + expect(await screen.findByRole('link', { name: 'Community' })).toBeInTheDocument(); + expect(await screen.findByRole('link', { name: 'Add data source' })).toHaveStyle('pointer-events: none'); + }); + + it('should not show the Explore button', async () => { + getDataSourcesMock.mockResolvedValue(getMockDataSources(3)); + setup({ isSortAscending: true }); + + expect(await screen.findAllByRole('link', { name: 'Build a Dashboard' })).toHaveLength(3); + expect(screen.queryAllByRole('link', { name: 'Explore' })).toHaveLength(0); + }); + + it('should not link cards to edit pages', async () => { + getDataSourcesMock.mockResolvedValue(getMockDataSources(1)); + setup({ isSortAscending: true }); + + expect(await screen.findByRole('heading', { name: 'dataSource-0' })).toBeInTheDocument(); + expect(await screen.queryByRole('link', { name: 'dataSource-0' })).toBeNull(); + }); + }); + + it('should show the Explore button', async () => { + getDataSourcesMock.mockResolvedValue(getMockDataSources(3)); setup({ isSortAscending: true }); - expect(await screen.findByRole('heading', { name: 'Configuration' })).toBeInTheDocument(); - expect(await screen.findByRole('link', { name: 'Documentation' })).toBeInTheDocument(); - expect(await screen.findByRole('link', { name: 'Support' })).toBeInTheDocument(); - expect(await screen.findByRole('link', { name: 'Community' })).toBeInTheDocument(); - expect(await screen.findByRole('link', { name: 'Add data source' })).toHaveStyle('pointer-events: none'); + expect(await screen.findAllByRole('link', { name: 'Build a Dashboard' })).toHaveLength(3); + expect(screen.queryAllByRole('link', { name: 'Explore' })).toHaveLength(3); + }); + + it('should link cards to edit pages', async () => { + getDataSourcesMock.mockResolvedValue(getMockDataSources(1)); + setup({ isSortAscending: true }); + + expect(await screen.findByRole('heading', { name: 'dataSource-0' })).toBeInTheDocument(); + expect(await screen.findByRole('link', { name: 'dataSource-0' })).toBeInTheDocument(); }); it('should render action bar and datasources', async () => {