fix: Security manager incorrect calls

[michael-s-molina]

SUMMARY

While testing 4.1 migrations, I noticed that the return types of the security manager calls were not being correctly validated by the IDE. To fix this, I added a type hint to our proxy definition and was able to find and fix problems in the code.

TESTING INSTRUCTIONS

CI should be sufficient.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[michael-s-molina]

This was a problem introduced with the catalog feature. @betodealmeida let me know if None is correct in this context or how we should get the catalog.

[betodealmeida]

Ah, yeah, we need to implement support for catalogs in the CSV upload feature.

Every time you need a catalog in a function and one is not explicitly passed you should use database.get_default_catalog() — it will return None for databases that don't support catalogs, and the default catalog for those that do.

In this case, we need to pass database.get_default_catalog() to get_schemas_accessible_by_user, otherwise the permissions check won't match if the database supports catalogs.

[codecov]

Codecov Report

Attention: Patch coverage is 81.25000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 83.64%. Comparing base (76d897e) to head (02bff33).
Report is 966 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/connectors/sqla/models.py	81.48%	5 Missing ⚠️
superset/models/helpers.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #29884       +/-   ##
===========================================
+ Coverage   60.48%   83.64%   +23.15%     
===========================================
  Files        1931      528     -1403     
  Lines       76236    38155    -38081     
  Branches     8568        0     -8568     
===========================================
- Hits        46114    31914    -14200     
+ Misses      28017     6241    -21776     
+ Partials     2105        0     -2105     

Flag	Coverage Δ
hive	48.95% <28.12%> (-0.21%)	⬇️
javascript	?
mysql	76.64% <81.25%> (?)
postgres	76.73% <81.25%> (?)
presto	53.50% <56.25%> (-0.30%)	⬇️
python	83.64% <81.25%> (+20.15%)	⬆️
sqlite	76.22% <81.25%> (?)
unit	60.31% <65.62%> (+2.68%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[michael-s-molina]

@betodealmeida @villebro @hughhhh Could you help me fix the following errors?

superset/models/helpers.py:824: error: Argument 1 to "get_rls_filters" of "SupersetSecurityManager" 
has incompatible type "ExploreMixin"; expected "BaseDatasource"  [arg-type]

superset/models/helpers.py:834: error: Argument 1 to "get_guest_rls_filters" of "SupersetSecurityManager" 
has incompatible type "ExploreMixin"; expected "BaseDatasource"  [arg-type]

This the signature of the methods:

def get_rls_filters(self, table: "BaseDatasource") -> list[SqlaQuery]:

def get_guest_rls_filters(self, dataset: "BaseDatasource") -> list[GuestTokenRlsRule]:

The calls to the security manager methods were introduced by #22853 and it seems that we already needed to pass table and dataset at that time but we never did. Do you know where to get them from?

[villebro]

Changes look good to me, but @betodealmeida and @hughhhh are probably best positioned to comment on the open questions here.

[betodealmeida]

For the type error, I think the current get_sqla_row_level_filters method should be moved out of ExploreMixin and into BaseDatasource, since only datasets can have RLS filters associarted with them (while a Query can't). We might need to leave a no-op method in the ExploreMixin, since I assume when building a chart from a query the method will get called.

@hughhhh does this seem right to you?

[michael-s-molina]

@betodealmeida @sadpandajoe This have the potential to break current security checks but it's the correct comparison for the defined type. Let me know if it's ok or if we should change the type to str instead.

[michael-s-molina]

@betodealmeida I think I addressed all your comments.

[dpgaspar]

nice this will help the IDE a lot!

[dpgaspar]

thank you for doing this