Allow re-unlock of wallets via /unlock

Fixes #1121.
Prior to this commit, if a user
lost access to the authentication token for
a session with the wallet RPC, they would not
be able to lock the existing wallet before
authenticating again (getting a new token for
the same wallet, or a different one).
After this commit, a call to the /unlock endpoint
will succeed even if an existing wallet is currently
unlocked (whether a different one or the same one).
The existing wallet service, if present, is shut down,
if the attempt to authenticate a new wallet, with a
password is successful (otherwise nothing happens).
A test is added to make sure that this re-unlock can work.

jmclient/jmclient/wallet-rpc-api.yaml

@@ -69,7 +69,7 @@ paths:
             type: string
       responses:
         '200':
-          $ref: "#/components/responses/Unlock-200-OK"
+          $ref: "#/components/responses/Lock-200-OK"
         '400':
           $ref: '#/components/responses/400-BadRequest'
         '401':

jmclient/jmclient/wallet_rpc.py

@@ -215,6 +215,8 @@ class JMWalletDaemon(Service):
         # Currently valid authorization tokens must be removed
         # from the daemon:
         self.cookie = None
+        if self.wss_factory:
+            self.wss_factory.valid_token = None
         # if the wallet-daemon is shut down, all services
         # it encapsulates must also be shut down.
         for name, service in self.services.items():
@@ -323,9 +325,17 @@ class JMWalletDaemon(Service):
         (currently THE wallet, daemon does not yet support multiple).
         This is maintained for 30 minutes currently, or until the user
         switches to a new wallet.
+        If an existing wallet_service was in place, it needs to be stopped.
         Here we must also register transaction update callbacks, to fire
         events in the websocket connection.
         """
+        if self.wallet_service:
+            # we allow a new successful authorization (with password)
+            # to shut down the currently running service(s), if there
+            # are any.
+            # This will stop all supporting services and wipe
+            # state (so wallet, maker service and cookie/token):
+            self.stopService()
         # any random secret is OK, as long as it is not deducible/predictable:
         secret_key = bintohex(os.urandom(16))
         encoded_token = jwt.encode({"wallet": wallet_name,
@@ -355,9 +365,10 @@ class JMWalletDaemon(Service):
         self.wallet_service.register_callbacks(
             [self.wss_factory.sendTxNotification], None)
         self.wallet_service.startService()
-        # now that the service is intialized, we want to
+        # now that the base WalletService is started, we want to
         # make sure that any websocket clients use the correct
-        # token:
+        # token. The wss_factory should have been created on JMWalletDaemon
+        # startup, so any failure to exist here is a logic error:
         self.wss_factory.valid_token = encoded_token
         # now that the WalletService instance is active and ready to
         # respond to requests, we return the status to the client:
@@ -639,6 +650,9 @@ class JMWalletDaemon(Service):
         def unlockwallet(self, request, walletname):
             """ If a user succeeds in authenticating and opening a
             wallet, we start the corresponding wallet service.
