fix: prevent guest users from changing columns #29530
Conversation
betodealmeida
force-pushed
the
prevent-column-modifications
branch
from
15467b8
to
08833b6
Compare
superset/security/manager.py
Outdated
| @@ -145,9 +145,9 @@ def __init__(self, **kwargs: Any) -> None: | |||
| RoleModelView.related_views = [] | |||
|
|
|||
|
|
|||
| def freeze_metric(metric: Metric) -> str: | |||
| def freeze_value(metric: Metric) -> str: | |||
There was a problem hiding this comment.
Super nitpicky, but maybe we want to also change the parameter name from metric to something generic as well (such as value)?
There was a problem hiding this comment.
D'oh, good point! I missed that!
superset/security/manager.py
Outdated
| for metric in query.metrics or [] | ||
| } | ||
| # compare columns and metrics in form_data with stored values | ||
| for key in ["metrics", "columns"]: |
There was a problem hiding this comment.
I think some viz types might have the columns listed as groupby in the form_data:
There was a problem hiding this comment.
I tested via the browser (just inspecting the request the frontend sends to the /api/v1/chart/data endpoint), but it's possible this structure changes until it gets to this point, so I could be wrong.
There was a problem hiding this comment.
I do see it listed as columns in the queries, tho:
|
Good point! I added |
betodealmeida
force-pushed
the
prevent-column-modifications
branch
from
3b395b8
to
bee3895
Compare
|
Question from the PR review session: If the last PR that this builds on caused a lot of Issues for embedded users, will there be any additional fallout from this PR, and will we be available to address these concerns? |
|
Not sure if/how this intersects with this issue. |
|
@rusackas I'm not expecting as much trouble as the first PR, since this one just extends the first one for a few more attributes. |
SUMMARY
An extension to #26749, preventing guest users from modifying adhoc columns when requesting charts.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Added unit tests.
ADDITIONAL INFORMATION