You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "john-bodley (via GitHub)" <gi...@apache.org> on 2023/06/15 00:08:56 UTC
[GitHub] [superset] john-bodley commented on a diff in pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
john-bodley commented on code in PR #24400:
URL: https://github.com/apache/superset/pull/24400#discussion_r1230284328
##########
superset/views/core.py:
##########
@@ -1363,27 +1087,17 @@ def welcome(self) -> FlaskResponse:
@has_access
@event_logger.log_this
- @expose("/profile/<username>/")
- def profile(self, username: str) -> FlaskResponse:
+ @expose("/profile/")
+ def profile(self) -> FlaskResponse:
"""User profile page"""
- user = (
- db.session.query(ab_models.User).filter_by(username=username).one_or_none()
- )
- # Prevent returning 404 when user is not found to prevent username scanning
- user_id = -1 if not user else user.id
- # Prevent unauthorized access to other user's profiles,
- # unless configured to do so with ENABLE_BROAD_ACTIVITY_ACCESS
- if error_obj := self.get_user_activity_access_error(user_id):
- return error_obj
-
payload = {
- "user": bootstrap_user_data(user, include_perms=True),
+ "user": bootstrap_user_data(g.user, include_perms=True),
"common": common_bootstrap_payload(g.user),
}
return self.render_template(
"superset/basic.html",
- title=_("%(user)s's profile", user=username).__str__(),
+ title=_("%(user)s's profile", user=g.user.username).__str__(),
Review Comment:
Maybe
```suggestion
title=_("%(user)s's profile", user=get_username()).__str__(),
```
for consistency as we use that elsewhere. That said I guess you're already using `g.user` previously.
##########
superset/views/core.py:
##########
@@ -1363,27 +1087,17 @@ def welcome(self) -> FlaskResponse:
@has_access
@event_logger.log_this
- @expose("/profile/<username>/")
- def profile(self, username: str) -> FlaskResponse:
+ @expose("/profile/")
+ def profile(self) -> FlaskResponse:
"""User profile page"""
- user = (
- db.session.query(ab_models.User).filter_by(username=username).one_or_none()
- )
- # Prevent returning 404 when user is not found to prevent username scanning
- user_id = -1 if not user else user.id
- # Prevent unauthorized access to other user's profiles,
- # unless configured to do so with ENABLE_BROAD_ACTIVITY_ACCESS
- if error_obj := self.get_user_activity_access_error(user_id):
- return error_obj
-
payload = {
- "user": bootstrap_user_data(user, include_perms=True),
+ "user": bootstrap_user_data(g.user, include_perms=True),
Review Comment:
@dpgaspar what happens if the user is a guest user? Is `g.user` guaranteed to exist and be non-`None`.
##########
superset/views/log/api.py:
##########
@@ -125,8 +125,7 @@ def recent_activity(self, user_id: int, **kwargs: Any) -> FlaskResponse:
500:
$ref: '#/components/responses/500'
"""
- if error_obj := self.get_user_activity_access_error(user_id):
- return error_obj
+ user_id = get_user_id()
Review Comment:
Maybe inline this on line 136? Alternatively given it seems like this is the only place which calls it should we augment the [get_recent_activity](https://github.com/apache/superset/blob/d2b0b8eac52ad8b68639c6581a1ed174a593f564/superset/views/log/dao.py#L36-L38) to not include the user ID, i.e., the recently activity always pertains to the current user? That's also likely safer (less vulnerable) from a security perspective.
--
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