Phishing Project UserIterator Implementation

Question

As I stated in an answer comment on my previous question, the UserIterator has a much larger footprint than I think most people realize. This question is here to further explain the project, the UserIterator's role, and what I'm trying to achieve with this specific piece of the code.

If any of you have had experience with Phishing Awareness Testing, you should understand this, but effectively, I am recreating my own version of this product. This is my thesis for my undergraduate degree. The idea here is to develop a user-friendly, minimal business impact, phishing awareness solution that small businesses can utilize. Most of the products like the one linked above can be expensive and hard to implement for businesses with sub-100 employees. The idea with this project is to change that and provide a solution that takes minimal training and comprehension to use.

That being said, the UserIterator is essential as you do not specify which users receive these phishing tests. The database is provided to the server by default, and this iterator is then provided to validate which users will receive the tests. I do not want to be sending the same test to the same users over and over again, so each time an email is sent, it is validated against a validation algorithm to check for redundancy or if the user absolutely needs to be tested.

The user will always receive the email if they have failed to receive a phishing awareness test with the last period of time specified by the user ($periodInWeeks). After this check, it will check the complex type of the last 2 tests the user has received. The complex type is either Advanced (adv) or Basic (bsc). If they have received a complex type that is the same as this tests complex type for both of the last 2 occurrences, they will not receive this test. It will then check the target type of the last 3 occurrences, and if it is the same, they will not receive this test. A target type can be Targeted (T) or Generic (G). Lastly, if the user's last received test was the identical test to what is currently being sent, meaning the complex type and target type are both the same, they will not receive the test.

---

Validation Algorithm

$sql = "SELECT max(SML_AccessTimestamp) as 'timestamp_check' from gaig_users.sent_email where SML_UserId = ? and SML_ProjectName = ?;";
$bindings = array($user['USR_UserId'],$user['USR_ProjectMostRecent']);
$timestampData = $db->query($sql,$bindings);
$result = $timestampData->fetch(\PDO::FETCH_ASSOC);

if($result['timestamp_check'] <= $date) {
        return true;
} else if($templateComplexType == substr($user['USR_ProjectMostRecent'],-5,3) &&
        $templateComplexType == substr($user['USR_ProjectPrevious'],-5,3)) {
        return false;
} else if($templateTarType == substr($user['USR_ProjectMostRecent'],-2,1) &&
        $templateTarType == substr($user['USR_ProjectPrevious'],-2,1) &&
        $templateTarType == substr($user['USR_ProjectLast'],-2,1)) {
        return false;
} else if($templateComplexType.$templateTarType ==
        substr($user['USR_ProjectMostRecent'],strpos($user['USR_ProjectMostRecent'],'-')+1,4)) {
        return false;
}

---

Obviously, the design and implementation of my algorithm could be more streamlined and separated out to it's own individual class or function. However, for right now it isn't.

With that explained, below is the most recent and up-to-date Email Class where the UserIterator is being implemented. I have added in two private variables that hold the Config objects to reduce the amount of objects that have to be passed around. I am also unsure about whether I need to move the random_str as was suggested because this class is the only class where it used.

However, I am completely at a loss as to the best way to separate out this iteration. The validation COULD be separated out into the User Model class, however, I will need to do some tweaking on the model to get this to work. This implementation is in no way perfect and is definitely a work-in-progress. Any suggestions as ways to extrapolate out this iteration would be appreciated.

---

Email

class Email {

private $templateConfig;
private $emailConfig;

/**
 * executeEmail
 * Public-facing method to send an email to a database of users if they are a valid recipient.
 *
 * @param   EmailConfiguration          $emailConfig            Email Configuration object containing required information to send an email
 * @param   TemplateConfiguration       $templateConfig         Template Configuration object containing required information to build a template
 * @param   int                         $periodInWeeks          Number of weeks to check back for recipient validation
 * @throws  EmailException                                      Custom Exception to embody any exceptions thrown in this class
 */
public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig,
    $periodInWeeks)
{
    self::setTemplateConfig($templateConfig);
    self::setEmailConfig($emailConfig);

    try {
        foreach($emailConfig->getUsersIterator() as $user) {
            if(self::validateUser($user, $periodInWeeks)) {
                self::sendEmail($user);
                self::updateUserProjects($user);
            }
        }
    } catch(Exception $e) {
        throw new EmailException(__CLASS__ . ' Exception',0,$e);
    }
}

