Changed how login verification is handled, add more useful error messages

closes #1955

src/pocketmine/Player.php

@@ -1848,7 +1848,7 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 		if(!$packet->skipVerification){
 			$this->server->getScheduler()->scheduleAsyncTask(new VerifyLoginTask($this, $packet));
 		}else{
-			$this->onVerifyCompleted($packet, true, true);
+			$this->onVerifyCompleted($packet, null, true);
 		}
 
 		return true;
@@ -1861,13 +1861,13 @@ class Player extends Human implements CommandSender, ChunkLoader, IPlayer{
 		$this->sendDataPacket($pk, false, $immediate);
 	}
 
-	public function onVerifyCompleted(LoginPacket $packet, bool $isValid, bool $isAuthenticated) : void{
+	public function onVerifyCompleted(LoginPacket $packet, ?string $error, bool $isAuthenticated) : void{
 		if($this->closed){
 			return;
 		}
 
-		if(!$isValid){
+		if($error !== null){
-			$this->close("", "disconnect.loginFailedInfo.invalidSession");
+			$this->close("", $this->server->getLanguage()->translateString("pocketmine.disconnect.invalidSession", [$error]));
 			return;
 		}
 

src/pocketmine/lang/locale

@@ -1 +1 @@
-Subproject commit 4b3e289b6b4a9e95e6b6e74deb54e9c933b399de
+Subproject commit 4fcd81fa95fe88d7a581141353185327d57239de

src/pocketmine/network/mcpe/VerifyLoginTask.php

@@ -36,12 +36,12 @@ class VerifyLoginTask extends AsyncTask{
 	private $packet;
 
 	/**
-	 * @var bool
+	 * @var string|null
-	 * Whether the keychain signatures were validated correctly. This will be set to false if any link in the keychain
+	 * Whether the keychain signatures were validated correctly. This will be set to an error message if any link in the
-	 * has an invalid signature. If false, the keychain might have been tampered with.
+	 * keychain is invalid for whatever reason (bad signature, not in nbf-exp window, etc). If this is non-null, the
-	 * The player will always be disconnected if this is false.
+	 * keychain might have been tampered with. The player will always be disconnected if this is non-null.
 	 */
-	private $valid = false;
+	private $error = "Unknown";
 	/**
 	 * @var bool
 	 * Whether the player is logged into Xbox Live. This is true if any link in the keychain is signed with the Mojang
@@ -58,31 +58,38 @@ class VerifyLoginTask extends AsyncTask{
 	public function onRun(){
 		$packet = $this->packet; //Get it in a local variable to make sure it stays unserialized
 
+		try{
 			$currentKey = null;
 			$first = true;
 
 			foreach($packet->chainData["chain"] as $jwt){
-			if(!$this->validateToken($jwt, $currentKey, $first)){
+				$this->validateToken($jwt, $currentKey, $first);
-				return;
-			}
 				$first = false;
 			}
 
-		if(!$this->validateToken($packet->clientDataJwt, $currentKey)){
+			$this->validateToken($packet->clientDataJwt, $currentKey);
-			return;
+
+			$this->error = null;
+		}catch(\Throwable $e){
+			$this->error = $e->getMessage();
+		}
 	}
 
-		$this->valid = true;
+	/**
-	}
+	 * @param string      $jwt
-
+	 * @param null|string $currentPublicKey
-	private function validateToken(string $jwt, ?string &$currentPublicKey, bool $first = false) : bool{
+	 * @param bool        $first
+	 *
+	 * @throws \RuntimeException if errors are encountered
+	 */
+	private function validateToken(string $jwt, ?string &$currentPublicKey, bool $first = false) : void{
 		[$headB64, $payloadB64, $sigB64] = explode('.', $jwt);
 
 		$headers = json_decode(base64_decode(strtr($headB64, '-_', '+/'), true), true);
 
 		if($currentPublicKey === null){
 			if(!$first){
-				return false; //we should have a key but the last link didn't have one
+				throw new \RuntimeException("%pocketmine.disconnect.invalidSession.missingKey");
 			}
 
 			//First link, check that it is self-signed
@@ -114,7 +121,7 @@ class VerifyLoginTask extends AsyncTask{
 
 		$v = openssl_verify("$headB64.$payloadB64", $derSignature, "-----BEGIN PUBLIC KEY-----\n" . wordwrap($currentPublicKey, 64, "\n", true) . "\n-----END PUBLIC KEY-----\n", OPENSSL_ALGO_SHA384);
 		if($v !== 1){
-			return false; //bad signature, it might have been tampered with
+			throw new \RuntimeException("%pocketmine.disconnect.invalidSession.badSignature");
 		}
 
 		if($currentPublicKey === self::MOJANG_ROOT_PUBLIC_KEY){
@@ -125,16 +132,14 @@ class VerifyLoginTask extends AsyncTask{
 
 		$time = time();
 		if(isset($claims["nbf"]) and $claims["nbf"] > $time){
-			return false; //token can't be used yet
+			throw new \RuntimeException("%pocketmine.disconnect.invalidSession.tooEarly");
 		}
 
 		if(isset($claims["exp"]) and $claims["exp"] < $time){
-			return false; //token has expired
+			throw new \RuntimeException("%pocketmine.disconnect.invalidSession.tooLate");
 		}
 
 		$currentPublicKey = $claims["identityPublicKey"] ?? null; //if there are further links, the next link should be signed with this
-
-		return true;
 	}
 
 	public function onCompletion(Server $server){
@@ -143,7 +148,7 @@ class VerifyLoginTask extends AsyncTask{
 		if($player->isClosed()){
 			$server->getLogger()->error("Player " . $player->getName() . " was disconnected before their login could be verified");
 		}else{
-			$player->onVerifyCompleted($this->packet, $this->valid, $this->authenticated);
+			$player->onVerifyCompleted($this->packet, $this->error, $this->authenticated);
 		}
 	}