gabotorresruiz commented on code in PR #45552: URL: https://github.com/apache/superset/pull/45552#discussion_r4350842274
########## tests/integration_tests/security/public_role_tests.py: ########## @@ -0,0 +1,74 @@ +# 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. +from contextlib import nullcontext + +import pytest +from flask.ctx import AppContext +from flask_appbuilder.security.sqla.models import PermissionView + +from superset.extensions import db, security_manager +from tests.integration_tests.fixtures.public_role import _public_role_like +from tests.integration_tests.test_app import app + + [email protected]("role_name", ["Gamma", "TestRole", "Public"]) [email protected]("test_raises", [False, True]) +def test_public_role_restored_after_commits( + app_context: AppContext, role_name: str, test_raises: bool +) -> None: + """Restore custom Public grants after session replacement and test failures.""" + original_value = app.config["PUBLIC_ROLE_LIKE"] + original_permission_ids = { + permission.id for permission in security_manager.get_public_role().permissions + } + try: + # Use a custom set that cannot be restored by re-syncing a built-in role. + app.config["PUBLIC_ROLE_LIKE"] = None + permissions = [ + security_manager.find_permission_view_menu("can_read", "Dashboard"), + security_manager.find_permission_view_menu("menu_access", "Security"), + ] + expected_permission_ids = {permission.id for permission in permissions} Review Comment: These six cases fail on all three dialects with `AttributeError: 'NoneType' object has no attribute 'id'`, because `find_permission_view_menu("menu_access", "Security")` returns `None` by the time this file runs. `tests/integration_tests/migrations/b4a3f2e1d0c9_fix_security_view_menu_case__tests.py` has a `_clean_security` fixture that deletes the `Security` view menu and its permission rows on both setup and teardown and never puts them back, and `migrations/` sorts before `security/`. Running that file followed by this one reproduces exactly these six, so it is the same class of fixture leak you just fixed, one layer down. Could the test own its permissions instead of borrowing two it does not? Creating a throwaway pair with `add_permission_view_menu` would make it order independent, and asserting the lookups are not `None` first would at least make the failure name its cause. ########## tests/integration_tests/fixtures/public_role.py: ########## @@ -14,48 +14,55 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. +from collections.abc import Iterator +from contextlib import contextmanager + import pytest from flask.ctx import AppContext +from flask_appbuilder.security.sqla.models import PermissionView from superset.extensions import db, security_manager from tests.integration_tests.test_app import app [email protected] -def public_role_like_gamma(app_context: AppContext): - app.config["PUBLIC_ROLE_LIKE"] = "Gamma" - security_manager.sync_role_definitions() - - yield - - security_manager.get_public_role().permissions = [] - db.session.commit() +@contextmanager +def _public_role_like(role_name: str) -> Iterator[None]: + """Temporarily sync Public to a role and restore its exact prior state.""" + original_value = app.config["PUBLIC_ROLE_LIKE"] + original_permission_ids = [ + permission.id for permission in security_manager.get_public_role().permissions + ] + try: + app.config["PUBLIC_ROLE_LIKE"] = role_name + security_manager.sync_role_definitions() + yield + finally: + app.config["PUBLIC_ROLE_LIKE"] = original_value + # Tests and role syncing can commit or replace the session's ORM objects. + security_manager.get_public_role().permissions = ( Review Comment: This block worries me a bit, and I think it is why the integration jobs are still red rather than anything being wrong with the restoration itself. `superset_test_config.py:122-123` set `PUBLIC_ROLE_LIKE = "Gamma"` and `AUTH_ROLE_PUBLIC = "Public"`, so `superset init` leaves the `Public` role holding the whole Gamma permission set (90 rows on my sqlite run), including `can_read on Dashboard` and `can_get_drill_info on Dataset`. The old unconditional `permissions = []` wiped those the first time `dashboard_tests.py` ran, and every later test that asserts an unauthenticated principal is rejected has been passing on that wipe. Restoring the exact prior state keeps them, so those tests now get `200` where they expect `401` or `403`. On one command, `dashboard_tests.py dashboards/api_tests.py users/api_tests.py themes/test_theme_api_permissions.py`, fresh sqlite DB: | tree | result | | --- | --- | | merge base `4e7e2b73` | `276 passed` | | this head | `5 failed, 289 passed` | The 5 are the four `TestCurrentUserApi` anonymous cases plus `test_anonymous_cannot_get_system_theme_slice`. Nothing caught it earlier because no test asserts what the `Public` baseline is between files. The smallest way out I found keeps your restoration logic, which nested use does need, but stops the shared teardown from being what grants `Public` anything: leave this file's teardown as it was, and let the guest label tests declare the grant themselves. ```python @pytest.mark.usefixtures("public_role_like_gamma") def test_get_dashboard_datasets_chart_display_labels_as_guest(self) -> None: ``` With that on the six methods behind the 12 failures, the same four file command gives `294 passed`. Reverting this file alone puts the 12 back, so the grant does have to live somewhere, just not in the session wide baseline. Would you rather go the other way and drop `PUBLIC_ROLE_LIKE = "Gamma"` from the integration config? That is the more honest fix, but it touches every test that quietly leans on it and I would not ask you to own that here. -- 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]
