2
\$\begingroup\$

I've gotten a lot of feedback about my tracking system already. So now I'm changing gears and looking at my email generation system. I have a class called Email which manages the sending of emails, validation of user email addresses, and validation of whether the user is a permissible "victim". To do this, I created a custom iterator to iterate over a PDO result set of users, called PDOIterator. The resulting Email class function gets called in PhishingController, where the data is assembled then passed to be sent.

As this is a complete rewrite of what I originally had, I have no specific direction that I'm asking for. So whatever you have works!

Email

/**
 * executeEmail
 * Public-facing method to send an email to a database of users if they are a valid recipient.
 *
 * @param   array                   $emailSettings      Host, port, username, and password variables for the mail server
 * @param   string                  $emailTemplate      Path to the blade.php template file from the views directory
 * @param   string                  $templateType       Specifies whether the email is an Advanced (adv) or Basic (bsc) scam
 * @param   string                  $templateTarget     Specifies whether the email is a Targeted (T) or Generic (G) scam
 * @param   int                     $period             Number of weeks to check back for recipient validation
 * @param   string                  $projectName        Name of this project
 * @param   int                     $projectId          ID of this project
 * @param   string                  $fromEmail          Email to be sent from
 * @param   string                  $companyName        Name of company sponsoring this awareness test
 * @param   string                  $subject            Subject of email
 * @throws  OutOfBoundsException
 * @throws  FatalErrorException
 * @throws  \PDOException                               Thrown from DBManager->query()
 * @throws  QueryException                              Thrown from DBManager->query()
 */
