tien238lnd commented on PR #43994: URL: https://github.com/apache/superset/pull/43994#issuecomment-5611655362
> Thanks for testing and for flagging this. The regression tests have been added in [d73aa7c](https://github.com/apache/superset/commit/d73aa7cc538dd9f23a3d02c974cd8bd28a73e7b6). Could you please share the specific path you have in mind where a Query reaches can_access_schema() with its database relationship unset? I'd like to verify it and add coverage if needed. @Kunal8954 sure — the guard on `Query.schema_perm` is right, but it sits one level below where the crash happens, so the path is still open on `eb7ebc9`. `can_access_schema()` reaches `can_access_database(datasource.database)` before it ever touches `schema_perm`. The widened gate admits a `Query` on `hasattr(datasource, "database")`, and for a transient query that attribute *exists* — its value is just `None`. So: ```python q = Query(client_id="abc", database_id=999, sql="SELECT 1", schema="s1") q.schema_perm # '' — your guard works sm.can_access_schema(q) # AttributeError: 'NoneType' object has no attribute 'perm' # manager.py:2259 can_access_schema -> manager.py:2217 can_access_database ``` Unpatched master returns `False` there (the `isinstance` gate excluded `Query` outright), so this one is new to the PR rather than pre-existing. Testing the value instead of the attribute closes it: ```python if isinstance(datasource, BaseDatasource) or ( getattr(datasource, "database", None) is not None and hasattr(datasource, "schema_perm") ): ``` I applied that on top of `eb7ebc9`: `can_access_schema(q)` returns `False` instead of raising, your new unit tests still pass (162 passed), and the explore controls from the issue are unaffected. Worth a test too — `test_can_access_schema_query_schema_access` and `test_can_access_schema_query_denied_ungranted_schema` both set `query.database = MagicMock()`, and `test_query_schema_perm_guards_unset_database` only exercises the property, so nothing currently covers `can_access_schema()` with the relationship unset. -- 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]
