You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/04/14 00:54:59 UTC
[GitHub] [superset] ktmud opened a new pull request, #19708: fix(nav): infinite redirect and upload dataset nav permissions
ktmud opened a new pull request, #19708:
URL: https://github.com/apache/superset/pull/19708
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
Currently there is an infinite redirect on the login page (when users are not logged in) that is triggered by an unfortunate sequence of changes:
1. [#17597](https://github.com/apache/superset/pull/17597) added redirect to login when API request returns 401
2. [#19051](https://github.com/apache/superset/pull/19051/files#diff-8e496bd140c9eb03feb8073a1b530f5e46cb013d35b0d1619ff0e95db8c8c7eeR211-R215) starts to call the `/api/v1/database` API to find out whether there are databases with file upload enabled, even when the menu is rendered on a page where users haven't logged in.
While debugging, I also found out that users without admin access will also see an empty "Data" menu, so I fixed that as well.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
When logged in as non admin users:
#### Before
<img width="345" alt="Xnip2022-04-13_16-57-59" src="https://user-images.githubusercontent.com/335541/163288964-cbb810d9-9973-4ca6-8b20-ab49693aa6a5.png">
There is an empty `Data` menu item with no submenu.
#### After
<img width="322" alt="Xnip2022-04-13_16-57-21" src="https://user-images.githubusercontent.com/335541/163289000-e715303d-a978-4526-b75d-e39530cb1985.png">
The `Data` menu item is removed
### TESTING INSTRUCTIONS
1. Logout and go to `<superset url>/login`. You may see a redirect if the login page isn't overritten.
### 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:
- [x] 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] ktmud merged pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
ktmud merged PR #19708:
URL: https://github.com/apache/superset/pull/19708
--
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] ktmud commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850003432
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
Be more explicit what is the default. I kind of feel this should probably default to `true` but that's another topic. Async API requests should never implicitly redirect users as it may often cause surprises. There should be better error handling on the API user side instead.
cc @geido @kgabryje could you give more context on why #17597 made redirects on 401 the default?
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -34,9 +34,11 @@ import {
import { DEFAULT_FETCH_RETRY_OPTIONS, DEFAULT_BASE_URL } from './constants';
const defaultUnauthorizedHandler = () => {
- window.location.href = `/login?next=${
- window.location.pathname + window.location.search
- }`;
+ if (!window.location.pathname.startsWith('/login')) {
Review Comment:
Only redirect when we are not on the login page so we never see infinite redirect caused by the API client ever again.
##########
superset-frontend/src/views/components/SubMenu.tsx:
##########
@@ -240,22 +240,22 @@ const SubMenuComponent: React.FunctionComponent<SubMenuProps> = props => {
if ((props.usesRouter || hasHistory) && !!tab.usesRouter) {
return (
<Menu.Item key={tab.label}>
- <li
+ <div
Review Comment:
Bycatch: fix a React warning about nested `<li />`s.
##########
superset-frontend/src/views/components/MenuRight.tsx:
##########
@@ -234,19 +238,21 @@ const RightMenu = ({
};
const onMenuOpen = (openKeys: string[]) => {
- if (openKeys.length) {
- return hasFileUploadEnabled();
+ if (openKeys.length && canUploadData) {
+ return checkAllowUploads();
}
return null;
};
return (
<StyledDiv align={align}>
- <DatabaseModal
- onHide={handleOnHideModal}
- show={showModal}
- dbEngine={engine}
- />
+ {canDatabase && (
Review Comment:
Bycatch: only render the DatabaseModal when users can create new database connection.
--
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] geido commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
geido commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850331398
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
We will have a conversation about having the default to true and we'll follow up. Thank you!
--
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] nytai commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
nytai commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850007401
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
I strongly agree here, this behavior is quite surprising from an api client. Though I guess this approach involves the least amount of code changes.
From a UX perspective I'd suggest presenting the user with the option to re-auth. There could be unsaved work that would be lost via a the redirect.
--
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] nytai commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
nytai commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850007401
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
I strongly agree here, this behavior is quite surprising from an api client. Though I guess this approach involves the least amount of code changes.
--
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 #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #19708:
URL: https://github.com/apache/superset/pull/19708#issuecomment-1098627472
# [Codecov](https://codecov.io/gh/apache/superset/pull/19708?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#19708](https://codecov.io/gh/apache/superset/pull/19708?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (20c2dda) into [master](https://codecov.io/gh/apache/superset/commit/5fc0651aaba9f2d7ed6605bea9cc44254cd6e02f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5fc0651) will **decrease** coverage by `0.00%`.
> The diff coverage is `40.00%`.
> :exclamation: Current head 20c2dda differs from pull request most recent head db9f124. Consider uploading reports for the commit db9f124 to get more accurate results
```diff
@@ Coverage Diff @@
## master #19708 +/- ##
==========================================
- Coverage 66.51% 66.50% -0.01%
==========================================
Files 1684 1684
Lines 64560 64567 +7
Branches 6625 6628 +3
==========================================
+ Hits 42939 42943 +4
- Misses 19926 19927 +1
- Partials 1695 1697 +2
```
| Flag | Coverage Δ | |
|---|---|---|
| javascript | `51.15% <40.00%> (+<0.01%)` | :arrow_up: |
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=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/19708?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...perset-frontend/src/views/components/MenuRight.tsx](https://codecov.io/gh/apache/superset/pull/19708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NvbXBvbmVudHMvTWVudVJpZ2h0LnRzeA==) | `65.21% <25.00%> (-1.45%)` | :arrow_down: |
| [...rset-ui-core/src/connection/SupersetClientClass.ts](https://codecov.io/gh/apache/superset/pull/19708/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGFja2FnZXMvc3VwZXJzZXQtdWktY29yZS9zcmMvY29ubmVjdGlvbi9TdXBlcnNldENsaWVudENsYXNzLnRz) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19708?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/19708?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [5fc0651...db9f124](https://codecov.io/gh/apache/superset/pull/19708?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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] ktmud commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850011545
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
@etr2460 This option was actually added by @suddjian because he had a case where redirects aren't desired: https://github.com/apache/superset/pull/19144 (i.e. when Superset in embedded in another app)
--
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] etr2460 commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
etr2460 commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850010855
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
I think we always want to redirect to login on a 401. 401 means that there's no auth token at all. 403 should be used if the user is forbidden, right?
--
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] etr2460 commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
etr2460 commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850011981
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
ahh i see. so we're fine with rendering some stuff even when a user is logged out. I guess that's fine then, carry on!
--
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] sadpandajoe commented on pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
sadpandajoe commented on PR #19708:
URL: https://github.com/apache/superset/pull/19708#issuecomment-1101820352
🏷️ preset:2022.15
--
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] ktmud commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850011545
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
@etr2460 This option was actually added by @suddjian because he had a case where redirect isn't desired: https://github.com/apache/superset/pull/19144 (i.e. when Superset in embedded in another app)
--
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] ktmud commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
ktmud commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850010688
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
@geido @kgabryje @suddjian do you mind following up here to change the default to `true` and set it to `false` where needed? I'm not familiar where we wanted the redirect behavior to be the default.
--
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] etr2460 commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
etr2460 commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850010855
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
I think we always want to redirect to login on a 401. 401 means that there's no auth token at all. 403 should be used if the the user is forbidden, right?
--
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] nytai commented on a diff in pull request #19708: fix(nav): infinite redirect and upload dataset nav permissions
Posted by GitBox <gi...@apache.org>.
nytai commented on code in PR #19708:
URL: https://github.com/apache/superset/pull/19708#discussion_r850007401
##########
superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts:
##########
@@ -161,7 +163,7 @@ export default class SupersetClientClass {
headers,
timeout,
fetchRetryOptions,
- ignoreUnauthorized,
+ ignoreUnauthorized = false,
Review Comment:
I strongly agree here, `401`s can happen for a variety of reasons and not just due to the session being expired.
--
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