fix: Redirect on 401 (apache#17597)

* Redirect on 401 * Bump FAB * Format * Update Cypress save test * Revert Cypress change * Bump FAB 3.4.1rc2 * Update test * Update return statement * Update api test * Update datasets api test * Update datasets api 401s to 403s * Add typeguard * Use Promise.resolve * Update callApiAndParseWithhTimeout test * Disable parseResponse test * Try catch * Handle npm 8 issues
preset-io · Dec 8, 2021 · 46cdc77 · 46cdc77
1 parent aee5c9a
commit 46cdc77
Show file tree

Hide file tree

Showing 13 changed files with 73 additions and 54 deletions.
diff --git a/requirements/base.txt b/requirements/base.txt
@@ -77,7 +77,7 @@ flask==1.1.4
     #   flask-openid
     #   flask-sqlalchemy
     #   flask-wtf
-flask-appbuilder==3.4.0
+flask-appbuilder==3.4.1rc2
     # via apache-superset
 flask-babel==1.0.0
     # via flask-appbuilder

diff --git a/setup.py b/setup.py
@@ -75,7 +75,7 @@ def get_git_sha() -> str:
         "cryptography>=3.3.2",
         "deprecation>=2.1.0, <2.2.0",
         "flask>=1.1.0, <2.0.0",
-        "flask-appbuilder>=3.4.0, <4.0.0",
+        "flask-appbuilder>=3.4.1rc2, <4.0.0",
         "flask-caching>=1.10.0",
         "flask-compress",
         "flask-talisman",
@@ -109,7 +109,8 @@ def get_git_sha() -> str:
         "sqlalchemy-utils>=0.37.8, <0.38",
         "sqlparse==0.3.0",  # PINNED! see https://github.com/andialbrecht/sqlparse/issues/562
         "tabulate==0.8.9",
-        "typing-extensions>=3.10, <4",  # needed to support Literal (3.8) and TypeGuard (3.10)
+        # needed to support Literal (3.8) and TypeGuard (3.10)
+        "typing-extensions>=3.10, <4",
         "wtforms-json",
     ],
     extras_require={

diff --git a/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts b/superset-frontend/packages/superset-ui-core/src/connection/SupersetClientClass.ts
@@ -33,6 +33,13 @@ import {
 } from './types';
 import { DEFAULT_FETCH_RETRY_OPTIONS, DEFAULT_BASE_URL } from './constants';
 
+function redirectUnauthorized() {
+  // the next param will be picked by flask to redirect the user after the login
+  setTimeout(() => {
+    window.location.href = `/login?next=${window.location.href}`;
+  });
+}
+
 export default class SupersetClientClass {
   credentials: Credentials;
 
@@ -151,6 +158,11 @@ export default class SupersetClientClass {
       headers: { ...this.headers, ...headers },
       timeout: timeout ?? this.timeout,
       fetchRetryOptions: fetchRetryOptions ?? this.fetchRetryOptions,
+    }).catch(res => {
+      if (res && res.status === 401) {
+        redirectUnauthorized();
+      }
+      return Promise.reject(res);
     });
   }
 

