Repository: joinmarket-webui/joinmarket-webui (js)

Issue:
Improve lock/unlock mechanism
Got several support requests from Users being legitimately confused about wallet locking/unlocking.

- [x] Wallet with active maker is no be displayed as "Inactive", even if no token is available
- [x] Warn user that locking wallet will abort a taker operation (if running)
- [x] Warn user that locking wallet will abort a maker operation (if running)

User journey is really a bit strange currently.. closing a tab and "re-unlocking" wallet does not stop the maker - What should we aim at here? Would it be good to inform the user in a modal with the ability to keep the maker running and "just logout" (removing only the token, not locking the wallet)?


Pull request:
fix: clear UI wallet if locking fails with status 401
This PR is a preparation for #157.

Most notable changes:
- add a "healthy" state to the websocket, which fixes the linear backoff interval if the token becomes invalid.
  socket would be opened correctly and error counter reset, shortly before the connection is closed by the server.
  Now, the error counter is reset only if the socket stays open at least short amount of time (currently 1000ms).

- Cleans the wallet if a user wants to lock the wallet with an invalid token. It will subsequently still display "Invalid Credentials" which might be confusing, but it's better than being trapped with an invalid token. 

Patch:
diff --git a/src/components/App.jsx b/src/components/App.jsx
index e095369..cd772c5 100644
--- a/src/components/App.jsx
+++ b/src/components/App.jsx
@@ -93,12 +93,12 @@ export default function App() {
         .then((res) => (res.ok ? res.json() : Promise.reject(new Error(res.statusText))))
         .then((data) => {
           const { maker_running, coinjoin_in_process, wallet_name } = data
-          const activeWallet = wallet_name !== 'None' ? wallet_name : null
+          const activeWalletName = wallet_name !== 'None' ? wallet_name : null
 
           setConnectionError(null)
           setMakerRunning(maker_running)
           setCoinjoinInProcess(coinjoin_in_process)
-          if (currentWallet && (!activeWallet || currentWallet.name !== activeWallet)) {
+          if (currentWallet && (!activeWalletName || currentWallet.name !== activeWalletName)) {
             setCurrentWallet(null)
             setCurrentWalletInfo(null)
             clearSession()
diff --git a/src/components/Send.jsx b/src/components/Send.jsx
index c3cc4bf..13ce609 100644
--- a/src/components/Send.jsx
+++ b/src/components/Send.jsx
@@ -8,7 +8,7 @@ import { useCurrentWalletInfo, useSetCurrentWalletInfo, useCurrentWallet } from
 import { useSettings } from '../context/SettingsContext'
 import * as Api from '../libs/JmWalletApi'
 
-// initial value for `minimum_markers` from the default joinmarket.cfg (last check on 2022-02-20 of v0.9.5)
+// initial value for `minimum_makers` from the default joinmarket.cfg (last check on 2022-02-20 of v0.9.5)
 const MINIMUM_MAKERS_DEFAULT_VAL = 4
 
 // not cryptographically random. returned number is in range [min, max] (both inclusive).
diff --git a/src/components/Wallet.jsx b/src/components/Wallet.jsx
index cbe29e4..108b5c2 100644
--- a/src/components/Wallet.jsx
+++ b/src/components/Wallet.jsx
@@ -26,17 +26,18 @@ export default function Wallet({ name, currentWallet, startWallet, stopWallet, s
       setIsUnlocking(true)
       try {
         const res = await Api.postWalletUnlock({ walletName }, { password })
+        const json = await res.json()
+        setIsUnlocking(false)
         if (res.ok) {
-          const { walletname: unlockedWalletName, token } = await res.json()
+          const { walletname: unlockedWalletName, token } = json
           startWallet(unlockedWalletName, token)
           navigate('/wallet')
         } else {
-          const { message } = await res.json()
+          const { message } = json
           setAlert({
             variant: 'danger',
             message: message.replace('Wallet', walletName),
           })
-          setIsUnlocking(false)
         }
       } catch (e) {
         setAlert({ variant: 'danger', message: e.message })
@@ -45,23 +46,26 @@ export default function Wallet({ name, currentWallet, startWallet, stopWallet, s
     }
   }
 
-  const lockWallet = async (name) => {
-    if (currentWallet && currentWallet.name !== name) {
-      setAlert({
-        variant: 'warning',
-        message: `${walletDisplayName(name)} is not unlocked.`,
-      })
-    }
-
+  const lockWallet = async () => {
     try {
       const { name: walletName, token } = currentWallet
       setAlert(null)
       setIsLocking(true)
 
       const res = await Api.getWalletLock({ walletName, token })
-      if (res.ok) {
-        const { walletname: lockedWalletName, already_locked } = await res.json()
+      const body = await res.json()
+
+      // On status OK or UNAUTHORIZED, stop the wallet and clear all local
+      // information. The token might have become invalid or another one might have been
+      // issued for the same wallet, etc.
+      // In any case, the user has no access to the wallet anymore.
+      if (res.ok || res.status === 401) {
         stopWallet()
+      }
+
+      if (res.ok) {
+        const { walletname: lockedWalletName, already_locked } = body
+
         setAlert({
           variant: already_locked ? 'warning' : 'success',
           message: `${walletDisplayName(lockedWalletName)} ${
@@ -70,8 +74,7 @@ export default function Wallet({ name, currentWallet, startWallet, stopWallet, s
           dismissible: true,
         })
       } else {
-        const { message } = await res.json()
-        setAlert({ variant: 'danger', message })
+        setAlert({ variant: 'danger', message: body.message })
       }
     } catch (e) {
       setAlert({ variant: 'danger', message: e.message })
@@ -95,7 +98,7 @@ export default function Wallet({ name, currentWallet, startWallet, stopWallet, s
           unlockWallet(name, password)
           break
         case 'lock':
-          lockWallet(name)
+          lockWallet()
           break
         default:
           break
@@ -147,7 +150,7 @@ export default function Wallet({ name, currentWallet, startWallet, stopWallet, s
                           Locking
                         </>
                       ) : (
-                        'Lock'
+                        <>Lock</>
                       )}
                     </rb.Button>
                   </>
@@ -182,7 +185,7 @@ export default function Wallet({ name, currentWallet, startWallet, stopWallet, s
                           Unlocking
                         </>
                       ) : (
-                        'Unlock'
+                        <>Unlock</>
                       )}
                     </rb.Button>
                     <rb.Form.Control.Feedback type="invalid">
diff --git a/src/context/WebsocketContext.jsx b/src/context/WebsocketContext.jsx
index 5e57da5..512f862 100644
--- a/src/context/WebsocketContext.jsx
+++ b/src/context/WebsocketContext.jsx
@@ -5,6 +5,9 @@ import { useCurrentWallet } from './WalletContext'
 const WEBSOCKET_RECONNECT_DELAY_STEP = 1_000
 const WEBSOCKET_RECONNECT_MAX_DELAY = 10_000
 
+// minimum amount of time in milliseconds the connection must stay open to be considered "healthy"
+const WEBSOCKET_CONNECTION_HEALTHY_DURATION = 1_000
+
 // webservers will close a websocket connection on inactivity (e.g nginx default is 60s)
 // specify the time in milliseconds at least one 'keepalive' message is sent
 const WEBSOCKET_KEEPALIVE_MESSAGE_INTERVAL = 30_000
@@ -58,6 +61,7 @@ const WebsocketContext = createContext()
 const WebsocketProvider = ({ children }) => {
   const [websocket, setWebsocket] = useState(initialWebsocket)
   const [websocketState, setWebsocketState] = useState(initialWebsocket.readyState)
+  const [isWebsocketHealthy, setIsWebsocketHealthy] = useState(false)
   const setConnectionErrorCount = useState(0)[1]
   const currentWallet = useCurrentWallet()
 
@@ -74,14 +78,31 @@ const WebsocketProvider = ({ children }) => {
     }
   }, [websocket])
 
+  useEffect(() => {
+    if (isWebsocketHealthy) {
+      // connection must be healthy before the error counter can be reset.
+      // otherwise the back-off mechanism assumes connections to be stable
+      // and will always use the minimum delay between reconnect attempts.
+      setConnectionErrorCount(0)
+    }
+  }, [isWebsocketHealthy, setConnectionErrorCount])
+
   // reconnect handling in case the socket is closed
   useEffect(() => {
-    const onOpen = () => setConnectionErrorCount(0)
+    let assumeHealthyDelayTimer
+    let retryDelayTimer
+    const onOpen = (event) => {
+      assumeHealthyDelayTimer = setTimeout(() => {
+        const stillConnectedToSameSocket = event.currentTarget === websocket
+        setIsWebsocketHealthy(stillConnectedToSameSocket)
+      }, WEBSOCKET_CONNECTION_HEALTHY_DURATION)
+    }
     const onClose = () => {
+      setIsWebsocketHealthy(false)
       setConnectionErrorCount((prev) => {
         const retryDelay = connectionRetryDelayLinear(prev + 1)
         console.log(`Retrying to connect websocket in ${retryDelay}ms`)
-        setTimeout(() => {
+        retryDelayTimer = setTimeout(() => {
           setWebsocket(createWebSocket())
         }, retryDelay)
         return prev + 1
@@ -92,6 +113,8 @@ const WebsocketProvider = ({ children }) => {
     websocket.addEventListener('close', onClose)
 
     return () => {
+      clearTimeout(assumeHealthyDelayTimer)
+      clearTimeout(retryDelayTimer)
       websocket && websocket.removeEventListener('close', onClose)
       websocket && websocket.removeEventListener('open', onOpen)
     }


Test patch:
diff --git a/src/components/Wallet.test.jsx b/src/components/Wallet.test.jsx
new file mode 100644
index 0000000..022a6fd
--- /dev/null
+++ b/src/components/Wallet.test.jsx
@@ -0,0 +1,224 @@
+import React from 'react'
+import { render, screen, waitFor } from '../testUtils'
+import { act } from 'react-dom/test-utils'
+import user from '@testing-library/user-event'
+import * as apiMock from '../libs/JmWalletApi'
+import { walletDisplayName } from '../utils'
+
+import Wallet from './Wallet'
+
+jest.mock('../libs/JmWalletApi')
+
+const mockedNavigate = jest.fn()
+jest.mock('react-router-dom', () => {
+  return {
+    ...jest.requireActual('react-router-dom'),
+    useNavigate: () => mockedNavigate,
+  }
+})
+
+describe('<Wallet />', () => {
+  const dummyWalletName = 'dummy.jmdat'
+  const dummyToken = 'dummyToken'
+
+  const mockStartWallet = jest.fn()
+  const mockStopWallet = jest.fn()
+  const mockSetAlert = jest.fn()
+
+  const setup = ({
+    name,
+    currentWallet = null,
+    startWallet = mockStartWallet,
+    stopWallet = mockStopWallet,
+    setAlert = mockSetAlert,
+  }) => {
+    render(
+      <Wallet
+        name={name}
+        currentWallet={currentWallet}
+        startWallet={startWallet}
+        stopWallet={stopWallet}
+        setAlert={setAlert}
+      />
+    )
+  }
+
+  it('should render inactive wallet without errors', () => {
+    act(() => setup({ name: dummyWalletName }))
+
+    expect(screen.getByText('dummy')).toBeInTheDocument()
+    expect(screen.getByText('Inactive')).toBeInTheDocument()
+    expect(screen.getByPlaceholderText('Password')).toBeInTheDocument()
+    expect(screen.getByText('Unlock')).toBeInTheDocument()
+    expect(screen.queryByText('Open')).not.toBeInTheDocument()
+    expect(screen.queryByText('Lock')).not.toBeInTheDocument()
+  })
+
+  it('should unlock inactive wallet successfully', async () => {
+    apiMock.postWalletUnlock.mockResolvedValueOnce({
+      ok: true,
+      json: () => Promise.resolve({ walletname: dummyWalletName, token: dummyToken }),
+    })
+
+    act(() => setup({ name: dummyWalletName }))
+
+    expect(screen.getByText('Inactive')).toBeInTheDocument()
+    expect(screen.getByText('Unlock')).toBeInTheDocument()
+    expect(screen.getByPlaceholderText('Password')).toBeInTheDocument()
+
+    await act(async () => {
+      user.type(screen.getByPlaceholderText('Password'), 'correct horse battery staple')
+      const unlockWalletButton = screen.getByText('Unlock')
+      user.click(unlockWalletButton)
+
+      await waitFor(() => screen.findByText(/Unlocking/))
+      await waitFor(() => screen.findByText('Unlock'))
+    })
+
+    expect(mockStartWallet).toHaveBeenCalledWith(dummyWalletName, dummyToken)
+    expect(mockedNavigate).toHaveBeenCalledWith('/wallet')
+  })
+
+  it('should add alert if unlocking of inactive wallet fails', async () => {
+    const apiErrorMessage = 'ANY_ERROR_MESSAGE with template <Wallet>'
+    apiMock.postWalletUnlock.mockResolvedValueOnce({
+      ok: false,
+      json: () => Promise.resolve({ message: apiErrorMessage }),
+    })
+
+    act(() => setup({ name: dummyWalletName }))
+
+    expect(screen.getByText('Inactive')).toBeInTheDocument()
+    expect(screen.getByText('Unlock')).toBeInTheDocument()
+    expect(screen.getByPlaceholderText('Password')).toBeInTheDocument()
+
+    await act(async () => {
+      user.type(screen.getByPlaceholderText('Password'), 'correct horse battery staple')
+      const unlockWalletButton = screen.getByText('Unlock')
+      user.click(unlockWalletButton)
+
+      await waitFor(() => screen.findByText(/Unlocking/))
+      await waitFor(() => screen.findByText('Unlock'))
+    })
+
+    expect(mockStartWallet).not.toHaveBeenCalled()
+    expect(mockedNavigate).not.toHaveBeenCalled()
+    expect(mockSetAlert).toHaveBeenCalledWith({
+      variant: 'danger',
+      message: apiErrorMessage.replace('Wallet', dummyWalletName),
+    })
+  })
+
+  it('should render active wallet without errors', () => {
+    act(() => setup({ name: dummyWalletName, currentWallet: { name: dummyWalletName, token: dummyToken } }))
+
+    expect(screen.getByText('dummy')).toBeInTheDocument()
+    expect(screen.getByText('Active')).toBeInTheDocument()
+    expect(screen.getByText('Open')).toBeInTheDocument()
+    expect(screen.getByText('Lock')).toBeInTheDocument()
+    expect(screen.queryByPlaceholderText('Password')).not.toBeInTheDocument()
+    expect(screen.queryByText('Unlock')).not.toBeInTheDocument()
+  })
+
+  it('should lock active wallet successfully', async () => {
+    apiMock.getWalletLock.mockResolvedValueOnce({
+      ok: true,
+      json: () => Promise.resolve({ walletname: dummyWalletName, already_locked: false }),
+    })
+
+    act(() => setup({ name: dummyWalletName, currentWallet: { name: dummyWalletName, token: dummyToken } }))
+
+    expect(screen.getByText('Active')).toBeInTheDocument()
+    expect(screen.getByText('Lock')).toBeInTheDocument()
+
+    await act(async () => {
+      const lockWalletButton = screen.getByText('Lock')
+      user.click(lockWalletButton)
+
+      await waitFor(() => screen.findByText(/Locking/))
+      await waitFor(() => screen.findByText('Lock'))
+    })
+
+    expect(mockStopWallet).toHaveBeenCalled()
+    expect(mockSetAlert).toHaveBeenCalledWith({
+      variant: 'success',
+      message: `${walletDisplayName(dummyWalletName)} locked successfully.`,
+      dismissible: true,
+    })
+  })
+
+  it('should add alert if locking active wallet fails', async () => {
+    const apiErrorMessage = 'ANY_ERROR_MESSAGE'
+    apiMock.getWalletLock.mockResolvedValueOnce({
+      ok: false,
+      status: 500, // any other error status than 401
+      json: () => Promise.resolve({ message: apiErrorMessage }),
+    })
+
+    act(() => setup({ name: dummyWalletName, currentWallet: { name: dummyWalletName, token: dummyToken } }))
+
+    expect(screen.getByText('Active')).toBeInTheDocument()
+    expect(screen.getByText('Lock')).toBeInTheDocument()
+
+    await act(async () => {
+      const lockWalletButton = screen.getByText('Lock')
+      user.click(lockWalletButton)
+
+      await waitFor(() => screen.findByText(/Locking/))
+      await waitFor(() => screen.findByText('Lock'))
+    })
+
+    // on errors other than 401, stopWallet should not have been called
+    expect(mockStopWallet).not.toHaveBeenCalled()
+    expect(mockSetAlert).toHaveBeenCalledWith({
+      variant: 'danger',
+      message: apiErrorMessage,
+    })
+  })
+
+  it('should add alert but clear wallet if locking active wallet fails with UNAUTHORIZED', async () => {
+    const apiErrorMessage = 'Invalid credentials.'
+    apiMock.getWalletLock.mockResolvedValueOnce({
+      ok: false,
+      status: 401,
+      json: () => Promise.resolve({ message: apiErrorMessage }),
+    })
+
+    act(() => setup({ name: dummyWalletName, currentWallet: { name: dummyWalletName, token: dummyToken } }))
+
+    expect(screen.getByText('Active')).toBeInTheDocument()
+    expect(screen.getByText('Lock')).toBeInTheDocument()
+
+    await act(async () => {
+      const lockWalletButton = screen.getByText('Lock')
+      user.click(lockWalletButton)
+
+      await waitFor(() => screen.findByText(/Locking/))
+      await waitFor(() => screen.findByText('Lock'))
+    })
+
+    // on 401 errors, stopWallet *should* have been called
+    expect(mockStopWallet).toHaveBeenCalled()
+    expect(mockSetAlert).toHaveBeenCalledWith({
+      variant: 'danger',
+      message: apiErrorMessage,
+    })
+  })
+
+  it('should render active wallet when token is missing', () => {
+    act(() => setup({ name: dummyWalletName, currentWallet: { name: dummyWalletName, token: null } }))
+
+    expect(
+      screen.getByText(
+        'This wallet is active, but there is no token to interact with it. Please remove the lock file on the server.'
+      )
+    ).toBeInTheDocument()
+
+    expect(screen.getByText('dummy')).toBeInTheDocument()
+    expect(screen.getByText('Active')).toBeInTheDocument()
+    expect(screen.queryByPlaceholderText('Password')).not.toBeInTheDocument()
+    expect(screen.queryByText('Open')).not.toBeInTheDocument()
+    expect(screen.queryByText('Lock')).not.toBeInTheDocument()
+    expect(screen.queryByText('Unlock')).not.toBeInTheDocument()
+  })
+})
diff --git a/src/components/Wallets.test.jsx b/src/components/Wallets.test.jsx
index bddc3e0..bf565fb 100644
--- a/src/components/Wallets.test.jsx
+++ b/src/components/Wallets.test.jsx
@@ -8,79 +8,77 @@ import Wallets from './Wallets'
 
 jest.mock('../libs/JmWalletApi')
 
-it('should render without errors', () => {
-  apiMock.getWalletAll.mockResolvedValueOnce(new Promise((r) => setTimeout(r, 1_000)))
-
-  act(() => {
+describe('<Wallets />', () => {
+  const setup = () => {
     render(<Wallets />)
-  })
+  }
 
-  expect(screen.getByText('wallets.title')).toBeInTheDocument()
-  expect(screen.getByText('wallets.text_loading')).toBeInTheDocument()
-  expect(screen.getByText('wallets.button_new_wallet')).toBeInTheDocument()
-})
+  it('should render without errors', () => {
+    apiMock.getWalletAll.mockResolvedValueOnce(new Promise((r) => setTimeout(r, 1_000)))
 
-it('should display error message when loading wallets fails', async () => {
-  apiMock.getWalletAll.mockResolvedValueOnce({
-    ok: false,
-  })
+    act(setup)
 
-  act(() => {
-    render(<Wallets />)
+    expect(screen.getByText('wallets.title')).toBeInTheDocument()
+    expect(screen.getByText('wallets.text_loading')).toBeInTheDocument()
+    expect(screen.getByText('wallets.button_new_wallet')).toBeInTheDocument()
   })
 
-  expect(screen.getByText('wallets.title')).toBeInTheDocument()
-  expect(screen.getByText('wallets.button_new_wallet')).toBeInTheDocument()
+  it('should display error message when loading wallets fails', async () => {
+    apiMock.getWalletAll.mockResolvedValueOnce({
+      ok: false,
+    })
 
-  await waitForElementToBeRemoved(screen.getByText('wallets.text_loading'))
+    act(setup)
 
-  expect(screen.getByText('wallets.error_loading_failed')).toBeInTheDocument()
-  expect(screen.getByText('wallets.button_new_wallet')).toBeInTheDocument()
-})
+    expect(screen.getByText('wallets.title')).toBeInTheDocument()
+    expect(screen.getByText('wallets.button_new_wallet')).toBeInTheDocument()
 
-it('should display big call-to-action button if no wallet has been created yet', async () => {
-  apiMock.getWalletAll.mockResolvedValueOnce({
-    ok: true,
-    json: () => Promise.resolve({ wallets: [] }),
-  })
+    await waitForElementToBeRemoved(screen.getByText('wallets.text_loading'))
 
-  act(() => {
-    render(<Wallets />)
+    expect(screen.getByText('wallets.error_loading_failed')).toBeInTheDocument()
+    expect(screen.getByText('wallets.button_new_wallet')).toBeInTheDocument()
   })
 
-  expect(screen.getByText('wallets.text_loading')).toBeInTheDocument()
+  it('should display big call-to-action button if no wallet has been created yet', async () => {
+    apiMock.getWalletAll.mockResolvedValueOnce({
+      ok: true,
+      json: () => Promise.resolve({ wallets: [] }),
+    })
 
-  const callToActionButtonBefore = screen.getByText('wallets.button_new_wallet')
-  expect(callToActionButtonBefore.classList.contains('btn')).toBe(true)
-  expect(callToActionButtonBefore.classList.contains('btn-lg')).toBe(false)
+    act(setup)
 
-  await waitForElementToBeRemoved(screen.getByText('wallets.text_loading'))
+    expect(screen.getByText('wallets.text_loading')).toBeInTheDocument()
 
-  expect(screen.getByText('wallets.subtitle_no_wallets')).toBeInTheDocument()
+    const callToActionButtonBefore = screen.getByText('wallets.button_new_wallet')
+    expect(callToActionButtonBefore.classList.contains('btn')).toBe(true)
+    expect(callToActionButtonBefore.classList.contains('btn-lg')).toBe(false)
 
-  const callToActionButtonAfter = screen.getByText('wallets.button_new_wallet')
-  expect(callToActionButtonAfter.classList.contains('btn-lg')).toBe(true)
-})
+    await waitForElementToBeRemoved(screen.getByText('wallets.text_loading'))
 
-it('should display login for available wallets', async () => {
-  apiMock.getWalletAll.mockResolvedValueOnce({
-    ok: true,
-    json: () => Promise.resolve({ wallets: ['wallet0.jmdat', 'wallet1.jmdat'] }),
-  })
+    expect(screen.getByText('wallets.subtitle_no_wallets')).toBeInTheDocument()
 
-  act(() => {
-    render(<Wallets />)
+    const callToActionButtonAfter = screen.getByText('wallets.button_new_wallet')
+    expect(callToActionButtonAfter.classList.contains('btn-lg')).toBe(true)
   })
 
-  expect(screen.getByText('wallets.text_loading')).toBeInTheDocument()
-  await waitForElementToBeRemoved(screen.getByText('wallets.text_loading'))
+  it('should display login for available wallets', async () => {
+    apiMock.getWalletAll.mockResolvedValueOnce({
+      ok: true,
+      json: () => Promise.resolve({ wallets: ['wallet0.jmdat', 'wallet1.jmdat'] }),
+    })
 
-  expect(screen.queryByText('wallets.alert_wallet_open')).not.toBeInTheDocument()
+    act(setup)
 
-  expect(screen.getByText('wallet0')).toBeInTheDocument()
-  expect(screen.getByText('wallet1')).toBeInTheDocument()
+    expect(screen.getByText('wallets.text_loading')).toBeInTheDocument()
+    await waitForElementToBeRemoved(screen.getByText('wallets.text_loading'))
 
-  const callToActionButton = screen.getByText('wallets.button_new_wallet')
-  expect(callToActionButton.classList.contains('btn')).toBe(true)
-  expect(callToActionButton.classList.contains('btn-lg')).toBe(false)
+    expect(screen.queryByText('wallets.alert_wallet_open')).not.toBeInTheDocument()
+
+    expect(screen.getByText('wallet0')).toBeInTheDocument()
+    expect(screen.getByText('wallet1')).toBeInTheDocument()
+
+    const callToActionButton = screen.getByText('wallets.button_new_wallet')
+    expect(callToActionButton.classList.contains('btn')).toBe(true)
+    expect(callToActionButton.classList.contains('btn-lg')).toBe(false)
+  })
 })