From ed7b4e19266ddad042c6afb9a245c555a0f1a527 Mon Sep 17 00:00:00 2001 From: Adam Gibson Date: Thu, 3 Feb 2022 12:29:30 +0000 Subject: [PATCH] Fix bugs in unlock-lock logic Fix shutdown of already open wallet on new unlock Prior to this commit, if a JSON-RPC API client called /unlock on a new wallet while an existing one was currently loaded, the preexisting wallet would not have its wallet service shut down, and thus the .lock file would not be removed, resulting in an inability to open that wallet a second time. After this commit, we correctly refer to the wallet service internally such that its service does get shut down, and the close event of the wallet occurs, removing the .lock file as required. Fix re-unlock of currently unlocked wallet This behaviour was originally intended by #1136. If a wallet is currently in an unlocked state in the daemon, but the user loses the auth token, and tries to re-unlock, then before this commit, the unlock operation would fail because there was an unsuccessful attempt to open the wallet file. After this commit, if the unlock request is applied to the wallet with the same name as the one currently active, then the password is authenticated by opening the wallet in read-only mode, preventing the above issue from occurring. Fix bug that lockwallet doesn't delete wallet_name Before this commit, the member variable wallet_name in the JMWalletDaemon object was not being reset to None in a call to the lockwallet endpoint which meant that the unlock event erroneously treated the wallet as already being active. This corrects that error by setting walet_name to None in lockwallet. --- jmclient/jmclient/wallet_rpc.py | 145 ++++++++++++++++++++----------- jmclient/test/test_wallet_rpc.py | 12 +-- 2 files changed, 99 insertions(+), 58 deletions(-) diff --git a/jmclient/jmclient/wallet_rpc.py b/jmclient/jmclient/wallet_rpc.py index f71ba57..df5c4f5 100644 --- a/jmclient/jmclient/wallet_rpc.py +++ b/jmclient/jmclient/wallet_rpc.py @@ -143,8 +143,6 @@ class JMWalletDaemon(Service): # allow the client to start/stop. self.services["wallet"] = None self.wallet_name = "None" - # label for convenience: - self.wallet_service = self.services["wallet"] # Client may start other services, but only # one instance. self.services["snicker"] = None @@ -324,6 +322,23 @@ class JMWalletDaemon(Service): request_cookie) + ", request rejected.") raise NotAuthorized() + def set_token(self, wallet_name): + """ This function creates a new JWT token and sets it as our + 'cookie' for API and WS. Note this always creates a new fresh token, + there is no option to manually set it, intentionally. + """ + # 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, + "exp" :datetime.datetime.utcnow( + )+datetime.timedelta(minutes=30)}, + secret_key) + self.cookie = encoded_token.strip() + # We want to make sure that any websocket clients use the correct + # 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 = self.cookie + def get_POST_body(self, request, keys): """ given a request object, retrieve values corresponding to keys keys in a dict, assuming they were encoded using JSON. @@ -349,24 +364,15 @@ class JMWalletDaemon(Service): Here we must also register transaction update callbacks, to fire events in the websocket connection. """ - if self.wallet_service: + if self.services["wallet"]: # 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, - "exp" :datetime.datetime.utcnow( - )+datetime.timedelta(minutes=30)}, - secret_key) - encoded_token = encoded_token.strip() - self.cookie = encoded_token - if self.cookie is None: - raise NotAuthorized("No cookie") - self.wallet_service = WalletService(wallet) + + self.services["wallet"] = WalletService(wallet) # restart callback needed, otherwise wallet creation will # automatically lead to shutdown. # TODO: this means that it's possible, in non-standard usage @@ -375,28 +381,26 @@ class JMWalletDaemon(Service): # or requesting rescans, none are implemented yet. def dummy_restart_callback(msg): jlog.warn("Ignoring rescan request from backend wallet service: " + msg) - self.wallet_service.add_restart_callback(dummy_restart_callback) + self.services["wallet"].add_restart_callback(dummy_restart_callback) self.wallet_name = wallet_name - self.wallet_service.register_callbacks( + self.services["wallet"].register_callbacks( [self.wss_factory.sendTxNotification], None) - self.wallet_service.startService() - # now that the base WalletService is started, we want to - # make sure that any websocket clients use the correct - # 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 + self.services["wallet"].startService() # now that the WalletService instance is active and ready to # respond to requests, we return the status to the client: + + # First, prepare authentication for the calling client: + self.set_token(wallet_name) if('seedphrase' in kwargs): return make_jmwalletd_response(request, status=201, walletname=self.wallet_name, - token=encoded_token, + token=self.cookie, seedphrase=kwargs.get('seedphrase')) else: return make_jmwalletd_response(request, walletname=self.wallet_name, - token=encoded_token) + token=self.cookie) def taker_finished(self, res, fromtx=False, waittime=0.0, txdetails=None): # This is a slimmed down version compared with what is seen in @@ -468,14 +472,14 @@ class JMWalletDaemon(Service): def displaywallet(self, request, walletname): print_req(request) self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: jlog.warn("displaywallet called, but no wallet loaded") raise NoWalletFound() if not self.wallet_name == walletname: jlog.warn("called displaywallet with wrong wallet") raise InvalidRequestFormat() else: - walletinfo = wallet_display(self.wallet_service, False, jsonified=True) + walletinfo = wallet_display(self.services["wallet"], False, jsonified=True) return make_jmwalletd_response(request, walletname=walletname, walletinfo=walletinfo) @app.route('/session', methods=['GET']) @@ -489,8 +493,8 @@ class JMWalletDaemon(Service): session = not self.cookie==None maker_running = self.coinjoin_state == CJ_MAKER_RUNNING coinjoin_in_process = self.coinjoin_state == CJ_TAKER_RUNNING - if self.wallet_service: - if self.wallet_service.isRunning(): + if self.services["wallet"]: + if self.services["wallet"].isRunning(): wallet_name = self.wallet_name else: wallet_name = "not yet loaded" @@ -512,12 +516,12 @@ class JMWalletDaemon(Service): "destination"]) if not payment_info_json: raise InvalidRequestFormat() - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() try: - tx = direct_send(self.wallet_service, + tx = direct_send(self.services["wallet"], int(payment_info_json["amount_sats"]), int(payment_info_json["mixdepth"]), destination=payment_info_json["destination"], @@ -542,7 +546,7 @@ class JMWalletDaemon(Service): "ordertype", "minsize"]) if not config_json: raise InvalidRequestFormat() - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() @@ -563,7 +567,7 @@ class JMWalletDaemon(Service): config_json["cjfee_factor"] = None config_json["size_factor"] = None - self.services["maker"] = YieldGeneratorService(self.wallet_service, + self.services["maker"] = YieldGeneratorService(self.services["wallet"], dhost, dport, [config_json[x] for x in ["txfee", "cjfee_a", "cjfee_r", "ordertype", "minsize", @@ -585,7 +589,7 @@ class JMWalletDaemon(Service): # sync has already happened (this is different from CLI yg). # note: an edge case of dusty amounts is lost here; it will get # picked up by Maker.try_to_create_my_orders(). - if not len(self.wallet_service.get_balance_by_mixdepth( + if not len(self.services["wallet"].get_balance_by_mixdepth( verbose=False, minconfs=1)) > 0: raise NotEnoughCoinsForMaker() @@ -600,7 +604,7 @@ class JMWalletDaemon(Service): @app.route('/wallet//maker/stop', methods=['GET']) def stop_maker(self, request, walletname): self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() @@ -643,19 +647,20 @@ class JMWalletDaemon(Service): def lockwallet(self, request, walletname): print_req(request) self.check_cookie(request) - if self.wallet_service and not self.wallet_name == walletname: + if self.services["wallet"] and not self.wallet_name == walletname: raise InvalidRequestFormat() - if not self.wallet_service: + if not self.services["wallet"]: jlog.warn("Called lock, but no wallet loaded") # we could raise NoWalletFound here, but is # easier for clients if they can gracefully call # lock multiple times: already_locked = True else: - self.wallet_service.stopService() + self.services["wallet"].stopService() self.cookie = None self.wss_factory.valid_token = None - self.wallet_service = None + self.services["wallet"] = None + self.wallet_name = None already_locked = False return make_jmwalletd_response(request, walletname=walletname, already_locked=already_locked) @@ -666,7 +671,7 @@ class JMWalletDaemon(Service): # we only handle one wallet at a time; # if there is a currently unlocked wallet, # refuse to process the request: - if self.wallet_service: + if self.services["wallet"]: raise WalletAlreadyUnlocked() request_data = self.get_POST_body(request, ["walletname", "password", "wallettype"]) @@ -716,8 +721,44 @@ class JMWalletDaemon(Service): if not auth_json: raise InvalidRequestFormat() password = auth_json["password"] - wallet_path = get_wallet_path(walletname, None) + + # for someone trying to re-access the wallet, using their + # password as authentication, and get a fresh token, we still + # need to authenticate against the password, but we cannot directly + # re-open the wallet file (it is currently locked), but we don't + # yet want to bother to shut down services - because if their + # authentication is successful, we can happily leave everything + # running (wallet service and e.g. a yieldgenerator). + # Hence here, if it is the same name, we do a read-only open + # and proceed to issue a new token if the open is successful, + # otherwise error. + if walletname == self.wallet_name: + try: + # returned wallet object is ditched: + open_test_wallet_maybe( + wallet_path, walletname, 4, + password=password.encode("utf-8"), + ask_for_password=False, + read_only=True) + except StoragePasswordError: + # actually effects authentication + raise NotAuthorized() + except StorageError: + # wallet is not openable, this should not happen + raise NoWalletFound() + except Exception: + # wallet file doesn't exist or is wrong format, + # this also shouldn't happen so raise: + raise NoWalletFound() + # no exceptions raised means we just return token: + self.set_token(self.wallet_name) + return make_jmwalletd_response(request, + walletname=self.wallet_name, + token=self.cookie) + + # This is a different wallet than the one currently open; + # try to open it, then initialize the service(s): try: wallet = open_test_wallet_maybe( wallet_path, walletname, 4, @@ -756,7 +797,7 @@ class JMWalletDaemon(Service): @app.route('/wallet//address/new/', methods=['GET']) def getaddress(self, request, walletname, mixdepth): self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() @@ -764,19 +805,19 @@ class JMWalletDaemon(Service): mixdepth = int(mixdepth) except ValueError: raise InvalidRequestFormat() - address = self.wallet_service.get_external_addr(mixdepth) + address = self.services["wallet"].get_external_addr(mixdepth) return make_jmwalletd_response(request, address=address) @app.route('/wallet//address/timelock/new/', methods=['GET']) def gettimelockaddress(self, request, walletname, lockdate): self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() try: timelockaddress = wallet_gettimelockaddress( - self.wallet_service.wallet, lockdate) + self.services["wallet"].wallet, lockdate) except Exception: raise InvalidRequestFormat() if timelockaddress == "": @@ -847,7 +888,7 @@ class JMWalletDaemon(Service): # note: this does not raise or fail if the applied # disable state (true/false) is the same as the current # one; that is accepted and not an error. - self.wallet_service.disable_utxo(txid, index, to_disable) + self.services["wallet"].disable_utxo(txid, index, to_disable) except AssertionError: # should be impossible because format checked by # utxostr_to_utxo: @@ -865,13 +906,13 @@ class JMWalletDaemon(Service): @app.route('/wallet//utxos',methods=['GET']) def listutxos(self, request, walletname): self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() # note: the output of `showutxos` is already a string for CLI; # but we return json: - utxos = json.loads(wallet_showutxos(self.wallet_service, False)) + utxos = json.loads(wallet_showutxos(self.services["wallet"], False)) utxos_response = self.get_listutxos_response(utxos) return make_jmwalletd_response(request, utxos=utxos_response) @@ -879,7 +920,7 @@ class JMWalletDaemon(Service): @app.route('/wallet//taker/stop', methods=['GET']) def stopcoinjoin(self, request, walletname): self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() @@ -892,7 +933,7 @@ class JMWalletDaemon(Service): @app.route('/wallet//taker/coinjoin', methods=['POST']) def docoinjoin(self, request, walletname): self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() @@ -928,7 +969,7 @@ class JMWalletDaemon(Service): raise ConfigNotPresent() max_cj_fee= get_max_cj_fee_values(jm_single().config, None, user_callback=dummy_user_callback) - self.taker = Taker(self.wallet_service, schedule, + self.taker = Taker(self.services["wallet"], schedule, max_cj_fee = max_cj_fee, callbacks=(self.filter_orders_callback, None, self.taker_finished)) @@ -948,9 +989,9 @@ class JMWalletDaemon(Service): @app.route('/wallet//getseed', methods=['GET']) def getseed(self, request, walletname): self.check_cookie(request) - if not self.wallet_service: + if not self.services["wallet"]: raise NoWalletFound() if not self.wallet_name == walletname: raise InvalidRequestFormat() - seedphrase, _ = self.wallet_service.get_mnemonic_words() + seedphrase, _ = self.services["wallet"].get_mnemonic_words() return make_jmwalletd_response(request, seedphrase=seedphrase) \ No newline at end of file diff --git a/jmclient/test/test_wallet_rpc.py b/jmclient/test/test_wallet_rpc.py index 5291012..4d7181f 100644 --- a/jmclient/test/test_wallet_rpc.py +++ b/jmclient/test/test_wallet_rpc.py @@ -71,11 +71,11 @@ class WalletRPCTestBase(object): # test down from 9 seconds to 1 minute 40s, which is too slow # to be acceptable. TODO: add a test with FB by speeding up # the sync for test, by some means or other. - self.daemon.wallet_service = make_wallets_to_list(make_wallets( + self.daemon.services["wallet"] = make_wallets_to_list(make_wallets( 1, wallet_structures=[wallet_structures[0]], mean_amt=self.mean_amt, wallet_cls=SegwitWalletFidelityBonds))[0] jm_single().bc_interface.tickchain() - sync_wallets([self.daemon.wallet_service]) + sync_wallets([self.daemon.services["wallet"]]) # dummy tx example to force a notification event: self.test_tx = CTransaction.deserialize(hextobin(test_tx_hex_1)) @@ -176,7 +176,7 @@ class TrialTestWRPC_DisplayWallet(WalletRPCTestBase, unittest.TestCase): """ # before starting, we have to shut down the existing # wallet service (usually this would be `lock`): - self.daemon.wallet_service = None + self.daemon.services["wallet"] = None self.daemon.stopService() self.daemon.auth_disabled = False @@ -268,12 +268,12 @@ class TrialTestWRPC_DisplayWallet(WalletRPCTestBase, unittest.TestCase): yield self.do_request(agent, b"POST", addr, body, self.process_direct_send_response) # before querying the wallet display, set a label to check: - labeladdr = self.daemon.wallet_service.get_addr(0,0,0) - self.daemon.wallet_service.set_address_label(labeladdr, + labeladdr = self.daemon.services["wallet"].get_addr(0,0,0) + self.daemon.services["wallet"].set_address_label(labeladdr, "test-wallet-rpc-label") # force the wallet service txmonitor to wake up, to see the new # tx before querying /display: - self.daemon.wallet_service.transaction_monitor() + self.daemon.services["wallet"].transaction_monitor() addr = self.get_route_root() addr += "/wallet/" addr += self.daemon.wallet_name