Updating user credentials

[blackhat]

I made a form where users can update username and password, but what if they don’t wanna update the password and leave it blank. I made this code, but I just wanna know about better ways to do it.

if (!empty($_POST['password'])) {
	$password = hash("sha256", $_POST['password']);
}
else {
	$dbQuery = "SELECT password FROM users WHERE users.id = ".$_GET["update"];
	$data = getContent($dbQuery);
		foreach($data as $row) {
			$password = $row['password'];	
		}
}

[SamA74]

@blackhat

$dbQuery = "SELECT password FROM users WHERE users.id = ".$_GET["update"];

<off-topic>
A $_GET directly in a query!
</off-topic>

You could just have a different UPDATE query that only updates the username, not the password. That would save making the extra call to the database to get the password out, just to put it back in the same.

[blackhat]

I should not use the GET and POST directly in the query ? Is making a new variable better ?

So make two update queries instead so only the one that qualifies will be run ?

[SamA74]

Validate/Sanitize it first.
Use prepared statements in the query.
Otherwise it is wide open to hacking through SQL injection.

[blackhat]

Would this be better

$sql=$oDB->Prepare("UPDATE users SET username=:username, fullaccess=:useraccess WHERE id=:uid");
$sql->execute(array(':username' => $username, ':useraccess' => $useraccess, ':uid' => $uid ));
		
		
	if (!empty($_POST['password'])) {
		$password = hash("sha256", $_POST['password']);
		$sql=$oDB->Prepare("UPDATE users SET password=:password WHERE id=:uid");
		$sql->execute(array(':password' => $password, ':uid' => $uid ));
	}

[SamA74]

That is much safer with the prepared statements.

Assuming the ID is an integer it can be cleaned with preg replace.

$uid = preg_replace('#[^0-9]#i', '', $_GET["update"]) ;

I was thinking the update could be done with a single query.

	if (!empty($_POST['password'])) {
		$password = hash("sha256", $_POST['password']);
		$sql=$oDB->Prepare("UPDATE users SET username=:username, fullaccess=:useraccess, password=:password WHERE id=:uid");
		$upData = array('username' => $username, 'useraccess' => $useraccess, 'password' => $password, 'uid' => $uid );
	}
        else {
		$sql=$oDB->Prepare("UPDATE users SET username=:username, fullaccess=:useraccess WHERE id=:uid");
		$upData = array('username' => $username, 'useraccess' => $useraccess, 'uid' => $uid );
	}
$sql->execute($upData);

It could probably be cut down further will a little more thought.

[SamA74]

Maybe this:

$fields = "username=:username, fullaccess=:useraccess" ;
$upData = array('username' => $username, 'useraccess' => $useraccess, 'uid' => $uid );

if (!empty($_POST['password'])) {
		$fields .= ", password=:password" ;
		$upData['password'] = hash("sha256", $_POST['password']);
}

$sql=$oDB->Prepare("UPDATE users SET $fields WHERE id=:uid");
$sql->execute($upData);

[blackhat]

That looks great. Thanks will try that

[ahundiak]

Just one minor suggestion, consider refactoring your code and move the update password functionality to it’s own little form and controller. Update password is different than updating most of the rest of the user profile information. Dealing with it in isolation might make your code a bit easier to maintain.