You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by am...@apache.org on 2026/10/20 15:23:05 UTC
(superset) 07/08: feat(extensions): let a page declare the permission it requires
This is an automated email from the ASF dual-hosted git repository.
amaannawab923 pushed a commit to branch feat/extension-pages
in repository https://gitbox.apache.org/repos/asf/superset.git
commit cd2e39deb44750e7594b27781baa1fa6fe530594
Author: amaannawab923 <am...@gmail.com>
AuthorDate: Tue Oct 20 20:34:30 2026 +0530
feat(extensions): let a page declare the permission it requires
Any signed-in user could open any contributed page. The page had to guard
itself, which meant every extension reimplemented authorisation, and a
page that forgot was simply open. That mattered more once pages moved to
paths of their own: /approvals reads as part of Superset, so being open by
default is a worse default than it was behind a namespace.
A page may now name a permission in its manifest. The host creates it
against the extension when the extension loads -- not during `superset
init`, because an extension is installed long after a deployment is
initialised and an administrator cannot grant a permission that does not
exist yet. It reads as "can review on preset.asset-approval" in the Roles
screen, so an extension's permissions group together and two extensions
naming the same permission stay separate.
Omitting the field keeps the page open to any signed-in user, which is
what every page did before. Adding this cannot lock anyone out of a page
that already worked.
A user without the permission gets 403 rather than FAB's has_access
redirect to the login page: they are already signed in, so sending them to
log in again says nothing true. Login still comes first, so an anonymous
visitor is redirected rather than refused -- 403 would disclose that the
page exists to someone who has not identified themselves.
The frontend is told which pages the user may open rather than having them
filtered out, because an extension's bundle calls registerPage for every
page it ships and dropping one would turn a missing permission into an
error in the user's console. The navigation entry is hidden and the page
renders a refusal; the server refuses the path independently.
Permission creation is idempotent and its failure is logged, not fatal:
every worker runs it at boot, so two starting together can race between
the lookup and the insert, and the loser would otherwise take down a
worker over a row the winner has already written.
---
.../src/superset_core/extensions/constants.py | 7 +
.../src/superset_core/extensions/types.py | 11 +
.../packages/superset-core/src/pages/index.ts | 21 ++
superset-frontend/src/core/pages/index.test.ts | 55 ++++-
superset-frontend/src/core/pages/index.ts | 25 ++-
.../src/extensions/ExtensionsLoader.ts | 8 +-
superset-frontend/src/features/home/Menu.tsx | 13 +-
.../src/pages/ExtensionPage/index.tsx | 18 ++
superset/extensions/utils.py | 12 +-
superset/extensions/view.py | 158 ++++++++++++--
superset/initialization/__init__.py | 6 +-
tests/unit_tests/extensions/test_pages_view.py | 237 ++++++++++++++++++++-
12 files changed, 521 insertions(+), 50 deletions(-)
diff --git a/superset-core/src/superset_core/extensions/constants.py b/superset-core/src/superset_core/extensions/constants.py
index dd6a3e84b5b..a0bc627e448 100644
--- a/superset-core/src/superset_core/extensions/constants.py
+++ b/superset-core/src/superset_core/extensions/constants.py
@@ -41,3 +41,10 @@ VERSION_PATTERN = r"^\d+\.\d+\.\d+$"
# into the Flask URL map, so anything a converter could interpret (``<``,
# ``>``, ``:``) must never reach it.
PAGE_PATH_PATTERN = r"^(/[a-z]([a-z0-9]*(-[a-z0-9]+)*)?)+$"
+
+# Permission name validation pattern: lowercase letters, digits and
+# underscores, starting with a letter. Deliberately permissive about the
+# `can_` prefix that Superset's own permissions use -- an extension may want
+# `approve_assets` -- but narrow enough that a declared name cannot be
+# confused for a different permission once it is rendered in the Roles screen.
+PERMISSION_NAME_PATTERN = r"^[a-z][a-z0-9_]*$"
diff --git a/superset-core/src/superset_core/extensions/types.py b/superset-core/src/superset_core/extensions/types.py
index 87333d8417e..d7195c56674 100644
--- a/superset-core/src/superset_core/extensions/types.py
+++ b/superset-core/src/superset_core/extensions/types.py
@@ -30,6 +30,7 @@ from pydantic import BaseModel, computed_field, Field # noqa: I001
from superset_core.extensions.constants import (
DISPLAY_NAME_PATTERN,
PAGE_PATH_PATTERN,
+ PERMISSION_NAME_PATTERN,
PUBLISHER_PATTERN,
TECHNICAL_NAME_PATTERN,
VERSION_PATTERN,
@@ -63,6 +64,16 @@ class Page(BaseModel):
default=None,
description="Navigation entry, when the page should appear in the menu",
)
+ permission: str | None = Field(
+ default=None,
+ description=(
+ "Permission a user must hold to open the page, granted on the "
+ "extension in the Roles screen. Omit to require only that the "
+ "user is signed in."
+ ),
+ pattern=PERMISSION_NAME_PATTERN,
+ max_length=100,
+ )
class Contributions(BaseModel):
diff --git a/superset-frontend/packages/superset-core/src/pages/index.ts b/superset-frontend/packages/superset-core/src/pages/index.ts
index fea9dddeb09..f397e1b79ba 100644
--- a/superset-frontend/packages/superset-core/src/pages/index.ts
+++ b/superset-frontend/packages/superset-core/src/pages/index.ts
@@ -80,6 +80,18 @@ export interface Page {
/** Defaults to the page's `name`. */
label?: string;
};
+ /**
+ * Permission a user must hold to open the page.
+ *
+ * Declared in the manifest; the host creates it against the extension when
+ * the extension loads, so an administrator can grant it in the Roles
+ * screen as, for example, `can_review on acme.tools`. The server refuses
+ * the page with 403 to anyone without it.
+ *
+ * Omit to require only that the user is signed in, which is how every page
+ * behaves without this field.
+ */
+ permission?: string;
}
/**
@@ -88,6 +100,15 @@ export interface Page {
export interface ResolvedPage extends Page {
/** The extension that contributed the page, as `publisher.name`. */
extensionId: string;
+ /**
+ * Whether the signed-in user may open this page.
+ *
+ * Always true for a page that declares no `permission`. The host hides a
+ * page the user may not open from the navigation bar and renders a refusal
+ * in its place, and the server refuses it independently -- this is what
+ * the host knows, not what enforces it.
+ */
+ permitted: boolean;
}
/**
diff --git a/superset-frontend/src/core/pages/index.test.ts b/superset-frontend/src/core/pages/index.test.ts
index 384f6a1a5cf..588c4b862d8 100644
--- a/superset-frontend/src/core/pages/index.test.ts
+++ b/superset-frontend/src/core/pages/index.test.ts
@@ -30,12 +30,16 @@ const disposables: Array<{ dispose: () => void }> = [];
const Component = () => React.createElement('div', null, 'page');
/** The manifest declaration the loader would have bound for these tests. */
-const DECLARED = ['/review-queue', '/review-queue-archive', '/a/b'];
+const DECLARED = [
+ { path: '/review-queue' },
+ { path: '/review-queue-archive' },
+ { path: '/a/b' },
+];
const register = (
extensionId: string,
page: { path: string; name: string },
- declared: readonly string[] = DECLARED,
+ declared: readonly { path: string; permitted?: boolean }[] = DECLARED,
) => registerPageFor(extensionId, declared, page, Component);
afterEach(() => {
@@ -49,7 +53,12 @@ test('a declared page registers at the path it declared', () => {
);
expect(getPages()).toEqual([
- { path: '/review-queue', name: 'Queue', extensionId: 'acme.tools' },
+ {
+ path: '/review-queue',
+ name: 'Queue',
+ extensionId: 'acme.tools',
+ permitted: true,
+ },
]);
});
@@ -65,7 +74,9 @@ test('a path the manifest does not declare is refused', () => {
test('the refusal names what was declared, so the fix is obvious', () => {
expect(() =>
- register('acme.tools', { path: '/welcome', name: 'X' }, ['/review-queue']),
+ register('acme.tools', { path: '/welcome', name: 'X' }, [
+ { path: '/review-queue' },
+ ]),
).toThrow(/Declared: \/review-queue/);
});
@@ -87,7 +98,7 @@ test.each([
// Checked before the manifest list, so a malformed path is reported as
// malformed even when it somehow appears in the manifest.
expect(() =>
- register('acme.tools', { path, name: 'X' }, [path]),
+ register('acme.tools', { path, name: 'X' }, [{ path }]),
).toThrow(/must be an absolute path/);
expect(getPages()).toEqual([]);
});
@@ -162,3 +173,37 @@ test('a path no extension claims matches nothing', () => {
test('resolvePage falls back to a placeholder for an unknown id', () => {
expect(resolvePage('acme.tools:/missing')).toBeTruthy();
});
+
+test('a page the user may not open is still registered, marked unpermitted', () => {
+ // Registered rather than dropped: the extension's bundle calls
+ // registerPage unconditionally, so refusing here would turn a permission
+ // the user lacks into an error in their console. The host renders a
+ // refusal instead, and the server refuses the path independently.
+ disposables.push(
+ register('acme.tools', { path: '/review-queue', name: 'Queue' }, [
+ { path: '/review-queue', permitted: false },
+ ]),
+ );
+
+ expect(getPages()).toEqual([
+ {
+ path: '/review-queue',
+ name: 'Queue',
+ extensionId: 'acme.tools',
+ permitted: false,
+ },
+ ]);
+});
+
+test('a declaration that says nothing about permission is treated as open', () => {
+ // The server omits the flag only for a page with no declared permission,
+ // and an older server omits it entirely. Defaulting to refused would hide
+ // every page from everyone the first time this ships.
+ disposables.push(
+ register('acme.tools', { path: '/review-queue', name: 'Queue' }, [
+ { path: '/review-queue' },
+ ]),
+ );
+
+ expect(getPages()[0].permitted).toBe(true);
+});
diff --git a/superset-frontend/src/core/pages/index.ts b/superset-frontend/src/core/pages/index.ts
index 257f4cf6624..a09a01daca6 100644
--- a/superset-frontend/src/core/pages/index.ts
+++ b/superset-frontend/src/core/pages/index.ts
@@ -51,6 +51,15 @@ import { createEventEmitter } from '../utils';
type Page = pagesApi.Page;
type ResolvedPage = pagesApi.ResolvedPage;
+/**
+ * A page as the server reported it: what the manifest declares, plus whether
+ * this user may open it. Bound per extension by the loader.
+ */
+export interface DeclaredPage {
+ path: string;
+ permitted?: boolean;
+}
+
/**
* Mirrors `PAGE_PATH_PATTERN` in `superset_core.extensions.constants`.
*
@@ -90,7 +99,7 @@ const refresh = () => {
*/
export const registerPageFor = (
extensionId: string,
- declaredPaths: readonly string[],
+ declared: readonly DeclaredPage[],
page: Page,
component: ComponentType,
): Disposable => {
@@ -102,18 +111,26 @@ export const registerPageFor = (
);
}
- if (!declaredPaths.includes(page.path)) {
+ const declaration = declared.find(entry => entry.path === page.path);
+ if (!declaration) {
throw new Error(
`Extension "${extensionId}" registered a page at "${page.path}", which ` +
`its manifest does not declare. Add it to contributions.pages and ` +
`rebuild: the server registers URL rules from the manifest, so this ` +
`page would work on client navigation and 404 on a cold load. ` +
- `Declared: ${declaredPaths.join(', ') || '(none)'}.`,
+ `Declared: ${declared.map(entry => entry.path).join(', ') || '(none)'}.`,
);
}
const id = `${extensionId}:${page.path}`;
- const resolved: ResolvedPage = { ...page, extensionId };
+ // Registered whether or not the user may open it, so that the component is
+ // there to render a refusal rather than a blank screen, and so an
+ // extension's own bundle never has to branch on permissions.
+ const resolved: ResolvedPage = {
+ ...page,
+ extensionId,
+ permitted: declaration.permitted !== false,
+ };
const replaced = pageRegistry.get(id);
if (replaced) {
diff --git a/superset-frontend/src/extensions/ExtensionsLoader.ts b/superset-frontend/src/extensions/ExtensionsLoader.ts
index 6ede4892008..e33fcb63655 100644
--- a/superset-frontend/src/extensions/ExtensionsLoader.ts
+++ b/superset-frontend/src/extensions/ExtensionsLoader.ts
@@ -49,7 +49,7 @@ export interface LoadedExtension extends Extension {
* paths the extension is allowed to register a component against.
*/
contributions?: {
- pages?: { path: string }[];
+ pages?: { path: string; permitted?: boolean }[];
};
}
@@ -226,9 +226,7 @@ class ExtensionsLoader {
// receives the same pre-bound instance. Parallel loading is safe because each
// container gets its own scope with its own resolved module instance.
const context = createExtensionContext(extension);
- const declaredPagePaths = (extension.contributions?.pages ?? []).map(
- page => page.path,
- );
+ const declaredPages = extension.contributions?.pages ?? [];
const scopedCore = {
...window.superset,
extensions: {
@@ -245,7 +243,7 @@ class ExtensionsLoader {
pages: {
...window.superset.pages,
registerPage: (page: pagesApi.Page, component: ComponentType) =>
- registerPageFor(extension.id, declaredPagePaths, page, component),
+ registerPageFor(extension.id, declaredPages, page, component),
},
};
// Reuse the version key webpack already resolved for the host's own
diff --git a/superset-frontend/src/features/home/Menu.tsx b/superset-frontend/src/features/home/Menu.tsx
index 2f42e03f569..aab1f27c7d7 100644
--- a/superset-frontend/src/features/home/Menu.tsx
+++ b/superset-frontend/src/features/home/Menu.tsx
@@ -217,9 +217,18 @@ const { useBreakpoint } = Grid;
*/
function withExtensionPages(
menu: MenuObjectProps[],
- pages: { name: string; path: string; nav?: { label?: string } }[],
+ pages: {
+ name: string;
+ path: string;
+ nav?: { label?: string };
+ permitted?: boolean;
+ }[],
): MenuObjectProps[] {
- const contributed = pages.filter(page => page.nav);
+ // A page the user may not open is left out rather than shown and refused:
+ // the server answers 403 for it, so an entry here would only advertise a
+ // dead end. The page stays registered, so a direct link still explains
+ // itself rather than rendering nothing.
+ const contributed = pages.filter(page => page.nav && page.permitted !== false);
if (!contributed.length) {
return menu;
}
diff --git a/superset-frontend/src/pages/ExtensionPage/index.tsx b/superset-frontend/src/pages/ExtensionPage/index.tsx
index 8d6e060f8c9..c2869813d91 100644
--- a/superset-frontend/src/pages/ExtensionPage/index.tsx
+++ b/superset-frontend/src/pages/ExtensionPage/index.tsx
@@ -61,6 +61,24 @@ export default function ExtensionPage() {
const page = findPageByPath(stripAppRoot(pathname));
+ if (page && !page.permitted) {
+ // The server has already refused a cold load of this path with 403; this
+ // is the client-side arrival, where saying so beats rendering the page's
+ // chrome around data its own API calls will refuse one by one.
+ return (
+ <ErrorAlert
+ errorType={t('Not authorized')}
+ message={t('You do not have permission to open this page.')}
+ description={t(
+ 'Ask an administrator to grant you the permission this page ' +
+ 'requires on the extension that provides it.',
+ )}
+ type="warning"
+ closable={false}
+ />
+ );
+ }
+
if (page) {
return resolvePage(`${page.extensionId}:${page.path}`);
}
diff --git a/superset/extensions/utils.py b/superset/extensions/utils.py
index 73d2a0181de..b5b6265ee63 100644
--- a/superset/extensions/utils.py
+++ b/superset/extensions/utils.py
@@ -240,10 +240,11 @@ def get_loaded_extension(
def build_extension_data(extension: LoadedExtension) -> dict[str, Any]:
- from superset.extensions.view import registered_page_paths
+ from superset.extensions.view import permitted_page_paths, registered_page_paths
manifest = extension.manifest
registered = set(registered_page_paths(current_app, manifest.id))
+ permitted = permitted_page_paths(current_app, manifest)
extension_data: dict[str, Any] = {
"id": manifest.id,
"publisher": manifest.publisher,
@@ -258,7 +259,14 @@ def build_extension_data(extension: LoadedExtension) -> dict[str, Any]:
"contributions": {
**manifest.contributions.model_dump(exclude_none=True),
"pages": [
- page.model_dump(exclude_none=True)
+ # `permitted` rather than filtering the page out: the
+ # extension's own bundle calls registerPage for every page it
+ # ships, and dropping one here would turn a permission the
+ # user lacks into a registration error in their console.
+ {
+ **page.model_dump(exclude_none=True),
+ "permitted": page.path in permitted,
+ }
for page in manifest.contributions.pages
if page.path in registered
],
diff --git a/superset/extensions/view.py b/superset/extensions/view.py
index 0b481c1b607..456c7f54424 100644
--- a/superset/extensions/view.py
+++ b/superset/extensions/view.py
@@ -15,11 +15,12 @@
# specific language governing permissions and limitations
# under the License.
import logging
+from typing import Callable
from flask import abort, Flask, g, render_template
from flask_appbuilder import expose
from flask_appbuilder.security.decorators import has_access, permission_name
-from superset_core.extensions.types import Manifest
+from superset_core.extensions.types import Manifest, Page
from werkzeug.exceptions import MethodNotAllowed, NotFound
from werkzeug.routing import RequestRedirect
@@ -48,29 +49,71 @@ class ExtensionsView(BaseSupersetView):
return super().render_app_template()
-def _render_page(**_kwargs: str) -> FlaskResponse:
- """Render the application shell for a contributed page.
+def _render_shell() -> FlaskResponse:
+ """The application shell, identical to the one ``/welcome/`` serves."""
+ return render_template("superset/spa.html", **get_spa_template_context())
+
- ``_kwargs`` absorbs the trailing path of the sub-path rule and is never
- read. It reaches the server only so the URL resolves; which page it
- denotes is decided by the React router once the extension's bundle has
- registered its component. The path is therefore not a value to be trusted
- or validated here, because nothing here acts on it.
+def may_open(extension_id: str, permission: str | None) -> bool:
+ """Whether the current user may open a page with this permission.
- Guarded by a login check rather than a permission, matching
- ``Superset.welcome``: the payload is a pure function of the requesting
- user and is byte-identical to the one ``/welcome/`` serves. It names no
- extension, carries no page data, and every API the page then calls is
- separately authorised. A permission here would gate the frame around the
- data, not the data.
+ A page without a declared permission is open to any signed-in user, which
+ is what every contributed page was before permissions existed. Keeping
+ that the default means adding this field cannot lock anyone out of a page
+ that already worked.
"""
- from superset import is_feature_enabled
+ if permission is None:
+ return True
+ from superset import security_manager
- if not is_feature_enabled("ENABLE_EXTENSIONS"):
- abort(404)
- if not g.user or not get_user_id():
- return redirect_to_login()
- return render_template("superset/spa.html", **get_spa_template_context())
+ return security_manager.can_access(permission, extension_id)
+
+
+def _make_page_view(extension_id: str, page: Page) -> Callable[..., FlaskResponse]:
+ """Build the view for one page, closing over what that page requires.
+
+ A view per page rather than one shared view read from the request: the
+ permission a page requires is part of the route, not something to re-derive
+ from the URL on the way in.
+ """
+
+ def view(**_kwargs: str) -> FlaskResponse:
+ """Render the shell, or refuse.
+
+ ``_kwargs`` absorbs the trailing path of the sub-path rule and is
+ never read. It reaches the server only so the URL resolves; which
+ screen it denotes is decided by the React router once the extension's
+ bundle has registered its component. The path is therefore not a value
+ to be trusted or validated here, because nothing here acts on it.
+
+ A signed-in user without the permission gets 403 rather than FAB's
+ ``has_access`` redirect to the login page. They are already signed in;
+ sending them to log in again says nothing true and leaves them
+ somewhere they cannot act on. 403 goes through Superset's own
+ ``HTTPException`` handler like any other refusal.
+ """
+ # Imported here rather than at module scope: the flag is read per
+ # request, so turning extensions off takes effect without waiting for
+ # the rules to disappear at the next restart.
+ from superset import is_feature_enabled
+
+ if not is_feature_enabled("ENABLE_EXTENSIONS"):
+ abort(404)
+ if not g.user or not get_user_id():
+ return redirect_to_login()
+ if not may_open(extension_id, page.permission):
+ logger.warning(
+ "User %s was refused the page '%s' of extension '%s', which "
+ "requires '%s'",
+ get_user_id(),
+ page.path,
+ extension_id,
+ page.permission,
+ )
+ abort(403)
+ return _render_shell()
+
+ return view
def _first_claimant(app: Flask, path: str) -> str | None:
@@ -147,19 +190,25 @@ def register_extension_pages(app: Flask, manifest: Manifest) -> None:
continue
endpoint = f"{_ENDPOINT_PREFIX}.{manifest.id}{page.path}"
+ view = _make_page_view(manifest.id, page)
app.add_url_rule(
page.path,
endpoint=endpoint,
- view_func=_render_page,
+ view_func=view,
methods=["GET"],
)
app.add_url_rule(
f"{page.path}/<path:rest>",
endpoint=f"{endpoint}/*",
- view_func=_render_page,
+ view_func=view,
methods=["GET"],
)
- logger.info("Extension '%s' serves a page at '%s'", manifest.id, page.path)
+ logger.info(
+ "Extension '%s' serves a page at '%s'%s",
+ manifest.id,
+ page.path,
+ f", requiring '{page.permission}'" if page.permission else "",
+ )
def registered_page_paths(app: Flask, extension_id: str) -> list[str]:
@@ -177,3 +226,66 @@ def registered_page_paths(app: Flask, extension_id: str) -> list[str]:
for rule in app.url_map.iter_rules()
if rule.endpoint.startswith(prefix) and not rule.endpoint.endswith("/*")
]
+
+
+def ensure_page_permissions(manifest: Manifest) -> None:
+ """Create the permissions this extension's pages require, if missing.
+
+ Created when the extension loads rather than by ``superset init``, because
+ an extension can be installed long after a deployment was initialised and
+ an administrator cannot grant a permission that does not exist yet.
+
+ The permission is attached to a view menu named after the extension, so
+ the Roles screen groups an extension's permissions together and reads as
+ "can_review on preset.asset-approval". Pages of the same extension may
+ share a permission or declare different ones; this registers whatever set
+ they name.
+
+ Failures are logged and swallowed. Every worker runs this at boot, so two
+ starting together can race between the lookup and the insert; the loser
+ would otherwise take the whole worker down over a row the winner has
+ already written. The permission check itself reads the database per
+ request, so a worker that lost the race still enforces correctly.
+ """
+ from superset import security_manager
+
+ required = {
+ page.permission
+ for page in manifest.contributions.pages
+ if page.permission is not None
+ }
+ for permission in sorted(required):
+ try:
+ if security_manager.find_permission_view_menu(permission, manifest.id):
+ continue
+ security_manager.add_permission_view_menu(permission, manifest.id)
+ logger.info(
+ "Created permission '%s' on '%s' for extension pages",
+ permission,
+ manifest.id,
+ )
+ except Exception: # pylint: disable=broad-except
+ # Most often a concurrent worker inserting the same row.
+ logger.warning(
+ "Could not create permission '%s' on '%s'; an administrator "
+ "may need to run `superset init` before it can be granted",
+ permission,
+ manifest.id,
+ exc_info=True,
+ )
+ security_manager.session.rollback()
+
+
+def permitted_page_paths(app: Flask, manifest: Manifest) -> set[str]:
+ """Paths of this extension's pages the current user may open.
+
+ Used to decide what the frontend is told about. The server refuses an
+ unpermitted page on its own, so this is not the enforcement -- it is what
+ keeps a navigation entry from advertising a page that would answer 403.
+ """
+ registered = set(registered_page_paths(app, manifest.id))
+ return {
+ page.path
+ for page in manifest.contributions.pages
+ if page.path in registered and may_open(manifest.id, page.permission)
+ }
diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py
index f6a0ca2746b..c9ce121b36a 100644
--- a/superset/initialization/__init__.py
+++ b/superset/initialization/__init__.py
@@ -657,7 +657,10 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods
get_extensions,
install_in_memory_importer,
)
- from superset.extensions.view import register_extension_pages
+ from superset.extensions.view import (
+ ensure_page_permissions,
+ register_extension_pages,
+ )
try:
extensions = get_extensions()
@@ -672,6 +675,7 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods
# manifest, so it does not depend on extension code importing
# cleanly, and a frontend-only extension contributes pages too.
register_extension_pages(self.superset_app, extension.manifest)
+ ensure_page_permissions(extension.manifest)
if backend_files := extension.backend:
install_in_memory_importer(
diff --git a/tests/unit_tests/extensions/test_pages_view.py b/tests/unit_tests/extensions/test_pages_view.py
index 860169ea884..d1de537e700 100644
--- a/tests/unit_tests/extensions/test_pages_view.py
+++ b/tests/unit_tests/extensions/test_pages_view.py
@@ -24,33 +24,78 @@ which is the whole question.
"""
import logging
+from types import SimpleNamespace
import pytest
from flask import Flask, g
from pydantic import ValidationError
from superset_core.extensions.types import Contributions, Manifest, Page
-from werkzeug.exceptions import NotFound
+from werkzeug.exceptions import Forbidden, NotFound
from superset.extensions import view
from superset.extensions.view import (
_first_claimant,
+ ensure_page_permissions,
+ permitted_page_paths,
register_extension_pages,
registered_page_paths,
)
-def manifest(*paths: str, extension_id: str = "acme.tools") -> Manifest:
+def manifest(
+ *paths: str,
+ extension_id: str = "acme.tools",
+ permission: str | None = None,
+) -> Manifest:
publisher, name = extension_id.split(".", 1)
return Manifest(
publisher=publisher,
name=name,
displayName="Tools",
contributions=Contributions(
- pages=[Page(path=path, name=path) for path in paths]
+ pages=[Page(path=path, name=path, permission=permission) for path in paths]
),
)
+class FakeSecurityManager:
+ """Stands in for the real one, recording what was asked and created."""
+
+ def __init__(self, granted: set[tuple[str, str]] | None = None) -> None:
+ self.granted = granted or set()
+ self.existing: set[tuple[str, str]] = set()
+ self.created: list[tuple[str, str]] = []
+ self.asked: list[tuple[str, str]] = []
+ self.rollbacks = 0
+
+ def can_access(self, permission: str, view: str) -> bool:
+ self.asked.append((permission, view))
+ return (permission, view) in self.granted
+
+ def find_permission_view_menu(self, permission: str, view: str) -> object | None:
+ return (permission, view) if (permission, view) in self.existing else None
+
+ def add_permission_view_menu(self, permission: str, view: str) -> None:
+ self.existing.add((permission, view))
+ self.created.append((permission, view))
+
+ # Named to match SupersetSecurityManager. A fake that invents its own
+ # attribute names lets production code call something that is not there.
+ @property
+ def session(self) -> SimpleNamespace:
+ self.rollbacks += 1
+ return SimpleNamespace(rollback=lambda: None)
+
+
+@pytest.fixture
+def security(monkeypatch: pytest.MonkeyPatch) -> FakeSecurityManager:
+ import superset
+
+ manager = FakeSecurityManager()
+ monkeypatch.setattr(superset, "security_manager", manager, raising=False)
+ return manager
+
+
def paths_in(app: Flask) -> set[str]:
return {rule.rule for rule in app.url_map.iter_rules()}
@@ -206,27 +251,33 @@ def render_with(
*,
extensions_enabled: bool = True,
user_id: int | None = 1,
+ permission: str | None = None,
+ granted: bool = False,
) -> object:
- """Run ``_render_page``'s guards without rendering a real template.
+ """Run one page view's guards without rendering a real template.
The guards are the whole of the view; what it renders on success is
``spa.html`` unchanged, which the host's own screens already cover.
"""
import superset
+ security = FakeSecurityManager(
+ granted={(permission, "acme.tools")} if permission and granted else set()
+ )
+ monkeypatch.setattr(superset, "security_manager", security, raising=False)
monkeypatch.setattr(
superset, "is_feature_enabled", lambda _flag: extensions_enabled
)
monkeypatch.setattr(view, "get_user_id", lambda: user_id)
- monkeypatch.setattr(view, "render_template", lambda *a, **k: "SHELL")
- monkeypatch.setattr(view, "get_spa_template_context", dict)
+ monkeypatch.setattr(view, "_render_shell", lambda: "SHELL")
monkeypatch.setattr(view, "redirect_to_login", lambda: "LOGIN")
- register_extension_pages(app, manifest("/approvals"))
+ page = Page(path="/approvals", name="Approvals", permission=permission)
+ page_view = view._make_page_view("acme.tools", page)
with app.test_request_context("/approvals"):
g.user = object() if user_id else None
- return view._render_page()
+ return page_view()
def test_the_flag_decides_what_a_registered_page_answers(
@@ -254,3 +305,173 @@ def test_a_signed_in_load_gets_the_shell(
monkeypatch: pytest.MonkeyPatch, app: Flask
) -> None:
assert render_with(monkeypatch, app) == "SHELL"
+
+
+def test_a_page_without_a_permission_is_open_to_any_signed_in_user(
+ security: FakeSecurityManager,
+) -> None:
+ # The default has to stay open, or adding this field would lock users out
+ # of pages that already worked.
+ assert view.may_open("acme.tools", None) is True
+ assert security.asked == []
+
+
+def test_a_declared_permission_is_checked_against_the_extension(
+ security: FakeSecurityManager,
+) -> None:
+ assert view.may_open("acme.tools", "can_review") is False
+
+ # Granted on the extension, which is what the Roles screen shows.
+ security.granted.add(("can_review", "acme.tools"))
+ assert view.may_open("acme.tools", "can_review") is True
+
+
+def test_the_same_permission_on_another_extension_does_not_carry_over(
+ security: FakeSecurityManager,
+) -> None:
+ security.granted.add(("can_review", "acme.tools"))
+
+ assert view.may_open("other.ext", "can_review") is False
+
+
+def test_permissions_are_created_for_every_page_that_names_one(
+ security: FakeSecurityManager,
+) -> None:
+ ensure_page_permissions(manifest("/a", "/b", permission="can_review"))
+
+ # One row, not one per page: pages of an extension may share a permission.
+ assert security.created == [("can_review", "acme.tools")]
+
+
+def test_nothing_is_created_for_pages_without_permissions(
+ security: FakeSecurityManager,
+) -> None:
+ ensure_page_permissions(manifest("/a", "/b"))
+
+ assert security.created == []
+
+
+def test_creating_permissions_is_idempotent(security: FakeSecurityManager) -> None:
+ # Every worker runs this at every boot.
+ ensure_page_permissions(manifest("/a", permission="can_review"))
+ ensure_page_permissions(manifest("/a", permission="can_review"))
+
+ assert security.created == [("can_review", "acme.tools")]
+
+
+def test_a_failure_to_create_a_permission_does_not_stop_the_boot(
+ monkeypatch: pytest.MonkeyPatch, security: FakeSecurityManager
+) -> None:
+ """Two workers starting together race between the lookup and the insert."""
+
+ def explode(permission: str, view_menu: str) -> None:
+ raise RuntimeError("duplicate key")
+
+ monkeypatch.setattr(security, "add_permission_view_menu", explode)
+
+ ensure_page_permissions(manifest("/a", permission="can_review"))
+
+ # And the session was rolled back, so the next write on this worker is
+ # not issued against a session left dirty by the failed insert.
+ assert security.rollbacks == 1
+
+
+def test_permitted_page_paths_reports_only_what_the_user_may_open(
+ app: Flask, security: FakeSecurityManager
+) -> None:
+ register_extension_pages(app, manifest("/open"))
+ register_extension_pages(app, manifest("/restricted", permission="can_review"))
+
+ assert permitted_page_paths(app, manifest("/open")) == {"/open"}
+ assert (
+ permitted_page_paths(app, manifest("/restricted", permission="can_review"))
+ == set()
+ )
+
+ security.granted.add(("can_review", "acme.tools"))
+ assert permitted_page_paths(
+ app, manifest("/restricted", permission="can_review")
+ ) == {"/restricted"}
+
+
+def test_a_refused_page_is_never_reported_as_permitted(
+ app: Flask, security: FakeSecurityManager
+) -> None:
+ """A page with no rule cannot be opened however the permissions read."""
+ app.add_url_rule("/welcome", endpoint="welcome", view_func=lambda: "")
+ security.granted.add(("can_review", "acme.tools"))
+
+ register_extension_pages(app, manifest("/welcome", permission="can_review"))
+
+ assert (
+ permitted_page_paths(app, manifest("/welcome", permission="can_review"))
+ == set()
+ )
+
+
+@pytest.mark.parametrize("permission", ["Can_Review", "can review", "can-review", ""])
+def test_a_malformed_permission_is_rejected_by_the_manifest(permission: str) -> None:
+ with pytest.raises(ValidationError, match="should match pattern"):
+ Page(path="/a", name="A", permission=permission)
+
+
+def test_a_signed_in_user_without_the_permission_gets_403_not_the_shell(
+ monkeypatch: pytest.MonkeyPatch, app: Flask
+) -> None:
+ with pytest.raises(Forbidden):
+ render_with(monkeypatch, app, permission="can_review", granted=False)
+
+
+def test_a_signed_in_user_with_the_permission_gets_the_shell(
+ monkeypatch: pytest.MonkeyPatch, app: Flask
+) -> None:
+ assert (
+ render_with(monkeypatch, app, permission="can_review", granted=True) == "SHELL"
+ )
+
+
+def test_an_anonymous_user_is_sent_to_login_rather_than_refused(
+ monkeypatch: pytest.MonkeyPatch, app: Flask
+) -> None:
+ """Login comes first, so a permissioned page does not 403 a visitor.
+
+ 403 would be both unhelpful and a disclosure: it says the page exists to
+ someone who has not identified themselves.
+ """
+ assert (
+ render_with(
+ monkeypatch, app, permission="can_review", granted=False, user_id=None
+ )
+ == "LOGIN"
+ )
+
+
+def test_the_fake_security_manager_only_names_real_attributes() -> None:
+ """Pins the stand-in above to the class it stands in for.
+
+ Without this a fake can invent an attribute -- `get_session` rather than
+ `session` -- and every test using it passes while the production call
+ raises AttributeError on the real object, inside the except branch meant
+ to keep a failed insert from stopping the boot.
+ """
+ from superset.security.manager import SupersetSecurityManager
+
+ used_by_production_code = (
+ "can_access",
+ "find_permission_view_menu",
+ "add_permission_view_menu",
+ "session",
+ )
+ missing = [
+ name
+ for name in used_by_production_code
+ if not hasattr(SupersetSecurityManager, name)
+ ]
+ assert not missing, (
+ f"FakeSecurityManager names attributes that do not exist: {missing}"
+ )
+
+ fake_surface = {
+ name for name in vars(FakeSecurityManager) if not name.startswith("_")
+ }
+ assert fake_surface <= set(used_by_production_code)