+            Notice that in the case the user fails for any reason,
+            then any existing wallet service, and corresponding token,
+            will remain active.
             """
             print_req(request)
             assert isinstance(request.content, BytesIO)
@@ -646,29 +660,25 @@ class JMWalletDaemon(Service):
             if not auth_json:
                 raise InvalidRequestFormat()
             password = auth_json["password"]
-            if self.wallet_service is None:
-                wallet_path = get_wallet_path(walletname, None)
-                try:
-                    wallet = open_test_wallet_maybe(
-                            wallet_path, walletname, 4,
-                            password=password.encode("utf-8"),
-                            ask_for_password=False)
-                except StoragePasswordError:
-                    raise NotAuthorized()
-                except RetryableStorageError:
-                    # .lock file exists
-                    raise LockExists()
-                except StorageError:
-                    # wallet is not openable
-                    raise NoWalletFound()
-                except Exception:
-                    # wallet file doesn't exist or is wrong format
-                    raise NoWalletFound()
-                return self.initialize_wallet_service(request, wallet, walletname)
-            else:
-                jlog.warn('Tried to unlock wallet, but one is already unlocked.')
-                jlog.warn('Currently only one active wallet at a time is supported.')
-                raise WalletAlreadyUnlocked()
+
+            wallet_path = get_wallet_path(walletname, None)
+            try:
+                wallet = open_test_wallet_maybe(
+                        wallet_path, walletname, 4,
+                        password=password.encode("utf-8"),
+                        ask_for_password=False)
+            except StoragePasswordError:
+                raise NotAuthorized()
+            except RetryableStorageError:
+                # .lock file exists
+                raise LockExists()
+            except StorageError:
+                # wallet is not openable
+                raise NoWalletFound()
+            except Exception:
+                # wallet file doesn't exist or is wrong format
+                raise NoWalletFound()
+            return self.initialize_wallet_service(request, wallet, walletname)
 
         #This route should return list of current wallets created.
         @app.route('/wallet/all', methods=['GET'])

jmclient/test/test_wallet_rpc.py

@@ -20,7 +20,7 @@ from test_websocket import (ClientTProtocol, test_tx_hex_1,
 
 testdir = os.path.dirname(os.path.realpath(__file__))
 
-testfileloc = "testwrpc.jmdat"
+testfilename = "testwrpc"
 
 jlog = get_log()
 
@@ -42,10 +42,12 @@ class WalletRPCTestBase(object):
     dport = 28183
     # the port for the ws
     wss_port = 28283
-    
+    # how many different wallets we need
+    num_wallet_files = 2
+
     def setUp(self):
         load_test_config()
-        self.clean_out_wallet_file()
+        self.clean_out_wallet_files()
         jm_single().bc_interface.tick_forward_chain_interval = 5
         jm_single().bc_interface.simulate_blocks()
         # a client connnection object which is often but not always
@@ -59,7 +61,7 @@ class WalletRPCTestBase(object):
         # because we sync and start the wallet service manually here
         # (and don't use wallet files yet), we won't have set a wallet name,
         # so we set it here:
-        self.daemon.wallet_name = testfileloc
+        self.daemon.wallet_name = self.get_wallet_file_name(1)
         r, s = self.daemon.startService()
         self.listener_rpc = r
         self.listener_ws = s
@@ -82,12 +84,21 @@ class WalletRPCTestBase(object):
         addr += api_version_string
         return addr
 
-    def clean_out_wallet_file(self):
-        if os.path.exists(os.path.join(".", "wallets", testfileloc)):
-            os.remove(os.path.join(".", "wallets", testfileloc))
+    def clean_out_wallet_files(self):
+        for i in range(1, self.num_wallet_files + 1):
+            wfn = self.get_wallet_file_name(i, fullpath=True)
+            if os.path.exists(wfn):
+                os.remove(wfn)
+
+    def get_wallet_file_name(self, i, fullpath=False):
+        tfn = testfilename + str(i) + ".jmdat"
+        if fullpath:
+            return os.path.join(".", "wallets", tfn)
+        else:
+            return tfn
 
     def tearDown(self):
-        self.clean_out_wallet_file()
+        self.clean_out_wallet_files()
         for dc in reactor.getDelayedCalls():
             dc.cancel()        
         d1 = defer.maybeDeferred(self.listener_ws.stopListening)
@@ -155,10 +166,13 @@ class TrialTestWRPC_DisplayWallet(WalletRPCTestBase, unittest.TestCase):
 
         1. create a wallet and have it persisted
            to disk in ./wallets, and get a token.
-        2. list wallets and check they contain the new
+        2. lock that wallet.
+        3. create a second wallet as above.
+        4. list wallets and check they contain the new
            wallet.
-        3. lock the existing wallet service, using the token.
-        4. Unlock the wallet with /unlock, get a token.
+        5. lock the existing wallet service, using the token.
+        6. Unlock the original wallet with /unlock, get a token.
+        7. Unlock the second wallet with /unlock, get a token.
         """
         # before starting, we have to shut down the existing
         # wallet service (usually this would be `lock`):
@@ -166,40 +180,67 @@ class TrialTestWRPC_DisplayWallet(WalletRPCTestBase, unittest.TestCase):
         self.daemon.stopService()
         self.daemon.auth_disabled = False
 
+        wfn1 = self.get_wallet_file_name(1)
+        wfn2 = self.get_wallet_file_name(2)
+        self.wfnames = [wfn1, wfn2]
         agent = get_nontor_agent()
         root = self.get_route_root()
