Hulp bij controleren script

Het ziet er wel veilig uit, ik snap alleen niet waarom je de foutmeldingen in een sessie opslaat?
Die meldingen zijn toch van belang voor deze pagina, en niet voor andere pagina's? Ikzelf zou liever voor een array gaan.

Ik plaats ze in een session zodat ze na reload weg zijn, hier had ik een topic voor aangemaakt. Maar ziet u nog exploits/lekken of andere buggs?

[size=xsmall]Toevoeging op 05/01/2017 01:05:29:[/size]

Of kan ik nog iets optimaliseren?

[size=xsmall]Toevoeging op 05/01/2017 01:10:46:[/size]

O sorry, ik had dit niet gelezen:

Ikzelf zou liever voor een array gaan.

Ik vind zelf een session handiger maar dat is mijn mening.

Als je ze in een array zet, zijn ze ook na een reload weg, mits je aan de juiste validatie voldoet.
In een sessie vind ik iets wat overkill.

Opsich zie ik niet veel algemene verbeterpunten, maar het is net wat je wilt verbeteren, welke kennis je hebt en of je er voor open staat. Altijd valt er wat aan te verbeteren tot het naar je zin is, en ik geloof dat de meeste ontwikkelaars het wel roerend met mij eens zijn.

Ik kan wel bijvoorbeeld opnoemen om bijv. een extended MySQLi-class te gebruiken voor overloading van de query-functie met ingebouwde foutafhandeling, en een structuur in OOP. Maar zoals ik al zei: Het ligt eraan waar je kennis ligt, en tot hoever je wilt gaan.... ;-)

- Rob - op 05/01/2017 01:03:53

Ik vind zelf een session handiger maar dat is mijn mening.

Euh... wat is er handiger aan een session dan een array? Het is toch data (foutmeldingen) die je niet nog een keer gebruikt na een refresh. Je POST-wordt opnieuw geladen, en dan wordt er gekeken of de validatie nu wel eens klopt. EN ind at geval ben je ingelogd.

Waar komt $ipadres vandaan en deze wordt nergens (of te vroeg?) ge-escaped.

Wat ben je in hemelsnaam met je gebruikersnaam aan het doen? Meerdere keren ge-escaped (zowel voor gebruik in database alsook in HTML, waarom?).

Waarom sla je een gebruikersnaam op in je sessie? Het enige wat je hier in op zou hoeven slaan is een user id ofzo. Voor de goede orde zou je toch elke page-access moeten controleren wat iemand zijn rechten zijn anders blijven er misschien instellingen hangen totdat iemand opnieuw inlogt.

En wellicht loont het de moeite om met objecten te gaan werken: een config-object, een database-object, een user-object, een sessie-object et cetera.

Daarnaast is het bovenstaande script te lang :p. Als je een user class maakt zou je "het proces van een gebruiker inloggen" hierin onder kunnen brengen. Dit hoef je dan niet uit te schrijven in bovenstaande code. Het voordeel hiervan is dat je taken delegeert naar specifieke objecten die de leesbaarheid van wat je aan het doet vergroot.

Ik dacht dat ik een structuur had in OOP? :P Ik denk dat ik mij hierin: extended MySQLi-class is ga inverdiepen, is wel handig voor groot schalig gebruik. En zelf wil ik mijn systemen zo gebruiksvriendelijk maken.

[size=xsmall]Toevoeging op 05/01/2017 01:26:34:[/size]

$ipadress komt van de functions.php en die wordt daar ge-escaped.

Gebruikersnaam heb ik zo gedaan zodat de input van de gebruikersnaam niet verloren gaat, en als je terug komt op de pagina dat je gebruikersnaam er nog staat. En hoe moet ik htmlspecialchars in html gebruiken, want dat heb ik niet?

page-acces is er toch? Als er geen POST request is -> terug naar index, als gebruiker al is ingelogd -> terug naar index. Wat moet er dan nog meer bij?

