fix: skip perms on query context update

[betodealmeida]

SUMMARY

When building a CSV report for an old chart, the worker needs to fetch a screenshot of the chart in order to populate the query_context column of the chart. This is needed to request CSV data for the chart if it hasn't been saved with its query_context.

One problem in doing this is that the worker generating the report might not own the chart, so they are unable to populate query_context. I added a check to bypass the permission check only when the request is just for query_object.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

1. Create a CSV report of a chart from the examples that hasn't been opened yet (so that query_context is null).
2. Configure screenshots with a low-level user that doesn't own the chart (ie, not "admin").
3. Reports should work fine.

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

[codecov]

Codecov Report

Merging #16250 (eb1fe87) into master (6cd15d5) will decrease coverage by 0.22%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #16250      +/-   ##
==========================================
- Coverage   76.75%   76.53%   -0.23%     
==========================================
  Files         997      997              
  Lines       53195    53198       +3     
  Branches     6764     6764              
==========================================
- Hits        40830    40715     -115     
- Misses      12135    12253     +118     
  Partials      230      230              

Flag	Coverage Δ
hive	?
mysql	81.54% <100.00%> (-0.04%)	⬇️
postgres	81.60% <100.00%> (+<0.01%)	⬆️
presto	?
python	81.69% <100.00%> (-0.43%)	⬇️
sqlite	81.25% <100.00%> (+0.04%)	⬆️

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

Impacted Files	Coverage Δ
superset/charts/commands/update.py	88.88% <100.00%> (+0.48%)	⬆️
superset/db_engines/hive.py	0.00% <0.00%> (-82.15%)	⬇️
superset/db_engine_specs/hive.py	69.80% <0.00%> (-16.87%)	⬇️
superset/db_engine_specs/presto.py	83.47% <0.00%> (-6.91%)	⬇️
superset/views/database/mixins.py	81.03% <0.00%> (-1.73%)	⬇️
superset/connectors/sqla/models.py	88.04% <0.00%> (-1.66%)	⬇️
superset/db_engine_specs/base.py	88.00% <0.00%> (-0.39%)	⬇️
superset/models/core.py	89.61% <0.00%> (-0.26%)	⬇️
superset/utils/core.py	88.98% <0.00%> (-0.13%)	⬇️

---

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 6cd15d5...eb1fe87. Read the comment docs.

[eschutho]

Looks good, but question for my understanding, why couldn't we give the machine user permission for the charts that it doesn't own?

[betodealmeida]

Looks good, but question for my understanding, why couldn't we give the machine user permission for the charts that it doesn't own?

I think we'd need to add the machine user as an owner to all the charts in order to do that, which would require hooks for new charts, and could potentially be confusing to the user (why is "machine" also an owner of my chart?). It would also allow the machine user to delete or modify charts, while with the approach in this PR we only let it update query_context.