Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(embedded+async queries): support async queries to work with embedded guest user #26332

Merged
merged 12 commits into from
Jan 9, 2024
Merged

fix(embedded+async queries): support async queries to work with embedded guest user #26332

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e76a4fa
chore: load guest user jwt into request context
Dec 19, 2023
48ed6c9
properly setting form_data into query context cache so that it can be…
Dec 21, 2023
5743dea
added tests
Dec 21, 2023
6891185
removed setting guest_token in g
Dec 22, 2023
1df0373
clean up
Dec 22, 2023
59d8703
pass guess token to load_chart_data task for user context
Dec 22, 2023
c681637
don't get user if id is None
Dec 22, 2023
2a6379b
adding guest user support for explore_json endpoint
Jan 3, 2024
65fc1f1
adding tests
Jan 4, 2024
9e6227e
fixed tests
Jan 4, 2024
822c537
addressed comment
Jan 4, 2024
9b3c559
DRY
Jan 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion superset/common/query_context_processor.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,15 @@ def get_payload(
set_and_log_cache(
cache_manager.cache,
cache_key,
{"data": self._query_context.cache_values},
{
"data": {
# setting form_data into query context cache value as well
# so that it can be used to reconstruct form_data field
# for query context object when reading from cache
"form_data": self._query_context.form_data,
Comment on lines +605 to +608
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have concerns here.. We've discussed this at length previously, and I feel we should always consider form_data to be chart specific internal state. IIRC, the original reason that form data is being passed along to the celery task is because the legacy charts require it to function. However, the v1 charts shouldn't need it. I know many fields within form_data are sort of standardized, but the whole point of QueryContext/QueryObject was to standardize the chart data request contract, thus decoupling the viz plugins from the backend. Therefore, I feel any field that is required from form_data should ideally be allocated a dedicated property on either the query context or query object.

**self._query_context.cache_values,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be used by this later https://github.com/apache/superset/blob/master/superset/security/manager.py#L1898C1-L1898C1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@villebro do you have any thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zephyring from where does the raise_for_access get called when it is passing in the query context from cache?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first call to /chart/data endpoint will set the query context into cache and return async job metadata.
Then later once job is done frontend will receive a job ready event with the result_url something like (/api/v1/chart/data/qc-2772209e0a4b1ca513375118855b27af).
Then it uses that to call chart/data/<cache> endpoint which invokes the ChartCommand and the command validation will call the raise_for_access

},
},
self.get_cache_timeout(),
)
return_value["cache_key"] = cache_key # type: ignore
Expand Down
8 changes: 6 additions & 2 deletions superset/security/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
from collections import defaultdict
from typing import Any, Callable, cast, NamedTuple, Optional, TYPE_CHECKING, Union

from flask import current_app, Flask, g, Request
from flask import _request_ctx_stack, current_app, Flask, g, Request
from flask_appbuilder import Model
from flask_appbuilder.security.sqla.manager import SecurityManager
from flask_appbuilder.security.sqla.models import (
Expand Down Expand Up @@ -2224,7 +2224,11 @@
logger.warning("Invalid guest token", exc_info=True)
return None

return self.get_guest_user_from_token(cast(GuestToken, token))
user = self.get_guest_user_from_token(cast(GuestToken, token))

Check warning on line 2227 in superset/security/manager.py

View check run for this annotation

Codecov / codecov/patch

superset/security/manager.py#L2227 

Added line #L2227 was not covered by tests

# set the guest user jwt to request context
g.guest_token = token
return user

Check warning on line 2231 in superset/security/manager.py

View check run for this annotation

Codecov / codecov/patch

superset/security/manager.py#L2230-L2231 

Added lines #L2230 - L2231 were not covered by tests

def get_guest_user_from_token(self, token: GuestToken) -> GuestUser:
return self.guest_user_cls(
Expand Down
Loading