fix: Redirect on 401 (#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
2021-12-08 22:14:30 +02:00 · 2021-12-08 22:14:30 +02:00 · 46cdc77ae6
parent aee5c9a3ea
commit 46cdc77ae6
13 changed files with 73 additions and 54 deletions
--- 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
--- a/setup.py
+++ b/setup.py
@ -75,7 +75,7 @@ setup(
        "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 @@ setup(
        "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={
--- 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);
    });
  }

--- a/superset-frontend/packages/superset-ui-core/test/connection/callApi/callApiAndParseWithTimeout.test.ts
+++ b/superset-frontend/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();
--- 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',
--- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ColumnSelect.test.tsx
+++ b/superset-frontend/src/dashboard/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',
--- 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: {
--- 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() {
--- 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: {
--- a/superset/config.py
+++ b/superset/config.py
@ -286,6 +286,8 @@ AUTH_TYPE = AUTH_DB
 #    { 'name': 'Yahoo', 'url': 'https://open.login.yahoo.com/' },
 #    { 'name': 'Flickr', 'url': 'https://www.flickr.com/<username>' },

+AUTH_STRICT_RESPONSE_CODES = True
+
 # ---------------------------------------------------
 # Roles config
 # ---------------------------------------------------
--- a/tests/integration_tests/databases/api_tests.py
+++ b/tests/integration_tests/databases/api_tests.py
@ -1141,7 +1141,7 @@ class TestDatabaseApi(SupersetTestCase):
        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):
        """
--- a/tests/integration_tests/datasets/api_tests.py
+++ b/tests/integration_tests/datasets/api_tests.py
@ -432,7 +432,7 @@ class TestDatasetApi(SupersetTestCase):
        }
        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 @@ class TestDatasetApi(SupersetTestCase):
        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 @@ class TestDatasetApi(SupersetTestCase):
        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 @@ class TestDatasetApi(SupersetTestCase):
        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 @@ class TestDatasetApi(SupersetTestCase):

        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 @@ class TestDatasetApi(SupersetTestCase):
        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")
--- a/tests/integration_tests/log_api_tests.py
+++ b/tests/integration_tests/log_api_tests.py
@ -102,10 +102,10 @@ class TestLogApi(SupersetTestCase):
        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):
        """