+
+        # 1. Create first
         addr = root + "/wallet/create"
         addr = addr.encode()
-        body = BytesProducer(json.dumps({"walletname": testfileloc,
+        body = BytesProducer(json.dumps({"walletname": wfn1,
                 "password": "hunter2", "wallettype": "sw-fb"}).encode())
         yield self.do_request(agent, b"POST", addr, body,
                               self.process_create_wallet_response)
 
+        # 2. now *lock*
+        addr = root + "/wallet/" + wfn1 + "/lock"
+        addr = addr.encode()
+        jlog.info("Using address: {}".format(addr))
+        yield self.do_request(agent, b"GET", addr, None,
+                self.process_lock_response, token=self.jwt_token)
+
+        # 3. Create this secondary wallet (so we can test re-unlock)
+        addr = root + "/wallet/create"
+        addr = addr.encode()
+        body = BytesProducer(json.dumps({"walletname": wfn2,
+                "password": "hunter3", "wallettype": "sw"}).encode())
+        yield self.do_request(agent, b"POST", addr, body,
+                              self.process_create_wallet_response)
+
+        # 4. List wallets
         addr = root + "/wallet/all"
         addr = addr.encode()
         # does not require a token, though we just got one.
         yield self.do_request(agent, b"GET", addr, None,
                                self.process_list_wallets_response)
 
-        # now *lock* the existing, which will shut down the wallet
-        # service associated.
-        addr = root + "/wallet/" + self.daemon.wallet_name + "/lock"
+        # 5. now *lock* the active.
+        addr = root + "/wallet/" + wfn2 + "/lock"
         addr = addr.encode()
         jlog.info("Using address: {}".format(addr))
         yield self.do_request(agent, b"GET", addr, None,
                 self.process_lock_response, token=self.jwt_token)
         # wallet service should now be stopped.
-        addr = root + "/wallet/" + self.daemon.wallet_name + "/unlock"
+        # 6. Unlock the original wallet
+        addr = root + "/wallet/" + wfn1 + "/unlock"
         addr = addr.encode()
         body = BytesProducer(json.dumps({"password": "hunter2"}).encode())
         yield self.do_request(agent, b"POST", addr, body,
                               self.process_unlock_response)
 
+        # 7. Unlock the second wallet again
+        addr = root + "/wallet/" + wfn2 + "/unlock"
+        addr = addr.encode()
+        body = BytesProducer(json.dumps({"password": "hunter3"}).encode())
+        yield self.do_request(agent, b"POST", addr, body,
+                              self.process_unlock_response)
 
     def process_create_wallet_response(self, response, code):
         assert code == 201
         json_body = json.loads(response.decode("utf-8"))
-        assert json_body["walletname"] == testfileloc
+        assert json_body["walletname"] in self.wfnames
         self.jwt_token = json_body["token"]
         # we don't use this in test, but it must exist:
         assert json_body["seedphrase"]
@@ -207,7 +248,7 @@ class TrialTestWRPC_DisplayWallet(WalletRPCTestBase, unittest.TestCase):
     def process_list_wallets_response(self, body, code):
         assert code == 200
         json_body = json.loads(body.decode("utf-8"))
-        assert json_body["wallets"] == [testfileloc]
+        assert set(json_body["wallets"]) == set(self.wfnames)
 
     @defer.inlineCallbacks
     def test_direct_send_and_display_wallet(self):
@@ -369,13 +410,13 @@ class TrialTestWRPC_DisplayWallet(WalletRPCTestBase, unittest.TestCase):
     def process_unlock_response(self, response, code):
         assert code == 200
         json_body = json.loads(response.decode("utf-8"))
-        assert json_body["walletname"] == testfileloc
+        assert json_body["walletname"] in self.wfnames
         self.jwt_token = json_body["token"]
 
     def process_lock_response(self, response, code):
         assert code == 200
         json_body = json.loads(response.decode("utf-8"))
-        assert json_body["walletname"] == testfileloc
+        assert json_body["walletname"] in self.wfnames
 
     @defer.inlineCallbacks
     def test_do_coinjoin(self):