Daarnaast is het bovenstaande script te lang :p. Als je een user class maakt zou je "het proces van een gebruiker inloggen" hierin onder kunnen brengen. Dit hoef je dan niet uit te schrijven in bovenstaande code. Het voordeel hiervan is dat je taken delegeert naar specifieke objecten die de leesbaarheid van wat je aan het doet vergroot.

Ik zal hier naar kijken

Dit is geen structuur in OOP/OO maar een procedurele opbouw van je code, waarin veel elementen zich herhalen, en waarvan de werking is ingeweefd in je code.

Als je meer wilt weten over OO, dan staat er op deze pagina een hoop leesvoer voor de beginner: http://www.phptuts.nl/view/45/
Maar OO-design is zeer uitgebreid en beperkt zich niet tot alleen classes en functies.

Dus ongeveer zoiets?

<?php
require_once($_SERVER["DOCUMENT_ROOT"] . "/paneel/include/init.php");

class User extends Object
{
	public function __construct()
	{
		parent::__construct();
	}
	
	public function loggedIn()
	{
		if (isset($_SESSION["habbonaam"]) && $_SESSION["habbonaam"] != "")
		{
			return true;
		}
		else
		{
			return false;
		}
	}
	
	public function idToUsername($user_id)
	{
		$query = sprintf(
			"	SELECT	`leden`.`habbonaam`
				FROM	`leden`
				WHERE	`leden`.`id` = %d
				LIMIT	1;",
			$user_id
		);
		$result = $this->getConnection()->query($query);
		
		list($habbonaam) = $this->getConnection()->fetch_row($result);
		
		return $habbonaam;
	}
	
	public function usernameToId($username)
	{
		$query = sprintf(
			"	SELECT	`leden`.`id`
				FROM	`leden`
				WHERE	`leden`.`habbonaam` = '%s'
				LIMIT	1;",
			$username
		);
		$result = $this->getConnection()->query($query);
		
		list($id) = $this->getConnection()->fetch_row($result);
		
		return $id;
	}
	
	public function createUser($username, $hashedPassword)
	{
		$query = sprintf(
			"	INSERT INTO `leden` (`habbonaam`, `wachtwoord`, `ip`, `regdatum`, `lastonline`)
				VALUES		('%s', '%s', '%s', NOW(), NOW())",
			$this->getConnection()->escape($username),
			$this->getConnection()->escape($hashedPassword),
			$_SERVER['REMOTE_ADDR']
		);
		echo $query;
		$this->getConnection()->query($query);
	}
	
	public function getUserVar($username, $var)
	{
		$query = sprintf(
			"	SELECT	`leden`.`%s`
				FROM	`leden`
				WHERE	`leden`.`habbonaam` = '%s'
				LIMIT	1;",
			$this->getConnection()->escape($var),
			$this->getConnection()->escape($username)
		);
		$result = $this->getConnection()->query($query);
		
		list($response) = $this->getConnection()->fetch_row($result);
		
		return $response;
	}
	
	public function setUserVar($var, $value, $username)
	{
		$query = sprintf(
			"	UPDATE	`leden`
				SET		`leden`.`%s` = '%s'
				WHERE	`leden`.`habbonaam` = '%s'",
			$this->getConnection()->escape($var),
			$this->getConnection()->escape($value),
			$this->getConnection()->escape($username)
		);
		$result = $this->getConnection()->query($query);
	}
	
	public function getRankLevelVar($rankName, $var)
	{
		$query = sprintf(
			"	SELECT	`paneel_rangniveau`.`%s`
				FROM	`paneel_rangniveau`
				WHERE	`paneel_rangniveau`.`rang_naam` = '%s'
				LIMIT	1;",
			$this->getConnection()->escape($var),
			$this->getConnection()->escape($rankName)
		);
		$result = $this->getConnection()->query($query);
		
		list($response) = $this->getConnection()->fetch_row($result);
		
		return $response;
	}
	
	public function sanitizeName($name)
	{
		return htmlspecialchars($name);
	}
	
