gkneighb commented on code in PR #41843:
URL: https://github.com/apache/superset/pull/41843#discussion_r3565525042
##########
superset/security/manager.py:
##########
@@ -1640,17 +1683,20 @@ def get_table_access_error_object(self, tables:
set["Table"]) -> SupersetError:
},
)
- def get_table_access_link( # pylint: disable=unused-argument
- self, tables: set["Table"]
- ) -> Optional[str]:
+ def get_table_access_link(self, tables: set["Table"]) -> Optional[str]:
"""
Return the access link for the denied SQL tables.
+ The configured ``PERMISSION_INSTRUCTIONS_LINK`` may template the denied
+ table names (and the current username) into the access URL.
+
:param tables: The set of denied SQL tables
:returns: The access URL
"""
- return get_conf().get("PERMISSION_INSTRUCTIONS_LINK")
+ return _render_permission_instructions_link(
+ table_names=",".join(str(table) for table in tables),
+ )
Review Comment:
Confirmed and fixed in 1eea2ae002 — Table.__str__ does URL-encode each part,
so the renderer's second encoding double-encoded. table_names is now built from
the raw catalog/schema/table parts (single encoding), with a test asserting %20
not %2520.
##########
superset/security/manager.py:
##########
@@ -1818,17 +1860,20 @@ def get_table_access_error_object(self, tables:
set["Table"]) -> SupersetError:
},
)
- def get_table_access_link( # pylint: disable=unused-argument
- self, tables: set["Table"]
- ) -> Optional[str]:
+ def get_table_access_link(self, tables: set["Table"]) -> Optional[str]:
"""
Return the access link for the denied SQL tables.
+ The configured ``PERMISSION_INSTRUCTIONS_LINK`` may template the denied
+ table names (and the current username) into the access URL.
+
:param tables: The set of denied SQL tables
:returns: The access URL
"""
- return get_conf().get("PERMISSION_INSTRUCTIONS_LINK")
+ return _render_permission_instructions_link(
+ table_names=",".join(str(table) for table in tables),
Review Comment:
Fixed in 1eea2ae002 — table names are sorted before joining, so links are
deterministic across runs.
##########
superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx:
##########
@@ -0,0 +1,156 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ReactNode } from 'react';
+import { t, tn } from '@apache-superset/core/translation';
+import { Typography } from '@superset-ui/core/components';
+
+import type { ErrorMessageComponentProps } from './types';
+import { IssueCode } from './IssueCode';
+import { ErrorAlert } from './ErrorAlert';
+
+interface DatasourceSecurityAccessExtra {
+ owners?: string[];
+ link?: string;
+ datasource?: number | string;
+ datasource_name?: string;
+ tables?: string[];
+ issue_codes?: {
+ code: number;
+ message: string;
+ }[];
+}
+
+/**
+ * Shown when a viewer opens a chart but lacks permission to its underlying
data
+ * (DATASOURCE_SECURITY_ACCESS_ERROR / TABLE_SECURITY_ACCESS_ERROR). Surfaces a
+ * plain-language explanation, who to contact, and a "Request access" link when
+ * the deployment configures PERMISSION_INSTRUCTIONS_LINK.
+ */
+export function DatasourceSecurityAccessErrorMessage({
+ error,
+ source,
+ closable,
+}: ErrorMessageComponentProps<DatasourceSecurityAccessExtra | null>) {
+ const { extra, level, message } = error;
+ const isVisualization = ['dashboard', 'explore'].includes(source || '');
+
+ // DATASOURCE_SECURITY_ACCESS_ERROR is also raised for non-access failures
+ // (e.g. virtual-dataset SQL validation: "Only SELECT statements are
+ // allowed"). Those errors carry no access payload — render them plainly
+ // rather than misleading the user with request-access guidance.
+ const isAccessDenial = !!extra?.datasource_name || !!extra?.tables?.length;
+ if (!isAccessDenial) {
+ return (
+ <ErrorAlert
+ errorType={t('Unexpected error')}
+ message={message}
+ type={level}
+ closable={closable}
+ />
+ );
Review Comment:
Fixed in 1eea2ae002 — compact is now accepted and forwarded to ErrorAlert on
both render paths.
##########
superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx:
##########
@@ -0,0 +1,156 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ReactNode } from 'react';
+import { t, tn } from '@apache-superset/core/translation';
+import { Typography } from '@superset-ui/core/components';
+
+import type { ErrorMessageComponentProps } from './types';
+import { IssueCode } from './IssueCode';
+import { ErrorAlert } from './ErrorAlert';
+
+interface DatasourceSecurityAccessExtra {
+ owners?: string[];
+ link?: string;
+ datasource?: number | string;
+ datasource_name?: string;
+ tables?: string[];
+ issue_codes?: {
+ code: number;
+ message: string;
+ }[];
+}
+
+/**
+ * Shown when a viewer opens a chart but lacks permission to its underlying
data
+ * (DATASOURCE_SECURITY_ACCESS_ERROR / TABLE_SECURITY_ACCESS_ERROR). Surfaces a
+ * plain-language explanation, who to contact, and a "Request access" link when
+ * the deployment configures PERMISSION_INSTRUCTIONS_LINK.
+ */
+export function DatasourceSecurityAccessErrorMessage({
+ error,
+ source,
+ closable,
+}: ErrorMessageComponentProps<DatasourceSecurityAccessExtra | null>) {
+ const { extra, level, message } = error;
+ const isVisualization = ['dashboard', 'explore'].includes(source || '');
+
+ // DATASOURCE_SECURITY_ACCESS_ERROR is also raised for non-access failures
+ // (e.g. virtual-dataset SQL validation: "Only SELECT statements are
+ // allowed"). Those errors carry no access payload — render them plainly
+ // rather than misleading the user with request-access guidance.
+ const isAccessDenial = !!extra?.datasource_name || !!extra?.tables?.length;
+ if (!isAccessDenial) {
+ return (
+ <ErrorAlert
+ errorType={t('Unexpected error')}
+ message={message}
+ type={level}
+ closable={closable}
+ />
+ );
+ }
+
+ let explanation: string;
+ if (extra?.datasource_name) {
+ explanation = isVisualization
+ ? t(
+ 'This chart uses the "%s" dataset, which you do not have ' +
+ 'permission to view.',
+ extra.datasource_name,
+ )
+ : t(
+ 'This query uses the "%s" dataset, which you do not have ' +
+ 'permission to view.',
+ extra.datasource_name,
+ );
+ } else {
+ explanation = isVisualization
+ ? t(
+ 'You do not have access to the data behind this chart ' +
+ '(tables: %s).',
+ extra?.tables?.join(', '),
+ )
+ : t(
+ 'You do not have access to the following tables: %s.', // sqllab
+ extra?.tables?.join(', '),
+ );
+ }
+
+ const owners = extra?.owners;
+ const ownerLine =
+ isVisualization && owners && owners.length > 0
+ ? tn(
+ 'To request access, reach out to the chart owner: %s.',
+ 'To request access, reach out to the chart owners: %s.',
+ owners.length,
+ owners.join(', '),
+ )
+ : t('To request access, contact your Superset administrator.');
Review Comment:
Confirmed and fixed in 1eea2ae002 — nothing populated extra.owners, so the
owner-contact line was dead. get_datasource_access_error_object now includes
sorted owner display names from the datasource, with a test.
##########
superset-frontend/src/components/ErrorMessage/DatasourceSecurityAccessErrorMessage.tsx:
##########
@@ -0,0 +1,156 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { ReactNode } from 'react';
+import { t, tn } from '@apache-superset/core/translation';
+import { Typography } from '@superset-ui/core/components';
+
+import type { ErrorMessageComponentProps } from './types';
+import { IssueCode } from './IssueCode';
+import { ErrorAlert } from './ErrorAlert';
+
+interface DatasourceSecurityAccessExtra {
+ owners?: string[];
+ link?: string;
+ datasource?: number | string;
+ datasource_name?: string;
+ tables?: string[];
+ issue_codes?: {
+ code: number;
+ message: string;
+ }[];
+}
+
+/**
+ * Shown when a viewer opens a chart but lacks permission to its underlying
data
+ * (DATASOURCE_SECURITY_ACCESS_ERROR / TABLE_SECURITY_ACCESS_ERROR). Surfaces a
+ * plain-language explanation, who to contact, and a "Request access" link when
+ * the deployment configures PERMISSION_INSTRUCTIONS_LINK.
+ */
+export function DatasourceSecurityAccessErrorMessage({
+ error,
+ source,
+ closable,
+}: ErrorMessageComponentProps<DatasourceSecurityAccessExtra | null>) {
+ const { extra, level, message } = error;
+ const isVisualization = ['dashboard', 'explore'].includes(source || '');
+
+ // DATASOURCE_SECURITY_ACCESS_ERROR is also raised for non-access failures
+ // (e.g. virtual-dataset SQL validation: "Only SELECT statements are
+ // allowed"). Those errors carry no access payload — render them plainly
+ // rather than misleading the user with request-access guidance.
+ const isAccessDenial = !!extra?.datasource_name || !!extra?.tables?.length;
+ if (!isAccessDenial) {
+ return (
+ <ErrorAlert
+ errorType={t('Unexpected error')}
+ message={message}
+ type={level}
+ closable={closable}
+ />
+ );
+ }
Review Comment:
Declining: the extra payload IS the discriminator. Genuine access denials
from the security manager always attach extra (datasource/datasource_name or
tables — see get_datasource_access_error_object /
get_table_access_error_object); the payload-less cases are precisely the
reused-error-type misuses (SQL validation) that must NOT get request-access
framing. If a payload-less genuine denial existed, the component degrades
gracefully to the plain error message rather than misleading guidance.
##########
tests/unit_tests/security/test_permission_instructions_link.py:
##########
@@ -0,0 +1,140 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Unit tests for templated PERMISSION_INSTRUCTIONS_LINK rendering."""
+
+from unittest.mock import MagicMock, patch
+
+from superset.security.manager import (
+ _render_permission_instructions_link,
+ SupersetSecurityManager,
+)
+from superset.sql.parse import Table
+
+MANAGER = "superset.security.manager"
Review Comment:
Declining, standing rationale: module-level constants and locals with
inferable types are unannotated across this codebase; mypy passes. Identical
future findings will be declined the same way.
##########
superset/security/manager.py:
##########
@@ -123,6 +124,42 @@ def get_conf() -> Any:
return current_app.config
+def _render_permission_instructions_link(
+ *,
+ datasource_id: str = "",
+ datasource_name: str = "",
+ table_names: str = "",
+) -> Optional[str]:
+ """Render the configured ``PERMISSION_INSTRUCTIONS_LINK``.
+
+ The configured URL may contain ``{datasource_id}``, ``{datasource_name}``,
+ ``{table_names}`` and ``{username}`` placeholders, which are substituted
with
+ URL-encoded values so the link can deep-link into an organization's access
+ request system. A URL with no placeholders is returned unchanged, and an
+ empty/unset config returns ``None`` (no link). Unsupplied placeholders are
+ replaced with an empty string.
+ """
+ link = get_conf().get("PERMISSION_INSTRUCTIONS_LINK")
Review Comment:
Declining, standing rationale: module-level constants and locals with
inferable types are unannotated across this codebase; mypy passes. Identical
future findings will be declined the same way.
##########
superset/security/manager.py:
##########
@@ -123,6 +124,42 @@ def get_conf() -> Any:
return current_app.config
+def _render_permission_instructions_link(
+ *,
+ datasource_id: str = "",
+ datasource_name: str = "",
+ table_names: str = "",
+) -> Optional[str]:
+ """Render the configured ``PERMISSION_INSTRUCTIONS_LINK``.
+
+ The configured URL may contain ``{datasource_id}``, ``{datasource_name}``,
+ ``{table_names}`` and ``{username}`` placeholders, which are substituted
with
+ URL-encoded values so the link can deep-link into an organization's access
+ request system. A URL with no placeholders is returned unchanged, and an
+ empty/unset config returns ``None`` (no link). Unsupplied placeholders are
+ replaced with an empty string.
+ """
+ link = get_conf().get("PERMISSION_INSTRUCTIONS_LINK")
+ if not link:
+ return None
+
+ username = ""
Review Comment:
Declining, standing rationale: module-level constants and locals with
inferable types are unannotated across this codebase; mypy passes. Identical
future findings will be declined the same way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
