amaannawab923 commented on code in PR #45522:
URL: https://github.com/apache/superset/pull/45522#discussion_r4388961447
##########
superset-core/src/superset_core/extensions/types.py:
##########
@@ -29,12 +29,85 @@
from superset_core.extensions.constants import (
DISPLAY_NAME_PATTERN,
+ PAGE_PATH_PATTERN,
+ PERMISSION_NAME_PATTERN,
PUBLISHER_PATTERN,
TECHNICAL_NAME_PATTERN,
VERSION_PATTERN,
)
+class PageNav(BaseModel):
+ """Placement of a page in the navigation."""
+
+ label: str = Field(
+ ...,
+ description="Text of the navigation entry",
+ min_length=1,
+ )
Review Comment:
Fixed. `PageNav.label` is optional now, which matches the TypeScript type,
the docs and the host's existing fallback to the page's name. c5538e9bf3
##########
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:
Removed, and the PR description now matches the code: every declared page is
reported with what became of it. c5538e9bf3
##########
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 — `superset init` doesn't reach these, since it syncs the
permissions of FAB-registered views and these pages are plain Flask rules. The
comment now says the permissions stay uncreated and the page answers 403 to
everyone including Admin, and names what to create out of band (`can_review` on
`extension:<publisher>.<name>`). c5538e9bf3
##########
tests/unit_tests/extensions/test_pages_view.py:
##########
@@ -0,0 +1,933 @@
+# 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, Page
+from werkzeug.exceptions import Forbidden, 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",
+ 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, object] = {"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) -> object
| 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) -> object | None:
+ return self.roles.get(name)
+
+ def add_permission_role(self, role: object, perm_view: object) -> 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)
Review Comment:
Fixed. The fake builds nothing but `SimpleNamespace`, so it's typed as that,
which makes the attribute reads check and lets all three `type: ignore`s go;
the lambda is a plain function set through `monkeypatch` now. The four errors
are gone. c5538e9bf3
##########
superset/initialization/__init__.py:
##########
@@ -667,6 +671,12 @@ def init_extensions(self) -> None:
return
for extension in extensions.values():
+ # Before the backend entrypoint runs: a page is declared in the
+ # manifest, so it does not depend on extension code importing
+ # cleanly, and a frontend-only extension contributes pages too.
+ register_extension_pages(self.superset_app, extension.manifest)
Review Comment:
Fixed by stubbing: the test's initializer gets an app and the two
page-registration calls are patched, so it stays on the logging path it was
written for. 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]
