amaannawab923 commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4388959590
##########
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:
Taken as suggested. The refusal returns the shell with a 403 now instead of
aborting, so a cold load or a bookmark gets the same "not authorized" screen as
an in-app arrival, and the status stays honest. c5538e9bf3
##########
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:
You're right that `name` was doing nothing there. `RightMenu` keys a
settings child by `child.name ?? child.label` now — `name` is optional on host
children, so they keep their current key — and the comment says which key it
means. c5538e9bf3
##########
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:
The behaviour is deliberate, the description wasn't. Both it and testing
step 6 now say the grant is repaired only while no role at all holds the
permission, so an operator who moves it to a role of their own keeps it there.
Log nit fixed too: the grant lines and the docstring name `extension:<id>`
rather than the bare id. c5538e9bf3
##########
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:
Good catch. The message gives both reasons there may be no rule now — the
path is already served, or the page was added after the server started, in
which case restart it — so it no longer points only at a conflict. c5538e9bf3
--
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]
