sadpandajoe commented on code in PR #45522: URL: https://github.com/apache/superset/pull/45522#discussion_r4412540238
########## 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() { + const { pathname } = useLocation(); + // Subscribed rather than read once: an extension registering after this + // first render is the normal case, not the exception. + usePages(); + const [extensionsSettled, setExtensionsSettled] = useState(false); + + useEffect(() => { + let cancelled = false; + // Resolves whether or not individual extensions failed; a failure to load + // one extension still settles the question for every other path. + ExtensionsLoader.getInstance() + .initializeExtensions() + .catch(() => undefined) + .finally(() => { + if (!cancelled) { + setExtensionsSettled(true); + } + }); + return () => { + cancelled = true; + }; + }, []); + + // `useLocation` reports a path the router has already stripped of the app + // root; stripping again would eat a real first segment wherever a page + // path begins with the same one. + const page = findPageByPath(pathname); + + if (page && !page.permitted) { Review Comment: `permitted` now also goes false when the page's `featureFlag` is off (`permitted_page_paths`), but this branch reads every `permitted: false` as a permission refusal. So with the flag off, SPA navigation to the page (a bookmark inside the app, a configured logo target) tells even an Admin to ask an administrator for a permission the page never declared, while a cold load of the same URL answers 404. Should the client tell "flag off" apart from "not permitted" and show the not-found state for it? ########## docs/developer_docs/extensions/contribution-types.md: ########## @@ -129,6 +129,132 @@ chat.registerChat( See [Chat](./extension-points/chat.md) for implementation details. +### Pages + +Extensions can add a page of their own: a whole screen at its own URL, as +opposed to a view that occupies a slot inside a screen Superset already ships. + +A page is declared in `extension.json` and registered in code. Both halves are +needed. The declaration is what the server reads to register a URL rule, so +that loading the page's URL directly works; the registration supplies the +component to render there. + +```json +{ + "contributions": { + "pages": [ + { + "path": "/review-queue", + "name": "Review queue", + "nav": { "label": "Review queue" }, + "permission": "can_review" + } + ] + } +} +``` + +```typescript +import { pages } from '@apache-superset/core'; +import ReviewQueue from './ReviewQueue'; + +pages.registerPage( + { path: '/review-queue', name: 'Review queue' }, + ReviewQueue, +); +``` + +#### Choosing a path + +The path is yours to choose, but it is not yours to take. On every start, +Superset checks each declared path against the routes that deployment actually +serves — its own screens and APIs, any blueprints the operator has registered, +and pages other extensions have already claimed. A path that is taken is +refused: the page is not registered, the server log names what holds the path, +and the rest of the extension loads normally. + +Pick something specific. A page owns its path **and everything beneath it**, so +`/review-queue` also answers `/review-queue/42/edit`. Short, generic paths are +the ones most likely to collide with a future Superset release. + +Paths are lowercase segments with a leading slash and no trailing slash: +`/review-queue`, `/acme/reports`. No parameters — a page routes within itself +on the client. + +#### Routing inside a page + +Because a page owns everything beneath its path, it can have more than one +screen. Two hooks read the host's router: + +```typescript +const subPath = pages.useSubPath(); // '/42/edit' at /review-queue/42/edit +const navigate = pages.useNavigate(); + +navigate('/42'); // -> /review-queue/42, a client-side transition +navigate('/'); // back to the page root +``` + +`navigate` takes a path relative to the page, so a page never writes its own +URL. These exist because `react-router-dom` is not shared with extensions: an +extension importing it would get a second copy with its own React context, and +`useLocation()` would throw. + +#### Appearing in the navigation bar + +Add `nav` and the page gets a top-level entry, labelled `nav.label` or the +page's `name`. Omit `nav` for a page reached from somewhere the extension +renders itself — a button on one of its own screens — and the page stays +reachable by URL. + +#### Restricting access + +Name a `permission` and only users holding it may open the page; anyone else +gets 403 and sees no navigation entry. Superset creates the permission when +the extension loads and shows it in **Settings → List Roles** as, for example, +`can review on extension:acme.tools`, ready to grant to any role. The Admin +role receives it automatically, unless some role already holds it. + +Omit `permission` and the page is open to any signed-in user. Review Comment: `featureFlag` is accepted by the manifest schema but appears nowhere in this page or in the `Page` type in `packages/superset-core/src/pages`, so an extension author can't discover it and `pages.getPages()[0].featureFlag` doesn't type-check. This line is also no longer true for a page that declares one: with the flag off it is hidden and 404s for every signed-in user. Should the field be documented here and added to `Page`, with this sentence qualified? ########## tests/unit_tests/extensions/test_pages_view.py: ########## @@ -0,0 +1,1191 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +"""Page-path registration and the collision check that guards it. + +Exercised against a bare Flask app rather than the Superset one: the thing +under test is how a declared path interacts with the URL map, and a bare app +lets each case state exactly which routes exist before the extension loads -- +which is the whole question. +""" + +import logging +from types import SimpleNamespace + +import pytest +from flask import Flask, g +from pydantic import ValidationError +from superset_core.extensions.types import ( + Contributions, + Manifest, + ManifestFrontend, + Page, +) +from werkzeug.exceptions import NotFound + +from superset.constants import EXTENSION_VIEW_MENU_PREFIX +from superset.extensions import view +from superset.extensions.view import ( + _first_claimant, + ensure_page_permissions, + permitted_page_paths, + register_extension_pages, + registered_page_paths, +) + +#: The view menu an extension's page permissions hang from. +VIEW_MENU = f"{EXTENSION_VIEW_MENU_PREFIX}acme.tools" + + +def manifest( + *paths: str, + extension_id: str = "acme.tools", + permission: str | None = None, +) -> Manifest: + publisher, name = extension_id.split(".", 1) + return Manifest( + publisher=publisher, + name=name, + displayName="Tools", + # A page needs a bundle to supply its component, so a manifest that + # contributes one always has a frontend. Fixtures say so too, or they + # would be testing a shape the loader never produces. + frontend=ManifestFrontend( + remoteEntry="remoteEntry.js", + moduleFederationName=f"{publisher}_{name.replace('-', '_')}", + ), + contributions=Contributions( + pages=[Page(path=path, name=path, permission=permission) for path in paths] + ), + ) + + +class FakeSecurityManager: + """Stands in for the real one, recording what was asked and created.""" + + def __init__(self, granted: set[tuple[str, str]] | None = None) -> None: + self.granted = granted or set() + self.existing: set[tuple[str, str]] = set() + self.created: list[tuple[str, str]] = [] + self.asked: list[tuple[str, str]] = [] + self.rollbacks = 0 + self.admin_role = SimpleNamespace(name="Admin", permissions=[]) + self.roles: dict[str, SimpleNamespace] = {"Admin": self.admin_role} + + def can_access(self, permission: str, view: str) -> bool: + self.asked.append((permission, view)) + return (permission, view) in self.granted + + def find_permission_view_menu( + self, permission: str, view: str + ) -> SimpleNamespace | None: + if (permission, view) not in self.existing: + return None + # FAB's PermissionView carries `role`, the roles holding it. The + # production code reads it, so the stand-in has to have it. + return SimpleNamespace( + permission=SimpleNamespace(name=permission), + view_menu=SimpleNamespace(name=view), + role=[ + role + for role in self.roles.values() + if (permission, view) in getattr(role, "permissions", []) + ], + ) + + def add_permission_view_menu(self, permission: str, view: str) -> None: + self.existing.add((permission, view)) + self.created.append((permission, view)) + + def find_role(self, name: str) -> SimpleNamespace | None: + return self.roles.get(name) + + def add_permission_role( + self, role: SimpleNamespace, perm_view: SimpleNamespace + ) -> None: + # Stored as the (permission, view_menu) pair the rest of this file + # asserts against, and the pair `find_permission_view_menu` derives + # `role` from -- so a grant made here is visible to the next lookup. + held = (perm_view.permission.name, perm_view.view_menu.name) + if held not in role.permissions: + role.permissions.append(held) + + # Named to match SupersetSecurityManager. A fake that invents its own + # attribute names lets production code call something that is not there. + @property + def session(self) -> SimpleNamespace: + return SimpleNamespace(rollback=self._record_rollback) + + def _record_rollback(self) -> None: + # Counts the call, not the read of `session`. Code that takes the + # session after a failed insert and never rolls back leaves the + # dirty state in place, and has to fail the assertion. + self.rollbacks += 1 + + [email protected] +def security(monkeypatch: pytest.MonkeyPatch) -> FakeSecurityManager: + import superset + + manager = FakeSecurityManager() + monkeypatch.setattr(superset, "security_manager", manager, raising=False) + return manager + + +def paths_in(app: Flask) -> set[str]: + return {rule.rule for rule in app.url_map.iter_rules()} + + [email protected] +def app() -> Flask: + return Flask(__name__) + + +def test_a_free_path_is_registered(app: Flask) -> None: + register_extension_pages(app, manifest("/approvals")) + + assert "/approvals" in paths_in(app) + # The page owns everything beneath it, so a deep link resolves on a cold + # load instead of 404ing before the client router ever runs. + assert "/approvals/<path:rest>" in paths_in(app) + + +def test_a_path_a_core_route_already_serves_is_refused( + app: Flask, caplog: pytest.LogCaptureFixture +) -> None: + app.add_url_rule("/welcome", endpoint="welcome", view_func=lambda: "") + + with caplog.at_level(logging.ERROR): + register_extension_pages(app, manifest("/welcome")) + + assert "/welcome/<path:rest>" not in paths_in(app) + assert "already served by welcome" in caplog.text + + +def test_a_parameterised_rule_is_recognised_as_the_claimant( + app: Flask, caplog: pytest.LogCaptureFixture +) -> None: + """The case a literal comparison against known routes gets wrong. + + ``/oauth-authorized/<provider>`` never string-matches the concrete path + it would answer, so a denylist of route templates waves this through -- + which is how an extension could end up in front of an OAuth callback. + """ + app.add_url_rule( + "/oauth-authorized/<provider>", endpoint="oauth", view_func=lambda provider: "" + ) + + with caplog.at_level(logging.ERROR): + register_extension_pages(app, manifest("/oauth-authorized/google")) + + assert "/oauth-authorized/google" not in paths_in(app) + assert "already served by oauth" in caplog.text + + +def test_a_path_whose_descendants_are_taken_is_refused( + app: Flask, caplog: pytest.LogCaptureFixture +) -> None: + """A page at ``/x`` would shadow ``/x/list`` through its sub-path rule.""" + app.add_url_rule("/report/list", endpoint="report_list", view_func=lambda: "") + + with caplog.at_level(logging.ERROR): + register_extension_pages(app, manifest("/report")) + + assert "/report" not in paths_in(app) + + +def test_a_rule_answering_other_methods_still_owns_the_path(app: Flask) -> None: + app.add_url_rule( + "/submit", endpoint="submit", view_func=lambda: "", methods=["POST"] + ) + + register_extension_pages(app, manifest("/submit")) + + assert "/submit/<path:rest>" not in paths_in(app) + + +def test_the_second_extension_to_claim_a_path_is_refused( + app: Flask, caplog: pytest.LogCaptureFixture +) -> None: + register_extension_pages(app, manifest("/approvals")) + + with caplog.at_level(logging.ERROR): + register_extension_pages(app, manifest("/approvals", extension_id="other.ext")) + + assert "other.ext" in caplog.text + # The first extension keeps the path; the loser is told, not silently + # left with a component nothing routes to. + assert len([r for r in app.url_map.iter_rules() if r.rule == "/approvals"]) == 1 + + +def test_one_refused_page_does_not_stop_the_others( + app: Flask, caplog: pytest.LogCaptureFixture +) -> None: + app.add_url_rule("/welcome", endpoint="welcome", view_func=lambda: "") + + with caplog.at_level(logging.ERROR): + register_extension_pages(app, manifest("/welcome", "/approvals")) + + assert "/approvals" in paths_in(app) + + +def test_an_extension_declaring_no_pages_registers_nothing(app: Flask) -> None: + before = paths_in(app) + + register_extension_pages(app, manifest()) + + assert paths_in(app) == before + + [email protected]( + "path", + [ + "review-queue", + "/review-queue/", + "/Review-Queue", + "", + "/review-queue/:id", + "/-queue", + "/a//b", + ], +) +def test_a_malformed_path_is_rejected_by_the_manifest(path: str) -> None: + """Rejected at parse time, so a bad path never reaches the URL map.""" + with pytest.raises(ValidationError, match="should match pattern"): + Page(path=path, name="X") + + +def test_first_claimant_reports_a_redirect_as_taken(app: Flask) -> None: + """``strict_slashes`` means ``/thing/`` already answers for ``/thing``.""" + app.add_url_rule("/thing/", endpoint="thing", view_func=lambda: "") + + assert _first_claimant(app, "/thing") is not None + + +def test_registered_page_paths_reports_only_what_was_accepted(app: Flask) -> None: + app.add_url_rule("/welcome", endpoint="welcome", view_func=lambda: "") + + register_extension_pages(app, manifest("/welcome", "/approvals")) + + # The refused page is absent, so nothing downstream -- the menu, the + # frontend's check on what an extension may register -- can offer a page + # that has no route behind it. + assert registered_page_paths(app, "acme.tools") == ["/approvals"] + + +def test_registered_page_paths_does_not_leak_across_extensions(app: Flask) -> None: + register_extension_pages(app, manifest("/approvals")) + register_extension_pages(app, manifest("/change-review", extension_id="other.ext")) + + assert registered_page_paths(app, "acme.tools") == ["/approvals"] + assert registered_page_paths(app, "other.ext") == ["/change-review"] + + +def render_with( + monkeypatch: pytest.MonkeyPatch, + app: Flask, + *, + extensions_enabled: bool = True, + user_id: int | None = 1, + permission: str | None = None, + granted: bool = False, + recorded: bool = True, + feature_flag: str | None = None, + flag_on: bool = True, +) -> object: + """Run one page view's guards without rendering a real template. + + The guards are the whole of the view; what it renders on success is + ``spa.html`` unchanged, which the host's own screens already cover. + + ``recorded`` stands in for what registration does: the requirement is + written to the app when the rule is added, and the view reads it on the + way in. Pass ``False`` to exercise a rule whose page nothing registered. + """ + import superset + + security = FakeSecurityManager( + granted={(permission, VIEW_MENU)} if permission and granted else set() + ) + monkeypatch.setattr(superset, "security_manager", security, raising=False) + monkeypatch.setattr( + superset, + "is_feature_enabled", + lambda flag: flag_on if flag == feature_flag else extensions_enabled, + ) + monkeypatch.setattr(view, "get_user_id", lambda: user_id) + monkeypatch.setattr(view, "_render_shell", lambda: "SHELL") + monkeypatch.setattr(view, "redirect_to_login", lambda: "LOGIN") + + if recorded: + view._requirements(app)[("acme.tools", "/approvals")] = ( + view.PageRequirement(permission=permission, feature_flag=feature_flag) Review Comment: Every flag test builds the `PageRequirement` by hand here, so nothing exercises the path from a manifest's `featureFlag` to the route or to `permitted_page_paths`. Dropping `feature_flag=page.featureFlag` in `register_extension_pages`, or the `page_available(...)` term in `permitted_page_paths`, would leave all of these green while a flagged-off page stayed open and in the nav. Could you add a case that registers a manifest page with `featureFlag` set and the flag off, and asserts the route 404s and `permitted_page_paths` excludes it (and the inverse with the flag on)? -- 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]
