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/03/14 23:23:30 UTC

[GitHub] [superset] suddjian opened a new pull request #19144: Client override unauthorized

suddjian opened a new pull request #19144:
URL: https://github.com/apache/superset/pull/19144


   <!---
   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 -->
   
   Requests should be able to be made without triggering the usual redirect-to-login if a 401 is encountered. Additionally, we need to have a default action other than a redirect if we get a 401 in the embedded page. (to be added soon)
   
   ### 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] codecov[bot] commented on pull request #19144: feat: SupersetClient config to override 401 behavior

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #19144:
URL: https://github.com/apache/superset/pull/19144#issuecomment-1072935152


   # [Codecov](https://codecov.io/gh/apache/superset/pull/19144?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 [#19144](https://codecov.io/gh/apache/superset/pull/19144?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5cfae15) into [master](https://codecov.io/gh/apache/superset/commit/e3e03d202db959d708acbfe8a25a50dddc85fa7f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e3e03d2) will **increase** coverage by `0.01%`.
   > The diff coverage is `68.42%`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #19144      +/-   ##
   ==========================================
   + Coverage   66.55%   66.56%   +0.01%     
   ==========================================
     Files        1646     1643       -3     
     Lines       63617    63664      +47     
     Branches     6471     6494      +23     
   ==========================================
   + Hits        42339    42379      +40     
   - Misses      19600    19602       +2     
   - Partials     1678     1683       +5     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | javascript | `51.31% <68.42%> (+0.04%)` | :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/19144?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [superset-frontend/src/embedded/index.tsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2VtYmVkZGVkL2luZGV4LnRzeA==) | `0.00% <0.00%> (ø)` | |
   | [...nd/src/components/MessageToasts/ToastPresenter.tsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy9Ub2FzdFByZXNlbnRlci50c3g=) | `93.33% <80.00%> (-6.67%)` | :arrow_down: |
   | [...rset-ui-core/src/connection/SupersetClientClass.ts](https://codecov.io/gh/apache/superset/pull/19144/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%> (ø)` | |
   | [...nd/src/components/MessageToasts/ToastContainer.tsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy9Ub2FzdENvbnRhaW5lci50c3g=) | `100.00% <100.00%> (ø)` | |
   | [...ontend/src/components/MessageToasts/withToasts.tsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTWVzc2FnZVRvYXN0cy93aXRoVG9hc3RzLnRzeA==) | `100.00% <100.00%> (ø)` | |
   | [...d/src/SqlLab/components/QueryAutoRefresh/index.jsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL1F1ZXJ5QXV0b1JlZnJlc2gvaW5kZXguanN4) | `61.29% <0.00%> (-22.05%)` | :arrow_down: |
   | [...rontend/src/explore/components/EmbedCodeButton.jsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29tcG9uZW50cy9FbWJlZENvZGVCdXR0b24uanN4) | `55.00% <0.00%> (-11.67%)` | :arrow_down: |
   | [...RUD/data/database/DatabaseModal/SqlAlchemyForm.tsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL0NSVUQvZGF0YS9kYXRhYmFzZS9EYXRhYmFzZU1vZGFsL1NxbEFsY2hlbXlGb3JtLnRzeA==) | `57.14% <0.00%> (-9.53%)` | :arrow_down: |
   | [...d/src/SqlLab/components/AceEditorWrapper/index.tsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9jb21wb25lbnRzL0FjZUVkaXRvcldyYXBwZXIvaW5kZXgudHN4) | `44.82% <0.00%> (-8.75%)` | :arrow_down: |
   | [...ontend/src/components/URLShortLinkButton/index.jsx](https://codecov.io/gh/apache/superset/pull/19144/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-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvVVJMU2hvcnRMaW5rQnV0dG9uL2luZGV4LmpzeA==) | `93.75% <0.00%> (-6.25%)` | :arrow_down: |
   | ... and [68 more](https://codecov.io/gh/apache/superset/pull/19144/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/19144?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/19144?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 [e3e03d2...5cfae15](https://codecov.io/gh/apache/superset/pull/19144?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] sadpandajoe commented on pull request #19144: feat: SupersetClient config to override 401 behavior

Posted by GitBox <gi...@apache.org>.
sadpandajoe commented on pull request #19144:
URL: https://github.com/apache/superset/pull/19144#issuecomment-1074197772


   🏷️  preset:2022.11


-- 
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] suddjian merged pull request #19144: feat: SupersetClient config to override 401 behavior

Posted by GitBox <gi...@apache.org>.
suddjian merged pull request #19144:
URL: https://github.com/apache/superset/pull/19144


   


-- 
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] suddjian commented on a change in pull request #19144: feat: SupersetClient config to override 401 behavior

Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19144:
URL: https://github.com/apache/superset/pull/19144#discussion_r830428467



##########
File path: superset-frontend/src/embedded/index.tsx
##########
@@ -75,23 +80,43 @@ if (!window.parent) {
 //   );
 // }
 
+let displayedUnauthorizedToast = false;
+
+/**
+ * If there is a problem with the guest token, we will start getting
+ * 401 errors from the api and SupersetClient will call this function.
+ */
+function guestUnauthorizedHandler() {
+  if (displayedUnauthorizedToast) return; // no need to display this message every time we get another 401
+  displayedUnauthorizedToast = true;
+  // If a guest user were sent to a login screen on 401, they would have no valid login to use.
+  // For embedded it makes more sense to just display a message
+  // and let them continue accessing the page, to whatever extent they can.
+  store.dispatch(
+    addDangerToast(
+      t(
+        'This session has encountered an interruption, and some controls may not work as intended. If you are the developer of this app, please check that the guest token is being generated correctly.',
+      ),
+      {
+        duration: -1, // stay open until closed
+        noDuplicate: true,
+      },
+    ),
+  );
+}
+
+/**
+ * Configures SupersetClient with the correct settings for the embedded dashboard page.
+ */
 function setupGuestClient(guestToken: string) {
-  // need to reconfigure SupersetClient to use the guest token
   setupClient({
     guestToken,
     guestTokenHeaderName: bootstrapData.config?.GUEST_TOKEN_HEADER_NAME,
+    unauthorizedHandler: guestUnauthorizedHandler,
   });
 }
 
 function validateMessageEvent(event: MessageEvent) {
-  if (

Review comment:
       This stuff isn't required, and I think it will be more confusing long-term than not having it 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

[GitHub] [superset] suddjian commented on a change in pull request #19144: feat: SupersetClient config to override 401 behavior

Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19144:
URL: https://github.com/apache/superset/pull/19144#discussion_r830427676



##########
File path: superset-frontend/src/components/MessageToasts/ToastPresenter.tsx
##########
@@ -21,10 +21,14 @@ import { styled } from '@superset-ui/core';
 import { ToastMeta } from 'src/components/MessageToasts/types';
 import Toast from './Toast';
 
-const StyledToastPresenter = styled.div`
+export interface VisualProps {
+  position: 'bottom' | 'top';
+}
+
+const StyledToastPresenter = styled.div<VisualProps>`
   max-width: 600px;
   position: fixed;
-  bottom: 0px;
+  ${({ position }) => (position === 'bottom' ? 'bottom' : 'top')}: 0px;

Review comment:
       By default toasts are at the bottom, but the bottom of an embedded iframe could be offscreen, so for embedded it looks better to have toasts at the top.




-- 
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] suddjian commented on a change in pull request #19144: feat: SupersetClient config to override 401 behavior

Posted by GitBox <gi...@apache.org>.
suddjian commented on a change in pull request #19144:
URL: https://github.com/apache/superset/pull/19144#discussion_r830428268



##########
File path: superset-frontend/src/embedded/index.tsx
##########
@@ -75,23 +80,43 @@ if (!window.parent) {
 //   );
 // }
 
+let displayedUnauthorizedToast = false;
+
+/**
+ * If there is a problem with the guest token, we will start getting
+ * 401 errors from the api and SupersetClient will call this function.
+ */
+function guestUnauthorizedHandler() {
+  if (displayedUnauthorizedToast) return; // no need to display this message every time we get another 401
+  displayedUnauthorizedToast = true;
+  // If a guest user were sent to a login screen on 401, they would have no valid login to use.
+  // For embedded it makes more sense to just display a message
+  // and let them continue accessing the page, to whatever extent they can.
+  store.dispatch(
+    addDangerToast(
+      t(
+        'This session has encountered an interruption, and some controls may not work as intended. If you are the developer of this app, please check that the guest token is being generated correctly.',
+      ),
+      {
+        duration: -1, // stay open until closed

Review comment:
       ```suggestion
           duration: -1, // stay open until manually closed
   ```




-- 
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