Phishing Project Sending Email Reworked

Question

@Pimgd gave some good feedback on the previous post. I've made the changes that I think best suit my application and am looking to see what people think of this implementation.

PhishingController

/**
 * sendEmail
 * Function mapped to Laravel route. Defines variable arrays and calls Email Class executeEmail.
 * 
 * @param   Request         $request            Request object passed via AJAX from client.
 */
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);
    $template = array($emailTemplate,$emailTemplateType,$emailTemplateTarget);

    $period = 4;
    $subject = $request['subject'];
    $projectName = $request['projectName'];
    $projectId = intval($projectName,strpos($projectName,'_'));
    $projectName = substr($projectName,0,strpos($projectName,'_')-1);
    $companyName = $request['companyName'];
    $params = array($period,$projectName,$projectId,$companyName,$subject);

    try {
        Email::executeEmail($emailSettings,$template,$params);
    } catch(OutOfBoundsException $oobe) {
        //mail server settings not valid
    } catch(FailureException $fee) {
        //email failed to be sent to server
    } catch(\PDOException $pdoe) {
        DBManager::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
    } catch(QueryException $qe) {
        DBManager::logQueryError(__CLASS__,__FUNCTION__,$qe);
    }
}

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   array                   $template               Email Template, Template Type, Template Target Type for user validation
 *          string                  Email Template          Path to the blade.php template file from the views directory
 *          string                  Template Type           Specifies whether the email is an Advanced (adv) or Basic (bsc) scam
 *          string                  Template Target Type    Specifies whether the email is a Targeted (T) or Generic (G) scam
 * @param   array                   $params                 Period, Project Name, Project ID, From Email Address, Company Name, Subject
 *          int                     Period                  Number of weeks to check back for recipient validation
 *          string                  Project Name            Name of this project
 *          int                     Project Id              ID of this project
 *          string                  From Email Address      Email to be sent from
 *          string                  Company Name            Name of company sponsoring this awareness test
 *          string                  Subject                 Subject of email
 * @throws  OutOfBoundsException                            Thrown from setEmailEnvironmentSettings if a setting is not valid
 * @throws  FailureException                                Thrown from sendEmail() if mail fails to be given to mail server
 * @throws  \PDOException                                   Thrown from DBManager->query()
 * @throws  QueryException                                  Thrown from DBManager->query()
 */
