amaannawab923 commented on issue #45248:
URL: https://github.com/apache/superset/issues/45248#issuecomment-6473280769
Some context first, since this started on a PR rather than here. #45522 adds
extension-contributed pages, and was opened in parallel with this SIP rather
than against it. Reviewing it, @geido pointed out that its registration API is
listed under Rejected Alternatives above, and asked us to either align with the
SIP or make the case on this thread. This is us doing the former, with one ask.
Author-chosen paths were our mistake. Host routes match by prefix, so a page
declared under an existing one silently renders the host screen instead. Review
of #45522 turned up 31 such paths. One namespaced route kills the whole class,
and deletes the conflict-detection code we wrote to cope with it. No argument
from us.
**One ask: let the manifest declare a permission for a page**, the way the
Possible follow-up section sketches.
Concretely, the code side stays exactly as proposed here:
```ts
views.registerView(
{ id: 'acme.tools.approvals', name: 'Approvals' },
'global.page',
Approvals,
);
menus.registerMenuItem({ page: 'acme.tools.approvals' }, 'global.navbar');
```
and `extension.json` gains one optional line, keyed on that same view id:
```json
{
"contributions": {
"pages": [
{ "id": "acme.tools.approvals", "permission": "can_review" }
]
}
}
```
What it would do: at boot the host creates `can_review` on a view menu named
after the extension, so an administrator can grant it from the Roles screen
like any other permission. The shell route then answers 403 to anyone without
it, and the extensions API reports the page as not permitted, so the navbar
entry is never rendered in the first place.
A page that omits `permission` behaves exactly as this SIP describes,
login-gated and nothing more. We are not asking to change the default, only to
make restricting a page possible at all.
Why it matters: plenty of extensions will own a page that not everyone
should see. An approvals queue, an audit trail, a console for the extension's
own configuration. Without this, such a page sits in every signed-in user's
Settings menu and the extension has no way to prevent it. Everything behind it
is `@protect()`ed so nothing leaks, but an administrative screen being visible
to all is its own problem, and it is the kind of thing a compliance review
picks up. Gating on a permission the extension's own API already defines would
suit us just as well as minting a new one.
Three smaller gaps we hit along the way, take or leave:
* **Page ids have no owner.** `registerView` is last write wins, so one
extension can quietly take over another extension's page id.
* **No reactive way to read query params**, which is what replaces
sub-routing. `react-router-dom` is not in the share scope, and `history.push`
fires no `popstate`.
* **Navbar entries have no overflow.** With `disabledOverflow` set, a few
contributed entries wrap under the logo and cover the page header. We capped
ours with a "More" dropdown if that is useful.
--
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]
