Player construction now happens when we're ready to create the player entity

this fixes a wide range of bugs with the initial spawn sequence, and allows to simplify a whole lot of player setup logic.

src/pocketmine/Player.php

@@ -62,7 +62,6 @@ use pocketmine\event\player\PlayerJumpEvent;
 use pocketmine\event\player\PlayerKickEvent;
 use pocketmine\event\player\PlayerLoginEvent;
 use pocketmine\event\player\PlayerMoveEvent;
-use pocketmine\event\player\PlayerPreLoginEvent;
 use pocketmine\event\player\PlayerQuitEvent;
 use pocketmine\event\player\PlayerRespawnEvent;
 use pocketmine\event\player\PlayerToggleFlightEvent;
@@ -96,9 +95,7 @@ use pocketmine\nbt\tag\CompoundTag;
 use pocketmine\nbt\tag\DoubleTag;
 use pocketmine\nbt\tag\ListTag;
 use pocketmine\network\mcpe\CompressBatchPromise;
-use pocketmine\network\mcpe\NetworkCipher;
 use pocketmine\network\mcpe\NetworkSession;
-use pocketmine\network\mcpe\ProcessLoginTask;
 use pocketmine\network\mcpe\protocol\AdventureSettingsPacket;
 use pocketmine\network\mcpe\protocol\AnimatePacket;
 use pocketmine\network\mcpe\protocol\AvailableCommandsPacket;
@@ -108,7 +105,6 @@ use pocketmine\network\mcpe\protocol\ClientboundPacket;
 use pocketmine\network\mcpe\protocol\EntityEventPacket;
 use pocketmine\network\mcpe\protocol\LevelEventPacket;
 use pocketmine\network\mcpe\protocol\LevelSoundEventPacket;
-use pocketmine\network\mcpe\protocol\LoginPacket;
 use pocketmine\network\mcpe\protocol\MobEffectPacket;
 use pocketmine\network\mcpe\protocol\ModalFormRequestPacket;
 use pocketmine\network\mcpe\protocol\MovePlayerPacket;
@@ -189,9 +185,6 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	/** @var NetworkSession */
 	protected $networkSession;
 
-	/** @var bool */
-	protected $loggedIn = false;
-
 	/** @var bool */
 	public $spawned = false;
 
@@ -206,9 +199,9 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	/** @var string */
 	protected $xuid = "";
 	/** @var bool */
-	protected $authenticated = false;
+	protected $authenticated;
-	/** @var PlayerInfo|null */
+	/** @var PlayerInfo */
-	protected $playerInfo = null;
+	protected $playerInfo;
 
 	protected $windowCnt = 2;
 	/** @var int[] */
@@ -551,7 +544,7 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 	 * @return bool
 	 */
 	public function isOnline() : bool{
-		return $this->isConnected() and $this->loggedIn;
+		return $this->isConnected();
 	}
 
 	/**
@@ -687,21 +680,6 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 
 	}
 
-	/**
-	 * @param Server         $server
-	 * @param NetworkSession $session
-	 */
-	public function __construct(Server $server, NetworkSession $session){
-		$this->server = $server;
-		$this->networkSession = $session;
-
-		$this->perm = new PermissibleBase($this);
-		$this->chunksPerTick = (int) $this->server->getProperty("chunk-sending.per-tick", 4);
-		$this->spawnThreshold = (int) (($this->server->getProperty("chunk-sending.spawn-radius", 4) ** 2) * M_PI);
-
-		$this->allowMovementCheats = (bool) $this->server->getProperty("player.anti-cheat.allow-movement-cheats", false);
-	}
-
 	/**
 	 * @return bool
 	 */
@@ -1712,8 +1690,19 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 		return ($targetDot - $eyeDot) >= -$maxDiff;
 	}
 
