fix: check if guest user modified query

[betodealmeida]

SUMMARY

Improve the logic that checks if a guest user has tampered with the query context or form data, preventing them from requesting metrics different from the ones saved in the chart.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:

After:

TESTING INSTRUCTIONS

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

[betodealmeida]

@michael-s-molina you'll be happy to see that I split this test into multiple tests, one for each case. 😉

[codecov]

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 69.72%. Comparing base (fde93dc) to head (6bf2f09).
Report is 1436 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/security/manager.py	78.94%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #27484      +/-   ##
==========================================
+ Coverage   67.34%   69.72%   +2.38%     
==========================================
  Files        1909     1909              
  Lines       74623    74676      +53     
  Branches     8324     8324              
==========================================
+ Hits        50255    52069    +1814     
+ Misses      22314    20553    -1761     
  Partials     2054     2054              

Flag	Coverage Δ
hive	53.71% <10.52%> (?)
mysql	78.01% <73.68%> (+0.01%)	⬆️
postgres	78.11% <73.68%> (+0.01%)	⬆️
presto	53.66% <10.52%> (?)
python	83.15% <78.94%> (+4.91%)	⬆️
sqlite	77.56% <73.68%> (-0.06%)	⬇️
unit	56.63% <78.94%> (?)

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.

[craig-rueda]

LGTM

[michael-s-molina]

@betodealmeida Does this fix #27177?

[JurajBurian]

Hello @betodealmeida,
@michael-s-molina, you closed #27177, but problem still exist. We have embeded dashboards with rls and native filters. After latst commit, embedding is not working. Message is the same :
superset_app | superset.exceptions.SupersetSecurityException: Guest user cannot modify chart payload

I patched manager.py on line 158 :

    # sanity checks
    if form_data is None or stored_chart is None:
        return False #replaced from True

and it help a lot, but then I hit the wall with another problem :

superset_app          |   File "/app/superset/security/manager.py", line 178, in <setcomp>
superset_app          |     frozenset(metric.items()) if isinstance(metric, dict) else metric
superset_app          | TypeError: unhashable type: 'dict'

unfortunatelly I am not Python expert, but this problem looks as "type" problem. Strange is, that exception happens only on tabs having some type of graphs - example mixed chart is affected , heat map or bar chart works fine.

[enzo-dechaene]

Hello @michael-s-molina the problem is also on the 3.0, can you add this label too ?

[betodealmeida]

superset_app          |   File "/app/superset/security/manager.py", line 178, in <setcomp>
superset_app          |     frozenset(metric.items()) if isinstance(metric, dict) else metric
superset_app          | TypeError: unhashable type: 'dict'

This error message is really weird, because we're explicitly checking if metric is a dict.

[betodealmeida]

#27177 should fix the RLS and mixed chart problems.