chore(security): Renaming access methods

[john-bodley]

SUMMARY

This PR addresses the following:

1. Ensures consistent naming of the "access" methods for the security manager, i.e., previously we had methods named can_access_all_queries and all_datasource_access (note the singular "datasource"). The former is prefer as the return type is implied. Note I believe the later existed for possible consistency with the underlying FAB per names, however this isn't a requirement per the can_access_all_queries example. The renaming is as follows:

   • can_access_datasource -> can_access_table (more explicit and resolves naming conflict with the datasource_access rename)
   • all_datasource_access -> can_access_all_datasources (plural)
   • all_database_access -> can_access_all_databases (plural)
   • database_access -> can_access_database
   • schema_access -> can_access_schema
   • datasource_access -> can_access_datasource

2. The can_access_table (previously named can_access_datasource) method signature has changed. The optional schema argument no longer exists as it is defined within the Table object.

3. Removes unnecessary checks, i.e., previous logic was in the form,

if security_manager.database_access(database) or security_manager.all_datasource_access():
    ...

however the database_access method (now named can_access_database) first checks whether the user can access all datasources (per here) and thus the second check is irrelevant.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

CI.

ADDITIONAL INFORMATION

• Has associated issue:
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

[codecov-commenter]

Codecov Report

Merging #10031 into master will increase coverage by 0.00%.
The diff coverage is 96.00%.

@@           Coverage Diff           @@
##           master   #10031   +/-   ##
=======================================
  Coverage   68.88%   68.88%           
=======================================
  Files         584      584           
  Lines       31058    31054    -4     
  Branches     3180     3180           
=======================================
- Hits        21393    21392    -1     
+ Misses       9555     9552    -3     
  Partials      110      110           

Flag	Coverage Δ
#cypress	53.89% <ø> (+0.07%)	⬆️
#javascript	59.48% <ø> (ø)
#python	67.33% <96.00%> (-0.01%)	⬇️

Impacted Files	Coverage Δ
superset/dashboards/filters.py	96.55% <ø> (-0.12%)	⬇️
superset/views/dashboard/filters.py	95.23% <ø> (-0.22%)	⬇️
superset/views/core.py	75.39% <80.00%> (ø)
superset/charts/filters.py	95.45% <100.00%> (ø)
superset/security/manager.py	89.04% <100.00%> (-0.08%)	⬇️
superset/views/base.py	73.47% <100.00%> (ø)
superset/views/chart/filters.py	100.00% <100.00%> (ø)
superset/views/database/decorators.py	96.15% <100.00%> (ø)
superset/views/database/filters.py	100.00% <100.00%> (ø)
superset/views/database/forms.py	90.69% <100.00%> (ø)
... and 2 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54c6ddb...1c0bb1d. Read the comment docs.

[villebro]

Consistent naming FTW 👍 LGTM, but if we decide to go down the aliasing with deprecation road I think that'd be really friendly to admins of Superset installations.

[john-bodley]

@villebro I agree with your comment however we run into a name conflict issue per #10030 (comment).

Note at Airbnb we have a unit test which checks for possible changes in the base security manager (as well as pylint checks which ensures that method signatures are consistent) given the potential impact of the change, i.e., one may no longer be overriding the same method.

import inspect
from typing import inspect

from superset.security import SupersetSecurityManager

import MySecurityManager


def test_sm_parity_public_methods() -> None:
    def _methods(klass: SupersetSecurityManager) -> Set[str]:
        return {
            name
            for name, _ in inspect.getmembers(klass, predicate=inspect.isfunction)
            if not name.startswith("_")
        }

    assert _methods(MySecurityManager) == _methods(SupersetSecurityManager)

Maybe it's worthwhile adding this to the documentation.

[villebro]

LGTM (in reference to #10030 )