	public function checkPermission($username, $permissie)
	{
		$query = sprintf(
			"	SELECT	`paneel_personeel`.`%s`
				FROM	`paneel_personeel`
				WHERE	`paneel_personeel`.`habbonaam` = '%s'
				LIMIT	1;",
			$this->getConnection()->escape($permissie),
			$this->getConnection()->escape($username)
		);
		$result = $this->getConnection()->query($query);
		
		list($response) = $this->getConnection()->fetch_row($result);
		
		if ($response == 1)
		{
			return true;
		}
		else
		{
			return false;
		}
		
		return $response;
	}
	
	public function getUserLevel($username)
	{
		$query = sprintf(
			"	SELECT		`paneel_admins`.`access_level`
				FROM		`paneel_admins`
				WHERE		`paneel_admins`.`habbonaam` = '%s'
				ORDER BY 	`paneel_admins`.`id` DESC
				LIMIT	1",
			$username
		);
		
		$result = $this->getConnection()->query($query);
		$count	= $this->getConnection()->num_rows($result);

		if ($count > 0)
		{
			list($level) = $this->getConnection()->fetch_row($result);
			
			return $level;
		}
		else
		{
			return 0;
		}
	}
	
	public function isAdmin($username)
	{
		if ($this->getUserLevel($username) > 0)
		{
			return true;
		}
		else
		{
			return false;
		}
	}
	
	public function isForumAdmin($username)
	{
		$userLevel = $this->getUserLevel($username);
		
		return $userLevel > 1;
	}
	
	public function getFunction($username, $tag = null)
	{
		$query = sprintf(
			"	SELECT		`paneel_rangverandering`.`rang_nieuw`
				FROM		`paneel_rangverandering`
				WHERE		`paneel_rangverandering`.`habbonaam` = '%s'
				ORDER BY 	`paneel_rangverandering`.`id` DESC
				LIMIT	1",
			$username
		);
		$result = $this->getConnection()->query($query);
		$count	= $this->getConnection()->num_rows($result);

		if ($count > 0)
		{
			list($rank_new) = $this->getConnection()->fetch_row($result);
			
			$query = sprintf(
				"	SELECT	`paneel_rangniveau`.`rang_naam`
					FROM	`paneel_rangniveau`
					WHERE	`paneel_rangniveau`.`rang_level` = %d
					LIMIT	1",
				$rank_new
			);

			$result = $this->getConnection()->query($query);
			list($rank) = $this->getConnection()->fetch_row($result);
		}
		else
		{
			$rank = "Geen rank bekend.";
		}
		
		return $rank;
	}
	
	public function inPromotionLimit($targetUser, $currentUser)
	{
		$cuRankName = $this->getFunction($currentUser);
		$tuRankName = $this->getFunction($targetUser);
		
		$cuRankLevel = $this->getRankLevelVar($cuRankName, "rang_level");
		$tuRankLevel = $this->getRankLevelVar($tuRankName, "rang_level");
		
		$cuMaxPromotionLevel = $this->getRankLevelVar($cuRankName, "promoveren_tot");
		
		if ($cuMaxPromotionLevel > $tuRankLevel)
		{
			return true;
		}
		else
		{
			return false;
		}
	}
	
	public function inDemotionLimit($targetUser, $currentUser)
	{
		$cuRankName = $this->getFunction($currentUser);
		$tuRankName = $this->getFunction($targetUser);
		
		$cuRankLevel = $this->getRankLevelVar($cuRankName, "rang_level");
		$tuRankLevel = $this->getRankLevelVar($tuRankName, "rang_level");
		
		$cuMaxDemotionLevel = $this->getRankLevelVar($cuRankName, "degraderen_tot");
		
		if ($cuMaxDemotionLevel >= $tuRankLevel)
		{
			return true;
		}
		else
		{
			return false;
		}
	}
	
