You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "jfrag1 (via GitHub)" <gi...@apache.org> on 2023/11/06 17:30:10 UTC
[PR] fix(embedded): Hide sensitive payload data from guest users [superset]
jfrag1 opened a new pull request, #25878:
URL: https://github.com/apache/superset/pull/25878
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Currently, guest users can get access to more data than they need to when viewing an embedded dashboard. The `api/v1/dashboard/{id}` and `api/v1/dashboard/{id}/datasets` endpoints would return owners' first and last names. The latter would return information about the underlying database(s) the datasets belong to, which could be useful for an attacker. None of this data is necessary for the embedded dashboard to render.
I recognize this PR does break typical API patterns by stripping out certain data depending on the type of user requesting, which is pretty ugly. However, this approach avoids making breaking changes for regular API use, while making the embedded experience more secure.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: https://github.com/apache/superset/issues/22960
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1395169526
##########
superset/dashboards/schemas.py:
##########
@@ -198,6 +199,15 @@ class DashboardGetResponseSchema(Schema):
changed_on_humanized = fields.String(data_key="changed_on_delta_humanized")
is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
+ # pylint: disable=unused-argument
+ @post_dump()
+ def post_dump(self, serialized: dict[str, Any], **kwargs: Any) -> dict[str, Any]:
+ if security_manager.is_guest_user():
+ serialized["owners"] = []
Review Comment:
I would prefer to fully remove the property `owners`, as replacing it with an empty list is IMO conveying incorrect info
##########
superset/dashboards/schemas.py:
##########
@@ -247,6 +257,14 @@ class DashboardDatasetSchema(Schema):
normalize_columns = fields.Bool()
always_filter_main_dttm = fields.Bool()
+ # pylint: disable=unused-argument
+ @post_dump()
+ def post_dump(self, serialized: dict[str, Any], **kwargs: Any) -> dict[str, Any]:
+ if security_manager.is_guest_user():
+ serialized["owners"] = []
Review Comment:
Same here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1389361013
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
> I do appreciate the discussion here, but this really feels like premature optimization to me. The changes in this PR only affect two endpoints, and I don't predict that this kind of logic will needed in a widespread manner.
I thought that by referencing the concept of [Column Level Security (CLS)](https://blog.fabric.microsoft.com/en-us/blog/announcing-column-level-row-level-security-for-fabric-warehouse-sql-endpoint/) it would be more clear why this is not a premature optimization and why we will have the same need in a widespread manner.
> I'd argue the API layer is where that kind of security check belongs
If you research a little bit about CLS you'll see that many of these security checks are implemented at the database layer, even further from the API layer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1388755564
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
I do appreciate the discussion here, but this really feels like premature optimization to me. The changes in this PR only affect two endpoints, and I don't predict that this kind of logic will needed in a widespread manner.
If I'm wrong and there is a larger need/desire for this kind of security measure, I definitely agree we should re-assess, but I personally don't see a need to come up with a more "perfect" solution at this point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1391677662
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
> I thought that by referencing the concept of [Object Level Security (OLS)](https://learn.microsoft.com/en-us/power-bi/enterprise/service-admin-ols) it would be more clear why this is not a premature optimization and why we will have the same need in a widespread manner.
This is referring to a security feature similar to row-level security where Superset could apply security filters to queries against connected databases, while the security issue at hand in this PR has to do with data from the Superset metadata DB.
> If you research a little bit about CLS you'll see that many of these security checks are implemented at the database layer, even further from the API layer.
My understanding is that CLS is a security feature commonly implemented in data warehouses/BI tools, not a guide for how a server should implement security checks for data it owns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1388845600
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
Yes. Ideally Marshmallow should be used to define both the request and response payload.
Additionally if this is going to reside in the API layer (at least in the interim) could we at least factor the code to adhere to the DRY principle?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on PR #25878:
URL: https://github.com/apache/superset/pull/25878#issuecomment-1839187688
@villebro Are you able to take another look at this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1387267008
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
@betodealmeida @dpgaspar @michael-s-molina @villebro what are your thoughts on where sanitization of the data response should occur? Note this doesn't seem to be API specific and thus I'm not sure we should explicitly be defining said logic there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1388787105
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
Here's a slight shift left solution, that's not quite to the DAO layer. Currently the DAO fetches the entire row, and then we're using Marshmallow to decide what is sent back to the client. Can we let Marshmallow decide what should/shouldn't be returned in the response schema since that is the mechanism that currently handles this responsibility?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1389690895
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
I agree it's risky to implement this fix in a single interface, as it gives no guarantees that this information won't be exposed via some other interface, either pre-existing, or one that's added later. Mind you, this is not the only security concern regarding guest tokens - the issue of exposing granular information about the underlying query has been raised multiple times in the community, and is also something I feel we need to tackle soon.
Could we setup a meeting to discuss viable long-term solutions to these issues? I propose meeting to agree on the the following:
- A long term vision for fixing this and other similar security concerns. I agree with @michael-s-molina that baking this into the security work I've started sounds like a good idea. I can share what I have so far and my thoughts of how this can be taken forward.
- Agreeing on a quick fix to unblock heavy users of guest tokens to bridge the gap between now and when we have a proper security mechanism in place for handling this and other similar security issues.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1388756778
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
> Ideally, you won't query for what you don't have access to, eliminating this concern because the values won't be in the session.
I'm not sure this is the right direction, imagine a situation where the backend needs to read a column for some reason, even though we don't want to surface that field to the logged-in user. It seems wrong to tie the server's hands behind its back and not allow it to access the data just because of the current logged-in user. I'd argue the API layer is where that kind of security check belongs, since it's sort of the gateway from the backend to the user.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1389361013
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
> I do appreciate the discussion here, but this really feels like premature optimization to me. The changes in this PR only affect two endpoints, and I don't predict that this kind of logic will needed in a widespread manner.
I thought that by referencing the concept of [Object Level Security (OLS)](https://learn.microsoft.com/en-us/power-bi/enterprise/service-admin-ols) it would be more clear why this is not a premature optimization and why we will have the same need in a widespread manner.
> I'd argue the API layer is where that kind of security check belongs
If you research a little bit about CLS you'll see that many of these security checks are implemented at the database layer, even further from the API layer.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1387960700
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
Implementing this at the API layer is problematic because there's no single point of verification and also because you will query data that you don't have access to and filter it out later, wasting resources (imagine a table with many columns and you only have access to one column). This will also be true for the suggested code below:
```
dashboard = db.session.query(...
if is_guest:
dashboard.owners = []
dashboard.changed_by = None
```
Considering our current architecture, where we offer a [RLS (Row Level Security)](https://docs.preset.io/docs/row-level-security-rls) solution instead of depending on the database layer for these types of controls, the correct way of implementing this is to augment our security model to also support [Column Level Security (CLS)](https://blog.fabric.microsoft.com/en-us/blog/announcing-column-level-row-level-security-for-fabric-warehouse-sql-endpoint/) or even [Object Level Security (OLS)](https://learn.microsoft.com/en-us/power-bi/enterprise/service-admin-ols).
> I don't like this since it introduces the possibility of these changes accidentally being committed
Ideally, you won't query for what you don't have access to, eliminating this concern because the values won't be in the session.
Pinging @villebro for awareness given his current work on an improved security model.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1385556613
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
@john-bodley Thanks for reviewing, but I'm not sure how we're supposed to properly filter out certain fields of a record in the DAO layer. As far as I'm aware the security logic in the DAO layer is only "row-level", protecting entire objects, there's no column-level protections. Are you suggesting in the appropriate DAO method do something like
```python
dashboard = db.session.query(...
if is_guest:
dashboard.owners = []
dashboard.changed_by = None
```
I don't like this since it introduces the possibility of these changes accidentally being committed, but please let me know if you had something else in mind or if this pattern does exist currently but I'm just not aware of it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1395226765
##########
superset/dashboards/schemas.py:
##########
@@ -198,6 +199,15 @@ class DashboardGetResponseSchema(Schema):
changed_on_humanized = fields.String(data_key="changed_on_delta_humanized")
is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
+ # pylint: disable=unused-argument
+ @post_dump()
+ def post_dump(self, serialized: dict[str, Any], **kwargs: Any) -> dict[str, Any]:
+ if security_manager.is_guest_user():
+ serialized["owners"] = []
Review Comment:
@villebro I agree. The only caveat would be if the owners field is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1385321761
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
@jfrag1 we should adhere to the "shift left" mentality and filter out inaccessible fields at the DAO layer as opposed to the API. This ensures we adhere to the DRY principle avoiding the need to repeat (and/or miss) said logic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 merged PR #25878:
URL: https://github.com/apache/superset/pull/25878
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 closed pull request #25878: fix(embedded): Hide sensitive payload data from guest users
URL: https://github.com/apache/superset/pull/25878
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1395301233
##########
superset/dashboards/schemas.py:
##########
@@ -198,6 +199,15 @@ class DashboardGetResponseSchema(Schema):
changed_on_humanized = fields.String(data_key="changed_on_delta_humanized")
is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
+ # pylint: disable=unused-argument
+ @post_dump()
+ def post_dump(self, serialized: dict[str, Any], **kwargs: Any) -> dict[str, Any]:
+ if security_manager.is_guest_user():
+ serialized["owners"] = []
Review Comment:
@john-bodley ideally I think we should relax that requirement if we want to make this data conditional on the permissions of the logged in user. Based on the proposed changes, I think it should be as follows:
- regular user: all relevant owner info is conveyed (current behavior)
- guest token: no owner property is present in the response
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1391677662
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
> I thought that by referencing the concept of [Object Level Security (OLS)](https://learn.microsoft.com/en-us/power-bi/enterprise/service-admin-ols) it would be more clear why this is not a premature optimization and why we will have the same need in a widespread manner.
This is referring to a security feature similar to row-level security where Superset could apply security filters to queries against connected databases, while the security issue at hand in this PR has to do with data from the Superset metadata DB.
> If you research a little bit about CLS you'll see that many of these security checks are implemented at the database layer, even further from the API layer.
My understanding is that CLS is for a security feature commonly implemented in data warehouses/BI tools, not a guide for how a server should implement security checks for data it owns
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1393615690
##########
superset/dashboards/api.py:
##########
@@ -341,7 +346,13 @@ def get(
404:
$ref: '#/components/responses/404'
"""
- result = self.dashboard_get_response_schema.dump(dash)
+ schema = (
+ self.dashboard_get_response_schema_guest
+ if security_manager.is_guest_user()
+ else self.dashboard_get_response_schema
Review Comment:
@jfrag1 I think we can even move the schema logic into Marshmallow dynamically with either a type function or a @marshmallow.post_dump decorator. That way the logic isn't in the api layer and you won't need two schemas.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
eschutho commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1393616870
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
I think it's a great idea to meet on long term solutions. If we were to move the logic for this endpoint into the schema (I left one more suggestion that would remove this from the api altogether) should we move forward with this for now as @villebro suggested?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "jfrag1 (via GitHub)" <gi...@apache.org>.
jfrag1 commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1396795819
##########
superset/dashboards/schemas.py:
##########
@@ -198,6 +199,15 @@ class DashboardGetResponseSchema(Schema):
changed_on_humanized = fields.String(data_key="changed_on_delta_humanized")
is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
+ # pylint: disable=unused-argument
+ @post_dump()
+ def post_dump(self, serialized: dict[str, Any], **kwargs: Any) -> dict[str, Any]:
+ if security_manager.is_guest_user():
+ serialized["owners"] = []
Review Comment:
@villebro I initially opted to keep the owners property in the payload because the dashboard frontend expects it to always be there, and fails to render if it's not.
I found a way to exclude it from the response payload and have the client tack the property on after the dashboard is requested, which allows the embedded dashboard to render successfully.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(embedded): Hide sensitive payload data from guest users [superset]
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #25878:
URL: https://github.com/apache/superset/pull/25878#discussion_r1394223395
##########
superset/dashboards/api.py:
##########
@@ -342,6 +342,11 @@ def get(
$ref: '#/components/responses/404'
"""
result = self.dashboard_get_response_schema.dump(dash)
+ if security_manager.is_guest_user():
Review Comment:
> I think it's a great idea to meet on long term solutions. If we were to move the logic for this endpoint into the schema (I left one more suggestion that would remove this from the api altogether) should we move forward with this for now as @villebro suggested?
@eschutho I also think moving this into the Marshmallow schema is probably the best solution for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org