public static function executeEmail($emailSettings, $template, $params) {
    self::setEmailEnvironmentSettings($emailSettings);
    $db = new DBManager();
    $sql = "SELECT * FROM gaig_users.users;";
    $users = $db->query($sql,array(),array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
    $userIterator = new PDOIterator($users);
    foreach($userIterator as $user) {
        if(self::validateUser($template[1],$template[2],$user,$params[0])) {
            $to = $user['USR_Email'];
            $urlId = self::getUrlId($user);
            $companyName = $params[3];
            $subject = $params[4];
            if(is_null($params[3])) {
                $companyName = 'your organization';
            }
            if(is_null($params[4])) {
                $subject = 'Corporate Communication';
            }
            $headers = array('companyName'=>$companyName,'projectName'=>$params[1],'projectId'=>$params[2],
                'lastName'=>$user['USR_LastName'],'username'=>$user['USR_Username'],'urlId'=>$urlId);
            self::sendEmail($template[0],$headers,$to,getenv('MAIL_USERNAME'),$subject);
            $project_new = $params[6] . '-' . $params[4];
            $projects = array($project_new,$user['USR_ProjectMostRecent'],$user['USR_ProjectPrevious']);
            self::updateUserProjects($projects,$user);
        }
    }
}

/**
 * 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]");
}

/**
 * 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   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 validateUser($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(!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($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;
}

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

/**
 * updateUserProjects
 * Updates the user with the newest project and rotates the old projects down one.
 *
 * @param   array           $projects       Most Recent Project, Previous Project, Oldest Project
 * @param   array           $user           User array extracted from PDOStatement
 */
private function updateUserProjects($projects,$user) {
    $db = new DBManager();
    $sql = "UPDATE gaig_users.users SET USR_ProjectMostRecent=?, USR_ProjectPrevious=?, 
                USR_ProjectLast=? WHERE USR_Username=?;";
    $bindings = array($projects[0],$projects[1],$projects[2],$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                   $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($template, $headers, $to, $from, $subject) {
    if(!Mail::send(['html' => $template],$headers, 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);
}

As always, thanks for the review!

Answer 1

1)

Looked at previous review and couldn't agree more about the complexity of the method signatures as well as the number of possible Exception types in play being very confusing an immediate red flags that your implementation is questionable.

You seem to be mainly struggling with composition in coding, which can take some time and practice to become more second nature. I do think you made some progress in trying to minimize number of parameters, but you still have a lot of complexity there.

Have you considered instead of passing arrays to the main method, passing objects? This would be taking more of a dependency injection approach as has been discussed previously in some of your other code reviews. This would also make your code MUCH more readable as passing numerically-indexed arrays makes code much harder to understand.

Pass this class the things it needs - DB object, template object, template data object, email configuration object, user object, etc. This allows you to enforce all the validations for these types of things and making sure you have these in the proper state BEFORE trying to work with them to actually send an email.

Don't ask your email class to understand how to interpret an AJAX request, instantiate and validate all its dependencies, how to validate email addresses, how to generate short URL's, how to set environmental variables (why would you be setting these in an email sending context anyway?), etc.

The email class should expect valid dependencies to be passed to it, so that your class code can focus on the actual email sending functionality.

2)

It seems you are struggling with when to use numerically-indexed arrays vs. associative arrays or objects as appropriate data structure.

Numerically-indexed arrays should typically only be used when you are working with an arbitrary length list of similar items or item order is important. Associative arrays or objects should typically be used in cases where items you are trying to store may be distinctly different and should therefore have a lookup key or property name that allows you to get directly to the information you want. Typically one does not care about order for associative array/objects and iterating over such objects is not as commonly done as with numerically-indexed arrays.

I personally tend to use objects (even stdClass objects) much more frequently than associative arrays when working with a related set of dissimilar data. I tend to use associative arrays more in PHP for hash table (hash map) use cases, since this is, in essence, PHP's implementation of that data structure. IMO, a lot of PHP examples out there in the wild overuse associative arrays for cases where objects are more appropriate (dealing with database result sets being one of the more frequent offenders in code examples).

I will use your email configuration array as an example of how this is a problem in your code, as this clearly should not be a numerically-indexed array as it is now. This array currently holds 4 distinct types of information - host name, port, username, password. Your code would read MUCH better if you you actually did something like:

// Associative array example

if(!preg_match($pattern,$emailSettings['host']) ||
   !filter_var($emailSettings['port'], FILTER_VALIDATE_INT) ||
   !filter_var($emailSettings['username'], FILTER_VALIDATE_EMAIL)) {

// object example (my personal preference here)
if(!preg_match($pattern,$emailSettings->host) ||
   !filter_var($emailSettings->port, FILTER_VALIDATE_INT) ||
   !filter_var($emailSettings->username, FILTER_VALIDATE_EMAIL)) {

You see here in the code that you now know EXACTLY what sorts of values I am dealing with. The code is more meaningful to the reader. (Note I also broke your code up across multiple lines as your code gets hard to read if you go beyond standard rule of thumb of ~80 characters per line.)

Of course maybe this example is moot here if you went with dependency injection approach, as you would then instantiate a emailCongfiguration object or similar and have all the data validation happening in that class, not in an email sending class.

3)

This code is redundant

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

Why check each condition twice? You may be better off just removing the outer "if" statement to something like this:

if(!preg_match($pattern,$emailSettings[0])) {
    throw new InvalidArgumentException(
        'Host is not a valid host name or IP address. Value provided: ' .
        var_export($emailSettings[0], true)
    );
}
if(!filter_var($emailSettings[1],FILTER_VALIDATE_INT)) {
    throw new InvalidArgumentException(
        'Port is not a valid integer. Value provided: ' .
        var_export($emailSettings[1], true)
    );
}
if(!filter_var($emailSettings[2],FILTER_VALIDATE_EMAIL)) {
    throw new InvalidArgumentException(
        'Username is not a valid email address. Value provided: ' .
        var_export($emailSettings[2], true)
    );
}

// or alternate if you need to check all parameters before failing
$exceptionMessage= '';
if(!preg_match($pattern,$emailSettings[0])) {
    $exceptionMessage.= 'Host is not a valid host name or IP address. ' .
        'Value provided: ' . var_export($emailSettings[0], true) . PHP_EOL;
}
if(!filter_var($emailSettings[1],FILTER_VALIDATE_INT)) {
    $exception_message .= 'Port is not a valid integer. ' . 
        'Value provided: ' . var_export($emailSettings[1], true) . PHP_EOL;
}
if(!filter_var($emailSettings[2],FILTER_VALIDATE_EMAIL)) {
    $exceptionMessage.= 'Username is not a valid email address. '
        'Value provided: ' . var_export($emailSettings[2], true) . PHP_EOL;
}
if(!empty($exception_message)) {
    throw new InvalidArgumentException($exceptionMessage);
}

Note I am using InvalidArgumentException which is more appropriate here.

Before this code however, you again are just assuming that $emailSettings is a valid array with appropriate values in it, so you might consider checking that email settings is indeed an array and that it is not empty. You may also want to check that each of the for expected keys is set (these would be good case for OutOfBoundsException) before trying to validate the content at each of those keys.

Or, you move this all into an email configuration class as mentioned earlier and do all that validation there, meaning the only validation you need to do here is make sure you have a valid instance of that class (something that can be enforced via type hinting on the parameter itself).

By the way, your domain validation regex is very limited in terms of top-level domains it supports. This might be by design, but if you are looking for any valid host name (by RFC 2396 standards for example), you should consider expanding your regex or possibly using FILTER_VALIDATE_URL.

4)

Your overall approach to exceptions needs some refinement. For example, you probably do not want your PhishingController to have to capture and perform different behaviors for all the underlying exceptions that might be thrown by the email class. The email class should perhaps understand these different exception types but ultimately wrap these to a standard exception type (i.e. Exception) or a class-specific exception type you create (i.e. EmailException) but it likely is adding no value to have the PhishingController class have to deal with different exception types.

A good rule of thumb when dealing with multiple dependencies and underlying exception types and how to surface those to calling code is to think about the perspective of the calling code. In your example, I don't see why the Phishing Controller cares what type of exception is thrown when it tries to send email, as you are not changing the behavior of the class to actually do something different for each of those cases. You are not retrying the operation, changing return results to the caller based on different exception types, messaging different exception types to the caller, etc. So really your controller class only cares about whether there is a successful email send or not. Since this is the case, why does it need to know about the various exceptions that may be thrown by email operation as opposed to handling a single exception type bubbled up from email class?

I think this StackOverflow question has a really good accepted answer that walks through typical use cases for utilizing exception handling:

https://stackoverflow.com/questions/5551668/what-are-the-best-practices-for-catching-and-re-throwing-exceptions

I am sure there are other similar resources out there on the web as well.

Answer 2

if(is_null($params[4])) {

This is not what I had in mind.

I had in mind an object. Something like an "Associative array", perhaps (array with string keys). What you have right now is condensed arguments to args[0], args[4]. That's even less readable than $subject, even if it condenses the argument list.