You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "dpgaspar (via GitHub)" <gi...@apache.org> on 2023/06/14 14:57:23 UTC
[GitHub] [superset] dpgaspar opened a new pull request, #24400: chore: remove deprecated apis on superset
dpgaspar opened a new pull request, #24400:
URL: https://github.com/apache/superset/pull/24400
### SUMMARY
Removes old deprecated APIs:
- `/superset/recent_activity/...`
- `/superset/fave_dashboards_by_username/...`
- `/superset/fave_dashboards/...`
- `/superset/created_dashboards/...`
- `/superset/user_slices/...`
- `/superset/created_slices/...`
- `/superset/fave_slices/...`
- `/superset/favstar/...`
Effort on removing deprecated APIs from `/superset`: https://github.com/apache/superset/discussions/24332
### 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 -->
- [ ] Has associated issue:
- [ ] 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
[GitHub] [superset] dpgaspar commented on a diff in pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #24400:
URL: https://github.com/apache/superset/pull/24400#discussion_r1230316614
##########
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:
Good point, it should be `AnonymousUser` from flask-login or `GuestUser`, but added some safe guard, and we're returning 404 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
[GitHub] [superset] john-bodley commented on a diff in pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24400:
URL: https://github.com/apache/superset/pull/24400#discussion_r1231427914
##########
superset/views/core.py:
##########
@@ -1363,27 +1076,20 @@ 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
-
+ user = g.user if hasattr(g, "user") and g.user else None
Review Comment:
@villebro having a `get_user` method [here](https://github.com/apache/superset/blob/892de8a232a5b40dd22bf07786cb7255ef633d44/superset/utils/core.py#L1398-L1426)—which `get_user_id` and `get_username` would call—would be useful. I did a pass previously in https://github.com/apache/superset/pull/20492 which ensured facets of this was normalized.
--
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
[GitHub] [superset] john-bodley commented on a diff in pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
john-bodley commented on code in PR #24400:
URL: https://github.com/apache/superset/pull/24400#discussion_r1231427914
##########
superset/views/core.py:
##########
@@ -1363,27 +1076,20 @@ 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
-
+ user = g.user if hasattr(g, "user") and g.user else None
Review Comment:
@villebro having a `get_user` method [here](https://github.com/apache/superset/blob/892de8a232a5b40dd22bf07786cb7255ef633d44/superset/utils/core.py#L1398-L1426)—which `get_user_id` and `get_username` would call—would be useful.
--
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
[GitHub] [superset] dpgaspar commented on a diff in pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar commented on code in PR #24400:
URL: https://github.com/apache/superset/pull/24400#discussion_r1230978626
##########
superset/views/core.py:
##########
@@ -1363,27 +1076,20 @@ 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
-
+ user = g.user if hasattr(g, "user") and g.user else None
Review Comment:
totally agree! would be great also to make anonymous users and guest user's more clear to check, use and test
--
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
[GitHub] [superset] villebro commented on a diff in pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24400:
URL: https://github.com/apache/superset/pull/24400#discussion_r1230955621
##########
superset/views/core.py:
##########
@@ -1363,27 +1076,20 @@ 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
-
+ user = g.user if hasattr(g, "user") and g.user else None
Review Comment:
I think this snippet or a variation of it appears in LOTS of places. We should replace all of these with a single util to DRY this up and add tests for all expected cases. I can do it if needed (I think this PR is big enough already, so it should not be obfuscated with more chores).
##########
superset/views/core.py:
##########
@@ -1363,27 +1076,20 @@ 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:
Review Comment:
❤️
--
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
[GitHub] [superset] dpgaspar merged pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
Posted by "dpgaspar (via GitHub)" <gi...@apache.org>.
dpgaspar merged PR #24400:
URL: https://github.com/apache/superset/pull/24400
--
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
[GitHub] [superset] codecov[bot] commented on pull request #24400: chore: remove deprecated apis on superset
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #24400:
URL: https://github.com/apache/superset/pull/24400#issuecomment-1591618073
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/24400?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#24400](https://app.codecov.io/gh/apache/superset/pull/24400?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (01515d7) into [master](https://app.codecov.io/gh/apache/superset/commit/c69634df279054de0eb145bdcc796cb8fdd26dd8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (c69634d) will **decrease** coverage by `1.88%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 01515d7 differs from pull request most recent head 4bcfb35. Consider uploading reports for the commit 4bcfb35 to get more accurate results
```diff
@@ Coverage Diff @@
## master #24400 +/- ##
==========================================
- Coverage 68.90% 67.03% -1.88%
==========================================
Files 1903 1903
Lines 74070 73932 -138
Branches 8110 8110
==========================================
- Hits 51041 49557 -1484
- Misses 20917 22263 +1346
Partials 2112 2112
```
| Flag | Coverage Δ | |
|---|---|---|
| hive | `?` | |
| mysql | `79.21% <100.00%> (?)` | |
| postgres | `79.30% <100.00%> (-0.03%)` | :arrow_down: |
| presto | `?` | |
| python | `79.36% <100.00%> (-3.86%)` | :arrow_down: |
| unit | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://app.codecov.io/gh/apache/superset/pull/24400?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [superset/views/base.py](https://app.codecov.io/gh/apache/superset/pull/24400?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvYmFzZS5weQ==) | `72.78% <ø> (ø)` | |
| [superset/views/core.py](https://app.codecov.io/gh/apache/superset/pull/24400?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `67.99% <100.00%> (-3.94%)` | :arrow_down: |
... and [101 files with indirect coverage changes](https://app.codecov.io/gh/apache/superset/pull/24400/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [superset] john-bodley commented on a diff in pull request #24400: chore: remove deprecated apis and ENABLE_BROAD_ACTIVITY_ACCESS
Posted by "john-bodley (via GitHub)" <gi...@apache.org>.
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