/**
 * validateUser
 * Function checks if the specified user has not received a test within the specified duration,
 *      if the template type is the same for the last two project participants, if the template target
 *      is the same for last three project participants, or if the last project is identical to the new project.
 *
 * @param   array                   $user               Associative Array containing the fields associated to the user
 * @param   int                     $periodInWeeks             Number of weeks to check back for recipient validation
 * @return  bool
 */
private function validateUser($user, $periodInWeeks) {
    $db = new DBManager();
    $templateComplexType = $this->templateConfig->getTemplateComplexityType();
    $templateTarType = $this->templateConfig->getTemplateTargetType();
    $date = date('Y-m-d',strtotime('-' . $periodInWeeks . 'weeks')) . '00:00:00';
    $sql = "SELECT max(SML_AccessTimestamp) as 'timestamp_check' from gaig_users.sent_email where SML_UserId = ? and SML_ProjectName = ?;";
    $bindings = array($user['USR_UserId'],$user['USR_ProjectMostRecent']);
    $timestampData = $db->query($sql,$bindings);
    $result = $timestampData->fetch(\PDO::FETCH_ASSOC);
    if(!filter_var($user['USR_Email'],FILTER_VALIDATE_EMAIL)) {
        $this->badEmailAddressWarning(['USR_Username'] . ' has a bad email address. email=' . $user['USR_Email']);
        return false;
    }
    if($result['timestamp_check'] <= $date) {
        return true;
    } else if($templateComplexType == substr($user['USR_ProjectMostRecent'],-5,3) &&
        $templateComplexType == substr($user['USR_ProjectPrevious'],-5,3)) {
        return false;
    } else if($templateTarType == substr($user['USR_ProjectMostRecent'],-2,1) &&
        $templateTarType == substr($user['USR_ProjectPrevious'],-2,1) &&
        $templateTarType == substr($user['USR_ProjectLast'],-2,1)) {
        return false;
    } else if($templateComplexType.$templateTarType ==
        substr($user['USR_ProjectMostRecent'],strpos($user['USR_ProjectMostRecent'],'-')+1,4)) {
        return false;
    }
    return true;
}

/**
 * getUrlId
 * Generates or retrieves the UniqueURLId of the passed user.
 *
 * @param   array           $user           User array extracted from PDOStatement
 * @param   int             $projectId      Project ID to be included in generated URL
 * @return  string
 */
private function getUrlId($user,$projectId) {
    if(!is_null($user['USR_UniqueURLId'])) {
        $urlId = $user['USR_UniqueURLId'];
    } else {
        $db = new DBManager();
        $urlId = $this->random_str(15) . $projectId;
        $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=? WHERE USR_UserId=?;";
        $bindings = array($urlId,$user['USR_UserId']);
        $db->query($sql,$bindings);
    }
    return $urlId;
}

/**
 * updateUserProjects
 * Updates the user with the newest project and rotates the old projects down one.
 *
 * @param   array           $user           User array extracted from PDOStatement
 */
private function updateUserProjects($user) {
    $db = new DBManager();
    $sql = "UPDATE gaig_users.users SET USR_ProjectMostRecent=?, USR_ProjectPrevious=?, 
                USR_ProjectLast=? WHERE USR_Username=?;";
    $bindings = array($this->templateConfig->getProjectName(),$user['USR_ProjectMostRecent'],$user['USR_ProjectPrevious'],$user['USR_Username']);
    $db->query($sql,$bindings);
}

/**
 * sendEmail
 * Iterates through the PDO Result Set of users. Calls validRecipientAlgo to validate user. Sends email if
 *      valid and updates user if valid.
 *
 * @param   array           $user           User array extracted from PDOStatement
 * @throws  FailureException
 */
private function sendEmail($user) {
    $urlId = $this->getUrlId($user,$this->templateConfig->getProjectId());
    $templateData = array(
        'companyName'=>$this->templateConfig->getCompanyName(),
        'projectName'=>$this->templateConfig->getProjectName(),
        'projectId'=>$this->templateConfig->getProjectId(),
        'lastName'=>$user['USR_LastName'],
        'username'=>$user['USR_Username'],
        'urlId'=>$urlId
    );
    $subject = $this->emailConfig->getSubject();
    $from = $this->emailConfig->getFromEmail();
    $to = $user['USR_Email'];
    if(!Mail::send(['html' => $this->templateConfig->getTemplate()],$templateData, function($m) use ($from, $to, $subject) {
        $m->from($from);
        $m->to($to)->subject($subject);
    })) {
        throw new FailureException('Email failed to send to ' . $to . ' from ' . $from);
    }
}

/**
 * random_str
 * Generates a random string.
 *
 * @param   int         $length         Length of string to be returned
 * @param   string      $keyspace       Allowed characters to be used in string
 * @return  string
 */