	public function logAction($user, $tag, $action, $target_user = null, $extra_data = null)
	{
		$username = $this->idToUsername($user);
		$target_username = $this->idToUsername($target_user);
		
		switch ($action)
		{
			case "convo-adduser":
				$message = "heeft de gebruiker " . $target_username . " toegevoegd aan de conversatie (ID): ". $extra_data .".";
				break;
			case "convo-new":
				$message = "heeft een nieuwe conversatie gemaakt met ID: ". $extra_data .".";
				break;
			case "convo-removeuser":
				$message = "heeft de gebruiker " . $target_username . " verwijderd uit de conversatie (ID): ". $extra_data .".";
				break;
			case "convo-reply":
				$message = "heeft een nieuwe reactie geplaatst in convo (ID): ". $extra_data .".";
				break;
			case "convo-leave":
				$message = "heeft het gesprek (ID): ". $extra_data ." verlaten.";
				break;
			case "ban-username":
				$message = "heeft de gebruiker ". $extra_data ." verbannen.";
				break;
			case "ban-ip":
				$message = "heeft het IP ". $extra_data ." verbannen.";
				break;
			case "granted-pw-change":
				$message = "heeft voor gebruiker ". $extra_data ." toegestaan zijn wachtwoord te veranderen.";
				break;
			case "created-hipchat-account":
				$message = "heeft voor gebruiker ". $extra_data ." een HipChat account gemaakt.";
				break;
			case "hipchat-pw-change":
				$message = "heeft voor gebruiker ". $extra_data ." zijn HipChat wachtwoord veranderd.";
				break;
			case "wrong-login":
				$message = "heeft proberen in te loggen met een fout wachtwoord.";
				break;
			case "succesfull-login":
				$message = "heeft zichzelf ingelogd.";
				break;
			case "registered":
				$message = "heeft zichzelf geregistreerd.";
				break;
			case "user-pass-reset-succes":
				$message = "heeft zijn/haar wachtwoord aangepast.";
				break;
			case "user-pass-reset-wrong":
				$message = "heeft een foutieve poging gedaan het wachtwoord aan te passen van " . $target_user;
				break;
		}
		
		$query = sprintf(
			"	INSERT INTO `paneel_logs` (`habbonaam`, `actie`, `ip`, `UA`, `datum`)
				VALUES		('%s', '%s', '%s', '%s', NOW())",
			$this->getConnection()->escape($username),
			$this->getConnection()->escape($message),
			$_SERVER['REMOTE_ADDR'],
			$_SERVER['HTTP_USER_AGENT']
		);
		$this->getConnection()->query($query);
	}
	
	public function getDataFromHabboAPI($username, $var)
	{
		$ch = curl_init();

		curl_setopt($ch, CURLOPT_URL, "https://www.habbo.nl/api/public/users?name=" . $username . "");
		curl_setopt($ch, CURLOPT_HEADER, false);
		curl_setopt($ch, CURLOPT_RETURNTRANSFER, true);
		curl_setopt($ch, CURLOPT_FOLLOWLOCATION, true);
		curl_setopt($ch, CURLOPT_SSL_VERIFYHOST, 0 );
		curl_setopt($ch, CURLOPT_SSL_VERIFYPEER, 0);
		curl_setopt($ch, CURLOPT_USERAGENT, "MozillaXYZ/1.0");
		curl_setopt($ch, CURLOPT_TIMEOUT, 10);
		$output = json_decode(curl_exec($ch));
		curl_close($ch);
		
		return $output->{$var};
	}
	
	public function getDataFromHipChatAPI($username)
	{
		require_once($_SERVER["DOCUMENT_ROOT"] . "/_paneel/classes/hipchat/user.hc.class.php");
		
		$HipChatUser = new HipChatAPIUser();
		$HipChatUserID = $this->getUserVar($username, "hipchat_user_id");
		
		return $HipChatUser->getData($HipChatUserID);
	}
	
	public function checkMotto($username, $code)
	{	
		if ($this->getDataFromHabboAPI($username, "motto") == $code) {
			return true;
		} else {
			return false;
		}
	}
}
?>

Gezien de manier van praktische opbouw wel, alleen qua theoretische opzet niet. Een LogAction() en een isForumAdmin() hebben niks met een User te maken, maar met een logging-class en een permissions class.

Dit staat heel mooi uitgelegd in het aangegeven artikel.