fix(QueryContext): validation does not validate query_context metrics #19753

ofekisr · 2022-04-18T10:59:05Z

There is an option to add custom SQL expression metrics inside some of the charts visualizations so the fetched data will be aggregated based on the SQL expression.
When the data is fetched the query context itself is validated if the user can access the data
but the additional sql query added from the metric was missed out and not validated so it can be manipulated so now not allowed data will be fetched too.

The PR adds logic for it. right now only to those visualizations their data fetched by Charts API

follow-up PR's would be:

apply also to custom SQL column expressions

codecov · 2022-04-18T16:02:53Z

Codecov Report

Attention: Patch coverage is 86.09865% with 31 lines in your changes missing coverage. Please review.

Project coverage is 66.58%. Comparing base (3c28cd4) to head (19f8874).
Report is 4280 commits behind head on master.

Files	Patch %	Lines
...data/query_context_validators/access_validators.py	87.65%	10 Missing ⚠️
superset/common/query_context.py	77.27%	5 Missing ⚠️
superset/charts/data/commands/command_factory.py	84.00%	4 Missing ⚠️
.../data/query_context_validators/validaor_factory.py	83.33%	4 Missing ⚠️
...t/charts/data/query_context_validators/__init__.py	77.77%	2 Missing ⚠️
...erset/charts/data/query_context_validators/impl.py	89.47%	2 Missing ⚠️
superset/common/query_object.py	60.00%	2 Missing ⚠️
superset/charts/data/api.py	85.71%	1 Missing ⚠️
superset/charts/data/commands/get_data_command.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #19753      +/-   ##
==========================================
+ Coverage   66.53%   66.58%   +0.05%     
==========================================
  Files        1692     1696       +4     
  Lines       64777    64988     +211     
  Branches     6660     6656       -4     
==========================================
+ Hits        43101    43275     +174     
- Misses      19977    20014      +37     
  Partials     1699     1699

Flag	Coverage Δ
hive	`52.89% <77.57%> (+0.03%)`	⬆️
mysql	`81.92% <86.09%> (+0.01%)`	⬆️
postgres	`81.96% <86.09%> (+0.01%)`	⬆️
presto	`52.74% <77.57%> (+0.03%)`	⬆️
python	`82.38% <86.09%> (+<0.01%)`	⬆️
sqlite	`81.73% <86.09%> (+0.01%)`	⬆️
unit	`47.79% <67.26%> (-0.14%)`	⬇️

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.

ofekisr · 2022-04-19T08:41:53Z

@AAfghahi
please review why your test is failed since #19700
I don't find why

ofekisr · 2022-04-19T17:46:42Z

@AAfghahi told me to change back the excepted result of the failed test until he will figure it out.
instead of it, I mark the test to be skipped

…lidation

…alidation

…tion

tests/integration_tests/databases/api_tests.py

eschutho · 2022-05-17T23:11:56Z

superset/charts/data/query_context_validators/validaor_factory.py

+        return QueryContextValidatorWrapper()
+
+    def _make_access_validator(self, is_sql_db: bool) -> QueryContextValidator:
+        if is_sql_db:


are you checking here that the db is sql and not no-sql Druid? If so, we've removed native Druid support in 2.0.

eschutho · 2022-05-17T23:26:00Z

superset/charts/data/commands/command_factory.py

+
+    def _make(self, query_context: QueryContext) -> BaseCommand:
+        validator = self._validator_factory.make(self._is_use_sql_db(query_context))
+        return ChartDataCommand(query_context, validator)


I'm wondering if this factory abstraction is necessary for the validation or if this is something we can just put into the command?

dpgaspar · 2022-05-18T08:21:30Z

superset/charts/data/commands/command_factory.py

+    from superset.commands.base import BaseCommand
+    from superset.common.query_context import QueryContext
+
+    from ..query_context_validators.validaor_factory import QueryContextValidatorFactory


We avoid using relative imports
there's seems to be a typo also: validaor_factory

zhaoyongjie · 2022-05-19T01:56:55Z

superset/charts/data/commands/command_factory.py

+    @classmethod
+    def init(cls, validator_factory: QueryContextValidatorFactory) -> None:
+        cls._instance = GetChartDataCommandFactory(validator_factory)
+
+    def __init__(self, validator_factory: QueryContextValidatorFactory):
+        self._validator_factory = validator_factory
+
+    @classmethod
+    def make(cls, query_context: QueryContext) -> BaseCommand:
+        if cls._instance is None:
+            raise RuntimeError("GetChartDataCommandFactory was not initialized")


Do we really need to make so many constructors?

rusackas · 2024-08-20T02:50:27Z

Hi @ofekisr - just checking in to see if you're still able/willing to get this across the finish line. I'll convert it to draft while it awaits a rebase and responses to comments. Thanks!

ofekisr · 2024-08-20T10:15:01Z

no I don't Ofek

…

On Tue, Aug 20, 2024 at 5:50 AM Evan Rusackas ***@***.***> wrote: Hi @ofekisr <https://github.com/ofekisr> - just checking in to see if you're still able/willing to get this across the finish line. I'll convert it to draft while it awaits a rebase and responses to comments. Thanks! — Reply to this email directly, view it on GitHub <#19753 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AIQMHESG3UAHMCVM23RQG7DZSKVI5AVCNFSM6AAAAABMY6ZXOSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEOJXHA3DGMBSGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