private function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
    $str = '';
    $max = mb_strlen($keyspace, '8bit') - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}

/**
 * badEmailAddressWarning
 * Logs error when a bad email address is found associated with a user
 *
 * @param   string          $message        Error message to be logged
 */
private function badEmailAddressWarning($message) {
    $path = '../storage/logs/badEmailAddress' . date('m-d-Y') . '.log';
    if(!file_exists($path)) {
        $file = fopen($path,'w');
        fclose($file);
    }
    error_log($message,3,$path);
}

private function setTemplateConfig($templateConfig) {
    $this->templateConfig = $templateConfig;
}

private function setEmailConfig($emailConfig) {
    $this->emailConfig = $emailConfig;
}
}

Answer 1

Thoughts in code comments below.

class Email {
/*
Declare these as static since that is how they are being used.
private static $templateConfig;
*/
private $templateConfig;
private $emailConfig;

public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig,
    $periodInWeeks)
{
/*
Validate $periodInWeeks here and throw as necessary.
If using PHP7, you can implement 'int' check in method signature.
*/

    self::setTemplateConfig($templateConfig);
    self::setEmailConfig($emailConfig);

    try {
/*
Not sure what getUsersIterator does, and still don't understand
why this object/array would come from EmailConfig class.
Should it be sent to method as different parameter (UserCollection
or similar) and decoupled from email configuration?
Also should you be operating directly on it here before validating
you get appropriate return from getUsersIterator()?
*/
        foreach($emailConfig->getUsersIterator() as $user) {
            if(self::validateUser($user, $periodInWeeks)) {
                self::sendEmail($user);
                self::updateUserProjects($user);
            }
        }
/*
Good usage of generic Exception here for your approach to this
section of code that could produce multiple different exception
types.  You are just chaining to custom exception type here, so
this is appropriate
*/
    } catch(Exception $e) {
        throw new EmailException(__CLASS__ . ' Exception',0,$e);
    }
}

/*
Static function.
I don't like this method name as in your other areas of code, you are
expecting a "validate*" function to throw an exception.  Here you are
just trying to determine if the user should be emailed or not. You
are not really validating the user object/array itself. Perhaps
userNeedsEmail() or similar.
*/
private function validateUser($user, $periodInWeeks) {
/*
try-catch needed?
*/
    $db = new DBManager();
/*
Access statically?
self::$templateConfig->getTemplateComplexityType();
*/
    $templateComplexType = $this->templateConfig->getTemplateComplexityType();
    $templateTarType = $this->templateConfig->getTemplateTargetType();
/*
Need space before "weeks" and "00:00:00"
*/
    $date = date('Y-m-d',strtotime('-' . $periodInWeeks . 'weeks')) . '00:00:00';
/*
Make SQL more readable. Consider syntax convention of all caps for MySQL
keywords and lower/snake case for DB objects. You are inconsistent here.

$sql = "
    SELECT MAX(SML_AccessTimestamp) as 'timestamp_check'
    FROM gaig_users.sent_email
    WHERE SML_UserId = ?
    AND SML_ProjectName = ?";
*/
    $sql = "SELECT max(SML_AccessTimestamp) as 'timestamp_check' from gaig_users.sent_email where SML_UserId = ? and SML_ProjectName = ?;";
/*
Is $user an object? If not, it probably should be. If so, use
object style access
*/
    $bindings = array($user['USR_UserId'],$user['USR_ProjectMostRecent']);
/*
This code section seems to expect happy path only. What happens if no rows
are selected from DB?
*/
    $timestampData = $db->query($sql,$bindings);
/*
Don't leak PDO implementation out to this level
*/
    $result = $timestampData->fetch(\PDO::FETCH_ASSOC);
/*
Should user not be validated long before this point?
At a minimum, validation should happen before you try to connect to DB
*/
    if(!filter_var($user['USR_Email'],FILTER_VALIDATE_EMAIL)) {
        $this->badEmailAddressWarning(['USR_Username'] . ' has a bad email address. email=' . $user['USR_Email']);
        return false;
    }
    if($result['timestamp_check'] <= $date) {
        return true;
/*
These next few sections are very hard to read.
You should do all this string manipulation in the user
data structure before even passing it to this method,
so that this method doesn't need to understand how to break apart
these values. Some of the lines of code are way too long as well.
*/
    } else if($templateComplexType == substr($user['USR_ProjectMostRecent'],-5,3) &&
        $templateComplexType == substr($user['USR_ProjectPrevious'],-5,3)) {
        return false;
    } else if($templateTarType == substr($user['USR_ProjectMostRecent'],-2,1) &&
        $templateTarType == substr($user['USR_ProjectPrevious'],-2,1) &&
        $templateTarType == substr($user['USR_ProjectLast'],-2,1)) {
        return false;
    } else if($templateComplexType.$templateTarType ==
        substr($user['USR_ProjectMostRecent'],strpos($user['USR_ProjectMostRecent'],'-')+1,4)) {
        return false;
    }
    return true;
}

