fix(QueryContext): validation does not validate query_context metrics

[ofekisr]

SUMMARY

close
continue in #19753

[codecov]

Codecov Report

Merging #16991 (66ac874) into master (f8a65f8) will increase coverage by 1.33%.
The diff coverage is 86.45%.

❗ Current head 66ac874 differs from pull request most recent head dd48e73. Consider uploading reports for the commit dd48e73 to get more accurate results

@@            Coverage Diff             @@
##           master   #16991      +/-   ##
==========================================
+ Coverage   67.10%   68.43%   +1.33%     
==========================================
  Files        1609     1590      -19     
  Lines       64897    65082     +185     
  Branches     6866     6963      +97     
==========================================
+ Hits        43547    44542     +995     
+ Misses      19484    18650     -834     
- Partials     1866     1890      +24     

Flag	Coverage Δ
hive	?
mysql	82.04% <86.45%> (-0.15%)	⬇️
postgres	82.05% <86.45%> (-0.20%)	⬇️
presto	?
python	82.13% <86.45%> (-0.60%)	⬇️
sqlite	81.73% <86.45%> (-0.20%)	⬇️

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

Impacted Files	Coverage Δ
superset/config.py	91.53% <ø> (-0.11%)	⬇️
superset/common/query_object.py	92.13% <60.00%> (-3.50%)	⬇️
superset/datasets/dao.py	93.24% <66.66%> (-0.55%)	⬇️
superset/common/query_context.py	86.15% <77.27%> (-6.58%)	⬇️
superset/tasks/async_queries.py	90.24% <83.33%> (-0.79%)	⬇️
...set/charts/data/query_context_validator_factory.py	85.71% <85.71%> (ø)
superset/charts/data/commands/get_data_command.py	96.96% <87.50%> (-3.04%)	⬇️
superset/charts/data/query_context_validator.py	89.41% <89.41%> (ø)
superset/charts/data/api.py	88.31% <100.00%> (-0.51%)	⬇️
superset/security/manager.py	91.92% <100.00%> (-0.05%)	⬇️
... and 660 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 f8a65f8...dd48e73. Read the comment docs.

[amitmiran137]

Extract until you can't

[amitmiran137]

LGTM !
Feature introduced here will enforce datasource access on custom SQL expression of metrics

@villebro we should add column in a follow up PR

[craig-rueda]

Thanks for this PR - it's bumping up security! I don't see any tests, however. Also, there's a lot of single line methods in the ContextValidator. Can we compress those a bit? Too much delegation makes the code a bit hard to follow along with.

[ofekisr]

@craig-rueda @hughhhh @jayakrishnankk
After all the recent refactor in ChartData, I recommitted the PR with all the adjustments to refactored code

[villebro]

First pass comments. I think this is a great improvement to the security model, but I'm concerned this may cause false positives (I did a similar change some time ago, which caused unpleasant regressions due to triggering false positives in edge cases that hadn't been thought of in the original PR). To make rolling out new security improvements less problematic, I would almost consider introducing (yet another) feature flag "SECURITY_BETA" where new security improvements can be introduced and kept until more comprehensive testing has been done.

[jayakrishnankk]

@craig-rueda - requesting review. bumping this.