diff --git a/...tend/packages/superset-ui-core/test/connection/callApi/callApiAndParseWithTimeout.test.ts b/...tend/packages/superset-ui-core/test/connection/callApi/callApiAndParseWithTimeout.test.ts
@@ -54,9 +54,13 @@ describe('callApiAndParseWithTimeout()', () => {
   });
 
   describe('parseResponse', () => {
-    it('calls parseResponse()', () => {
+    it('calls parseResponse()', async () => {
       const parseSpy = jest.spyOn(parseResponse, 'default');
-      callApiAndParseWithTimeout({ url: mockGetUrl, method: 'GET' });
+
+      await callApiAndParseWithTimeout({
+        url: mockGetUrl,
+        method: 'GET',
+      });
 
       expect(parseSpy).toHaveBeenCalledTimes(1);
       parseSpy.mockClear();

diff --git a/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx b/superset-frontend/src/dashboard/components/PropertiesModal/PropertiesModal.test.tsx
@@ -85,7 +85,7 @@ fetchMock.get(
   },
 );
 
-fetchMock.get('http://localhost/api/v1/dashboard/26', {
+fetchMock.get('glob:*/api/v1/dashboard/26', {
   body: {
     result: {
       certified_by: 'John Doe',

diff --git a/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.test.tsx b/...board/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.test.tsx
@@ -24,7 +24,7 @@ import { Column, JsonObject } from '@superset-ui/core';
 import userEvent from '@testing-library/user-event';
 import { ColumnSelect } from './ColumnSelect';
 
-fetchMock.get('http://localhost/api/v1/dataset/123', {
+fetchMock.get('glob:*/api/v1/dataset/123', {
   body: {
     result: {
       columns: [
@@ -35,7 +35,7 @@ fetchMock.get('http://localhost/api/v1/dataset/123', {
     },
   },
 });
-fetchMock.get('http://localhost/api/v1/dataset/456', {
+fetchMock.get('glob:*/api/v1/dataset/456', {
   body: {
     result: {
       columns: [
@@ -47,7 +47,7 @@ fetchMock.get('http://localhost/api/v1/dataset/456', {
   },
 });
 
-fetchMock.get('http://localhost/api/v1/dataset/789', { status: 404 });
+fetchMock.get('glob:*/api/v1/dataset/789', { status: 404 });
 
 const createProps = (extraProps: JsonObject = {}) => ({
   filterId: 'filterId',

diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx b/superset-frontend/src/explore/components/ExploreChartHeader/ExploreChartHeader.test.tsx
@@ -21,11 +21,8 @@ import React from 'react';
 import { Slice } from 'src/types/Chart';
 import { render, screen } from 'spec/helpers/testing-library';
 import userEvent from '@testing-library/user-event';
-import fetchMock from 'fetch-mock';
 import ExploreHeader from '.';
 
-fetchMock.get('http://localhost/api/v1/chart/318', {});
-
 const createProps = () => ({
   chart: {
     latestQueryFormData: {

diff --git a/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx b/superset-frontend/src/explore/components/ExploreChartHeader/index.jsx
@@ -143,26 +143,32 @@ export class ExploreChartHeader extends React.PureComponent {
 
   async fetchChartDashboardData() {
     const { dashboardId, slice } = this.props;
-    const response = await SupersetClient.get({
+    await SupersetClient.get({
       endpoint: `/api/v1/chart/${slice.slice_id}`,
-    });
-    const chart = response.json.result;
-    const dashboards = chart.dashboards || [];
-    const dashboard =
-      dashboardId &&
-      dashboards.length &&
-      dashboards.find(d => d.id === dashboardId);
+    })
+      .then(res => {
+        const response = res?.json?.result;
+        if (response && response.dashboards && response.dashboards.length) {
+          const { dashboards } = response;
+          const dashboard =
+            dashboardId &&
+            dashboards.length &&
+            dashboards.find(d => d.id === dashboardId);
 
-    if (dashboard && dashboard.json_metadata) {
-      // setting the chart to use the dashboard custom label colors if any
-      const labelColors =
-        JSON.parse(dashboard.json_metadata).label_colors || {};
-      const categoricalNamespace = CategoricalColorNamespace.getNamespace();
+          if (dashboard && dashboard.json_metadata) {
+            // setting the chart to use the dashboard custom label colors if any
+            const labelColors =
+              JSON.parse(dashboard.json_metadata).label_colors || {};
+            const categoricalNamespace =
+              CategoricalColorNamespace.getNamespace();
 
-      Object.keys(labelColors).forEach(label => {
-        categoricalNamespace.setColor(label, labelColors[label]);
-      });
-    }
+            Object.keys(labelColors).forEach(label => {
+              categoricalNamespace.setColor(label, labelColors[label]);
+            });
+          }
+        }
+      })
+      .catch(() => {});
   }
 
   getSliceName() {

diff --git a/superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx b/superset-frontend/src/explore/components/PropertiesModal/PropertiesModal.test.tsx
@@ -70,7 +70,7 @@ const createProps = () => ({
   onSave: jest.fn(),
 });
 
-fetchMock.get('http://localhost/api/v1/chart/318', {
+fetchMock.get('glob:*/api/v1/chart/318', {
   body: {
     description_columns: {},
     id: 318,
@@ -128,23 +128,20 @@ fetchMock.get('http://localhost/api/v1/chart/318', {
   },
 });
 
-fetchMock.get(
-  'http://localhost/api/v1/chart/related/owners?q=(filter:%27%27)',
-  {
-    body: {
-      count: 1,
-      result: [
-        {
-          text: 'Superset Admin',
-          value: 1,
-        },
-      ],
-    },
-    sendAsJson: true,
+fetchMock.get('glob:*/api/v1/chart/related/owners?q=(filter:%27%27)', {
+  body: {
+    count: 1,
+    result: [
+      {
+        text: 'Superset Admin',
+        value: 1,
+      },
+    ],
   },
-);
+  sendAsJson: true,
+});
 
-fetchMock.put('http://localhost/api/v1/chart/318', {
+fetchMock.put('glob:*/api/v1/chart/318', {
   body: {
     id: 318,
     result: {

diff --git a/superset/config.py b/superset/config.py
@@ -286,6 +286,8 @@ def _try_json_readsha(filepath: str, length: int) -> Optional[str]:
 #    { 'name': 'Yahoo', 'url': 'https://open.login.yahoo.com/' },
 #    { 'name': 'Flickr', 'url': 'https://www.flickr.com/<username>' },
 
+AUTH_STRICT_RESPONSE_CODES = True
+
 # ---------------------------------------------------
 # Roles config
 # ---------------------------------------------------

diff --git a/tests/integration_tests/databases/api_tests.py b/tests/integration_tests/databases/api_tests.py
@@ -1141,7 +1141,7 @@ def test_export_database_not_allowed(self):
         argument = [database.id]
         uri = f"api/v1/database/export/?q={prison.dumps(argument)}"
         rv = self.client.get(uri)
-        assert rv.status_code == 401
+        assert rv.status_code == 403
 
     def test_export_database_non_existing(self):
         """

diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py
@@ -432,7 +432,7 @@ def test_create_dataset_item_gamma(self):
         }
         uri = "api/v1/dataset/"
         rv = self.client.post(uri, json=table_data)
-        assert rv.status_code == 401
+        assert rv.status_code == 403
 
     def test_create_dataset_item_owner(self):
         """
@@ -986,7 +986,7 @@ def test_update_dataset_item_gamma(self):
         table_data = {"description": "changed_description"}
         uri = f"api/v1/dataset/{dataset.id}"
         rv = self.client.put(uri, json=table_data)
-        assert rv.status_code == 401
+        assert rv.status_code == 403
         db.session.delete(dataset)
         db.session.commit()
 
@@ -1094,7 +1094,7 @@ def test_delete_dataset_item_not_authorized(self):
         self.login(username="gamma")
         uri = f"api/v1/dataset/{dataset.id}"
         rv = self.client.delete(uri)
-        assert rv.status_code == 401
+        assert rv.status_code == 403
         db.session.delete(dataset)
         db.session.commit()
 
@@ -1313,7 +1313,7 @@ def test_bulk_delete_dataset_item_not_authorized(self):
         self.login(username="gamma")
         uri = f"api/v1/dataset/?q={prison.dumps(dataset_ids)}"
         rv = self.client.delete(uri)
-        assert rv.status_code == 401
+        assert rv.status_code == 403
 
     @pytest.mark.usefixtures("create_datasets")
     def test_bulk_delete_dataset_item_incorrect(self):
@@ -1438,7 +1438,7 @@ def test_export_dataset_gamma(self):
 
         self.login(username="gamma")
         rv = self.client.get(uri)
-        assert rv.status_code == 401
+        assert rv.status_code == 403
 
         perm1 = security_manager.find_permission_view_menu("can_export", "Dataset")
 
@@ -1516,7 +1516,7 @@ def test_export_dataset_bundle_gamma(self):
         self.login(username="gamma")
         rv = self.client.get(uri)
         # gamma users by default do not have access to this dataset
-        assert rv.status_code == 401
+        assert rv.status_code == 403
 
     @unittest.skip("Number of related objects depend on DB")
     @pytest.mark.usefixtures("load_birth_names_dashboard_with_slices")

diff --git a/tests/integration_tests/log_api_tests.py b/tests/integration_tests/log_api_tests.py
@@ -102,10 +102,10 @@ def test_get_list_not_allowed(self):
         self.login(username="gamma")
         uri = "api/v1/log/"
         rv = self.client.get(uri)
-        self.assertEqual(rv.status_code, 401)
+        self.assertEqual(rv.status_code, 403)
         self.login(username="alpha")
         rv = self.client.get(uri)
-        self.assertEqual(rv.status_code, 401)
+        self.assertEqual(rv.status_code, 403)
 
     def test_get_item(self):
         """