/*
Static?
*/
private function getUrlId($user,$projectId) {
/*
Don't understand the logic in this method.
So sometimes the user object has URLId and sometimes
it doesn't?  Can this be enforced on the user object so
this class doesn't need to hold this logic?
*/
    if(!is_null($user['USR_UniqueURLId'])) {
        $urlId = $user['USR_UniqueURLId'];
    } else {
/*
try-catch?
*/
        $db = new DBManager();
/*
static access to random_str?
*/
        $urlId = $this->random_str(15) . $projectId;
        $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=? WHERE USR_UserId=?;";
        $bindings = array($urlId,$user['USR_UserId']);
/*
happy path only? Does this query method acutally bind the result to
$urlId variables?  Otherwise I don't see how this is set.  Should you
explicitly bind variables outside call to query method? Or rename method
to bindAndQuery() or something that makes behavior clear.
*/
*/
        $db->query($sql,$bindings);
    }
    return $urlId;
}

/*
static?
*/
private function updateUserProjects($user) {
/*
try-catch?
*/
    $db = new DBManager();
/*
clean up SQL presentation
*/
    $sql = "UPDATE gaig_users.users SET USR_ProjectMostRecent=?, USR_ProjectPrevious=?, 
                USR_ProjectLast=? WHERE USR_Username=?;";
/*
line too long
*/
    $bindings = array($this->templateConfig->getProjectName(),$user['USR_ProjectMostRecent'],$user['USR_ProjectPrevious'],$user['USR_Username']);
/*
happy path only?
*/
    $db->query($sql,$bindings);
/*
Do you need any return to indicate success to caller?
*/
}

/*
static?
*/
private function sendEmail($user) {
/*
If you had URL ID determined on $user before passing to this method,
you could eliminate this from this section of code.

Static call to getUrlId()?
*/
    $urlId = $this->getUrlId($user,$this->templateConfig->getProjectId());
/*
Consider using stdClass here if your Mail::send() method supports it.
*/
    $templateData = array(
/*
Static access for these $this references?
*/
        'companyName'=>$this->templateConfig->getCompanyName(),
        'projectName'=>$this->templateConfig->getProjectName(),
        'projectId'=>$this->templateConfig->getProjectId(),
        'lastName'=>$user['USR_LastName'],
        'username'=>$user['USR_Username'],
        'urlId'=>$urlId
    );
/*
Static access instead of $this?
*/
    $subject = $this->emailConfig->getSubject();
    $from = $this->emailConfig->getFromEmail();
    $to = $user['USR_Email'];
/*
Too long of a line. Perhaps break this up like this:
$send_result = Mail::send(
    ['html' => self::$templateConfig->getTemplate()],
    $templateData,
    function($mail) use ($from, $to, $subject) {
        $mail->from($from); chain with below if possible
        $mail->to($to)
             ->subject($subject);
    }
);
if(!$send_result) { throw ... }
*/
    if(!Mail::send(['html' => $this->templateConfig->getTemplate()],$templateData, function($m) use ($from, $to, $subject) {
        $m->from($from);
        $m->to($to)->subject($subject);
    })) {
        throw new FailureException('Email failed to send to ' . $to . ' from ' . $from);
    }
}

/*
Still think this should be in common functions or someplace else
*/
private function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
    $str = '';
/*
No need for multibyte function here as you have only single byte
character set.
*/
    $max = mb_strlen($keyspace, '8bit') - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}

/*
Name doesn't indicate what this really does - error logging.I still think
email address stuff should be worked out in user object before even getting
to the point of sending the user info to an emailing class.
*/
private function badEmailAddressWarning($message) {
    $path = '../storage/logs/badEmailAddress' . date('m-d-Y') . '.log';
    if(!file_exists($path)) {
        $file = fopen($path,'w');
        fclose($file);
    }
    error_log($message,3,$path);
}
/*
Both of these methods are being called statically.
You should declare them as such.
Variable setting should be static:
self::$templateConfig = $templateConfig;
*/
private function setTemplateConfig($templateConfig) {
    $this->templateConfig = $templateConfig;
}

private function setEmailConfig($emailConfig) {
    $this->emailConfig = $emailConfig;
}
}