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:03 UTC
(superset) 05/08: feat(extensions): let a page choose its path, and check it at boot
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 92ccddae1364cf70080c5621fc1ac2a77b1ff37b
Author: amaannawab923 <am...@gmail.com>
AuthorDate: Tue Oct 20 18:37:21 2026 +0530
feat(extensions): let a page choose its path, and check it at boot
Pages were served from /extensions/<publisher>.<name>/<slug>, a namespace
the host derived so an extension could not name a URL. It worked, but it
made every contributed page read as plumbing, and the prefix bought
safety that can be had another way.
A page now declares an absolute path in its manifest -- /approvals rather
than /extensions/preset.asset-approval/approvals -- and the server checks
it against the routes the deployment actually serves before registering a
rule for it. The check asks the live URL map rather than comparing
strings: /oauth-authorized/<provider> never string-matches the request it
would answer, which is how an earlier revision of this code waved that
path through. It also scans rule templates for the same prefix, because a
page takes everything beneath its path and would otherwise shadow a core
route sitting there.
Running at every boot is what makes this hold over time. Routes Superset
registers conditionally -- the OAuth callback exists only under
AUTH_TYPE = AUTH_OAUTH -- are absent in a deployment with the feature
off, so a page may legitimately take that path there. Turning the feature
on later puts the real rule in the map first, and the next boot refuses
the page loudly instead of leaving an extension in front of the callback.
Registration runs after operator blueprints, the FAB auth views and every
core view, so the map it checks is the complete one.
Two layers back the check up. The client route is a catch-all pushed last,
so <Switch> gives every host screen precedence the same way Werkzeug gives
its static rules precedence. And the extensions API reports only pages
that actually got a rule, read back from the URL map, so a refused page
cannot reach the menu or accept a component.
A refused page is skipped, not fatal: one extension claiming a taken path
should not stop the server starting.
Also mounts superset-core and superset-extensions-cli in the light compose
stack. Both are installed editable, so without this an edit to the
extension contract is invisible in the container while the import still
succeeds against the copy baked into the image.
---
docker-compose-light.yml | 5 +
.../src/superset_core/extensions/constants.py | 8 +
.../src/superset_core/extensions/types.py | 49 ++++
.../src/superset_extensions_cli/cli.py | 1 +
.../packages/superset-core/src/pages/index.ts | 63 ++---
superset-frontend/src/core/pages/index.test.ts | 126 +++++-----
superset-frontend/src/core/pages/index.ts | 65 +++---
.../src/extensions/ExtensionsLoader.ts | 34 ++-
.../src/pages/ExtensionPage/index.tsx | 13 +-
superset-frontend/src/views/routes.tsx | 36 +--
superset/extensions/utils.py | 15 ++
superset/extensions/view.py | 192 +++++++++++-----
superset/initialization/__init__.py | 14 +-
tests/unit_tests/extensions/test_pages_catchall.py | 141 ------------
tests/unit_tests/extensions/test_pages_view.py | 256 +++++++++++++++++++++
15 files changed, 670 insertions(+), 348 deletions(-)
diff --git a/docker-compose-light.yml b/docker-compose-light.yml
index 0b62ab2fabd..9de8ce641e0 100644
--- a/docker-compose-light.yml
+++ b/docker-compose-light.yml
@@ -65,6 +65,11 @@ x-superset-volumes: &superset-volumes
- ./docker:/app/docker
- ./superset:/app/superset
- ./superset-frontend:/app/superset-frontend
+ # Both are installed editable, so without these an edit to the extension
+ # contract is invisible in the container while the import still succeeds
+ # against the copy baked into the image.
+ - ./superset-core:/app/superset-core
+ - ./superset-extensions-cli:/app/superset-extensions-cli
- superset_home_light:/app/superset_home
- ./tests:/app/tests
x-common-build: &common-build
diff --git a/superset-core/src/superset_core/extensions/constants.py b/superset-core/src/superset_core/extensions/constants.py
index 965270d04ce..dd6a3e84b5b 100644
--- a/superset-core/src/superset_core/extensions/constants.py
+++ b/superset-core/src/superset_core/extensions/constants.py
@@ -33,3 +33,11 @@ DISPLAY_NAME_PATTERN = r"^[a-zA-Z][a-zA-Z0-9\s\-_\.]*$"
# Version pattern for semantic versioning
VERSION_PATTERN = r"^\d+\.\d+\.\d+$"
+
+# Page path validation pattern: an absolute path of one or more lowercase
+# segments, in the same shape as a technical name. No trailing slash, no
+# parameters, no wildcards -- a page owns a fixed prefix and routes within it
+# on the client. Kept deliberately narrow: the host registers these verbatim
+# 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]+)*)?)+$"
diff --git a/superset-core/src/superset_core/extensions/types.py b/superset-core/src/superset_core/extensions/types.py
index 1a067b52d44..87333d8417e 100644
--- a/superset-core/src/superset_core/extensions/types.py
+++ b/superset-core/src/superset_core/extensions/types.py
@@ -29,12 +29,57 @@ from pydantic import BaseModel, computed_field, Field # noqa: I001
from superset_core.extensions.constants import (
DISPLAY_NAME_PATTERN,
+ PAGE_PATH_PATTERN,
PUBLISHER_PATTERN,
TECHNICAL_NAME_PATTERN,
VERSION_PATTERN,
)
+class PageNav(BaseModel):
+ """Placement of a page in the navigation bar."""
+
+ label: str = Field(
+ ...,
+ description="Text of the navigation entry",
+ min_length=1,
+ )
+
+
+class Page(BaseModel):
+ """A page an extension serves at a path of its own."""
+
+ path: str = Field(
+ ...,
+ description="Absolute path the page is served at, e.g. '/approvals'",
+ pattern=PAGE_PATH_PATTERN,
+ )
+ name: str = Field(
+ ...,
+ description="Human-readable page name",
+ min_length=1,
+ )
+ nav: PageNav | None = Field(
+ default=None,
+ description="Navigation entry, when the page should appear in the menu",
+ )
+
+
+class Contributions(BaseModel):
+ """What an extension adds to the host.
+
+ Declared rather than merely registered at runtime, because the host has to
+ act on it in two places that never see each other: Flask registers a rule
+ per page so a cold load resolves, and the React router mounts the
+ component. One declaration keeps them from disagreeing.
+ """
+
+ pages: list[Page] = Field(
+ default_factory=list,
+ description="Pages contributed by this extension",
+ )
+
+
class BaseExtension(BaseModel):
"""Base fields shared by ExtensionConfig and Manifest."""
@@ -77,6 +122,10 @@ class BaseExtension(BaseModel):
default_factory=list,
description="Permissions required by this extension",
)
+ contributions: Contributions = Field(
+ default_factory=Contributions,
+ description="What this extension contributes to the host",
+ )
@computed_field(description="Composite extension ID (publisher.name)") # type: ignore[prop-decorator]
@property
diff --git a/superset-extensions-cli/src/superset_extensions_cli/cli.py b/superset-extensions-cli/src/superset_extensions_cli/cli.py
index d050fbf8e4e..2d4ff7c7145 100644
--- a/superset-extensions-cli/src/superset_extensions_cli/cli.py
+++ b/superset-extensions-cli/src/superset_extensions_cli/cli.py
@@ -179,6 +179,7 @@ def build_manifest(cwd: Path, remote_entry: str | None) -> Manifest:
version=extension.version,
permissions=extension.permissions,
dependencies=extension.dependencies,
+ contributions=extension.contributions,
frontend=frontend,
backend=backend,
)
diff --git a/superset-frontend/packages/superset-core/src/pages/index.ts b/superset-frontend/packages/superset-core/src/pages/index.ts
index b1519f1c181..fea9dddeb09 100644
--- a/superset-frontend/packages/superset-core/src/pages/index.ts
+++ b/superset-frontend/packages/superset-core/src/pages/index.ts
@@ -23,16 +23,23 @@
* A page is a screen of the extension's own, at its own URL, as opposed to a
* view that occupies a slot inside a screen the host already ships.
*
- * An extension names a page by a bare `slug`; the host derives the URL from
- * the extension's identity, exactly as it does for an extension's REST APIs.
- * An extension cannot choose, prefix or spoof its own URL space.
+ * A page is declared in the extension's manifest and registered at runtime.
+ * Both halves are needed: the manifest declaration is what the server reads
+ * to register a URL rule, so a cold load of the page resolves; the runtime
+ * registration is what supplies the component to render there.
+ *
+ * The path is the extension author's to choose, but it is not theirs to take.
+ * The server checks every declared path against the routes the deployment
+ * actually serves, on every boot, and refuses a page whose path is already
+ * answered by Superset, by an operator's blueprint, or by another extension.
*
* @example
* ```typescript
+ * // extension.json
+ * // "contributions": { "pages": [{ "path": "/review-queue", "name": "Queue" }] }
* import { pages } from '@apache-superset/core';
*
- * pages.registerPage({ slug: 'queue', name: 'Review queue' }, ReviewQueue);
- * // served at /extensions/<publisher>.<name>/queue
+ * pages.registerPage({ path: '/review-queue', name: 'Queue' }, ReviewQueue);
* ```
*/
@@ -44,15 +51,19 @@ import { Disposable, Event } from '../common';
*/
export interface Page {
/**
- * Names the page within its extension. Lowercase letters, digits and
- * hyphens.
+ * The absolute path the page is served at, such as `/review-queue`.
+ *
+ * Lowercase segments, leading slash, no trailing slash, no parameters: a
+ * page owns a fixed prefix and routes within it on the client. The page
+ * also owns everything beneath that prefix, so pick a path specific enough
+ * that nothing else will want it.
*
- * This is a slug, not a path: the host prefixes it with the extension's own
- * namespace. A slug of `queue` becomes `/extensions/<publisher>.<name>/queue`.
- * Declaring a full path would let one extension claim another's URL, or the
- * host's.
+ * Must match a path declared in the extension's manifest. The host registers
+ * rules from the manifest, so a component registered against an undeclared
+ * path would mount on client navigation and 404 on a cold load; the host
+ * rejects that registration rather than let the two disagree.
*/
- slug: string;
+ path: string;
/** The display name of the page. */
name: string;
/** Optional description, for display in contribution manifests. */
@@ -72,13 +83,11 @@ export interface Page {
}
/**
- * A page together with the URL the host derived for it.
+ * A page together with the extension that contributed it.
*/
export interface ResolvedPage extends Page {
/** The extension that contributed the page, as `publisher.name`. */
extensionId: string;
- /** The application-root-relative path the page is served at. */
- path: string;
}
/**
@@ -87,18 +96,16 @@ export interface ResolvedPage extends Page {
* The component renders inside the host's own chrome, under an error
* boundary.
*
- * It cannot use the host's router or store. `react-router-dom` and
- * `react-redux` are not in the Module Federation share scope, so an
+ * It cannot import the host's router or store directly: `react-router-dom`
+ * and `react-redux` are not in the Module Federation share scope, so an
* extension importing either gets a second module instance with its own
- * React context, and `useLocation()` throws at runtime. A page is therefore
- * a single screen today: there is no supported way to navigate within it or
- * to read a sub-path, even though the host serves every path beneath the
- * page and resolves it back to this component. Keep sub-screen state in the
- * component until the host offers a navigation primitive.
+ * React context, and `useLocation()` throws at runtime. Use `useSubPath` and
+ * `useNavigate` below, which read the host's router from inside the host
+ * bundle, and `@superset-ui/core` -- which is shared -- for `SupersetClient`.
*
- * Registering the same slug twice within one extension replaces the first
- * registration and logs a warning; slugs cannot collide across extensions,
- * because each extension's pages live under its own namespace.
+ * Registering the same path twice within one extension replaces the first
+ * registration and logs a warning. Across extensions it cannot happen: the
+ * server refuses a duplicate path at boot, before either page loads.
*
* @param page The page descriptor.
* @param component The React component to render at that route.
@@ -119,8 +126,8 @@ export declare function getPages(): ResolvedPage[];
*
* The host serves every path under a page and resolves it back to the same
* component, so a page can own a sub-path the way a host screen owns one --
- * `/queue/42` and `/queue/42/edit` both reach the `queue` page, and this
- * reports `/42` and `/42/edit`. On the page's own root it reports `/`.
+ * `/review-queue/42` and `/review-queue/42/edit` both reach the page, and
+ * this reports `/42` and `/42/edit`. On the page's own root it reports `/`.
*
* This exists because `react-router-dom` is not shared: an extension that
* imported it would get a second module instance with its own React context
@@ -146,7 +153,7 @@ export declare function useSubPath(): string;
* @example
* ```typescript
* const navigate = pages.useNavigate();
- * navigate(`/${row.id}`); // -> /extensions/<publisher>.<name>/queue/42
+ * navigate(`/${row.id}`); // -> /review-queue/42
* navigate('/'); // back to the page root
* ```
*/
diff --git a/superset-frontend/src/core/pages/index.test.ts b/superset-frontend/src/core/pages/index.test.ts
index a5b49a17093..384f6a1a5cf 100644
--- a/superset-frontend/src/core/pages/index.test.ts
+++ b/superset-frontend/src/core/pages/index.test.ts
@@ -22,7 +22,6 @@ import { logging } from '@apache-superset/core/utils';
import {
findPageByPath,
getPages,
- pagePath,
registerPageFor,
resolvePage,
} from './index';
@@ -30,67 +29,86 @@ import {
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 register = (
+ extensionId: string,
+ page: { path: string; name: string },
+ declared: readonly string[] = DECLARED,
+) => registerPageFor(extensionId, declared, page, Component);
+
afterEach(() => {
disposables.forEach(d => d.dispose());
disposables.length = 0;
});
-test('the host derives the URL from the extension, not the extension', () => {
- // The extension supplies only a slug. If it could supply a path it could
- // claim the host's URLs, or another extension's.
+test('a declared page registers at the path it declared', () => {
disposables.push(
- registerPageFor('acme.tools', { slug: 'queue', name: 'Queue' }, Component),
+ register('acme.tools', { path: '/review-queue', name: 'Queue' }),
);
expect(getPages()).toEqual([
- {
- slug: 'queue',
- name: 'Queue',
- extensionId: 'acme.tools',
- path: '/extensions/acme.tools/queue',
- },
+ { path: '/review-queue', name: 'Queue', extensionId: 'acme.tools' },
]);
});
-test('two extensions may use the same slug without colliding', () => {
- // Namespacing is what makes this safe; without it one would replace the
- // other and the loser would simply stop rendering.
- disposables.push(
- registerPageFor('acme.tools', { slug: 'queue', name: 'A' }, Component),
- registerPageFor('other.ext', { slug: 'queue', name: 'B' }, Component),
- );
+test('a path the manifest does not declare is refused', () => {
+ // The server registers URL rules from the manifest, so a component
+ // registered against an undeclared path would work on client navigation
+ // and 404 on a cold load. Refusing here keeps the halves in agreement.
+ expect(() =>
+ register('acme.tools', { path: '/welcome', name: 'X' }),
+ ).toThrow(/manifest does not declare/);
+ expect(getPages()).toEqual([]);
+});
- expect(getPages().map(p => p.path)).toEqual([
- '/extensions/acme.tools/queue',
- '/extensions/other.ext/queue',
- ]);
+test('the refusal names what was declared, so the fix is obvious', () => {
+ expect(() =>
+ register('acme.tools', { path: '/welcome', name: 'X' }, ['/review-queue']),
+ ).toThrow(/Declared: \/review-queue/);
+});
+
+test('an extension with no declared pages can register none', () => {
+ expect(() =>
+ register('acme.tools', { path: '/review-queue', name: 'X' }, []),
+ ).toThrow(/Declared: \(none\)/);
});
test.each([
- ['a path rather than a slug', '/queue'],
- ['a nested path', 'queue/42'],
- ['an attempt to escape the namespace', '../../login'],
- ['uppercase', 'Queue'],
+ ['relative', 'review-queue'],
+ ['an attempt to climb out', '/../login'],
+ ['a trailing slash', '/review-queue/'],
+ ['uppercase', '/Review-Queue'],
['empty', ''],
- ['a leading hyphen', '-queue'],
-])('a slug that is %s is refused', (_label, slug) => {
+ ['a route parameter', '/review-queue/:id'],
+ ['a leading hyphen', '/-queue'],
+])('a path that is %s is refused', (_label, path) => {
+ // Checked before the manifest list, so a malformed path is reported as
+ // malformed even when it somehow appears in the manifest.
expect(() =>
- registerPageFor('acme.tools', { slug, name: 'X' }, Component),
- ).toThrow(/must be lowercase letters/);
+ register('acme.tools', { path, name: 'X' }, [path]),
+ ).toThrow(/must be an absolute path/);
expect(getPages()).toEqual([]);
});
-test('re-registering a slug within one extension replaces it, and says so', () => {
+test('a multi-segment path is allowed', () => {
+ disposables.push(register('acme.tools', { path: '/a/b', name: 'Nested' }));
+
+ expect(getPages().map(p => p.path)).toEqual(['/a/b']);
+});
+
+test('re-registering a path within one extension replaces it, and says so', () => {
const warn = jest.spyOn(logging, 'warn').mockImplementation(() => {});
try {
disposables.push(
- registerPageFor('acme.tools', { slug: 'queue', name: 'First' }, Component),
- registerPageFor('acme.tools', { slug: 'queue', name: 'Second' }, Component),
+ register('acme.tools', { path: '/review-queue', name: 'First' }),
+ register('acme.tools', { path: '/review-queue', name: 'Second' }),
);
expect(getPages().map(p => p.name)).toEqual(['Second']);
expect(warn).toHaveBeenCalledWith(
- expect.stringContaining('acme.tools.queue'),
+ expect.stringContaining('acme.tools:/review-queue'),
);
} finally {
warn.mockRestore();
@@ -100,13 +118,12 @@ test('re-registering a slug within one extension replaces it, and says so', () =
test('disposing a replaced registration leaves the replacement alone', () => {
const warn = jest.spyOn(logging, 'warn').mockImplementation(() => {});
try {
- const first = registerPageFor(
- 'acme.tools',
- { slug: 'queue', name: 'First' },
- Component,
- );
+ const first = register('acme.tools', {
+ path: '/review-queue',
+ name: 'First',
+ });
disposables.push(
- registerPageFor('acme.tools', { slug: 'queue', name: 'Second' }, Component),
+ register('acme.tools', { path: '/review-queue', name: 'Second' }),
);
first.dispose();
@@ -118,33 +135,30 @@ test('disposing a replaced registration leaves the replacement alone', () => {
});
test('a path is matched by prefix, so deep links reach their page', () => {
- // The server serves `<path:rest>` under the page, so the client has to
+ // The server registers `<path:rest>` under each page, so the client has to
// resolve the same way or a deep link would render "not found".
disposables.push(
- registerPageFor('acme.tools', { slug: 'queue', name: 'Queue' }, Component),
+ register('acme.tools', { path: '/review-queue', name: 'Queue' }),
);
- expect(findPageByPath('/extensions/acme.tools/queue')?.slug).toBe('queue');
- expect(findPageByPath('/extensions/acme.tools/queue/42/edit')?.slug).toBe(
- 'queue',
- );
+ expect(findPageByPath('/review-queue')?.name).toBe('Queue');
+ expect(findPageByPath('/review-queue/42/edit')?.name).toBe('Queue');
});
test('a path no extension claims matches nothing', () => {
disposables.push(
- registerPageFor('acme.tools', { slug: 'queue', name: 'Queue' }, Component),
+ register('acme.tools', { path: '/review-queue', name: 'Queue' }),
+ register('acme.tools', {
+ path: '/review-queue-archive',
+ name: 'Archive',
+ }),
);
- expect(findPageByPath('/extensions/acme.tools/nonexistent')).toBeUndefined();
- expect(findPageByPath('/extensions/nobody.nothing/x')).toBeUndefined();
- // A sibling slug sharing a prefix must not be swallowed.
- expect(findPageByPath('/extensions/acme.tools/queue-archive')).toBeUndefined();
+ expect(findPageByPath('/welcome')).toBeUndefined();
+ // A sibling sharing a prefix must not be swallowed by the shorter one.
+ expect(findPageByPath('/review-queue-archive')?.name).toBe('Archive');
});
test('resolvePage falls back to a placeholder for an unknown id', () => {
- expect(resolvePage('acme.tools.missing')).toBeTruthy();
-});
-
-test('pagePath is the single definition of the namespace', () => {
- expect(pagePath('acme.tools', 'queue')).toBe('/extensions/acme.tools/queue');
+ expect(resolvePage('acme.tools:/missing')).toBeTruthy();
});
diff --git a/superset-frontend/src/core/pages/index.ts b/superset-frontend/src/core/pages/index.ts
index 624cc052d97..257f4cf6624 100644
--- a/superset-frontend/src/core/pages/index.ts
+++ b/superset-frontend/src/core/pages/index.ts
@@ -24,10 +24,12 @@
* subscription so a registration arriving after first render still lands, and
* an `ExtensionPlaceholder` for a page whose component has not arrived.
*
- * The one thing this adds is namespacing. An extension registers a bare slug;
- * the host prefixes it with the extension's identity to produce both the id
- * and the URL. The extension never supplies a path, so it cannot claim a URL
- * belonging to the host or to another extension.
+ * The one thing this adds is holding an extension to its manifest. A page's
+ * path is declared there, which is what the server reads when it registers a
+ * URL rule and checks the path against every route the deployment serves. A
+ * component registered against a path the manifest does not declare has no
+ * rule behind it: it would mount on client navigation and 404 on a cold load.
+ * Rejecting it here keeps the two halves from disagreeing.
*/
import React, {
@@ -49,12 +51,14 @@ import { createEventEmitter } from '../utils';
type Page = pagesApi.Page;
type ResolvedPage = pagesApi.ResolvedPage;
-/** Slugs are a single path segment, in the same shape as an extension name. */
-const SLUG_PATTERN = /^[a-z]([a-z0-9]*(-[a-z0-9]+)*)?$/;
-
-/** The namespace every contributed page is served under. */
-export const pagePath = (extensionId: string, slug: string): string =>
- `/extensions/${extensionId}/${slug}`;
+/**
+ * Mirrors `PAGE_PATH_PATTERN` in `superset_core.extensions.constants`.
+ *
+ * Checked again here because this is the last point before a path is handed
+ * to the router, and because the message a developer sees for a malformed
+ * path should arrive in the console they are already watching.
+ */
+const PAGE_PATH_PATTERN = /^(\/[a-z]([a-z0-9]*(-[a-z0-9]+)*)?)+$/;
const pageRegistry: Map<string, { page: ResolvedPage; component: ComponentType }> =
new Map();
@@ -79,35 +83,44 @@ const refresh = () => {
/**
* Registers a page on behalf of a specific extension.
*
- * Not part of the public API: the loader binds `extensionId` per extension
- * before handing the namespace to extension code, so an extension cannot
- * name another extension's id. See `ExtensionsLoader.loadModule`.
+ * Not part of the public API: the loader binds `extensionId` and the paths
+ * that extension's manifest declares before handing the namespace to
+ * extension code, so an extension can neither claim another extension's id
+ * nor register a path it did not declare. See `ExtensionsLoader.loadModule`.
*/
export const registerPageFor = (
extensionId: string,
+ declaredPaths: readonly string[],
page: Page,
component: ComponentType,
): Disposable => {
- if (!page?.slug || !SLUG_PATTERN.test(page.slug)) {
+ if (!page?.path || !PAGE_PATH_PATTERN.test(page.path)) {
+ throw new Error(
+ `Page path "${page?.path ?? '<missing>'}" must be an absolute path of ` +
+ `lowercase segments, such as "/review-queue", with no trailing ` +
+ `slash and no parameters.`,
+ );
+ }
+
+ if (!declaredPaths.includes(page.path)) {
throw new Error(
- `Page slug "${page?.slug ?? '<missing>'}" must be lowercase letters, ` +
- `digits and single hyphens. A slug names a page within an extension; ` +
- `the host supplies the rest of the URL.`,
+ `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)'}.`,
);
}
- const id = `${extensionId}.${page.slug}`;
- const resolved: ResolvedPage = {
- ...page,
- extensionId,
- path: pagePath(extensionId, page.slug),
- };
+ const id = `${extensionId}:${page.path}`;
+ const resolved: ResolvedPage = { ...page, extensionId };
const replaced = pageRegistry.get(id);
if (replaced) {
- // Within one extension only -- ids are namespaced, so this can never be
- // another extension's page. Silent replacement would simply make the
- // first registration stop rendering with nothing said about it.
+ // Within one extension only -- ids carry the extension, and the server
+ // has already refused a path two extensions both declare. Silent
+ // replacement would make the first registration stop rendering with
+ // nothing said about it.
logging.warn(
`Page "${id}" is already registered; it has been replaced and the ` +
`earlier component no longer renders.`,
diff --git a/superset-frontend/src/extensions/ExtensionsLoader.ts b/superset-frontend/src/extensions/ExtensionsLoader.ts
index 529ac0e9261..6ede4892008 100644
--- a/superset-frontend/src/extensions/ExtensionsLoader.ts
+++ b/superset-frontend/src/extensions/ExtensionsLoader.ts
@@ -18,9 +18,11 @@
*/
import { SupersetClient } from '@superset-ui/core';
import { logging } from '@apache-superset/core/utils';
-import type { common as core } from '@apache-superset/core';
+import type {
+ common as core,
+ pages as pagesApi,
+} from '@apache-superset/core';
import type { ComponentType } from 'react';
-import type { pages as pagesApi } from '@apache-superset/core';
import { registerPageFor } from 'src/core/pages';
import { makeUrl } from 'src/utils/navigationUtils';
import 'src/extensions/Namespaces';
@@ -39,6 +41,16 @@ export interface LoadedExtension extends Extension {
remoteEntry?: string;
/** Webpack Module Federation container name (maps to window[name]). */
moduleFederationName?: string;
+ /**
+ * What the extension's manifest declares it contributes.
+ *
+ * The server has already acted on this -- a URL rule per page, refused if
+ * the path is taken -- so it is sent here as the authoritative list of
+ * paths the extension is allowed to register a component against.
+ */
+ contributions?: {
+ pages?: { path: string }[];
+ };
}
/**
@@ -214,22 +226,26 @@ 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 scopedCore = {
...window.superset,
extensions: {
...window.superset.extensions,
getContext: () => context,
},
- // `registerPage` is bound to this extension's id here, for the same
- // reason the backend derives an extension's URL namespace from an
- // ambient context rather than an argument: an extension states which
- // page it is contributing, never whose. Because module federation
- // resolves @apache-superset/core once per container, every import
- // inside this extension receives this bound instance.
+ // `registerPage` is bound to this extension's id and to the paths its
+ // manifest declares, for the same reason the backend derives an
+ // extension's API namespace from an ambient context rather than an
+ // argument: an extension states which page it is contributing, never
+ // whose. Because module federation resolves @apache-superset/core once
+ // per container, every import inside this extension receives this
+ // bound instance.
pages: {
...window.superset.pages,
registerPage: (page: pagesApi.Page, component: ComponentType) =>
- registerPageFor(extension.id, page, component),
+ registerPageFor(extension.id, declaredPagePaths, page, component),
},
};
// Reuse the version key webpack already resolved for the host's own
diff --git a/superset-frontend/src/pages/ExtensionPage/index.tsx b/superset-frontend/src/pages/ExtensionPage/index.tsx
index 13e45e0e813..8d6e060f8c9 100644
--- a/superset-frontend/src/pages/ExtensionPage/index.tsx
+++ b/superset-frontend/src/pages/ExtensionPage/index.tsx
@@ -29,12 +29,11 @@ import { ErrorAlert } from 'src/components/ErrorMessage/ErrorAlert';
/**
* Renders whichever extension page claims the current URL.
*
- * The server serves the application shell for every path under the extension
- * namespace without knowing which pages exist -- that is what lets one route
- * cover every extension. The consequence is that deciding a URL is genuinely
- * unclaimed happens here, and it can only be decided once extensions have
- * finished loading. Until then this must not say "not found", because the
- * page may be moments away.
+ * Mounted on a catch-all route that every host route outranks, so this is
+ * reached only for a path the host itself does not serve. Whether an
+ * extension serves it can only be decided once extensions have finished
+ * loading, so until then this must not say "not found": the page may be
+ * moments away.
*/
export default function ExtensionPage() {
const { pathname } = useLocation();
@@ -63,7 +62,7 @@ export default function ExtensionPage() {
const page = findPageByPath(stripAppRoot(pathname));
if (page) {
- return resolvePage(`${page.extensionId}.${page.slug}`);
+ return resolvePage(`${page.extensionId}:${page.path}`);
}
if (!extensionsSettled) {
diff --git a/superset-frontend/src/views/routes.tsx b/superset-frontend/src/views/routes.tsx
index dbf3f0e1afb..ca65545d692 100644
--- a/superset-frontend/src/views/routes.tsx
+++ b/superset-frontend/src/views/routes.tsx
@@ -318,27 +318,29 @@ if (isAdmin) {
}
}
+if (authRegistrationEnabled) {
+ routes.push({ path: RoutePaths.REGISTRATIONS, Component: UserRegistrations });
+}
+
if (isFeatureEnabled(FeatureFlag.EnableExtensions)) {
- // One route for every page every extension contributes, mirroring the
- // single server-side rule. `:extensionId` is matched loosely here and
- // validated by the server; what actually decides whether a path is served
- // is whether an extension registered a page for it.
+ // Pushed last, and deliberately a catch-all rather than one route per
+ // page. `<Switch>` takes the first match in array order, so every route
+ // the host ships wins over this one -- the same precedence Werkzeug gives
+ // the server's static rules over an extension's, arrived at a different
+ // way. An extension therefore cannot shadow a host screen here either,
+ // whatever path it declared.
+ //
+ // The route table is built once at module evaluation, while extensions
+ // load afterwards, so enumerating pages here is not possible; `/:path+`
+ // defers the question to `ExtensionPage`, which resolves it against the
+ // registry once extensions have settled.
+ //
+ // `+` requires at least one segment, which keeps `/` free for the
+ // `<Redirect to="/welcome/">` that follows the generated routes.
routes.push({
- path: '/extensions/:extensionId/:slug',
+ path: '/:extensionPagePath+',
Component: ExtensionPage,
});
- // The server serves the shell for an extension id with no slug too, so
- // without this a trimmed or truncated URL renders an empty body rather
- // than the "page not found" the two-segment route gives. Registered after
- // it, so a real page still wins.
- routes.push({
- path: '/extensions/:extensionId',
- Component: ExtensionPage,
- });
-}
-
-if (authRegistrationEnabled) {
- routes.push({ path: RoutePaths.REGISTRATIONS, Component: UserRegistrations });
}
const frontEndRoutes: Record<string, boolean> = routes
diff --git a/superset/extensions/utils.py b/superset/extensions/utils.py
index 82f7b475d7f..73d2a0181de 100644
--- a/superset/extensions/utils.py
+++ b/superset/extensions/utils.py
@@ -240,7 +240,10 @@ def get_loaded_extension(
def build_extension_data(extension: LoadedExtension) -> dict[str, Any]:
+ from superset.extensions.view import registered_page_paths
+
manifest = extension.manifest
+ registered = set(registered_page_paths(current_app, manifest.id))
extension_data: dict[str, Any] = {
"id": manifest.id,
"publisher": manifest.publisher,
@@ -248,6 +251,18 @@ def build_extension_data(extension: LoadedExtension) -> dict[str, Any]:
"version": extension.version,
"description": manifest.description or "",
"dependencies": manifest.dependencies,
+ # Only the pages that actually got a URL rule. A page whose path was
+ # already taken was refused at boot and is filtered out here, so the
+ # frontend never offers a menu entry -- or accepts a component -- for
+ # a page that would 404 on a cold load.
+ "contributions": {
+ **manifest.contributions.model_dump(exclude_none=True),
+ "pages": [
+ page.model_dump(exclude_none=True)
+ for page in manifest.contributions.pages
+ if page.path in registered
+ ],
+ },
}
if manifest.frontend:
frontend = manifest.frontend
diff --git a/superset/extensions/view.py b/superset/extensions/view.py
index 0b72b73f2ca..0b481c1b607 100644
--- a/superset/extensions/view.py
+++ b/superset/extensions/view.py
@@ -14,20 +14,27 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
-from flask import abort, g
+import logging
+
+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.constants import (
- PUBLISHER_PATTERN,
- TECHNICAL_NAME_PATTERN,
-)
+from superset_core.extensions.types import Manifest
+from werkzeug.exceptions import MethodNotAllowed, NotFound
+from werkzeug.routing import RequestRedirect
from superset.constants import MODEL_VIEW_RW_METHOD_PERMISSION_MAP
from superset.superset_typing import FlaskResponse
from superset.utils.core import get_user_id
-from superset.views.base import BaseSupersetView
+from superset.views.base import BaseSupersetView, get_spa_template_context
from superset.views.utils import redirect_to_login
+logger = logging.getLogger(__name__)
+
+#: Prefix for the Flask endpoints these rules register under, so a page is
+#: identifiable in the URL map and cannot collide with a view's endpoint name.
+_ENDPOINT_PREFIX = "extension_page"
+
class ExtensionsView(BaseSupersetView):
route_base = "/extensions"
@@ -41,61 +48,132 @@ class ExtensionsView(BaseSupersetView):
return super().render_app_template()
-#: An extension's composite id (``publisher.name``) as one path segment, built
-#: from the same patterns the manifest validates against, minus their anchors.
-#: A dot cannot appear in either half, which is what keeps this rule from ever
-#: matching the ``/extensions/<publisher>/<name>/...`` routes that
-#: extension-contributed REST APIs register -- there the publisher is a literal
-#: segment of its own.
-EXTENSION_ID_REGEX = (
- f"{PUBLISHER_PATTERN.strip('^$')}\\.{TECHNICAL_NAME_PATTERN.strip('^$')}"
-)
+def _render_page(**_kwargs: str) -> FlaskResponse:
+ """Render the application shell for a contributed page.
+
+ ``_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.
+
+ 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.
+ """
+ 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()
+ return render_template("superset/spa.html", **get_spa_template_context())
-class ExtensionPagesView(BaseSupersetView):
- """Serves the application shell for every page an extension contributes.
+def _first_claimant(app: Flask, path: str) -> str | None:
+ """The endpoint already answering ``path`` or anything under it.
- One rule covers every extension and every page inside it. The alternative
- -- a Flask view per declared page -- ties the URL map to whichever
- extensions happen to be on disk, so two workers with different filesystem
- state answer differently, and it needs collision detection and path
- validation for author-supplied paths that this does not.
+ Two checks, because they catch different things and neither subsumes the
+ other.
- ``rest`` is never read. It reaches the server only so the URL resolves; the
- 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.
+ Matching ``path`` against the live URL map catches a rule that answers it
+ without resembling it. A literal comparison is what let an earlier
+ revision of this code wave through ``/oauth-authorized/google/``: the rule
+ is registered as ``/oauth-authorized/<provider>`` and never string-matches
+ the request it would answer. A redirect counts as taken --
+ ``strict_slashes`` raises ``RequestRedirect`` for a rule that exists under
+ a different trailing slash -- and so does a method mismatch, since a rule
+ answering only POST still owns the path.
- Deliberately no ``class_permission_name`` and no ``@has_access``: this is a
- separate class from ``ExtensionsView`` precisely so its ``Extensions``
- permission -- which is admin-only -- cannot reach these routes.
+ Scanning rule templates for the same prefix catches the opposite case: a
+ page takes everything beneath its path through its ``<path:rest>`` rule,
+ so a page at ``/report`` would shadow a core ``/report/list``. Matching
+ cannot find that one -- there is no single URL to probe that a static
+ descendant is guaranteed to answer -- so the templates are read directly.
"""
+ adapter = app.url_map.bind("localhost")
+ try:
+ endpoint, _args = adapter.match(path, method="GET")
+ except NotFound:
+ pass
+ except RequestRedirect as redirect:
+ return f"a rule redirecting to {redirect.new_url}"
+ except MethodNotAllowed:
+ return "a rule accepting other methods"
+ else:
+ return endpoint
- route_base = ""
- # FAB links a menu entry to `default_view` and assumes "list" when unset.
- # These routes carry no menu entry, but the attribute is set so adding one
- # later cannot silently break every page that renders the menu.
- default_view = "page"
-
- @expose(f'/extensions/<regex("{EXTENSION_ID_REGEX}"):extension_id>/')
- @expose(f'/extensions/<regex("{EXTENSION_ID_REGEX}"):extension_id>/<path:rest>')
- def page(self, extension_id: str, rest: str = "") -> FlaskResponse:
- """Render the shell, or 404 when extensions are switched off.
-
- Guarded by a login check rather than a permission, matching
- ``Superset.welcome``: the payload this returns 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.
- """
- # Imported here rather than at module scope: the flag is read per
- # request, and the route is registered unconditionally so that the URL
- # map does not vary with configuration.
- 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()
- return super().render_app_template()
+ prefix = f"{path}/"
+ for rule in app.url_map.iter_rules():
+ if rule.rule.startswith(prefix):
+ return f"{rule.endpoint} (at {rule.rule}, which this page would shadow)"
+
+ return None
+
+
+def register_extension_pages(app: Flask, manifest: Manifest) -> None:
+ """Register one URL rule per page an extension declares.
+
+ Called once per extension during initialisation, after operator
+ blueprints, the FAB auth views and every core view are already in the URL
+ map -- so the check below sees the complete set of routes this deployment
+ actually serves, not a list of the ones that existed when the extension
+ was written.
+
+ Re-running it on every boot is the point. A route Superset only registers
+ under some configurations -- ``/oauth-authorized/<provider>`` exists only
+ when ``AUTH_TYPE`` is ``AUTH_OAUTH`` -- is absent from the map in a
+ deployment that has the feature switched off, so a page may legitimately
+ take that path there. Turning the feature on later puts the real rule in
+ the map first, and the next boot refuses the page loudly instead of
+ silently leaving the extension in front of the callback.
+
+ A refused page is skipped, not fatal: one extension claiming a taken path
+ should not stop the server from starting.
+ """
+ for page in manifest.contributions.pages:
+ if claimed := _first_claimant(app, page.path):
+ logger.error(
+ "Extension '%s' declares a page at '%s', which is already "
+ "served by %s. The page has not been registered and will not "
+ "load. Give it a different path in its manifest.",
+ manifest.id,
+ page.path,
+ claimed,
+ )
+ continue
+
+ endpoint = f"{_ENDPOINT_PREFIX}.{manifest.id}{page.path}"
+ app.add_url_rule(
+ page.path,
+ endpoint=endpoint,
+ view_func=_render_page,
+ methods=["GET"],
+ )
+ app.add_url_rule(
+ f"{page.path}/<path:rest>",
+ endpoint=f"{endpoint}/*",
+ view_func=_render_page,
+ methods=["GET"],
+ )
+ logger.info("Extension '%s' serves a page at '%s'", manifest.id, page.path)
+
+
+def registered_page_paths(app: Flask, extension_id: str) -> list[str]:
+ """The paths this extension actually has a URL rule for.
+
+ Read back from the URL map rather than remembered from registration, so
+ there is one answer to "does this page exist" and it is the routing
+ table's. A page the check above refused has no rule, and so is absent
+ here -- which is what keeps the frontend from offering a menu entry, or
+ accepting a component, for a page that would 404 on a cold load.
+ """
+ prefix = f"{_ENDPOINT_PREFIX}.{extension_id}"
+ return [
+ rule.rule
+ for rule in app.url_map.iter_rules()
+ if rule.endpoint.startswith(prefix) and not rule.endpoint.endswith("/*")
+ ]
diff --git a/superset/initialization/__init__.py b/superset/initialization/__init__.py
index 8a58370e069..f6a0ca2746b 100644
--- a/superset/initialization/__init__.py
+++ b/superset/initialization/__init__.py
@@ -219,7 +219,7 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods
from superset.explore.api import ExploreRestApi
from superset.explore.form_data.api import ExploreFormDataRestApi
from superset.explore.permalink.api import ExplorePermalinkRestApi
- from superset.extensions.view import ExtensionPagesView, ExtensionsView
+ from superset.extensions.view import ExtensionsView
from superset.importexport.api import ImportExportRestApi
from superset.queries.api import QueryRestApi
from superset.queries.saved_queries.api import SavedQueryRestApi
@@ -481,12 +481,6 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods
),
)
- # Registered unconditionally, and answering 404 when the feature flag
- # is off, so that the URL map does not vary with configuration -- two
- # workers reading different config would otherwise disagree about
- # whether a URL exists.
- appbuilder.add_view_no_menu(ExtensionPagesView)
-
appbuilder.add_view(
TaskModelView,
"Tasks",
@@ -663,6 +657,7 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods
get_extensions,
install_in_memory_importer,
)
+ from superset.extensions.view import register_extension_pages
try:
extensions = get_extensions()
@@ -673,6 +668,11 @@ class SupersetAppInitializer: # pylint: disable=too-many-public-methods
return
for extension in extensions.values():
+ # Before the backend entrypoint runs: a page is declared in the
+ # 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)
+
if backend_files := extension.backend:
install_in_memory_importer(
backend_files,
diff --git a/tests/unit_tests/extensions/test_pages_catchall.py b/tests/unit_tests/extensions/test_pages_catchall.py
deleted file mode 100644
index 6b5165b39c3..00000000000
--- a/tests/unit_tests/extensions/test_pages_catchall.py
+++ /dev/null
@@ -1,141 +0,0 @@
-# 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.
-"""The one route that serves every extension-contributed page."""
-
-import importlib
-import os
-
-import pytest
-
-
-def _build_app(enable_extensions: bool = True):
- from superset.app import SupersetApp
- from superset.extensions import appbuilder
- from superset.initialization import SupersetAppInitializer
-
- # "superset", not __name__: Flask resolves its template folder from the
- # import name, so a test module's name makes `superset/spa.html`
- # unfindable and the shell renders as a 500.
- app = SupersetApp("superset")
- app.config.from_object("superset.config")
- app.config["SQLALCHEMY_DATABASE_URI"] = (
- os.environ.get("SUPERSET__SQLALCHEMY_DATABASE_URI") or "sqlite://"
- )
- app.config["WTF_CSRF_ENABLED"] = False
- app.config["TESTING"] = True
- app.config["RATELIMIT_ENABLED"] = False
- app.config["CACHE_CONFIG"] = {"CACHE_TYPE": "NullCache"}
- app.config["DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "NullCache"}
- # These default to `SupersetMetastoreCache`, which this test does not
- # exercise and which needs a metadata database to construct.
- app.config["FILTER_STATE_CACHE_CONFIG"] = {"CACHE_TYPE": "NullCache"}
- app.config["EXPLORE_FORM_DATA_CACHE_CONFIG"] = {"CACHE_TYPE": "NullCache"}
- app.config["SERVER_NAME"] = "example.com"
- app.config["APPLICATION_ROOT"] = "/"
- app.config["STATIC_ASSETS_PREFIX"] = ""
- app.config["FEATURE_FLAGS"] = {
- **app.config.get("FEATURE_FLAGS", {}),
- "ENABLE_EXTENSIONS": enable_extensions,
- }
- appbuilder.baseviews = []
- SupersetAppInitializer(app).init_app()
- with app.app_context():
- import superset.views.base
-
- importlib.reload(superset.views.base)
- return app
-
-
-@pytest.fixture(scope="module")
-def app_with_pages():
- app = _build_app()
- yield app
- from superset.extensions import appbuilder
-
- appbuilder.baseviews = []
-
-
-def test_the_route_exists_without_any_extension_installed(app_with_pages) -> None:
- """The whole point of one rule: it does not depend on what is on disk.
-
- A per-page route would tie the URL map to the filesystem, so two workers
- with different state would answer the same URL differently.
- """
- rules = {str(rule) for rule in app_with_pages.url_map.iter_rules()}
- assert any("extension_id" in rule and "rest" in rule for rule in rules), rules
-
-
-def test_it_renders_the_shell_for_any_path_under_an_extension_id(
- app_with_pages, full_api_access
-) -> None:
- client = app_with_pages.test_client()
- for path in (
- "/extensions/acme.asset-approval/",
- "/extensions/acme.asset-approval/queue/",
- "/extensions/acme.asset-approval/queue/42/edit/",
- ):
- response = client.get(path)
- body = response.get_data(as_text=True)
- assert response.status_code == 200, (path, body[:300])
- assert 'id="app"' in body, path
- assert "data-bootstrap" in body, path
-
-
-@pytest.mark.parametrize(
- "path",
- [
- # Not a dotted composite id, so the rule must not match at all.
- "/extensions/acme/",
- "/extensions/acme/queue/",
- # The host's own extensions screen keeps its route.
- "/extensions/list/",
- # Uppercase and spaces are outside the identifier patterns.
- "/extensions/Acme.Tools/queue/",
- ],
-)
-def test_it_does_not_match_anything_that_is_not_an_extension_id(
- app_with_pages, full_api_access, path: str
-) -> None:
- """The dotted id is what keeps this rule out of the extension API namespace.
-
- `/extensions/<publisher>/<name>/...` already belongs to extension-contributed
- REST APIs, where the publisher is a literal segment that cannot contain a dot.
- """
- response = app_with_pages.test_client().get(path)
- assert response.status_code != 200 or 'id="app"' not in response.get_data(
- as_text=True
- ), f"{path} was served the shell"
-
-
-def test_an_unauthenticated_load_is_sent_to_login_not_404(app_with_pages) -> None:
- response = app_with_pages.test_client().get(
- "/extensions/acme.asset-approval/queue/", follow_redirects=False
- )
- assert response.status_code in (301, 302, 308), response.status_code
- assert "/login" in response.headers.get("Location", "")
-
-
-def test_it_is_404_when_extensions_are_switched_off(full_api_access) -> None:
- """The route is always registered; the flag decides what it answers."""
- app = _build_app(enable_extensions=False)
- try:
- response = app.test_client().get("/extensions/acme.asset-approval/queue/")
- assert response.status_code == 404
- finally:
- from superset.extensions import appbuilder
-
- appbuilder.baseviews = []
diff --git a/tests/unit_tests/extensions/test_pages_view.py b/tests/unit_tests/extensions/test_pages_view.py
new file mode 100644
index 00000000000..860169ea884
--- /dev/null
+++ b/tests/unit_tests/extensions/test_pages_view.py
@@ -0,0 +1,256 @@
+# 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.
+
+"""Page-path registration and the collision check that guards it.
+
+Exercised against a bare Flask app rather than the Superset one: the thing
+under test is how a declared path interacts with the URL map, and a bare app
+lets each case state exactly which routes exist before the extension loads --
+which is the whole question.
+"""
+
+import logging
+
+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 superset.extensions import view
+from superset.extensions.view import (
+ _first_claimant,
+ register_extension_pages,
+ registered_page_paths,
+)
+
+
+def manifest(*paths: str, extension_id: str = "acme.tools") -> 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]
+ ),
+ )
+
+
+def paths_in(app: Flask) -> set[str]:
+ return {rule.rule for rule in app.url_map.iter_rules()}
+
+
+@pytest.fixture
+def app() -> Flask:
+ return Flask(__name__)
+
+
+def test_a_free_path_is_registered(app: Flask) -> None:
+ register_extension_pages(app, manifest("/approvals"))
+
+ assert "/approvals" in paths_in(app)
+ # The page owns everything beneath it, so a deep link resolves on a cold
+ # load instead of 404ing before the client router ever runs.
+ assert "/approvals/<path:rest>" in paths_in(app)
+
+
+def test_a_path_a_core_route_already_serves_is_refused(
+ app: Flask, caplog: pytest.LogCaptureFixture
+) -> None:
+ app.add_url_rule("/welcome", endpoint="welcome", view_func=lambda: "")
+
+ with caplog.at_level(logging.ERROR):
+ register_extension_pages(app, manifest("/welcome"))
+
+ assert "/welcome/<path:rest>" not in paths_in(app)
+ assert "already served by welcome" in caplog.text
+
+
+def test_a_parameterised_rule_is_recognised_as_the_claimant(
+ app: Flask, caplog: pytest.LogCaptureFixture
+) -> None:
+ """The case a literal comparison against known routes gets wrong.
+
+ ``/oauth-authorized/<provider>`` never string-matches the concrete path
+ it would answer, so a denylist of route templates waves this through --
+ which is how an extension could end up in front of an OAuth callback.
+ """
+ app.add_url_rule(
+ "/oauth-authorized/<provider>", endpoint="oauth", view_func=lambda provider: ""
+ )
+
+ with caplog.at_level(logging.ERROR):
+ register_extension_pages(app, manifest("/oauth-authorized/google"))
+
+ assert "/oauth-authorized/google" not in paths_in(app)
+ assert "already served by oauth" in caplog.text
+
+
+def test_a_path_whose_descendants_are_taken_is_refused(
+ app: Flask, caplog: pytest.LogCaptureFixture
+) -> None:
+ """A page at ``/x`` would shadow ``/x/list`` through its sub-path rule."""
+ app.add_url_rule("/report/list", endpoint="report_list", view_func=lambda: "")
+
+ with caplog.at_level(logging.ERROR):
+ register_extension_pages(app, manifest("/report"))
+
+ assert "/report" not in paths_in(app)
+
+
+def test_a_rule_answering_other_methods_still_owns_the_path(app: Flask) -> None:
+ app.add_url_rule(
+ "/submit", endpoint="submit", view_func=lambda: "", methods=["POST"]
+ )
+
+ register_extension_pages(app, manifest("/submit"))
+
+ assert "/submit/<path:rest>" not in paths_in(app)
+
+
+def test_the_second_extension_to_claim_a_path_is_refused(
+ app: Flask, caplog: pytest.LogCaptureFixture
+) -> None:
+ register_extension_pages(app, manifest("/approvals"))
+
+ with caplog.at_level(logging.ERROR):
+ register_extension_pages(app, manifest("/approvals", extension_id="other.ext"))
+
+ assert "other.ext" in caplog.text
+ # The first extension keeps the path; the loser is told, not silently
+ # left with a component nothing routes to.
+ assert len([r for r in app.url_map.iter_rules() if r.rule == "/approvals"]) == 1
+
+
+def test_one_refused_page_does_not_stop_the_others(
+ app: Flask, caplog: pytest.LogCaptureFixture
+) -> None:
+ app.add_url_rule("/welcome", endpoint="welcome", view_func=lambda: "")
+
+ with caplog.at_level(logging.ERROR):
+ register_extension_pages(app, manifest("/welcome", "/approvals"))
+
+ assert "/approvals" in paths_in(app)
+
+
+def test_an_extension_declaring_no_pages_registers_nothing(app: Flask) -> None:
+ before = paths_in(app)
+
+ register_extension_pages(app, manifest())
+
+ assert paths_in(app) == before
+
+
+@pytest.mark.parametrize(
+ "path",
+ [
+ "review-queue",
+ "/review-queue/",
+ "/Review-Queue",
+ "",
+ "/review-queue/:id",
+ "/-queue",
+ "/a//b",
+ ],
+)
+def test_a_malformed_path_is_rejected_by_the_manifest(path: str) -> None:
+ """Rejected at parse time, so a bad path never reaches the URL map."""
+ with pytest.raises(ValidationError, match="should match pattern"):
+ Page(path=path, name="X")
+
+
+def test_first_claimant_reports_a_redirect_as_taken(app: Flask) -> None:
+ """``strict_slashes`` means ``/thing/`` already answers for ``/thing``."""
+ app.add_url_rule("/thing/", endpoint="thing", view_func=lambda: "")
+
+ assert _first_claimant(app, "/thing") is not None
+
+
+def test_registered_page_paths_reports_only_what_was_accepted(app: Flask) -> None:
+ app.add_url_rule("/welcome", endpoint="welcome", view_func=lambda: "")
+
+ register_extension_pages(app, manifest("/welcome", "/approvals"))
+
+ # The refused page is absent, so nothing downstream -- the menu, the
+ # frontend's check on what an extension may register -- can offer a page
+ # that has no route behind it.
+ assert registered_page_paths(app, "acme.tools") == ["/approvals"]
+
+
+def test_registered_page_paths_does_not_leak_across_extensions(app: Flask) -> None:
+ register_extension_pages(app, manifest("/approvals"))
+ register_extension_pages(app, manifest("/change-review", extension_id="other.ext"))
+
+ assert registered_page_paths(app, "acme.tools") == ["/approvals"]
+ assert registered_page_paths(app, "other.ext") == ["/change-review"]
+
+
+def render_with(
+ monkeypatch: pytest.MonkeyPatch,
+ app: Flask,
+ *,
+ extensions_enabled: bool = True,
+ user_id: int | None = 1,
+) -> object:
+ """Run ``_render_page``'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
+
+ 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, "redirect_to_login", lambda: "LOGIN")
+
+ register_extension_pages(app, manifest("/approvals"))
+
+ with app.test_request_context("/approvals"):
+ g.user = object() if user_id else None
+ return view._render_page()
+
+
+def test_the_flag_decides_what_a_registered_page_answers(
+ monkeypatch: pytest.MonkeyPatch, app: Flask
+) -> None:
+ """Read per request, so turning extensions off takes effect immediately.
+
+ The rule stays in the URL map until the process restarts; the flag is
+ what stops it serving, rather than leaving pages reachable in the window
+ between flipping the flag and a restart.
+ """
+ with pytest.raises(NotFound):
+ render_with(monkeypatch, app, extensions_enabled=False)
+
+
+def test_an_anonymous_load_is_sent_to_login_not_404(
+ monkeypatch: pytest.MonkeyPatch, app: Flask
+) -> None:
+ # 404 here would tell an anonymous visitor the page does not exist, and
+ # send them nowhere useful; every other host screen redirects.
+ assert render_with(monkeypatch, app, user_id=None) == "LOGIN"
+
+
+def test_a_signed_in_load_gets_the_shell(
+ monkeypatch: pytest.MonkeyPatch, app: Flask
+) -> None:
+ assert render_with(monkeypatch, app) == "SHELL"