amaannawab923 commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4400650206
##########
superset/extensions/view.py:
##########
@@ -32,3 +51,553 @@ class ExtensionsView(BaseSupersetView):
@permission_name("read")
def list(self) -> FlaskResponse:
return super().render_app_template()
+
+
+def _render_shell() -> str:
+ """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.
+
+ The refusal renders the shell with a 403 status rather than aborting.
+ ``show_http_exception`` only serves an HTML page for 404 and 500, so
+ ``abort(403)`` answered a browser with a JSON error body carrying the
+ generic "unexpected error" issue code -- for a refusal that is neither
+ unexpected nor a backend fault. Rendering the shell gives a cold load
+ or a bookmark the same "not authorized" screen the client already
+ shows when the user arrives in-app, and keeps the status honest.
+ """
+ # 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,
+ )
+ return _render_shell(), 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):
+ # Held by something -- Admin, or a role the operator chose instead.
+ # Checked before looking up the role, so the ordinary boot, where a
+ # role already holds every permission, reads nothing further.
+ return
+
+ view_menu = _view_menu(extension_id)
+ role_name = current_app.config.get("AUTH_ROLE_ADMIN", "Admin")
+ role = security_manager.find_role(role_name)
+ if role is None:
+ logger.warning(
+ "No '%s' role to grant '%s' on '%s' to", role_name, permission,
view_menu
+ )
+ return
+
+ # Idempotent and self-committing in FAB. An ordinary boot does no write,
+ # because by then a role holds it and the check above has returned.
+ security_manager.add_permission_role(role, pvm)
+ logger.info(
+ "Granted '%s' on '%s' to the '%s' role",
+ permission,
+ view_menu,
+ role_name,
+ )
+
+
+def ensure_page_permissions(app: Flask, manifest: Manifest) -> None:
+ """Create the permissions this extension's pages require, and grant Admin.
+
+ Created when the extension loads rather than by ``superset init``, because
+ an extension can be installed long after a deployment was initialised and
+ an administrator cannot grant a permission that does not exist yet.
+
+ The permission is attached to a view menu named after the extension, so
+ the Roles screen groups an extension's permissions together and reads as
+ "can_review on extension:preset.asset-approval". Pages of the same
+ extension may share a permission or declare different ones; this
+ registers whatever set they name, and gives the Admin role each one it
+ creates.
+
+ Failures are logged and swallowed. Every worker runs this at boot, so two
+ starting together can race between the lookup and the insert; the loser
+ would otherwise take the whole worker down over a row the winner has
+ already written. The permission check itself reads the database per
+ request, so a worker that lost the race still enforces correctly.
+ """
+ if not current_app.config.get("EXTENSIONS_SYNC_PAGE_PERMISSIONS", True):
+ # The operator provisions these out of band, or the metadata database
+ # must not be written during start. Pages still load, and the checks
+ # that enforce them still read whatever permissions exist.
+ return
+
+ from superset import security_manager
+
+ # Only pages that actually got a rule. A refused page is not reachable,
+ # so creating its permission would leave a row -- and an Admin grant --
+ # for a page that does not exist, which an administrator then has to
+ # reason about in the Roles screen.
+ registered = set(registered_page_paths(app, manifest.id))
+ required = {
+ page.permission
+ for page in manifest.contributions.pages
+ if page.permission is not None and page.path in registered
+ }
+ view_menu = _view_menu(manifest.id)
+ for permission in sorted(required):
+ try:
+ if not security_manager.find_permission_view_menu(permission,
view_menu):
+ security_manager.add_permission_view_menu(permission,
view_menu)
+ logger.info(
+ "Created permission '%s' on '%s' for extension pages",
+ permission,
+ view_menu,
+ )
+
+ # Runs whether or not this boot created the permission, so a grant
+ # that never happened -- no Admin role yet, a lost create race, a
+ # failure partway -- is repaired rather than missing forever.
+ # `_grant_to_admin` declines once any role holds it.
+ # Re-read rather than reusing the lookup above, which may have
+ # been None before the create. Passed in so the grant does not
+ # repeat it.
+ pvm = security_manager.find_permission_view_menu(permission,
view_menu)
+ if pvm is not None:
+ _grant_to_admin(permission, manifest.id, pvm)
+ except Exception: # pylint: disable=broad-except
+ # Most often a concurrent worker inserting the same row.
+ logger.warning(
+ "Could not create permission '%s' on '%s'; an administrator "
+ "may need to run `superset init` before it can be granted",
+ permission,
+ view_menu,
+ exc_info=True,
+ )
+ # Rolled back here rather than through `@transaction`, which
+ # would wrap the whole loop: one permission that cannot be
+ # written must not discard the ones already created.
+ security_manager.session.rollback() # pylint:
disable=consider-using-transaction
+
+
+def permitted_page_paths(app: Flask, manifest: Manifest) -> set[str]:
+ """Paths of this extension's pages the current user may open.
+
+ Used to decide what the frontend is told about. The server refuses an
+ unpermitted page on its own, so this is not the enforcement -- it is what
+ keeps a navigation entry from advertising a page that would answer 403.
+ """
+ registered = set(registered_page_paths(app, manifest.id))
+ return {
+ page.path
+ for page in manifest.contributions.pages
+ if page.path in registered and may_open(manifest.id, page.permission)
Review Comment:
Worth noting this one closed before the reply: `permitted_page_paths` used
to read `page.permission` from the manifest row it was iterating, which is
where the looser duplicate got in. It now reads the requirement recorded when
the rule was registered, so both rows resolve to the permission the first
declaration registered and the unrestricted duplicate cannot relax it.
f17429210a adds the regression test for the manifest you described, including
the case where the user does hold `can_review`.
On the validator: rejecting duplicate paths outright still seems worth
doing, since the manifest is otherwise silently half-applied and the nav would
render the path twice. Happy to take that here or separately, whichever you
prefer.
##########
superset-frontend/src/core/pages/index.ts:
##########
@@ -0,0 +1,319 @@
+/**
+ * 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) {
+ // The path was taken by Superset, an operator's blueprint, or another
+ // extension. The author cannot see that from here, so the message says
+ // where to look rather than implying they got it wrong.
+ logging.error(
+ `Extension "${extensionId}" declares a page at "${page.path}", which ` +
+ `this deployment already serves, so no route was registered for it. ` +
+ `The page was ignored; the server log names what holds the path. ` +
+ `Choose a different path in the manifest.`,
+ );
+ return INERT;
+ }
+
+ const id = `${extensionId}:${page.path}`;
+ // Registered whether or not the user may open it, so that the component is
+ // there to render a refusal rather than a blank screen, and so an
+ // extension's own bundle never has to branch on permissions.
+ //
+ // The manifest's own fields come first, so a page that declares `nav` or
+ // `name` there does not have to repeat them in code to be seen. They are
+ // only defaults: anything the registration passes wins, because that is the
+ // call the author wrote.
+ const {
+ path: _path,
+ registered: _registered,
+ permitted,
+ ...declaredFields
+ } = declaration;
+ const resolved: ResolvedPage = {
+ ...(declaredFields as Partial<Page>),
+ ...page,
+ extensionId,
+ permitted: permitted !== false,
+ };
+
+ const replaced = pageRegistry.get(id);
+ if (replaced) {
+ // 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.`,
+ );
+ }
+
+ const entry = { page: resolved, component };
+ pageRegistry.set(id, entry);
+ refresh();
+ registerEmitter.fire(resolved);
+
+ return new Disposable(() => {
+ // Only while this registration still holds the id: disposing one that has
+ // since been replaced would remove the replacement instead.
+ if (pageRegistry.get(id) !== entry) {
+ return;
+ }
+ pageRegistry.delete(id);
+ refresh();
+ unregisterEmitter.fire(resolved);
+ });
+};
+
+export const getPages = (): ResolvedPage[] => snapshot;
+
+export const usePages = (): ResolvedPage[] =>
+ useSyncExternalStore(subscribe, getPages, getPages);
+
+/** The page registered for a path, or undefined when nothing claims it. */
+export const findPageByPath = (path: string): ResolvedPage | undefined =>
+ // Longest match, not the first registered. `/reports` and
+ // `/reports/archive` can both claim `/reports/archive`, and taking the
+ // first leaves the deeper page unreachable whenever it happens to be
+ // registered second -- which is load order, not anything an author
+ // controls. The server resolves the same way, because Werkzeug prefers the
+ // rule with more static segments.
+ snapshot
+ .filter(page => path === page.path || path.startsWith(`${page.path}/`))
+ .sort((a, b) => b.path.length - a.path.length)[0];
+
+/**
+ * The root path of the page currently rendering.
+ *
+ * Supplied by the host when it mounts a page, so `useSubPath`/`useNavigate`
+ * can work in paths relative to that page without the page knowing its own
+ * URL. Defaults to empty, which makes both hooks inert rather than wrong if
+ * they are somehow called outside a page.
+ */
+export const PageRootContext = createContext<string>('');
+
+/**
+ * The host's own router is read here, inside the host bundle.
+ *
+ * An extension cannot do this for itself: `react-router-dom` is not in the
+ * share scope, so an extension's import resolves to a second module instance
+ * whose context is never populated by the host's `<Router>`. Exposing these
+ * two hooks through `@apache-superset/core`, which *is* shared, is what lets
+ * a page navigate at all.
+ */
+export const useSubPath: typeof pagesApi.useSubPath = () => {
+ const root = useContext(PageRootContext);
+ const { pathname } = useLocation();
+ if (!root) {
+ return '/';
+ }
+ // `useLocation` reports a path the router has already stripped of the app
+ // root, so stripping again is wrong rather than merely redundant: it would
+ // eat a real first segment wherever a page path happens to begin with the
+ // same one, leaving the page unable to read its own sub-path.
+ const stripped = pathname;
+ const sub = stripped.startsWith(root) ? stripped.slice(root.length) : '';
+ return sub.startsWith('/') ? sub : `/${sub}`;
+};
+
+export const useNavigate: typeof pagesApi.useNavigate = () => {
+ const root = useContext(PageRootContext);
+ const history = useHistory();
+ return useCallback(
+ (subPath: string) => {
+ if (!root) {
+ return;
+ }
+ // Relative by contract: a page states where it is going within itself,
+ // and so cannot navigate out of its own namespace by accident.
+ const suffix = subPath.startsWith('/') ? subPath : `/${subPath}`;
+ // No `ensureAppRoot` here. Superset's router is mounted as
+ // `<BrowserRouter basename={applicationRoot()}>`, so it adds the app
+ // root on push and strips it from `location.pathname`. Adding it here
+ // too produced `/root/root/page` under a subdirectory deployment --
+ // invisible wherever the app root is empty, which is most deployments.
+ history.push(`${root}${suffix === '/' ? '' : suffix}`);
Review Comment:
Fixed in f17429210a, as a correctness fix rather than a containment one.
Reproduced it first against a real `BrowserRouter basename="/analytics"`:
`history.push` keeps `..` verbatim, the WHATWG parser strips it on `pushState`,
so the address bar read `/analytics/welcome` while react-router still matched
the extension page -- and `/../../superset/welcome` climbed past the basename
too. `%2E%2E` behaves the same, since history v4 runs `decodeURI` first.
The target is now parsed with `new URL` against a constant base, which is
the same algorithm `pushState` applies, and one resolving outside the page root
is logged and dropped rather than stripped -- stripping would navigate
somewhere the author did not ask for. `search` and `hash` are re-appended so
`navigate("/42?tab=x#y")` keeps them.
Not described as a boundary, because it is not one: extension bundles are
injected into the host document and can call `window.history` directly. The
published doc said a page "cannot navigate out of its namespace by accident",
and that sentence is what was wrong; it now says the navigation is ignored.
--
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]