public static function executeEmail($emailSettings, $emailTemplate, $templateType, $templateTarget, $period,
                            $projectName, $projectId, $companyName = 'your organization',
                             $subject = 'Corporate Communication') {
    self::setEmailEnvironmentSettings($emailSettings);
    $db = new DBManager();
    $sql = "SELECT * FROM gaig_users.users;";
    $bindings = array();
    $users = $db->query($sql,$bindings,array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
    self::sendEmail(array($templateType,$templateTarget,$users,$period,$emailTemplate,$companyName,
        $projectName,$projectId),$emailSettings[2],$subject);
}

/**
 * setEmailEnvironmentSettings
 * Checks if the settings are valid settings, then sets or returns an exception.
 *
 * @param   array                   $emailSettings      Host, port, username, and password variables for the mail server
 * @throws  OutOfBoundsException
 */
private function setEmailEnvironmentSettings($emailSettings) {
    $pattern = ';(?:https?://)?(?:[a-zA-Z0-9.-]+?\.(?:com|net|org|gov|edu|mil)|\d+\.\d+\.\d+\.\d+);';
    if(!preg_match($pattern,$emailSettings[0]) || !filter_var($emailSettings[1],FILTER_VALIDATE_INT) ||
        !filter_var($emailSettings[2],FILTER_VALIDATE_EMAIL)) {
        $message = '';
        if(!preg_match($pattern,$emailSettings[0])) {
            $message .= 'Host is not a valid host name or IP address. host=' . $emailSettings[0] . '\n';
        }
        if(!filter_var($emailSettings[1],FILTER_VALIDATE_INT)) {
            $message .= 'Port is not a valid integer. port=' . $emailSettings[1] . '\n';
        }
        if(!filter_var($emailSettings[2],FILTER_VALIDATE_EMAIL)) {
            $message .= 'Username is not a valid email address. username=' . $emailSettings[2] . '\n';
        }
        throw new OutOfBoundsException($message);
    }
    putenv("MAIL_HOST=$emailSettings[0]");
    putenv("MAIL_PORT=$emailSettings[1]");
    putenv("MAIL_USERNAME=$emailSettings[2]");
    putenv("MAIL_PASSWORD=$emailSettings[3]");
}

/**
 * validRecipientAlgo
 * 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   string                  $templateType       Specifies whether the email is an Advanced (adv) or Basic (bsc) scam
 * @param   string                  $templateTarget     Specifies whether the email is a Targeted (T) or Generic (G) scam
 * @param   array                   $user               Associative Array containing the fields associated to the user
 * @param   int                     $period             Number of weeks to check back for recipient validation
 * @return  bool
 */
private function validRecipientAlgo($templateType,$templateTarget,$user,$period) {
    $db = new DBManager();
    $date = date('Y-m-d',strtotime('-' . $period . '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($result['timestamp_check'] <= $date) {
        return true;
    } else if($templateType == substr($user['USR_ProjectMostRecent'],-5,3) &&
        $templateType == substr($user['USR_ProjectPrevious'],-5,3)) {
        return false;
    } else if($templateTarget == substr($user['USR_ProjectMostRecent'],-2,1) &&
        $templateTarget == substr($user['USR_ProjectPrevious'],-2,1) &&
        $templateTarget == substr($user['USR_ProjectLast'],-2,1)) {
        return false;
    } else if($templateType.$templateTarget ==
        substr($user['USR_ProjectMostRecent'],strpos($user['USR_ProjectMostRecent'],'-')+1,4)) {
        return false;
    }
    return true;
}

/**
 * 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                   $params             Required parameters to pass to the email template
 * @param   string                  $from               Email to be sent from
 * @param   string                  $subject            Subject of email
 * @throws  FatalErrorException
 */
private function sendEmail($params, $from, $subject) {
    $userIterator = new PDOIterator($params[2]);
    foreach($userIterator as $user) {
        if($this->validRecipientAlgo($params[0],$params[1],$user,$params[3])) {
            $to = $user['USR_Email'];
            if(!filter_var($to,FILTER_VALIDATE_EMAIL)) {
                ErrorLogging::badEmailAddress(['USR_Username'] . ' has a bad email address. email=' . $user['USR_Email']);
            } else {
                $db = new DBManager();
                $urlId = null;
                if(!is_null($user['USR_UniqueURLId'])) {
                    $urlId = $user['USR_UniqueURLId'];
                }
                $urlId = $this->random_str(15);
                $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=?;";
                $bindings = array($urlId);
                if($updateUniqueURLId = $db->query($sql,$bindings)) {
                    $headers = array('companyName'=>$params[5],'projectName'=>$params[6],'projectId'=>$params[7],
                        'lastName'=>$user['USR_LastName'],'username'=>$user['USR_Username'],'urlId'=>$urlId);
                    if(!Mail::send(['html' => $params[4]],$headers, function($m) use ($from, $to, $subject) {
                        $m->from($from);
                        $m->to($to)->subject($subject);
                    })) {
                        throw new FatalErrorException('Email failed to send to ' . $to . ' from ' . $from);
                    }
                    $project_new = $params[6] . '-' . $params[4];
                    $project_mostRecent = $user['USR_ProjectMostRecent'];
                    $project_previous = $user['USR_ProjectPrevious'];
                    $sql = "UPDATE gaig_users.users SET USR_ProjectMostRecent=?, USR_ProjectPrevious=?, 
                          USR_ProjectLast=? WHERE USR_Username=?;";
                    $bindings = array($project_new,$project_mostRecent,$project_previous,$user['USR_Username']);
                    $updateProjects = $db->query($sql,$bindings);
                }
            }
        }
    }
}

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;
}

PDOIterator - No PHPDoc Yet

private $pdoStatement;
private $key;
private $result;
private $valid;

public function __construct(\PDOStatement $pdoStatement) {
    $this->pdoStatement = $pdoStatement;
}

public function next() {
    $this->key++;
    $this->result = $this->pdoStatement->fetch(
        \PDO::FETCH_OBJ,
        \PDO::FETCH_ORI_ABS,
        $this->key
    );
    if ($this->result === false) {
        $this->valid = false;
        return null;
    }
}

public function valid() {
    return $this->valid;
}

public function current() {
    return $this->result;
}

public function rewind() {
    $this->key = 0;
}

public function key() {
    return $this->key;
}

PhishingController - No PHPDoc Yet

public function sendEmail(Request $request) {
    $fromEmail = $request['fromEmail'];
    $fromPass = $request['fromPass'];
    $host = $request['hostName'];
    $port = $request['port'];
    $emailSettings = array($host,$port,$fromEmail,$fromPass);
    $emailTemplate = 'emails.' . $request['emailTemplate'];
    $emailTemplateType = substr($request['emailTemplate'],0,3);
    $emailTemplateTarget = substr($request['emailTemplate'],3,1);
    $period = 4;
    $subject = $request['subject'];
    $projectName = $request['projectName'];
    $projectId = intval($projectName,strpos($projectName,'_'));
    $projectName = substr($projectName,0,strpos($projectName,'_')-1);
    $companyName = $request['companyName'];

    try {
        Email::executeEmail($emailSettings,$emailTemplate,$emailTemplateType,$emailTemplateTarget,
            $period,$projectName,$projectId,$companyName,$subject);
    } catch(OutOfBoundsException $oobe) {

    } catch(FatalErrorException $fee) {

    } catch(\PDOException $pdoe) {
        ErrorLogging::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
    } catch(QueryException $qe) {
        ErrorLogging::logQueryError(__CLASS__,__FUNCTION__,$qe);
    }
}

As always, thanks for the review!

\$\endgroup\$
1
  • \$\begingroup\$ Parameter count, function length and single responsibility principle spring to mind by glancing at this. Immediately I had a flashback to a book that I read : amazon.com/gp/aw/d/1491953527/…. The book is basically a bunch of guidelines on how to build Maintainable Software and provides ways in which to solve the exact problems in your code I mentioned above \$\endgroup\$ Jul 15, 2016 at 14:03

1 Answer 1

3
\$\begingroup\$

Too many parameters

/**
 * executeEmail
 * Public-facing method to send an email to a database of users if they are a valid recipient.
 *
 * @param   array                   $emailSettings      Host, port, username, and password variables for the mail server
 * @param   string                  $emailTemplate      Path to the blade.php template file from the views directory
 * @param   string                  $templateType       Specifies whether the email is an Advanced (adv) or Basic (bsc) scam
 * @param   string                  $templateTarget     Specifies whether the email is a Targeted (T) or Generic (G) scam
 * @param   int                     $period             Number of weeks to check back for recipient validation
 * @param   string                  $projectName        Name of this project
 * @param   int                     $projectId          ID of this project
 * @param   string                  $fromEmail          Email to be sent from
 * @param   string                  $companyName        Name of company sponsoring this awareness test
 * @param   string                  $subject            Subject of email
 * @throws  OutOfBoundsException
 * @throws  FatalErrorException
 * @throws  \PDOException                               Thrown from DBManager->query()
 * @throws  QueryException                              Thrown from DBManager->query()
 */
public static function executeEmail($emailSettings, $emailTemplate, $templateType, $templateTarget, $period,
                            $projectName, $projectId, $companyName = 'your organization',
                             $subject = 'Corporate Communication') {
    self::setEmailEnvironmentSettings($emailSettings);
    $db = new DBManager();
    $sql = "SELECT * FROM gaig_users.users;";
    $bindings = array();
    $users = $db->query($sql,$bindings,array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
    self::sendEmail(array($templateType,$templateTarget,$users,$period,$emailTemplate,$companyName,
        $projectName,$projectId),$emailSettings[2],$subject);
}

When you have this many parameters, you have to know that you're doing something wrong.

You still need all of them, of course. But individually specifying each of them causes huge method signatures like this one. You already encapsulated some of the parameters - $emailSettings contains a host, port, username and password. That's good! But it can be taken further.

Consider making a class whose only goal is to be a wrapper for some of these parameters. For example, a Template which represents $emailTemplate, $templateType and $templateTarget.

You can have static methods that take each of these parameters and return the wrapper objects.

The end result will be something that you pass server settings, a template, a project, and maybe something else (for the rest of the parameters) into. By doing so, you can reduce the mental load by a huge factor. Because as it is right now, your public API is unusable.

Semantic disconnects

Additionally, executeEmail is throwing PDOException and QueryException why?! How is sending an email going to databases? It turns out this is because you retrieve a list of users in the executeEmail function. executeEmail is a poor name; a better one could be found if you simplified the arguments list and it's responsibilities to something more manageable.

Similarily, here's sendEmail:

/**
 * sendEmail
 * Iterates through the PDO Result Set of users. Calls validRecipientAlgo to validate user. Sends email if
 *      valid and updates user if valid.

Stop. That's not sendEmail. That's validateAndUpdateUsersAndSendEmailToValidatedUsers. If that sounds too long winded, that's because this method is doing too much. Eventually, you'd want to see a method that calls methods for validating the users, updating the users, and sending mails.

\$\endgroup\$
2
  • \$\begingroup\$ With the Too Many Parameters part, is it specifically the parameters or the length of the PHPDoc? I've modified the code to use 3 parameter arrays but have modified the PHPDoc to explain what each component in each array contains (so it's the same length effectively). \$\endgroup\$
    – Trojan404
    Jul 13, 2016 at 14:31
  • \$\begingroup\$ @throck95 It's specifically the parameter count. \$\endgroup\$
    – Pimgd
    Jul 13, 2016 at 14:33

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.