Skip to content

Commit

Permalink
Consistent get_<DATA_ASSET> permissions - Dashboards (#1729)
Browse files Browse the repository at this point in the history
- Feature
- Bugfix

This is a continuation of
#1727 but for Dashboards.

- removes GET_DASHBOARD permission check on get_dashboard
- adds a restriction field in the Dashboard type with the restricted
information
- implement resolver to return the restricted information that checks
GET_DASHBOARD permissions. Return defaults for users with no permissions
- Adapt frontend to use the restricted fields

In addition i made some fixes in the frontend, for example, the default
view I changed it to Overview because it improves the experience of a
data consumer that is exploring the dashboard metadata. I also removed
the forever circular progress in the viewer page if there are errors in
the loading of the reader url.

- #1727

Please answer the questions below briefly where applicable, or write
`N/A`. Based on
[OWASP 10](https://owasp.org/Top10/en/).

- Does this PR introduce or modify any input fields or queries - this
includes
fetching data from storage outside the application (e.g. a database, an
S3 bucket)?
  - Is the input sanitized?
- What precautions are you taking before deserializing the data you
consume?
  - Is injection prevented by parametrizing queries?
  - Have you ensured no `eval` or similar functions are used?
- Does this PR introduce any functionality or component that requires
authorization?
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
  - Are you logging failed auth attempts?
- Are you using or adding any cryptographic features?
  - Do you use a standard proven implementations?
  - Are the used keys controlled by the customer? Where are they stored?
- Are you introducing any new policies/roles/users?
  - Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.
  • Loading branch information
dlpzx committed Dec 9, 2024
1 parent ff9a8bd commit 283fadc
Show file tree
Hide file tree
Showing 10 changed files with 42 additions and 13 deletions.
6 changes: 6 additions & 0 deletions backend/dataall/modules/dashboards/api/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ def get_dashboard(context: Context, source, dashboardUri: str = None):
return DashboardService.get_dashboard(uri=dashboardUri)


def get_dashboard_restricted_information(context: Context, source: Dashboard):
if not source:
return None
return DashboardService.get_dashboard_restricted_information(uri=source.dashboardUri, dashboard=source)


def resolve_user_role(context: Context, source: Dashboard):
if context.username and source.owner == context.username:
return DashboardRole.Creator.value
Expand Down
13 changes: 11 additions & 2 deletions backend/dataall/modules/dashboards/api/types.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,19 @@
from dataall.base.api import gql
from dataall.modules.dashboards.api.resolvers import (
DashboardRole,
get_dashboard_organization,
get_dashboard_restricted_information,
resolve_glossary_terms,
resolve_upvotes,
resolve_user_role,
)

from dataall.core.environment.api.resolvers import resolve_environment

DashboardRestrictedInformation = gql.ObjectType(
name='DashboardRestrictedInformation',
fields=[gql.Field('AwsAccountId', type=gql.String), gql.Field('region', type=gql.String)],
)

Dashboard = gql.ObjectType(
name='Dashboard',
fields=[
Expand All @@ -19,10 +24,14 @@
gql.Field('DashboardId', type=gql.String),
gql.Field('tags', type=gql.ArrayType(gql.String)),
gql.Field('created', type=gql.String),
gql.Field('AwsAccountId', type=gql.String),
gql.Field('updated', type=gql.String),
gql.Field('owner', type=gql.String),
gql.Field('SamlGroupName', type=gql.String),
gql.Field(
'restricted',
type=DashboardRestrictedInformation,
resolver=get_dashboard_restricted_information,
),
gql.Field(
'environment',
type=gql.Ref('EnvironmentSimplified'),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,15 @@ class DashboardService:
"""Service that serves request related to dashboard"""

@staticmethod
@ResourcePolicyService.has_resource_permission(GET_DASHBOARD)
def get_dashboard(uri: str) -> Dashboard:
with get_context().db_engine.scoped_session() as session:
return DashboardRepository.get_dashboard_by_uri(session, uri)

@staticmethod
@ResourcePolicyService.has_resource_permission(GET_DASHBOARD)
def get_dashboard_restricted_information(uri: str, dashboard: Dashboard):
return dashboard

@staticmethod
@TenantPolicyService.has_tenant_permission(MANAGE_DASHBOARDS)
@ResourcePolicyService.has_resource_permission(CREATE_DASHBOARD)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ export const DashboardListItem = (props) => {
</Grid>
<Grid item md={8} xs={6}>
<Typography color="textPrimary" variant="body2">
{dashboard.AwsAccountId}
{dashboard.restricted.AwsAccountId}
</Typography>
</Grid>
</Grid>
Expand All @@ -178,7 +178,7 @@ export const DashboardListItem = (props) => {
</Grid>
<Grid item md={8} xs={12}>
<Typography color="textPrimary" variant="body2">
{dashboard.environment.region}
{dashboard.restricted.region}
</Typography>
</Grid>
</Grid>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ export const DashboardOverview = (props) => {
<Grid item lg={4} xl={3} xs={12}>
<ObjectMetadata
environment={dashboard.environment}
region={dashboard.environment.region}
region={dashboard.restricted?.region}
organization={dashboard.environment.organization}
owner={dashboard.owner}
admins={dashboard.SamlGroupName || '-'}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ export const DashboardViewer = ({ dashboard }) => {
const client = useClient();
const [dashboardRef] = useState(createRef());
const [sessionUrl, setSessionUrl] = useState(null);
const [loading, setLoading] = useState(false);

const fetchReaderSessionUrl = useCallback(async () => {
setLoading(true);
const response = await client.query(
getReaderSession(dashboard.dashboardUri)
);
Expand All @@ -40,6 +42,7 @@ export const DashboardViewer = ({ dashboard }) => {
} else {
dispatch({ type: SET_ERROR, error: response.errors[0].message });
}
setLoading(false);
}, [client, dispatch, dashboard, dashboardRef]);

useEffect(() => {
Expand All @@ -50,7 +53,10 @@ export const DashboardViewer = ({ dashboard }) => {
}
}, [client, dispatch, fetchReaderSessionUrl, sessionUrl]);

if (!sessionUrl) {
if (!sessionUrl && !loading) {
return null;
}
if (loading) {
return <CircularProgress />;
}
return (
Expand Down
5 changes: 4 additions & 1 deletion frontend/src/modules/Dashboards/services/getDashboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ export const getDashboard = (dashboardUri) => ({
userRoleForDashboard
DashboardId
upvotes
restricted {
region
AwsAccountId
}
environment {
environmentUri
label
region
organization {
organizationUri
label
Expand Down
6 changes: 4 additions & 2 deletions frontend/src/modules/Dashboards/services/searchDashboards.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@ export const searchDashboards = (filter) => ({
name
owner
SamlGroupName
AwsAccountId
restricted {
region
AwsAccountId
}
description
label
created
Expand All @@ -27,7 +30,6 @@ export const searchDashboards = (filter) => ({
environment {
environmentUri
label
region
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions frontend/src/modules/Dashboards/views/DashboardView.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ const DashboardView = () => {
const client = useClient();
const { enqueueSnackbar } = useSnackbar();
const navigate = useNavigate();
const [currentTab, setCurrentTab] = useState('viewer');
const [currentTab, setCurrentTab] = useState('overview');
const [loading, setLoading] = useState(true);
const [isUpVoted, setIsUpVoted] = useState(false);
const [upVotes, setUpvotes] = useState(null);
Expand Down Expand Up @@ -119,7 +119,7 @@ const DashboardView = () => {
const fetchItem = useCallback(async () => {
setLoading(true);
const response = await client.query(getDashboard(params.uri));
if (!response.errors) {
if (response.data.getDashboard !== null) {
setDashboard(response.data.getDashboard);
setUpvotes(response.data.getDashboard.upvotes);
setIsAdmin(
Expand Down
1 change: 0 additions & 1 deletion tests/modules/dashboards/test_dashboards.py
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ def test_get_dashboard(client, env_fixture, db, dashboard, group):
environment {
environmentUri
label
region
organization {
organizationUri
label
Expand Down

0 comments on commit 283fadc

Please sign in to comment.