<?php
if (auth($username) == 'admin') {
$admin = TRUE;
} else {
$admin = FALSE;
}
?>This seems safe enough, because it’s easy to comprehend at a glance. Imagine a slightly more elaborate example that sets variables for name and email as well, for convenience:<?php
if (auth($username) == 'admin') {
$name = 'Administrator';
$email = 'admin@example.org';
$admin = TRUE;
} else {
/* Get the name and email from the database. */
$query = $db->prepare('SELECT name, email
FROM users
WHERE username = :username');
$query->execute(array('username' => $clean['username']));
$result = $query->fetch(PDO::FETCH_ASSOC);
$name = $result['name'];
$email = $result['email'];
$admin = FALSE;
}
?>Because $admin is still always explicitly set to either TRUE or FALSE, all is well, but if a developer later adds an elseif, there’s an opportunity to forget:<?php
if (auth($username) == 'admin') {
$name = 'Administrator';
$email = 'admin@example.org';
$admin = TRUE;
} elseif (auth($username) == 'mod') {
$name = 'Moderator';
$email = 'mod@example.org';
$moderator = TRUE;
} else {
/* Get the name and email. */
$query = $db->prepare('SELECT name, email
FROM users
WHERE username = :username');
$query->execute(array('username' => $clean['username']));
$result = $query->fetch(PDO::FETCH_ASSOC);
$name = $result['name'];
$email = $result['email'];
$admin = FALSE;
$moderator = FALSE;
}
?>If a user provides a username that triggers the elseif condition, $admin is not initialized. This can lead to unwanted behavior, or worse, a security vulnerability. Additionally, a similar situation now exists for $moderator, which is not initialized in the first condition.By first initializing $admin and $moderator, it’s easy to avoid this scenario altogether:
<?php
$admin = FALSE;
$moderator = FALSE;
if (auth($username) == 'admin') {
$name = 'Administrator';
$email = 'admin@example.org';
$admin = TRUE;
} elseif (auth($username) == 'mod') {
$name = 'Moderator';
$email = 'mod@example.org';
$moderator = TRUE;
} else {
/* Get the name and email. */
$query = $db->prepare('SELECT name, email
FROM users
WHERE username = :username');
$query->execute(array('username' => $clean['username']));
$result = $query->fetch(PDO::FETCH_ASSOC);
$name = $result['name'];
$email = $result['email'];
}
?>Regardless of what the rest of the code does, it’s now clear that $admin is FALSE unless it is explicitly set to something else, and the same is true for $moderator. This also hints at another good security practice, which is to fail safely. The worst that can happen as a result of not modifying $admin or $moderator in any of the conditions is that someone who is an administrator or moderator is not treated as one.If you want to shortcut something, and you’re feeling a little disappointed that our example includes an else, we have a bonus tip that might interest you. We’re not certain it can be considered a shortcut, but we hope it’s helpful nonetheless.
Consider a function that determines whether a user is authorized to view a particular page:
<?php
function authorized($username, $page) {
if (!isBlacklisted($username)) {
if (isAdmin($username)) {
return TRUE;
} elseif (isAllowed($username, $page)) {
return TRUE;
} else {
return FALSE;
}
} else {
return FALSE;
}
}
?>This example is actually pretty simple, because there are only three rules to consider: administrators are always allowed access; those who are blacklisted are never allowed access; and isAllowed() determines whether anyone else has access. (A special case exists when an administrator is blacklisted, but that is an unlikely possibility, so we’re ignoring it here.) We use functions for the rules to keep the code simple and to focus on the logical structure.There are numerous ways this example can be improved. If you want to reduce the number of lines, a compound conditional can help:
<?php
function authorized($username, $page) {
if (!isBlacklisted($username)) {
if (isAdmin($username) || isAllowed($username, $page)) {
return TRUE;
} else {
return FALSE;
}
} else {
return FALSE;
}
}
?>In fact, you can reduce the entire function to a single compound conditional:<?php
function authorized($username, $page) {
if (!isBlacklisted($username) && (isAdmin($username) || isAllowed($username, $page)) {
return TRUE;
} else {
return FALSE;
}
}
?>Finally, this can be reduced to a single return:<?php
function authorized($username, $page) {
return (!isBlacklisted($username) && (isAdmin($username) || isAllowed($username, $page));
}
?>If your goal is to reduce the number of lines, you’re done. However, note that we’re using isBlacklisted(), isAdmin(), and isAllowed() as placeholders. Depending on what’s involved in making these determinations, reducing everything to a compound conditional may not be as attractive.This brings us to our tip. A return immediately exits the function, so if you return as soon as possible, you can express these rules very simply:
<?php
function authorized($username, $page) {
if (isBlacklisted($username)) {
return FALSE;
}
if (isAdmin($username)) {
return TRUE;
}
return isAllowed($username, $page);
}
?>This uses more lines of code, but it’s very simple and unimpressive (we’re proudest of our code when it’s the least impressive). More importantly, this approach reduces the amount of context you must keep up with. For example, as soon as you’ve determined whether the user is blacklisted, you can safely forget about it. This is particularly helpful when your logic is more complicated.
0 comments:
Post a Comment