Checking a user's emails using an ID

Question

I'm trying to do this:

1. Getting user's ID based on his email name.
2. Using that user_id to check all his emails that have been sent for him so far.

// db connection here
$sth = $dbh_conn
->prepare("SELECT id FROM users WHERE email = ?")
->execute(array($email));
$num_rows = $sth->fetch(PDO::FETCH_ASSOC);

if($num_rows) {

    $user_id = $num_rows['id'];

    $stmt = $dbh_conn
    ->prepare("SELECT 1
               FROM ( SELECT count(*) AS num_week,
                             sum(date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1  day))) as num_day,
                             sum(date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1  hour))) as num_hour,
                             sum(date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1 minute))) as num_1min
                       FROM resend_pass
                      WHERE user_id   = ?  
                        AND date_time > unix_timestamp(DATE_SUB(now(), INTERVAL 1 WEEK))
                    ) a 
                WHERE num_week < 12 AND num_day < 6 AND num_hour < 4 AND num_1min < 1;")
    ->execute(array($num_rows['id']));
    $result = $stmt->fetchColumn();

    if ( $result ) {
        echo 'send email';
    } else {
        echo 'you have sent lot of requests to reset your password. try it later';
    }

} else {
    echo 'invalid email address';
}

My code works as well. I guess it can be better.

Here is my table's structure:

// users
+----+--------+----------------------+------------+
| id |  name  |         email        | date_time  |
+----+--------+----------------------+------------+
| 1  | Jack   | [email protected]    | 1467911301 |
| 2  | John   | [email protected]       | 1467920198 |
| 3  | Peter  | [email protected]   | 1467933196 |
+----+--------+----------------------+------------+

// resend_pass
+----+---------+-----------------+------------+
| id | user_id |      token      |  date_time |
+----+---------+-----------------+------------+
| 1  | 3       | 8u2934hfr43...  | 1467922396 |
+----+---------+-----------------+------------+

And I'm trying to follow this rule:

Basic rule of thumb: anytime you have nested queries in a loop, and inner loop is using data from the outer loop, you should re-write as a single joined query.

Do you have any advice?

Answer 1

Some thoughts:

• You should strongly consider using appropriate datetime, timestamp, etc. column types when storing date and/or time information in your DB. Unix timestamps may seem simple for working with in PHP, but at some point you will have to query on those fields and having to convert to date/time entities in a query will disallow indexing on that field.
• I am concerned that you don't store a flag to indicate if password reset has actually been used (i.e. user clicks on link in email). Why would you care if a user reset their password 10 times in an hour if they actually completed the whole process each time (something a probing attacker would presumably not be able to do without access to email account)? I would be worried about the number of unused reset attempts. I would actually also be more concerned with frequent activity coming from a single IP trying to submit different email addresses, but that may require more network-layer security controls.
• I think you could simplify your approach by only looking for X number of unused requests within a given time frame. Perhaps 12 per week. An automated attacker would violate this constraint potentially within seconds anyway, so I don't see why having different thresholds for different periods of time makes much sense, it only adds to complexity. You could possibly remove time threshold altogether and just limit it to X unused attempts total. At that point you probably either have a legitimate attacker (not likely) or you have a user experience/confusion problem in your process where you might need to give the user more specific guidance, or put them in touch with support, or whatever.
• You also may have need to mark a record as expired, that is if there are 10 unsuccessful requests, only the most recent one would actually still work.
• I don't like fluent style when performing prepared statements used here, as I think you need to be able to try the prepare first and validate that it worked before trying to execute it.
• Consider better name than $num_rows. You are storing a database record (in this case an array with one element) there, not a row count.

Answer 2

Overall, fine. It does its job. But...

✘ No comments

Currently, as it stands, theres no code comments. Comments explaining what is happening. Having meaningful comments throughout your code will help you in the future and your co-workers.

✘ E-mail confirmation false positives

This process may give false-positives depending on how you're checking the e-mail was delivered. For example; if you're sending the e-mail by mail() and marking them as sent if it returned true, then it just means it's been accepted for delivery by the reciever, but not actually recieved. It may have landed in their spam/junk box.

✘ No hit-rate limiting

Not having hit-rate limiting on a feature that is designed to reset your password allows an attacker to enumerate through an e-mail list, potentially resetting users passwords - then (this all depends on how you handle the reset token - if it expires after x time, then fine, ignore this last bit) brute-forcing the reset token to gain access to their account.

Though it looks like you don't have an expire time on your tokens, unless they're cleaned with a cron/background task.

---

Some fun notes

• Your output must start with captial letters, be nice to your UX.

• Having 4 reset tokens an hour seems a little too much, possibly allowing an attacker more chances to brute force a token ($token_lifetime * 4)

• You can modify your query so that you don't have a sub-SELECT by using HAVING.

Basic rule of thumb: anytime you have nested queries in a loop

• I cannot see any looping logic which encapulates a query.