aminghadersohi commented on code in PR #39604:
URL: https://github.com/apache/superset/pull/39604#discussion_r3253725583
##########
superset/security/manager.py:
##########
@@ -1361,6 +1361,15 @@ def create_custom_permissions(self) -> None:
self.add_permission_view_menu("can_tag", "Chart")
self.add_permission_view_menu("can_tag", "Dashboard")
+ # API Key permissions (FAB's ApiKeyApi blueprint).
+ # Superset uses AppBuilder(update_perms=False) so FAB skips
+ # permission creation during blueprint registration. Create them
+ # explicitly here so that ``superset init`` picks them up and
+ # sync_role_definitions assigns them to the Admin role.
+ if current_app.config.get("FAB_API_KEY_ENABLED", False):
+ for perm in ("can_list", "can_create", "can_get", "can_delete"):
+ self.add_permission_view_menu(perm, "ApiKey")
Review Comment:
Addressed in commit `8a9d51173c` — the 3 failing tests now patch
`superset.mcp_service.auth.security_manager` (where the binding lives after the
module-level import) instead of `superset.security_manager`. Same fix applied
to the 4th similar test for consistency.
##########
superset/security/manager.py:
##########
@@ -1361,6 +1361,15 @@ def create_custom_permissions(self) -> None:
self.add_permission_view_menu("can_tag", "Chart")
self.add_permission_view_menu("can_tag", "Dashboard")
+ # API Key permissions (FAB's ApiKeyApi blueprint).
+ # Superset uses AppBuilder(update_perms=False) so FAB skips
+ # permission creation during blueprint registration. Create them
+ # explicitly here so that ``superset init`` picks them up and
+ # sync_role_definitions assigns them to the Admin role.
+ if current_app.config.get("FAB_API_KEY_ENABLED", False):
+ for perm in ("can_list", "can_create", "can_get", "can_delete"):
+ self.add_permission_view_menu(perm, "ApiKey")
Review Comment:
The `"ApiKey"` entry is unconditional but harmless when
`FAB_API_KEY_ENABLED=False` — no PVMs exist under that view menu when the
feature is off, so `_is_admin_only_pvm` never fires. When enabled, FAB
registers the `ApiKeyApi` blueprint and its PVMs; the static entry correctly
classifies them admin-only without any runtime config lookup. Making it
conditional would require either mutating the class-level set at init time or
overriding `_is_admin_only_pvm`, both of which add complexity without a safety
benefit. Happy to switch to a conditional approach if you still prefer it.
--
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]