-	public function handleLogin(LoginPacket $packet) : bool{
+	/**
-		$this->playerInfo = $packet->playerInfo;
+	 * @param Server         $server
+	 * @param NetworkSession $session
+	 * @param PlayerInfo     $playerInfo
+	 * @param bool           $authenticated
+	 */
+	public function __construct(Server $server, NetworkSession $session, PlayerInfo $playerInfo, bool $authenticated){
+		$this->server = $server;
+		$this->networkSession = $session;
+		$this->playerInfo = $playerInfo;
+		$this->authenticated = $authenticated;
+		$this->skin = $this->playerInfo->getSkin();
+
 		$this->username = TextFormat::clean($this->playerInfo->getUsername());
 		$this->displayName = $this->username;
 		$this->iusername = strtolower($this->username);
@@ -1722,82 +1711,14 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 
 		$this->uuid = $this->playerInfo->getUuid();
 		$this->rawUUID = $this->uuid->toBinary();
-		$this->xuid = $this->playerInfo->getXuid();
+		$this->xuid = $authenticated ? $this->playerInfo->getXuid() : "";
 
-		$this->setSkin($this->playerInfo->getSkin());
+		$this->perm = new PermissibleBase($this);
+		$this->chunksPerTick = (int) $this->server->getProperty("chunk-sending.per-tick", 4);
+		$this->spawnThreshold = (int) (($this->server->getProperty("chunk-sending.spawn-radius", 4) ** 2) * M_PI);
 
-		$ev = new PlayerPreLoginEvent(
+		$this->allowMovementCheats = (bool) $this->server->getProperty("player.anti-cheat.allow-movement-cheats", false);
-			$this->playerInfo,
-			$this->networkSession->getIp(),
-			$this->networkSession->getPort(),
-			$this->server->requiresAuthentication()
-		);
-		if($this->server->getNetwork()->getConnectionCount() > $this->server->getMaxPlayers()){
-			$ev->setKickReason(PlayerPreLoginEvent::KICK_REASON_SERVER_FULL, "disconnectionScreen.serverFull");
-		}
-		if(!$this->server->isWhitelisted($this->username)){
-			$ev->setKickReason(PlayerPreLoginEvent::KICK_REASON_SERVER_WHITELISTED, "Server is whitelisted");
-		}
-		if($this->isBanned() or $this->server->getIPBans()->isBanned($this->getAddress())){
-			$ev->setKickReason(PlayerPreLoginEvent::KICK_REASON_BANNED, "You are banned");
-		}
 
-		$ev->call();
-		if(!$ev->isAllowed()){
-			$this->close("", $ev->getFinalKickMessage());
-			return true;
-		}
-
-		if(!$packet->skipVerification){
-			$this->server->getAsyncPool()->submitTask(new ProcessLoginTask($this, $packet, $ev->isAuthRequired(), NetworkCipher::$ENABLED));
-		}else{
-			$this->setAuthenticationStatus(false, false, null);
-			$this->networkSession->onLoginSuccess();
-		}
-
-		return true;
-	}
-
-	public function setAuthenticationStatus(bool $authenticated, bool $authRequired, ?string $error) : bool{
-		if($this->networkSession === null){
-			return false;
-		}
-
-		if($authenticated and $this->xuid === ""){
-			$error = "Expected XUID but none found";
-		}
-
-		if($error !== null){
-			$this->close("", $this->server->getLanguage()->translateString("pocketmine.disconnect.invalidSession", [$error]));
-
-			return false;
-		}
-
-		$this->authenticated = $authenticated;
-
-		if(!$this->authenticated){
-			if($authRequired){
-				$this->close("", "disconnectionScreen.notAuthenticated");
-				return false;
-			}
-
-			$this->server->getLogger()->debug($this->getName() . " is NOT logged into Xbox Live");
-			if($this->xuid !== ""){
-				$this->server->getLogger()->warning($this->getName() . " has an XUID, but their login keychain is not signed by Mojang");
-				$this->xuid = "";
-			}
-		}else{
-			$this->server->getLogger()->debug($this->getName() . " is logged into Xbox Live");
-		}
-
-		return $this->server->getNetwork()->getSessionManager()->kickDuplicates($this->networkSession);
-	}
-
-	public function onLoginSuccess() : void{
-		$this->loggedIn = true;
-	}
-
-	public function _actuallyConstruct(){
 		$namedtag = $this->server->getOfflinePlayerData($this->username); //TODO: make this async
 
 		$spawnReset = false;
@@ -1834,6 +1755,7 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 		}
 
 		parent::__construct($level, $namedtag);
+
 		$ev = new PlayerLoginEvent($this, "Plugin reason");
 		$ev->call();
 		if($ev->isCancelled()){
@@ -2832,14 +2754,12 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 			$this->usedChunks = [];
 			$this->loadQueue = [];
 
-			if($this->loggedIn){
 			foreach($this->server->getOnlinePlayers() as $player){
 				if(!$player->canSee($this)){
 					$player->showPlayer($this);
 				}
 			}
 			$this->hiddenPlayers = [];
-			}
 
 			$this->removeAllWindows(true);
 			$this->windows = [];
@@ -2847,17 +2767,11 @@ class Player extends Human implements CommandSender, ChunkLoader, ChunkListener,
 			$this->cursorInventory = null;
 			$this->craftingGrid = null;
 
-			if($this->constructed){
 			parent::close();
-			}else{
+
-				$this->closed = true;
-			}
 			$this->spawned = false;
 
-			if($this->loggedIn){
-				$this->loggedIn = false;
 			$this->server->removeOnlinePlayer($this);
-			}
 
 			$this->server->getLogger()->info($this->getServer()->getLanguage()->translateString("pocketmine.player.logOut", [
 				TextFormat::AQUA . $this->getName() . TextFormat::WHITE,

src/pocketmine/PlayerInfo.php

@@ -24,6 +24,7 @@ declare(strict_types=1);
 namespace pocketmine;
 
 use pocketmine\entity\Skin;
+use pocketmine\utils\TextFormat;
 use pocketmine\utils\UUID;
 
 /**
@@ -45,7 +46,7 @@ class PlayerInfo{
 	private $clientId;
 
 	public function __construct(string $username, UUID $uuid, Skin $skin, string $locale, string $xuid, int $clientId){
-		$this->username = $username;
+		$this->username = TextFormat::clean($username);
 		$this->uuid = $uuid;
 		$this->skin = $skin;
 		$this->locale = $locale;

src/pocketmine/entity/Entity.php

@@ -378,12 +378,7 @@ abstract class Entity extends Location implements Metadatable, EntityIds{
 	/** @var TimingsHandler */
 	protected $timings;
 
-	/** @var bool */
-	protected $constructed = false;
-
-
 	public function __construct(Level $level, CompoundTag $nbt){
-		$this->constructed = true;
 		$this->timings = Timings::getEntityTimings($this);
 
 		$this->temporalVector = new Vector3();

src/pocketmine/network/mcpe/NetworkSession.php

@@ -58,7 +58,7 @@ class NetworkSession{
 	/** @var Server */
 	private $server;
 	/** @var Player|null */
-	private $player;
+	private $player = null;
 	/** @var NetworkSessionManager */
 	private $manager;
 	/** @var NetworkInterface */
@@ -79,6 +79,8 @@ class NetworkSession{
 	private $connected = true;
 	/** @var bool */
 	private $loggedIn = false;
+	/** @var bool */
+	private $authenticated = false;
 	/** @var int */
 	private $connectTime;
 
@@ -102,10 +104,7 @@ class NetworkSession{
 
 		$this->connectTime = time();
 
-		//TODO: this should happen later in the login sequence
+		$this->setHandler(new LoginSessionHandler($this->server, $this));
-		$this->createPlayer();
-
-		$this->setHandler(new LoginSessionHandler($this->player, $this));
 
 		$this->manager->add($this);
 	}
@@ -119,7 +118,7 @@ class NetworkSession{
 		 * @var Player $player
 		 * @see Player::__construct()
 		 */
-		$this->player = new $class($this->server, $this);
+		$this->player = new $class($this->server, $this, $this->info, $this->authenticated);
 	}
 
 	public function getPlayer() : ?Player{
@@ -166,7 +165,7 @@ class NetworkSession{
 	}
 
 	public function getDisplayName() : string{
-		return ($this->player !== null and $this->player->getName() !== "") ? $this->player->getName() : $this->ip . " " . $this->port;
+		return $this->info !== null ? $this->info->getUsername() : $this->ip . " " . $this->port;
 	}
 
 	/**
@@ -385,7 +384,9 @@ class NetworkSession{
 	 */
 	public function disconnect(string $reason, bool $notify = true) : void{
 		if($this->checkDisconnect()){
+			if($this->player !== null){
 				$this->player->close($this->player->getLeaveMessage(), $reason);
+			}
 			$this->doServerDisconnect($reason, $notify);
 		}
 	}
@@ -426,11 +427,41 @@ class NetworkSession{
 	 * @param string $reason
 	 */
 	public function onClientDisconnect(string $reason) : void{
-		if($this->checkDisconnect()){
+		if($this->checkDisconnect() and $this->player !== null){
 			$this->player->close($this->player->getLeaveMessage(), $reason);
 		}
 	}
 
+	public function setAuthenticationStatus(bool $authenticated, bool $authRequired, ?string $error) : bool{
+		if($authenticated and $this->info->getXuid() === ""){
+			$error = "Expected XUID but none found";
+		}
+
+		if($error !== null){
+			$this->disconnect($this->server->getLanguage()->translateString("pocketmine.disconnect.invalidSession", [$error]));
+
+			return false;
+		}
+
+		$this->authenticated = $authenticated;
+
+		if(!$this->authenticated){
+			if($authRequired){
+				$this->disconnect("disconnectionScreen.notAuthenticated");
+				return false;
+			}
+
+			$this->server->getLogger()->debug($this->getDisplayName() . " is NOT logged into Xbox Live");
+			if($this->info->getXuid() !== ""){
+				$this->server->getLogger()->warning($this->getDisplayName() . " has an XUID, but their login keychain is not signed by Mojang");
+			}
+		}else{
+			$this->server->getLogger()->debug($this->getDisplayName() . " is logged into Xbox Live");
+		}
+
+		return $this->manager->kickDuplicates($this);
+	}
+
 	public function enableEncryption(string $encryptionKey, string $handshakeJwt) : void{
 		$pk = new ServerToClientHandshakePacket();
 		$pk->jwt = $handshakeJwt;
@@ -449,12 +480,11 @@ class NetworkSession{
 		$pk->status = PlayStatusPacket::LOGIN_SUCCESS;
 		$this->sendDataPacket($pk);
 
-		$this->player->onLoginSuccess();
+		$this->setHandler(new ResourcePacksSessionHandler($this->server, $this, $this->server->getResourcePackManager()));
-		$this->setHandler(new ResourcePacksSessionHandler($this->player, $this, $this->server->getResourcePackManager()));
 	}
 
 	public function onResourcePacksDone() : void{
-		$this->player->_actuallyConstruct();
+		$this->createPlayer();
 
 		$this->setHandler(new PreSpawnSessionHandler($this->server, $this->player, $this));
 	}

src/pocketmine/network/mcpe/ProcessLoginTask.php

@@ -33,7 +33,6 @@ use Mdanter\Ecc\Serializer\PublicKey\DerPublicKeySerializer;
 use Mdanter\Ecc\Serializer\PublicKey\PemPublicKeySerializer;
 use Mdanter\Ecc\Serializer\Signature\DerSignatureSerializer;
 use pocketmine\network\mcpe\protocol\LoginPacket;
-use pocketmine\Player;
 use pocketmine\scheduler\AsyncTask;
 use function assert;
 use function base64_decode;
@@ -100,8 +99,8 @@ class ProcessLoginTask extends AsyncTask{
 	/** @var string|null */
 	private $handshakeJwt = null;
 
-	public function __construct(Player $player, LoginPacket $packet, bool $authRequired, bool $useEncryption = true){
+	public function __construct(NetworkSession $session, LoginPacket $packet, bool $authRequired, bool $useEncryption = true){
-		$this->storeLocal($player);
+		$this->storeLocal($session);
 		$this->packet = $packet;
 		$this->authRequired = $authRequired;
 		$this->useEncryption = $useEncryption;
@@ -243,15 +242,15 @@ class ProcessLoginTask extends AsyncTask{
 	}
 
 	public function onCompletion() : void{
-		/** @var Player $player */
+		/** @var NetworkSession $session */
-		$player = $this->fetchLocal();
+		$session = $this->fetchLocal();
-		if(!$player->isConnected()){
+		if(!$session->isConnected()){
-			$this->worker->getLogger()->error("Player " . $player->getName() . " was disconnected before their login could be verified");
+			$this->worker->getLogger()->error("Player " . $session->getDisplayName() . " was disconnected before their login could be verified");
-		}elseif($player->setAuthenticationStatus($this->authenticated, $this->authRequired, $this->error)){
+		}elseif($session->setAuthenticationStatus($this->authenticated, $this->authRequired, $this->error)){
 			if(!$this->useEncryption){
-				$player->getNetworkSession()->onLoginSuccess();
+				$session->onLoginSuccess();
 			}else{
-				$player->getNetworkSession()->enableEncryption($this->aesKey, $this->handshakeJwt);
+				$session->enableEncryption($this->aesKey, $this->handshakeJwt);
 			}
 		}
 	}

src/pocketmine/network/mcpe/handler/LoginSessionHandler.php

@@ -23,25 +23,30 @@ declare(strict_types=1);
 
 namespace pocketmine\network\mcpe\handler;
 
+use pocketmine\event\player\PlayerPreLoginEvent;
+use pocketmine\network\mcpe\NetworkCipher;
 use pocketmine\network\mcpe\NetworkSession;
+use pocketmine\network\mcpe\ProcessLoginTask;
 use pocketmine\network\mcpe\protocol\LoginPacket;
 use pocketmine\network\mcpe\protocol\PlayStatusPacket;
 use pocketmine\network\mcpe\protocol\ProtocolInfo;
 use pocketmine\Player;
+use pocketmine\Server;
 
 /**
  * Handles the initial login phase of the session. This handler is used as the initial state.
  */
 class LoginSessionHandler extends SessionHandler{
 
-	/** @var Player */
+	/** @var Server */
-	private $player;
+	private $server;
 	/** @var NetworkSession */
 	private $session;
 
-	public function __construct(Player $player, NetworkSession $session){
+
-		$this->player = $player;
+	public function __construct(Server $server, NetworkSession $session){
 		$this->session = $session;
+		$this->server = $server;
 	}
 
 	public function handleLogin(LoginPacket $packet) : bool{
@@ -55,7 +60,7 @@ class LoginSessionHandler extends SessionHandler{
 
 			//This pocketmine disconnect message will only be seen by the console (PlayStatusPacket causes the messages to be shown for the client)
 			$this->session->disconnect(
-				$this->player->getServer()->getLanguage()->translateString("pocketmine.disconnect.incompatibleProtocol", [$packet->protocol]),
+				$this->server->getLanguage()->translateString("pocketmine.disconnect.incompatibleProtocol", [$packet->protocol]),
 				false
 			);
 
@@ -74,13 +79,45 @@ class LoginSessionHandler extends SessionHandler{
 			return true;
 		}
 
-		if($this->player->handleLogin($packet)){
+		$ev = new PlayerPreLoginEvent(
-			if($this->session->isConnected() and $this->session->getHandler() === $this){ //when login verification is disabled, the handler will already have been replaced
+			$packet->playerInfo,
-				$this->session->setHandler(new NullSessionHandler()); //drop packets received during login verification
+			$this->session->getIp(),
+			$this->session->getPort(),
+			$this->server->requiresAuthentication()
+		);
+		if($this->server->getNetwork()->getConnectionCount() > $this->server->getMaxPlayers()){
+			$ev->setKickReason(PlayerPreLoginEvent::KICK_REASON_SERVER_FULL, "disconnectionScreen.serverFull");
 		}
+		if(!$this->server->isWhitelisted($packet->playerInfo->getUsername())){
+			$ev->setKickReason(PlayerPreLoginEvent::KICK_REASON_SERVER_WHITELISTED, "Server is whitelisted");
+		}
+		if($this->server->getNameBans()->isBanned($packet->playerInfo->getUsername()) or $this->server->getIPBans()->isBanned($this->session->getIp())){
+			$ev->setKickReason(PlayerPreLoginEvent::KICK_REASON_BANNED, "You are banned");
+		}
+
+		$ev->call();
+		if(!$ev->isAllowed()){
+			$this->session->disconnect($ev->getFinalKickMessage());
 			return true;
 		}
-		return false;
+
+		$this->processLogin($packet, $ev->isAuthRequired());
+
+		return true;
+	}
+
+	/**
+	 * TODO: This is separated for the purposes of allowing plugins (like Specter) to hack it and bypass authentication.
+	 * In the future this won't be necessary.
+	 *
+	 * @param LoginPacket $packet
+	 * @param bool        $authRequired
+	 *
+	 * @throws \InvalidArgumentException
+	 */
+	protected function processLogin(LoginPacket $packet, bool $authRequired) : void{
+		$this->server->getAsyncPool()->submitTask(new ProcessLoginTask($this->session, $packet, $authRequired, NetworkCipher::$ENABLED));
+		$this->session->setHandler(new NullSessionHandler()); //drop packets received during login verification
 	}
 
 	protected function isCompatibleProtocol(int $protocolVersion) : bool{

src/pocketmine/network/mcpe/handler/ResourcePacksSessionHandler.php

@@ -30,9 +30,9 @@ use pocketmine\network\mcpe\protocol\ResourcePackClientResponsePacket;
 use pocketmine\network\mcpe\protocol\ResourcePackDataInfoPacket;
 use pocketmine\network\mcpe\protocol\ResourcePacksInfoPacket;
 use pocketmine\network\mcpe\protocol\ResourcePackStackPacket;
-use pocketmine\Player;
 use pocketmine\resourcepacks\ResourcePack;
 use pocketmine\resourcepacks\ResourcePackManager;
+use pocketmine\Server;
 use function ceil;
 use function implode;
 use function strpos;
@@ -45,8 +45,8 @@ use function substr;
 class ResourcePacksSessionHandler extends SessionHandler{
 	private const PACK_CHUNK_SIZE = 1048576; //1MB
 
-	/** @var Player */
+	/** @var Server */
-	private $player;
+	private $server;
 	/** @var NetworkSession */
 	private $session;
 	/** @var ResourcePackManager */
@@ -55,8 +55,9 @@ class ResourcePacksSessionHandler extends SessionHandler{
 	/** @var bool[][] uuid => [chunk index => hasSent] */
 	private $downloadedChunks = [];
 
-	public function __construct(Player $player, NetworkSession $session, ResourcePackManager $resourcePackManager){
+
-		$this->player = $player;
+	public function __construct(Server $server, NetworkSession $session, ResourcePackManager $resourcePackManager){
+		$this->server = $server;
 		$this->session = $session;
 		$this->resourcePackManager = $resourcePackManager;
 	}
@@ -69,7 +70,7 @@ class ResourcePacksSessionHandler extends SessionHandler{
 	}
 
 	private function disconnectWithError(string $error) : void{
-		$this->player->getServer()->getLogger()->error("Error while downloading resource packs for " . $this->player->getName() . ": " . $error);
+		$this->server->getLogger()->error("Error while downloading resource packs for " . $this->session->getDisplayName() . ": " . $error);
 		$this->session->disconnect("disconnectionScreen.resourcePack");
 	}