geido commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4351300746
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -199,6 +200,108 @@ const StyledImage = styled(Image)`
const { useBreakpoint } = Grid;
+/**
+ * What the two helpers below need of a contributed page, and no more, so the
+ * menu does not take a dependency on the pages registry. One shape shared by
+ * both, so a field one of them reads cannot go missing from the other.
+ */
+type ContributedPage = {
+ name: string;
+ path: string;
+ nav?: { label?: string; category?: string };
+ permitted?: boolean;
+};
+
+/**
+ * Folds pages contributed by extensions into the server-rendered menu.
+ *
+ * The navbar is built on the server from FAB's menu, which an extension
+ * cannot write to: an entry there needs a `menu_access` permission created
+ * at boot, and extensions register in the browser. A page that opted in with
+ * `nav` is therefore added here, client-side, so that a contributed page has
+ * somewhere a user could find it at all.
+ *
+ * A page without `nav.category` sits at the top level. One that names a
+ * category goes into that group of the Settings menu instead, which is
+ * where an administrative screen belongs -- see `withExtensionSettings`.
+ */
+export function withExtensionPages(
+ menu: MenuObjectProps[],
+ pages: ContributedPage[],
+): MenuObjectProps[] {
+ // 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.nav.category && page.permitted !== false,
+ );
+ if (!contributed.length) {
+ return menu;
+ }
+
+ return [
Review Comment:
There's no limit on contributed top-level entries, and the main nav is
rendered with `disabledOverflow`, so there's no "…" fallback. Once the entries
don't fit, the nav wraps under the logo and covers the page header and list
toolbars. With the labels "Review queue / Approvals / Nested page / Plus page",
3 entries already wrap at 1280px and 4 wrap at 1366–1440px. They fit at 1536px.
The order is also non-deterministic. Entries are appended in registration
order and bundles load in parallel, so delaying one extension's `remoteEntry`
by 1.5s moves its entries ahead of the other extension's. SIP-235 adds an
`order` field. Either way, this needs some cap or overflow handling.
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -199,6 +200,108 @@ const StyledImage = styled(Image)`
const { useBreakpoint } = Grid;
+/**
+ * What the two helpers below need of a contributed page, and no more, so the
+ * menu does not take a dependency on the pages registry. One shape shared by
+ * both, so a field one of them reads cannot go missing from the other.
+ */
+type ContributedPage = {
+ name: string;
+ path: string;
+ nav?: { label?: string; category?: string };
+ permitted?: boolean;
+};
+
+/**
+ * Folds pages contributed by extensions into the server-rendered menu.
+ *
+ * The navbar is built on the server from FAB's menu, which an extension
+ * cannot write to: an entry there needs a `menu_access` permission created
+ * at boot, and extensions register in the browser. A page that opted in with
+ * `nav` is therefore added here, client-side, so that a contributed page has
+ * somewhere a user could find it at all.
+ *
+ * A page without `nav.category` sits at the top level. One that names a
+ * category goes into that group of the Settings menu instead, which is
+ * where an administrative screen belongs -- see `withExtensionSettings`.
+ */
+export function withExtensionPages(
+ menu: MenuObjectProps[],
+ pages: ContributedPage[],
+): MenuObjectProps[] {
+ // 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.nav.category && page.permitted !== false,
+ );
+ if (!contributed.length) {
+ return menu;
+ }
+
+ return [
+ ...menu,
+ ...contributed.map(
+ page =>
+ ({
+ label: page.nav?.label ?? page.name,
+ // `name` is the React key for the entry, and the label is the
+ // extension author's to choose -- a page labelled "Charts" would
+ // collide with the host's own entry. The path cannot collide,
+ // because the server refused any page that shared one.
+ name: `extension:${page.path}`,
+ url: page.path,
+ }) as unknown as MenuObjectProps,
+ ),
+ ];
+}
+
+/**
+ * Places a contributed page inside a group of the Settings menu.
+ *
+ * `MenuWrapper` splits the FAB menu into the top navbar and `settings` by
+ * group name -- Data, Security and Manage -- so a page that belongs under
+ * Settings cannot be appended to the navbar list and reach it. It has to
+ * join the group's own children, which is what this does.
+ *
+ * A group the host did not render is not invented: FAB builds Manage only
+ * when the user may see something in it, and a group conjured here would be
+ * one the rest of Superset does not agree exists. The page keeps its URL
+ * either way; it is the entry that is missing.
+ */
+export function withExtensionSettings(
+ settings: MenuObjectProps[],
+ pages: ContributedPage[],
+): MenuObjectProps[] {
+ const contributed = pages.filter(
+ page => page.nav?.category && page.permitted !== false,
+ );
+ if (!contributed.length) {
+ return settings;
+ }
+
+ return settings.map(group => {
+ const mine = contributed.filter(page => page.nav?.category === group.name);
+ if (!mine.length) {
+ return group;
+ }
+ return {
+ ...group,
+ childs: [
+ ...(group.childs ?? []),
+ ...mine.map(page => ({
+ label: page.nav?.label ?? page.name,
+ // The React key, as for the navbar entries: the label is the
+ // author's to choose and could collide with a host item.
+ name: `extension:${page.path}`,
Review Comment:
`RightMenu`'s `buildSettingsMenuItems` keys settings children by
`child.label`, not `name`, so this doesn't prevent collisions there. A page
with `nav: { label: 'Tags', category: 'Manage' }` triggers React's "Encountered
two children with the same key, `Tags`" warning (I checked with a dev build).
Either key by `name` in `RightMenu`, or drop the claim from the comment.
##########
superset/extensions/view.py:
##########
@@ -32,3 +51,544 @@ class ExtensionsView(BaseSupersetView):
@permission_name("read")
def list(self) -> FlaskResponse:
return super().render_app_template()
+
+
+def _render_shell() -> FlaskResponse:
+ """The application shell, identical to the one ``/welcome/`` serves."""
+ return render_template("superset/spa.html", **get_spa_template_context())
+
+
+def _view_menu(extension_id: str) -> str:
+ """The FAB view menu an extension's page permissions hang from.
+
+ Prefixed so the security manager can tell these apart from a core view's
+ permissions without keeping a registry of installed extensions. See
+ ``SupersetSecurityManager._is_admin_only``.
+ """
+ return f"{EXTENSION_VIEW_MENU_PREFIX}{extension_id}"
+
+
+def may_open(extension_id: str, permission: str | None) -> bool:
+ """Whether the current user may open a page with this permission.
+
+ 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.
+ """
+ if permission is None:
+ return True
+ from superset import security_manager
+
+ return security_manager.can_access(permission, _view_menu(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)
Review Comment:
On a cold load, this 403 goes through `show_http_exception`, which only
serves an HTML page for 404/500. The user gets a raw JSON body with a
misleading issue code:
`{"errors": [{"message": "403 Forbidden: ...", "error_type":
"GENERIC_BACKEND_ERROR", ... "Issue 1011 - Superset encountered an unexpected
error."}]}`
`ExtensionPage` already shows a clean "Not authorized" state when the user
arrives in-app. Rendering the shell with a 403 status (e.g. `return
_render_shell(), 403`) would give the same result on a refresh or a bookmark.
##########
superset/extensions/utils.py:
##########
@@ -240,14 +240,39 @@ def get_loaded_extension(
def build_extension_data(extension: LoadedExtension) -> dict[str, Any]:
+ 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,
"name": extension.name,
"version": extension.version,
"description": manifest.description or "",
"dependencies": manifest.dependencies,
+ # Only the pages that actually got a URL rule. A page whose path was
Review Comment:
Nit: this comment says refused pages are "filtered out here", but the code
(and the comment just below) reports every declared page with
`registered`/`permitted`. The PR description repeats the first version
("reports only pages that actually got a rule").
##########
superset-frontend/packages/superset-core/src/pages/index.ts:
##########
@@ -0,0 +1,197 @@
+/**
+ * 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.
+ */
+
+/**
+ * @fileoverview Pages API for Superset extensions.
+ *
+ * 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.
+ *
+ * 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({ path: '/review-queue', name: 'Queue' }, ReviewQueue);
+ * ```
+ */
+
+import { ComponentType } from 'react';
+import { Disposable, Event } from '../common';
+
+/**
+ * A page contributed by an extension.
+ */
+export interface Page {
+ /**
+ * 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.
+ *
+ * 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.
+ */
+ path: string;
+ /** The display name of the page. */
+ name: string;
+ /** Optional description, for display in contribution manifests. */
+ description?: string;
+ /**
+ * Opt the page into Superset's navigation bar.
+ *
+ * Without this a page is reachable only by its URL, which is rarely what
+ * an extension wants: there is otherwise nowhere in Superset a user could
+ * discover it. Omit it for a page reached from somewhere the extension
+ * renders itself, such as a button on one of its own screens.
+ */
+ nav?: {
+ /** Defaults to the page's `name`. */
+ label?: string;
+ /**
+ * A group of the Settings menu to sit in, such as `Manage`, rather than
+ * the top-level navigation bar.
+ *
+ * Use it for a screen an administrator goes looking for rather than one
+ * a user works in daily. The group has to be one Superset already
+ * renders: it builds Manage only when the reader may see something in
+ * it, and an entry cannot create a group of its own.
+ */
+ category?: 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;
+}
+
+/**
+ * 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;
+ /**
+ * 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;
+}
+
+/**
+ * Registers a page, mounted by the host as a client-side route.
+ *
+ * The component renders inside the host's own chrome, under an error
+ * boundary.
+ *
+ * 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. 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 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.
+ * @returns A Disposable that unmounts the page when disposed.
+ */
+export declare function registerPage(
Review Comment:
SIP-235 lists this API under rejected alternatives: "A separate
`navigation.registerPage` API … Pages are views at a reserved location". In the
revised SIP, a page is `views.registerView({ id, name }, 'global.page',
Component)` and its navbar entry is `menus.registerMenuItem({ page: id, icon,
order }, 'global.navbar')`. Could we align with the SIP, or make the case for a
separate registry on the SIP thread, before this becomes public API?
##########
superset-frontend/webpack.config.js:
##########
@@ -211,6 +211,24 @@ const plugins = [
singleton: true,
eager: true,
},
+ // Shared for two reasons. First, the registries in here hold their
+ // state in module-level closures, so a second copy of the module is a
+ // second, unreachable registry: an extension calling
+ // `getExtensionsRegistry().set(...)` would write somewhere the host
+ // never reads, and the call would look like it worked.
+ //
+ // Second, this is where `SupersetClient` lives. Without it an extension
+ // that talks to Superset's API has to hand-roll `fetch`, including
+ // fetching and attaching a CSRF token for every mutation -- which is
+ // most of what `SupersetClient` exists to do.
+ //
+ // The host loads this module on every page already, so providing it
+ // eagerly costs no extra bytes and keeps extensions off an async
+ // boundary to reach it.
+ '@superset-ui/core': {
Review Comment:
Sharing `@superset-ui/core` effectively makes the whole package extension
API. That's a bigger contract decision than the comment suggests, and SIP-235
lists no new dependencies. There's also no published package an extension can
build against: npm's latest `@superset-ui/core` is 0.20.4 from Dec 2024 and
doesn't match the host (see the scaffold thread). For the CSRF case,
`@apache-superset/core` already exposes `authentication.getCSRFToken()`, so an
extension can make authenticated calls without this.
##########
superset-frontend/src/core/pages/index.ts:
##########
@@ -0,0 +1,321 @@
+/**
+ * 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.
+ */
+
+/**
+ * @fileoverview Host-side registry for extension-contributed pages.
+ *
+ * Mirrors `src/core/views`: module-level state, a `useSyncExternalStore`
+ * 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 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, {
+ ComponentType,
+ createContext,
+ useCallback,
+ useContext,
+ useSyncExternalStore,
+} from 'react';
+import { useHistory, useLocation } from 'react-router-dom';
+import type { pages as pagesApi } from '@apache-superset/core';
+import { logging } from '@apache-superset/core/utils';
+import PageBoundary from './PageBoundary';
+import ExtensionPlaceholder from 'src/extensions/ExtensionPlaceholder';
+import { Disposable } from '../models';
+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;
+ /** False when the server refused the path and registered no rule for it. */
+ registered?: boolean;
+ permitted?: boolean;
+ /** Everything else the manifest declared about the page. */
+ [field: string]: unknown;
+}
+
+/** A registration that did not take, so a caller can dispose it harmlessly. */
+const INERT = new Disposable(() => {});
+
+/**
+ * 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();
+
+const syncListeners = new Set<() => void>();
+const subscribe = (listener: () => void) => {
+ syncListeners.add(listener);
+ return () => syncListeners.delete(listener);
+};
+
+const registerEmitter = createEventEmitter<ResolvedPage>();
+const unregisterEmitter = createEventEmitter<ResolvedPage>();
+
+// `useSyncExternalStore` requires a referentially stable snapshot between
+// notifications, so the array is rebuilt only when the registry changes.
+let snapshot: ResolvedPage[] = [];
+const refresh = () => {
+ snapshot = Array.from(pageRegistry.values(), entry => entry.page);
+ syncListeners.forEach(listener => listener());
+};
+
+/**
+ * Registers a page on behalf of a specific extension.
+ *
+ * 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,
+ declared: readonly DeclaredPage[],
+ page: Page,
+ component: ComponentType,
+): Disposable => {
+ // Nothing here throws. An extension calls `registerPage` at module scope,
+ // so a throw takes down the whole bundle -- its other pages, its views, its
+ // commands -- over one page. Worse, one of these conditions is not the
+ // author's doing at all: the server refuses a path it has already given to
+ // something else, and the author cannot see that from their own source. The
+ // page is dropped, loudly, and the extension carries on.
+ if (!page?.path || !PAGE_PATH_PATTERN.test(page.path)) {
+ logging.error(
+ `Extension "${extensionId}" registered a page at ` +
+ `"${page?.path ?? '<missing>'}", which is not an absolute path of ` +
+ `lowercase segments such as "/review-queue". The page was ignored.`,
+ );
+ return INERT;
+ }
+
+ const declaration = declared.find(entry => entry.path === page.path);
+ if (!declaration) {
+ logging.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: ${declared.map(entry => entry.path).join(', ') ||
'(none)'}.`,
+ );
+ return INERT;
+ }
+
+ if (declaration.registered === false) {
Review Comment:
`registered: false` also covers a page that was just added after the server
started. The extensions API re-reads bundles on every request, but rules are
only registered at boot. So a page added to a `LOCAL_EXTENSIONS` or
`EXTENSIONS_PATH` bundle without a restart is reported as `registered: false`
and 404s on a cold load. This branch then logs "this deployment already
serves", which sends the author looking for a conflict that doesn't exist.
Could the message mention that a restart may be needed, or could the API tell
"refused" apart from "not registered yet"?
##########
superset/extensions/view.py:
##########
@@ -32,3 +51,544 @@ class ExtensionsView(BaseSupersetView):
@permission_name("read")
def list(self) -> FlaskResponse:
return super().render_app_template()
+
+
+def _render_shell() -> FlaskResponse:
+ """The application shell, identical to the one ``/welcome/`` serves."""
+ return render_template("superset/spa.html", **get_spa_template_context())
+
+
+def _view_menu(extension_id: str) -> str:
+ """The FAB view menu an extension's page permissions hang from.
+
+ Prefixed so the security manager can tell these apart from a core view's
+ permissions without keeping a registry of installed extensions. See
+ ``SupersetSecurityManager._is_admin_only``.
+ """
+ return f"{EXTENSION_VIEW_MENU_PREFIX}{extension_id}"
+
+
+def may_open(extension_id: str, permission: str | None) -> bool:
+ """Whether the current user may open a page with this permission.
+
+ 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.
+ """
+ if permission is None:
+ return True
+ from superset import security_manager
+
+ return security_manager.can_access(permission, _view_menu(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 _segments(path: str) -> list[str]:
+ """A page path as its path segments, ignoring trailing slashes."""
+ return [segment for segment in path.split("/") if segment]
+
+
+def _rule_segments(rule: Rule) -> list[tuple[str, list[str]]] | None:
+ """Each segment of a rule as its static text and its converter arguments.
+
+ Read from the rule's own parse rather than by splitting its text, because
+ a converter need not occupy a whole segment. ``/plugin/v<int:version>``
+ splits into a segment ``v<int:version>``, which no comparison against
+ ``v1`` recognises as dynamic -- and the page would then take
+ ``/plugin/v1/data`` from it.
+
+ Returns ``None`` when the rule cannot be read, so the caller falls back to
+ comparing text. That over-refuses, which is the safe direction.
+ """
+ try:
+ trace = rule._trace # noqa: SLF001
+ except AttributeError: # pragma: no cover - guards a Werkzeug change
+ return None
+
+ segments: list[tuple[str, list[str]]] = []
+ literals: list[str] = []
+ arguments: list[str] = []
+ started = False
+
+ for is_dynamic, data in trace:
+ if not is_dynamic and data == "|":
+ continue
+ if not is_dynamic and data == "/":
+ if started:
+ segments.append(("".join(literals), arguments))
+ literals, arguments = [], []
+ started = True
+ continue
+ if is_dynamic:
+ arguments.append(data)
+ else:
+ literals.append(data)
+
+ if started:
+ segments.append(("".join(literals), arguments))
+
+ # A trailing slash leaves an empty final segment; it denotes no position.
+ return [segment for segment in segments if segment[0] or segment[1]]
+
+
+def _converter_can_match(rule: Rule, argument: str, text: str) -> bool:
+ """Whether a converter could ever match this literal segment.
+
+ Asked of the converter's own pattern, so it holds for the custom ones
+ Superset registers as well as Werkzeug's. Anything unreadable is treated
+ as able to match, which keeps an unfamiliar converter on the refusing
+ side.
+ """
+ converter = getattr(rule, "_converters", {}).get(argument)
+ pattern = getattr(converter, "regex", None)
+ if not pattern:
+ return True
+ try:
+ return re.fullmatch(pattern, text) is not None
+ except re.error: # pragma: no cover - a converter with a broken pattern
+ return True
+
+
+def _is_path_converter(rule: Rule, argument: str) -> bool:
+ converter = getattr(rule, "_converters", {}).get(argument)
+ return isinstance(converter, PathConverter)
+
+
+def _text_reaches_into(page_path: str, rule_path: str) -> bool:
+ """The fallback when a rule's parse cannot be read: compare the text.
+
+ Treats any segment containing ``<`` as dynamic, which is coarse in both
+ directions but never accepts a page a converter could reach.
+ """
+ page_parts = _segments(page_path)
+ rule_parts = _segments(rule_path)
+ for index, page_part in enumerate(page_parts):
+ if index >= len(rule_parts):
+ return False
+ rule_part = rule_parts[index]
+ if "<" in rule_part:
+ if "<path:" in rule_part:
+ return True
+ continue
+ if rule_part != page_part:
+ return False
+ return True
+
+
+def _rule_reaches_into(page_path: str, rule: Rule) -> bool:
+ """Whether ``rule`` can serve a URL at or beneath ``page_path``.
+
+ Compared segment by segment, because a converter matches text that does
+ not resemble it. Comparing templates as strings misses the case this
+ exists for: ``/api/v1/datasource/<datasource_type>/...`` does not start
+ with ``/api/v1/datasource/table/``, yet it answers
+ ``/api/v1/datasource/table/42``. A page at ``/api/v1/datasource/table``
+ accepted by a string comparison would then take that request, answering
+ the app shell with HTTP 200 where a JSON API belongs.
+
+ A ``path`` converter spans every remaining segment, so a rule carrying one
+ above the page's depth reaches the whole subtree.
+
+ Where a segment is exactly one converter, its own pattern decides: an
+ ``<int:slice_id>`` can never match ``review``, so a page at
+ ``/slice/review`` does not conflict with ``/slice/<int:slice_id>/``.
+ Refusing it would cost paths that can never collide and widen the set of
+ pages a future core route breaks. A segment mixing text and a converter is
+ treated as able to match, since proving otherwise would mean composing the
+ pattern with the surrounding text for no practical gain.
+
+ Otherwise a page owns its subtree exclusively, so any rule reaching into
+ it is a conflict -- including one Werkzeug's precedence would have
+ resolved in the host's favour. Reimplementing that precedence here would
+ be wrong in a way nobody notices, and "this path is in use, pick another"
+ is a better error than a route that works until the arithmetic shifts.
+ """
+ rule_parts = _rule_segments(rule)
+ if rule_parts is None:
+ return _text_reaches_into(page_path, rule.rule)
+
+ for index, page_part in enumerate(_segments(page_path)):
+ if index >= len(rule_parts):
+ # The rule ran out of segments without a `path` converter, so it
+ # serves URLs above the page rather than inside it.
+ return False
+
+ literal, arguments = rule_parts[index]
+
+ if not arguments:
+ if literal != page_part:
+ return False
+ continue
+
+ if any(_is_path_converter(rule, argument) for argument in arguments):
+ return True
+
+ if literal or len(arguments) > 1:
+ # Mixed segment: assume it could match.
+ continue
+
+ if not _converter_can_match(rule, arguments[0], page_part):
+ return False
+
+ return True
+
+
+def _first_claimant(app: Flask, path: str) -> str | None:
+ """The endpoint already answering ``path`` or anything beneath it.
+
+ Two checks, because they catch different things and neither subsumes the
+ other.
+
+ Matching ``path`` against the live URL map catches a rule that answers it
+ without resembling it, and catches what matching alone can report: 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.
+
+ Then every rule is compared segment-wise, which is what finds a rule that
+ reaches *into* the page's subtree. Matching cannot find those: there is no
+ single URL to probe that an arbitrary descendant is guaranteed to answer.
+ """
+ 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
+
+ for rule in app.url_map.iter_rules():
+ if _rule_reaches_into(path, rule):
+ return f"{rule.endpoint} (at {rule.rule}, which this page would
shadow)"
+
+ return None
+
+
+def _permission_problem(permission: str | None) -> str | None:
+ """Why this permission cannot be used for a page, or None.
+
+ Two sets of names carry meaning in Superset's role predicates, and a page
+ naming one gets that meaning rather than the one it asked for.
+
+ ``OBJECT_SPEC_PERMISSIONS`` is how ``_is_admin_pvm`` decides what Admin
+ holds, by excluding exactly those. A page naming one would have its Admin
+ grant stripped the next time ``superset init`` ran, leaving a page nobody
+ could open and nothing in the logs to say why.
+
+ ``ACCESSIBLE_PERMS`` is worse, because it defeats the restriction
+ entirely. Both ``_is_alpha_pvm`` and ``_is_gamma_pvm`` end in
+ ``... or self._is_accessible_to_all(pvm)``, evaluated *after* the
+ ``_is_admin_only`` test that keeps extension permissions off those roles.
+ A page declaring ``can_userinfo`` therefore lands on stock Gamma however
+ the view menu is named -- the same defect the view-menu prefix was added
+ to fix, reached through the other override set.
+
+ Unlike a route, both sets are small, fixed and owned by Superset's own
+ security model, so refusing names from them is a rule that stays true.
+ """
+ if permission is None:
+ return None
+
+ from superset.security.manager import SupersetSecurityManager
+
+ if permission in SupersetSecurityManager.OBJECT_SPEC_PERMISSIONS:
+ return (
+ f"'{permission}' is reserved by Superset for object-level access "
+ f"and cannot be used for a page"
+ )
+ if permission in SupersetSecurityManager.ACCESSIBLE_PERMS:
+ return (
+ f"'{permission}' is reserved by Superset as a permission every "
+ f"role holds, so a page declaring it would be open to everyone"
+ )
+ 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 reads the routes this deployment actually
+ serves rather than a list of the ones that existed when the extension was
+ written.
+
+ It sees what is in the map at that moment, which is not quite everything:
+ a blueprint registered later, by an extension loaded after this one or by
+ code running after initialisation, is not there to be found. Extensions
+ are checked in load order, so the first to claim a path keeps it.
+
+ 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 problem := _permission_problem(page.permission):
+ logger.error(
+ "Extension '%s' declares a page at '%s' requiring a "
+ "permission that cannot be used: %s. The page has not been "
+ "registered and will not load.",
+ manifest.id,
+ page.path,
+ problem,
+ )
+ continue
+
+ 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}"
+ view = _make_page_view(manifest.id, page)
+ app.add_url_rule(
+ page.path,
+ endpoint=endpoint,
+ view_func=view,
+ methods=["GET"],
+ # Most Superset screens are reached with a trailing slash, so a
+ # page declared without one would hard-404 on `/review-queue/`
+ # rather than redirect.
+ strict_slashes=False,
+ )
+ app.add_url_rule(
+ f"{page.path}/<path:rest>",
+ endpoint=f"{endpoint}/*",
+ view_func=view,
+ methods=["GET"],
+ )
+ 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]:
+ """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.
+ """
+ # The trailing slash is load-bearing. Extension ids may contain hyphens,
+ # so without a separator `acme.review` is a prefix of `acme.review-plus`
+ # and would read back that extension's pages as its own -- undoing a
+ # refusal, and letting one extension's component be accepted for another
+ # extension's path.
+ 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("/*")
+ ]
+
+
+def _grant_to_admin(permission: str, extension_id: str, pvm: object) -> None:
+ """Give the Admin role a page permission, if it does not already hold it.
+
+ Not a policy this invents. ``sync_role_definitions`` builds the Admin
+ role from every permission that is not one of the four object-level ones,
+ so ``superset init`` would hand Admin this permission anyway. Doing it
+ here means an administrator who installs an extension can open its pages
+ without first running init and without granting themselves access to
+ their own deployment.
+
+ Granted only while *no role at all* holds the permission.
+
+ Two failures pull in opposite directions here. Granting on every boot
+ reinstates a grant an operator deliberately removed, so withholding it
+ from Admin stops being something they can decide. Granting only on the
+ boot that creates the permission leaves it ungranted forever if that boot
+ had no Admin role yet, or lost the create race to another worker, or
+ failed partway -- and nothing retries.
+
+ A permission no role holds gates nothing and leaves the page unreachable
+ by anyone, which is the state those failures produce and is not a state an
+ operator can be aiming for. So it is repaired. An operator who moves the
+ grant to a role of their own keeps it there: something holds it, and this
+ does not touch it.
+
+ The residual is narrow and deliberate: an operator who wants the page
+ reachable by nobody has to remove the page, not the permission.
+ """
+ from superset import security_manager
+
+ if getattr(pvm, "role", None):
Review Comment:
This doesn't match the PR description ("the grant is re-checked each boot so
a role that lost it repairs itself") or testing step 6. If any other role holds
the permission, revoking it from Admin and restarting leaves Admin without it:
there's no `Granted ...` log line and admin gets 403 on the page. Admin only
gets it back when no role holds the permission. I verified both cases.
The docstring says this is deliberate, so at least the description and step
6 need updating. It's also worth deciding whether Admin should always be able
to open extension pages, given that `superset init` grants it back anyway.
Nit: the log line below says `on 'acme.tools'`, but the view menu is
`extension:acme.tools`.
##########
superset-frontend/src/features/home/Menu.tsx:
##########
@@ -199,6 +200,108 @@ const StyledImage = styled(Image)`
const { useBreakpoint } = Grid;
+/**
+ * What the two helpers below need of a contributed page, and no more, so the
+ * menu does not take a dependency on the pages registry. One shape shared by
+ * both, so a field one of them reads cannot go missing from the other.
+ */
+type ContributedPage = {
+ name: string;
+ path: string;
+ nav?: { label?: string; category?: string };
+ permitted?: boolean;
+};
+
+/**
+ * Folds pages contributed by extensions into the server-rendered menu.
+ *
+ * The navbar is built on the server from FAB's menu, which an extension
+ * cannot write to: an entry there needs a `menu_access` permission created
+ * at boot, and extensions register in the browser. A page that opted in with
+ * `nav` is therefore added here, client-side, so that a contributed page has
+ * somewhere a user could find it at all.
+ *
+ * A page without `nav.category` sits at the top level. One that names a
+ * category goes into that group of the Settings menu instead, which is
+ * where an administrative screen belongs -- see `withExtensionSettings`.
+ */
+export function withExtensionPages(
+ menu: MenuObjectProps[],
+ pages: ContributedPage[],
+): MenuObjectProps[] {
+ // 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.nav.category && page.permitted !== false,
+ );
+ if (!contributed.length) {
+ return menu;
+ }
+
+ return [
+ ...menu,
+ ...contributed.map(
+ page =>
+ ({
+ label: page.nav?.label ?? page.name,
+ // `name` is the React key for the entry, and the label is the
+ // extension author's to choose -- a page labelled "Charts" would
+ // collide with the host's own entry. The path cannot collide,
+ // because the server refused any page that shared one.
+ name: `extension:${page.path}`,
Review Comment:
This key never gets into `selectedKeys`: `activeTabs` only handles
Dashboards, Charts, Datasets and SQL. So a contributed entry is never
highlighted while its page is open, as you can see in the [happy-path
video](https://gist.githubusercontent.com/geido/6de6b5435657f4d5b8065274de1f8b2a/raw/e34c4e31ee54381527dd6e5af1183d31bf57869a/01-happy-path-admin.mp4).
SIP-235 covers this with active-tab highlighting keyed on the page.
##########
superset-frontend/src/pages/ExtensionPage/index.tsx:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { useEffect, useState } from 'react';
+import { useLocation } from 'react-router-dom';
+import { t } from '@apache-superset/core/translation';
+import { Loading } from '@superset-ui/core/components';
+import { findPageByPath, resolvePage, usePages } from 'src/core/pages';
+import ExtensionsLoader from 'src/extensions/ExtensionsLoader';
+import { ErrorAlert } from 'src/components/ErrorMessage/ErrorAlert';
+
+/**
+ * Renders whichever extension page claims the current URL.
+ *
+ * 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() {
Review Comment:
While an extension page is open, `navigation.getPage()` still returns
`'home'`, because `derivePage` falls back to it for anything not in
`PAGE_ROUTES`. Going from Home to an extension page therefore fires no
page-change event, and extensions listening to `onDidChangePage` think the user
is still on Home. SIP-235 adds `'extension'` to the `Page` union for this.
##########
superset-frontend/src/views/routes.tsx:
##########
@@ -315,6 +321,27 @@ if (authRegistrationEnabled) {
routes.push({ path: RoutePaths.REGISTRATIONS, Component: UserRegistrations
});
}
+if (isFeatureEnabled(FeatureFlag.EnableExtensions)) {
+ // 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: '/:extensionPagePath+',
Review Comment:
Confirmed, and it isn't limited to `/welcome/`. For every host client route,
I ran `<route>/acme-x` through `_first_claimant` against the live URL map, then
through react-router's `matchPath` in route-table order. 31 of those paths are
accepted at boot, and 30 of them render the host screen instead of the page
(`/sqllab/…`, `/dashboard/list/…`, `/chart/list/…`, `/roles/…`, `/users/…`,
`/tags/…`, `/login/…`, `/logout/…`, …). Only `/annotationlayer/acme-x` reaches
the extension. The scripts are `repro-shadow_check.py` and
`repro-shadow_client.js` in
https://gist.github.com/geido/6de6b5435657f4d5b8065274de1f8b2a.
Refusing these at boot would mean mirroring React Router's prefix matching
on the server. A namespaced shell route as in SIP-235
(`/extensions/pages/<id>`) removes the whole class of problem instead.
##########
superset/config.py:
##########
@@ -2403,6 +2403,21 @@ def allowed_schemas_for_csv_upload( # pylint:
disable=unused-argument
# configuration. These blueprints will get integrated in the app
BLUEPRINTS: list[Blueprint] = []
+# Whether Superset keeps the permissions that extension pages declare in step
+# with the metadata database as extensions load.
+#
+# On by default, because an extension is installed long after a deployment is
+# initialised and an administrator cannot grant a permission that does not
+# exist. In the steady state this reads a few rows per worker at start and
+# writes nothing; it writes only the first time a page's permission is seen,
+# or when no role holds one.
+#
+# Turn it off where the metadata database must not be written during start --
+# a read-only replica, or a deployment that provisions roles and permissions
+# out of band. Pages still load and are still enforced; the permissions have
+# to exist already, which `superset init` will create.
Review Comment:
Confirmed. With `EXTENSIONS_SYNC_PAGE_PERMISSIONS = False` and a page that
declares a new permission, `superset init` creates nothing for it (`select
count(*) from ab_permission where name='can_audit2'` returns 0), and Admin gets
403 on the page.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