pull-request-size bot added the size/XXL label Apr 18, 2022

ofekisr mentioned this pull request Apr 18, 2022

fix(QueryContext): validation does not validate query_context metrics #16991

Closed

ofekisr force-pushed the fix/sql_metric_access branch from cf8be09 to 8fbce8e Compare April 18, 2022 11:15

ofekisr closed this Apr 18, 2022

ofekisr deleted the fix/sql_metric_access branch April 18, 2022 14:53

ofekisr reopened this Apr 18, 2022

ofekisr restored the fix/sql_metric_access branch April 18, 2022 14:55

ofekisr closed this Apr 18, 2022

ofekisr reopened this Apr 18, 2022

ofekisr force-pushed the fix/sql_metric_access branch from 8fbce8e to b2db382 Compare April 18, 2022 14:58

pull-request-size bot added size/XS and removed size/XXL labels Apr 18, 2022

ofekisr closed this Apr 18, 2022

ofekisr force-pushed the fix/sql_metric_access branch from b2db382 to cf51459 Compare April 18, 2022 15:04

ofekisr reopened this Apr 18, 2022

pull-request-size bot added size/XXL and removed size/XS labels Apr 18, 2022

ofekisr force-pushed the fix/sql_metric_access branch from f382cc8 to 913e4f1 Compare April 18, 2022 15:11

ofekisr force-pushed the fix/sql_metric_access branch from d34bcb5 to f33a120 Compare April 18, 2022 21:32

ofekisr force-pushed the fix/sql_metric_access branch from 3e7fdbd to 7be5bf2 Compare April 19, 2022 17:49

ofekisr added 7 commits April 20, 2022 15:16

add database property to query_context to be used by query_context_va…

ef1fef6

…lidation

add datasource property to query_object to be used by query_context_v…

92cd6af

…alidation

add query_context_validator unit

b836667

refactor(security_manager): extract to method to prevent code reusing

b7900e1

add new class methods to fetch datasets by sql components

3f92bbd

refactor(query_context): decouple query_context from processor

071ccd4

add query object generator method

01e4f02

ofekisr added 11 commits April 20, 2022 15:16

add query_context_access_validators

aa45c1e

add query_context validator as old behavior

58e64c7

add validator factory to be used by command factory (next-commit)

d0bdfa9

add command factory to be used by api.py

fc10245

add new single_column_example table for tests setups

186a4b6

add new feature flag for indicating of using new query context valida…

b810a33

…tion

add new query_context_validation to the chart data flows

ae99fe7

fix lint

119a9ae

fix failed tests on postgress

e278ce6

fix failed tests

fd7eb45

fix failed tests

7ee4698

ofekisr force-pushed the fix/sql_metric_access branch from 7be5bf2 to 7ee4698 Compare April 20, 2022 12:18

fix failed tests

a9b9291

AAfghahi reviewed Apr 20, 2022

View reviewed changes

tests/integration_tests/databases/api_tests.py Show resolved Hide resolved

ofekisr requested a review from amitmiran137 April 20, 2022 14:47

fix failed tests

6ba9a35

ofekisr force-pushed the fix/sql_metric_access branch from 7613eb7 to 6ba9a35 Compare April 20, 2022 17:11

add skip test if backend is presto/hive

19f8874

amitmiran137 requested a review from a team April 24, 2022 14:00

hughhhh requested a review from betodealmeida April 26, 2022 16:45

eschutho reviewed May 17, 2022

View reviewed changes

dpgaspar reviewed May 18, 2022

View reviewed changes

zhaoyongjie reviewed May 19, 2022

View reviewed changes

rusackas marked this pull request as draft August 20, 2024 02:50

michael-s-molina added the review:draft label Aug 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(QueryContext): validation does not validate query_context metrics #19753

fix(QueryContext): validation does not validate query_context metrics #19753

ofekisr commented Apr 18, 2022 •

edited

Loading

codecov bot commented Apr 18, 2022 •

edited

Loading

ofekisr commented Apr 19, 2022 •

edited

Loading

ofekisr commented Apr 19, 2022

eschutho May 17, 2022

eschutho May 17, 2022

dpgaspar May 18, 2022 •

edited

Loading

zhaoyongjie May 19, 2022

rusackas commented Aug 20, 2024

ofekisr commented Aug 20, 2024 via email

fix(QueryContext): validation does not validate query_context metrics #19753

Are you sure you want to change the base?

fix(QueryContext): validation does not validate query_context metrics #19753

Conversation

ofekisr commented Apr 18, 2022 • edited Loading

codecov bot commented Apr 18, 2022 • edited Loading

Codecov Report

ofekisr commented Apr 19, 2022 • edited Loading

ofekisr commented Apr 19, 2022

eschutho May 17, 2022

Choose a reason for hiding this comment

eschutho May 17, 2022

Choose a reason for hiding this comment

dpgaspar May 18, 2022 • edited Loading

Choose a reason for hiding this comment

zhaoyongjie May 19, 2022

Choose a reason for hiding this comment

rusackas commented Aug 20, 2024

ofekisr commented Aug 20, 2024 via email

ofekisr commented Apr 18, 2022 •

edited

Loading

codecov bot commented Apr 18, 2022 •

edited

Loading

ofekisr commented Apr 19, 2022 •

edited

Loading

dpgaspar May 18, 2022 •

edited

Loading