Improve user redirection to login page (#1238)

Signed-off-by: Cintia Sanchez Garcia <cynthiasg@icloud.com>

internal/user/tmpl_password_reset_success.go

@@ -119,7 +119,7 @@ var passwordResetSuccessTmpl = template.Must(template.New("").Parse(`
                                 <table border="0" cellpadding="0" cellspacing="0" style="border-collapse: separate; mso-table-lspace: 0pt; mso-table-rspace: 0pt; width: auto;">
                                   <tbody>
                                     <tr>
-                                      <td style="font-family: sans-serif; font-size: 14px; border-radius: 5px; vertical-align: top; text-align: center;"> <a href="{{ .baseURL }}/login?modal=login" target="_blank" style="display: inline-block; color: #ffffff; background-color: #39596C; border: solid 1px #39596C; border-radius: 5px; box-sizing: border-box; cursor: pointer; text-decoration: none; font-size: 14px; font-weight: bold; margin: 0; padding: 12px 25px; text-transform: capitalize; border-color: #39596C;">Login</a> </td>
+                                      <td style="font-family: sans-serif; font-size: 14px; border-radius: 5px; vertical-align: top; text-align: center;"> <a href="{{ .baseURL }}/?modal=login" target="_blank" style="display: inline-block; color: #ffffff; background-color: #39596C; border: solid 1px #39596C; border-radius: 5px; box-sizing: border-box; cursor: pointer; text-decoration: none; font-size: 14px; font-weight: bold; margin: 0; padding: 12px 25px; text-transform: capitalize; border-color: #39596C;">Login</a> </td>
                                     </tr>
                                   </tbody>
                                 </table>
@@ -131,7 +131,7 @@ var passwordResetSuccessTmpl = template.Must(template.New("").Parse(`
                           <tbody>
                             <tr>
                               <td class="content-block powered-by" style="font-family: sans-serif; vertical-align: top; font-size: 11px; color: #545454; padding-bottom: 30px; padding-top: 10px;">
-                                <p style="color: #545454; font-size: 11px; text-decoration: none;">Or you can copy-paste this link: <span style="color: #545454; background-color: #ffffff;">{{ .baseURL }}/login?modal=login</span></p>
+                                <p style="color: #545454; font-size: 11px; text-decoration: none;">Or you can copy-paste this link: <span style="color: #545454; background-color: #ffffff;">{{ .baseURL }}/?modal=login</span></p>
                               </td>
                             </tr>
                           </tbody>

web/src/api/index.ts

@@ -91,9 +91,18 @@ const handleErrors = async (res: any) => {
     let error: Error;
     switch (res.status) {
       case 401:
+        try {
+          let text = await res.json();
+          error = {
+            kind: ErrorKind.Unauthorized,
+            message: text.message !== '' ? text.message : undefined,
+          };
+        } catch {
           error = {
             kind: ErrorKind.Unauthorized,
           };
+        }
+
         break;
       case 404:
         error = {

web/src/context/AppCtx.tsx

@@ -4,6 +4,7 @@ import React, { createContext, useContext, useEffect, useReducer } from 'react';
 import { API } from '../api';
 import useSystemThemeMode from '../hooks/useSystemThemeMode';
 import { Prefs, Profile, ThemePrefs, UserFullName } from '../types';
+import cleanLoginUrlParams from '../utils/cleanLoginUrlParams';
 import detectActiveThemeMode from '../utils/detectActiveThemeMode';
 import history from '../utils/history';
 import isControlPanelSectionAvailable from '../utils/isControlPanelSectionAvailable';
@@ -83,12 +84,26 @@ export async function refreshUserProfile(dispatch: React.Dispatch<any>, redirect
   try {
     const profile: Profile = await API.getUserProfile();
     dispatch({ type: 'signIn', profile });
+    const currentUrl = `${window.location.pathname}${
+      window.location.search !== '' ? `?${cleanLoginUrlParams(window.location.search)}` : ''
+    }`;
     if (!isUndefined(redirectUrl)) {
+      if (redirectUrl === currentUrl) {
+        history.replace(redirectUrl);
+      } else {
         // Redirect to correct route when neccessary
-      history.push({ pathname: redirectUrl });
+        history.push(redirectUrl);
       }
-  } catch {
+    }
+  } catch (err) {
     dispatch({ type: 'signOut' });
+    if (err.message === 'invalid session') {
+      history.push(
+        `${window.location.pathname}${
+          window.location.search === '' ? '?' : `${window.location.search}&`
+        }modal=login&redirect=${encodeURIComponent(`${window.location.pathname}${window.location.search}`)}`
+      );
+    }
   }
 }
 

web/src/layout/App.tsx

@@ -97,7 +97,12 @@ export default function App() {
                 const searchParams = buildSearchParams(location.search);
                 return (
                   <>
-                    <Navbar isSearching={isSearching} searchText={searchParams.tsQueryWeb} />
+                    <Navbar
+                      redirect={getQueryParam(location.search, 'redirect') || undefined}
+                      visibleModal={getQueryParam(location.search, 'modal') || undefined}
+                      isSearching={isSearching}
+                      searchText={searchParams.tsQueryWeb}
+                    />
                     <div className="d-flex flex-column flex-grow-1">
                       <SearchView
                         {...searchParams}
@@ -118,7 +123,11 @@ export default function App() {
               exact
               render={({ location, match }) => (
                 <>
-                  <Navbar isSearching={isSearching} />
+                  <Navbar
+                    isSearching={isSearching}
+                    redirect={getQueryParam(location.search, 'redirect') || undefined}
+                    visibleModal={getQueryParam(location.search, 'modal') || undefined}
+                  />
                   <div className="d-flex flex-column flex-grow-1">
                     <PackageView
                       hash={location.hash}
@@ -171,9 +180,13 @@ export default function App() {
             <Route
               path="/stats"
               exact
-              render={() => (
+              render={({ location }) => (
                 <>
-                  <Navbar isSearching={isSearching} />
+                  <Navbar
+                    isSearching={isSearching}
+                    redirect={getQueryParam(location.search, 'redirect') || undefined}
+                    visibleModal={getQueryParam(location.search, 'modal') || undefined}
+                  />
                   <div className="d-flex flex-column flex-grow-1">
                     <StatsView />
                   </div>

web/src/layout/controlPanel/index.tsx

@@ -39,14 +39,14 @@ const ControlPanelView = (props: Props) => {
   const [lastSelectedOrg, setLastSelectedOrg] = useState<string | undefined>(ctx.prefs.controlPanel.selectedOrg);
 
   const onAuthError = (): void => {
-    dispatch(signOut());
-    history.push(
-      `/login?redirect=/control-panel/${activeSection}${!isNull(activeSubsection) ? `/${activeSubsection}` : ''}`
-    );
     alertDispatcher.postAlert({
       type: 'danger',
       message: 'Sorry, you are not authorized to complete this action, please make sure you are signed in',
     });
+    dispatch(signOut());
+    history.push(
+      `/?modal=login&redirect=/control-panel/${activeSection}${!isNull(activeSubsection) ? `/${activeSubsection}` : ''}`
+    );
   };
 
   const checkIfAuthorizationIsActive = (newCtx: string): boolean => {

web/src/layout/navigation/LogIn.test.tsx

@@ -190,7 +190,7 @@ describe('LogIn', () => {
 
       await waitFor(() => {
         expect(mockHistoryReplace).toHaveBeenCalledTimes(1);
-        expect(mockHistoryReplace).toHaveBeenCalledWith({ pathname: '/' });
+        expect(mockHistoryReplace).toHaveBeenCalledWith({ pathname: '/', search: '' });
       });
     });
 

web/src/layout/navigation/LogIn.tsx

@@ -9,6 +9,7 @@ import { useHistory } from 'react-router-dom';
 import { API } from '../../api';
 import { AppCtx, refreshUserProfile, signOut } from '../../context/AppCtx';
 import { ErrorKind, RefInputField, UserLogin } from '../../types';
+import cleanLoginUrlParams from '../../utils/cleanLoginUrlParams';
 import compoundErrorMessage from '../../utils/compoundErrorMessage';
 import InputField from '../common/InputField';
 import Modal from '../common/Modal';
@@ -59,9 +60,10 @@ const LogIn = (props: Props) => {
   const onCloseModal = () => {
     if (!isUndefined(props.redirect) || !isUndefined(props.visibleModal)) {
       // If redirect option is defined and user closes login modal,
-      // querystring is cleaned to avoid open modal again on refresh
+      // querystring with login parameters is cleaned to avoid open modal again on refresh
       history.replace({
-        pathname: '/',
+        pathname: window.location.pathname,
+        search: cleanLoginUrlParams(window.location.search),
       });
     }
     setVisibleResetPassword(false);

web/src/layout/package/StarButton.test.tsx

@@ -1,5 +1,6 @@
 import { fireEvent, render, waitFor } from '@testing-library/react';
 import React from 'react';
+import { BrowserRouter as Router } from 'react-router-dom';
 import { mocked } from 'ts-jest/utils';
 
 import { API } from '../../api';
@@ -33,6 +34,15 @@ const mockCtx = {
 
 const mockDispatch = jest.fn();
 
+const mockHistoryPush = jest.fn();
+
+jest.mock('react-router-dom', () => ({
+  ...(jest.requireActual('react-router-dom') as {}),
+  useHistory: () => ({
+    push: mockHistoryPush,
+  }),
+}));
+
 describe('StarButton', () => {
   afterEach(() => {
     jest.resetAllMocks();
@@ -43,7 +53,9 @@ describe('StarButton', () => {
 
     const result = render(
       <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+        <Router>
           <StarButton {...defaultProps} />
+        </Router>
       </AppCtx.Provider>
     );
 
@@ -61,7 +73,9 @@ describe('StarButton', () => {
 
         const { getByText, getByTestId, getByRole, queryByRole, getAllByText } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <StarButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -95,7 +109,9 @@ describe('StarButton', () => {
 
         const { getByText, getByTestId, getByRole } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <StarButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -123,7 +139,9 @@ describe('StarButton', () => {
 
         const { getByTestId } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <StarButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -150,7 +168,9 @@ describe('StarButton', () => {
 
         const { getByTestId } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <StarButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -170,35 +190,6 @@ describe('StarButton', () => {
           });
         });
       });
-
-      it('when user is not logged in', async () => {
-        mocked(API).getStars.mockResolvedValue({ stars: 4 });
-        mocked(API).toggleStar.mockRejectedValue({ kind: ErrorKind.Unauthorized });
-
-        const { getByTestId } = render(
-          <AppCtx.Provider value={{ ctx: mockCtx, dispatch: mockDispatch }}>
-            <StarButton {...defaultProps} />
-          </AppCtx.Provider>
-        );
-
-        await waitFor(() => {
-          expect(API.getStars).toHaveBeenCalledTimes(1);
-        });
-
-        const btn = getByTestId('toggleStarBtn');
-        expect(btn).toBeInTheDocument();
-        fireEvent.click(btn);
-
-        await waitFor(() => {
-          expect(mockDispatch).toHaveBeenCalledTimes(1);
-          expect(mockDispatch).toHaveBeenCalledWith({ type: 'signOut' });
-          expect(alertDispatcher.postAlert).toHaveBeenCalledTimes(1);
-          expect(alertDispatcher.postAlert).toHaveBeenCalledWith({
-            type: 'danger',
-            message: 'You must be signed in to unstar a package',
-          });
-        });
-      });
     });
 
     describe('when user is not signed in', () => {
@@ -206,7 +197,9 @@ describe('StarButton', () => {
         mocked(API).getStars.mockResolvedValue({ stars: 4 });
         const { getByRole } = render(
           <AppCtx.Provider value={{ ctx: { ...mockCtx, user: null }, dispatch: jest.fn() }}>
+            <Router>
               <StarButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -226,7 +219,9 @@ describe('StarButton', () => {
 
         const component = (
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <StarButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -249,7 +244,9 @@ describe('StarButton', () => {
 
         render(
           <AppCtx.Provider value={{ ctx: { ...mockCtx, user: undefined }, dispatch: jest.fn() }}>
+            <Router>
               <StarButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -258,5 +255,33 @@ describe('StarButton', () => {
         });
       });
     });
+
+    describe('calls to sign out', () => {
+      it('when user is not logged in to star/unstar pkg', async () => {
+        mocked(API).getStars.mockResolvedValue({ stars: 4 });
+        mocked(API).toggleStar.mockRejectedValue({ kind: ErrorKind.Unauthorized });
+
+        const { getByTestId } = render(
+          <AppCtx.Provider value={{ ctx: mockCtx, dispatch: mockDispatch }}>
+            <Router>
+              <StarButton {...defaultProps} />
+            </Router>
+          </AppCtx.Provider>
+        );
+
+        await waitFor(() => {
+          expect(API.getStars).toHaveBeenCalledTimes(1);
+        });
+
+        const btn = getByTestId('toggleStarBtn');
+        expect(btn).toBeInTheDocument();
+        fireEvent.click(btn);
+
+        await waitFor(() => {
+          expect(mockDispatch).toHaveBeenCalledTimes(1);
+          expect(mockDispatch).toHaveBeenCalledWith({ type: 'signOut' });
+        });
+      });
+    });
   });
 });

web/src/layout/package/StarButton.tsx

@@ -4,6 +4,7 @@ import isNull from 'lodash/isNull';
 import isUndefined from 'lodash/isUndefined';
 import React, { useContext, useEffect, useState } from 'react';
 import { FaRegStar, FaStar } from 'react-icons/fa';
+import { useHistory } from 'react-router';
 
 import { API } from '../../api';
 import { AppCtx, signOut } from '../../context/AppCtx';
@@ -19,6 +20,7 @@ interface Props {
 
 const StarButton = (props: Props) => {
   const { ctx, dispatch } = useContext(AppCtx);
+  const history = useHistory();
   const [packageStars, setPackageStars] = useState<PackageStars | undefined | null>(undefined);
   const [isSending, setIsSending] = useState(false);
   const [isGettingIfStarred, setIsGettingIfStarred] = useState<boolean | undefined>(undefined);
@@ -67,23 +69,23 @@ const StarButton = (props: Props) => {
       getPackageStars();
       setIsSending(false);
     } catch (err) {
-      let errMessage = `An error occurred ${
-        notStarred ? 'starring' : 'unstarring'
-      } the package, please try again later.`;
       setIsSending(false);
 
       // On unauthorized, we force sign out
       if (err.kind === ErrorKind.Unauthorized) {
-        errMessage = `You must be signed in to ${notStarred ? 'star' : 'unstar'} a package`;
         dispatch(signOut());
-      }
+        history.push(`${window.location.pathname}?modal=login&redirect=${window.location.pathname}`);
-
+      } else {
+        let errMessage = `An error occurred ${
+          notStarred ? 'starring' : 'unstarring'
+        } the package, please try again later.`;
         alertDispatcher.postAlert({
           type: 'danger',
           message: errMessage,
         });
       }
     }
+  }
 
   useEffect(() => {
     let timeout: NodeJS.Timeout;

web/src/layout/package/SubscriptionsButton.test.tsx

@@ -1,5 +1,6 @@
 import { fireEvent, render, waitFor } from '@testing-library/react';
 import React from 'react';
+import { BrowserRouter as Router } from 'react-router-dom';
 import { mocked } from 'ts-jest/utils';
 
 import { API } from '../../api';
@@ -75,7 +76,9 @@ describe('SubscriptionsButton', () => {
 
     const result = render(
       <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+        <Router>
           <SubscriptionsButton {...defaultProps} />
+        </Router>
       </AppCtx.Provider>
     );
 
@@ -93,7 +96,9 @@ describe('SubscriptionsButton', () => {
 
         const { getByText, getByTestId, queryByRole } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <SubscriptionsButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -135,7 +140,9 @@ describe('SubscriptionsButton', () => {
 
         const { getByTestId } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <SubscriptionsButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -160,7 +167,9 @@ describe('SubscriptionsButton', () => {
 
         const { rerender } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <SubscriptionsButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -188,7 +197,9 @@ describe('SubscriptionsButton', () => {
 
         const { container } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <SubscriptionsButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -207,7 +218,9 @@ describe('SubscriptionsButton', () => {
       it('when user is not signed in', async () => {
         const { container } = render(
           <AppCtx.Provider value={{ ctx: mockNotSignedInCtx, dispatch: jest.fn() }}>
+            <Router>
               <SubscriptionsButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -221,7 +234,9 @@ describe('SubscriptionsButton', () => {
       it('when ctx.user is not initialized', async () => {
         const { container } = render(
           <AppCtx.Provider value={{ ctx: mockUndefinedUserCtx, dispatch: jest.fn() }}>
+            <Router>
               <SubscriptionsButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -240,7 +255,9 @@ describe('SubscriptionsButton', () => {
 
         const { getByTestId, getByRole } = render(
           <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+            <Router>
               <SubscriptionsButton {...defaultProps} />
+            </Router>
           </AppCtx.Provider>
         );
 
@@ -279,7 +296,9 @@ describe('SubscriptionsButton', () => {
 
       const { getByText, getByTestId } = render(
         <AppCtx.Provider value={{ ctx: mockCtx, dispatch: jest.fn() }}>
+          <Router>
             <SubscriptionsButton {...defaultProps} />
+          </Router>
         </AppCtx.Provider>
       );
 

web/src/layout/package/SubscriptionsButton.tsx

@@ -4,9 +4,10 @@ import isUndefined from 'lodash/isUndefined';
 import React, { useContext, useEffect, useRef, useState } from 'react';
 import { FaCaretDown, FaRegCheckCircle, FaRegCircle } from 'react-icons/fa';
 import { MdNotificationsActive, MdNotificationsOff } from 'react-icons/md';
+import { useHistory } from 'react-router';
 
 import { API } from '../../api';
-import { AppCtx } from '../../context/AppCtx';
+import { AppCtx, signOut } from '../../context/AppCtx';
 import useOutsideClick from '../../hooks/useOutsideClick';
 import { ErrorKind, EventKind, Subscription } from '../../types';
 import alertDispatcher from '../../utils/alertDispatcher';
@@ -18,7 +19,8 @@ interface Props {
 }
 
 const SubscriptionsButton = (props: Props) => {
-  const { ctx } = useContext(AppCtx);
+  const { ctx, dispatch } = useContext(AppCtx);
+  const history = useHistory();
   const [openStatus, setOpenStatus] = useState(false);
   const [activeSubscriptions, setActiveSubscriptions] = useState<Subscription[] | undefined | null>(undefined);
   const [isLoading, setIsLoading] = useState<boolean | null>(null);
@@ -70,6 +72,8 @@ const SubscriptionsButton = (props: Props) => {
           setIsLoading(false);
         }
       } catch (err) {
+        setActiveSubscriptions(null);
+
         if (visibleLoading) {
           setIsLoading(false);
           if (err.kind !== ErrorKind.Unauthorized) {
@@ -78,8 +82,10 @@ const SubscriptionsButton = (props: Props) => {
               message: 'An error occurred getting your subscriptions, please try again later.',
             });
           }
+        } else {
+          dispatch(signOut());
+          history.push(`${window.location.pathname}?modal=login&redirect=${window.location.pathname}`);
         }
-        setActiveSubscriptions(null);
       }
     }
   }

web/src/layout/starredPackages/index.test.tsx

@@ -117,7 +117,7 @@ describe('StarredPackagesView', () => {
 
       await waitFor(() => {
         expect(mockHistoryPush).toHaveBeenCalledTimes(1);
-        expect(mockHistoryPush).toHaveBeenCalledWith('/login?redirect=/packages/starred');
+        expect(mockHistoryPush).toHaveBeenCalledWith('/?modal=login&redirect=/packages/starred');
       });
     });
   });

web/src/layout/starredPackages/index.tsx

@@ -20,7 +20,7 @@ const StarredPackagesView = () => {
 
   const onAuthError = (): void => {
     dispatch(signOut());
-    history.push('/login?redirect=/packages/starred');
+    history.push('/?modal=login&redirect=/packages/starred');
   };
 
   useEffect(() => {

web/src/utils/cleanLoginUrlParams.tsx

@@ -0,0 +1,6 @@
+export default (url: string): string => {
+  const urlParams = new URLSearchParams(url);
+  urlParams.delete('modal');
+  urlParams.delete('redirect');
+  return urlParams.toString();
+};