4
\$\begingroup\$

As suggested by @MikeBrant in the previous question, I've added in two configuration classes to simplify and more broadly organize the objects required to send my mail messages. This also removes the need to actively read and write with the .env file. I did not have time to write new PHPDocs for any of the changes today, so methods have their old PHPDoc followed by a generated PHPDoc (if needed). EmailException and ConfigurationException currently don't do anything other than extend Exception. They are merely just a name for now. This may change later after review of the code for specific details the exception need's access to.

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) {
    $projectName = $request['projectName'];
    $projectId = intval($projectName,strpos($projectName,'_'));
    $projectName = substr($projectName,0,strpos($projectName,'_')-1);
    $period = 4;

    try {
        $templateConfig = new TemplateConfiguration(
            array(
                'templateName'=>$request['emailTemplate'],
                'companyName'=>$request['companyName'],
                'projectName'=>$projectName,
                'projectId'=>$projectId
            )
        );

        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request['hostName'],
                'port'=>$request['port'],
                'authUsername'=>$request['username'],
                'authPassword'=>$request['password'],
                'fromEmail'=>$request['fromEmail'],
                'subject'=>$request['subject']
            )
        );

        Email::executeEmail($emailConfig,$templateConfig,$period);
    } catch(ConfigurationException $ce) {

    } catch(EmailException $ee) {

    }
}

EmailConfiguration

private $host;
private $port;
private $authUsername;
private $authPassword;
private $fromEmail;
private $subject;
private $usersIterator;

/**
 * EmailConfiguration constructor.
 * @param $emailSettings
 */
public function __construct($emailSettings) {
    try {
        if($this->areSettingsValid($emailSettings)) {
            $this->host = $emailSettings['host'];
            $this->port = $emailSettings['port'];
            $this->authUsername = $emailSettings['authUsername'];
            $this->authPassword = $emailSettings['authPassword'];
            $this->fromEmail = $emailSettings['fromEmail'];
            $this->subject = $emailSettings['subject'];

            try {
                $db = new DBManager();
                $sql = "SELECT * FROM gaig_users.users;";
                $users = $db->query($sql,array(),array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
                $this->usersIterator = new PDOIterator($users);
            } catch(\PDOException $pdoe) {
                DBManager::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
                throw new ConfigurationException('Failed to connect to database.',0,$pdoe);
            } catch(QueryException $qe) {
                DBManager::logQueryError(__CLASS__,__FUNCTION__,$qe);
                throw new ConfigurationException('Failed to query users from database.',0,$qe);
            }
        }
    } catch(OutOfBoundsException $oobe) {
        throw new ConfigurationException('Invalid email setting(s).',0,$oobe);
    } catch(InvalidArgumentException $iae) {
        throw new ConfigurationException('Invalid email setting(s).',0,$iae);
    }

}

/**
 * @param $emailSettings
 * @return bool
 */
private function areSettingsValid($emailSettings) {
    $message = '';
    if(empty($emailSettings)) {
        throw new OutOfBoundsException('No settings specified.');
    }
    if(!is_array($emailSettings)) {
        throw new OutOfBoundsException('Expected array, received ' . get_class($emailSettings) . ' Object');
    }
    if(is_null($emailSettings['host'])) {
        $message .= 'Host value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['port'])) {
        $message .= 'Port value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['authUsername'])) {
        $message .= 'Authenticating username value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['authPassword'])) {
        $message .= 'Authenticating password value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['fromEmail'])) {
        $message .= 'From email address value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['subject'])) {
        $message .= 'Subject value cannot be null.' . PHP_EOL;
    }
    if(!empty($message)) {
        throw new OutOfBoundsException($message);
    }

    if($this->isValidDomainName($emailSettings['host'])) {
        if(!filter_var($emailSettings['port'],FILTER_VALIDATE_INT)) {
            $message .= 'Port is not a valid integer. Value provided: ' . var_export(['port'],true) . PHP_EOL;
        }
        if(!filter_var($emailSettings['fromEmail'],FILTER_VALIDATE_EMAIL)) {
            $message .= 'From Email is not a valid email address. Value provided: ' . var_export($emailSettings['fromEmail'],true) . PHP_EOL;
        }
    } else {
        if(filter_var($emailSettings['host'],FILTER_VALIDATE_IP)) {
            if(!filter_var($emailSettings['port'],FILTER_VALIDATE_INT)) {
                $message .= 'Port is not a valid integer. Value provided: ' . var_export(['port'],true) . PHP_EOL;
            }
            if(!filter_var($emailSettings['fromEmail'],FILTER_VALIDATE_EMAIL)) {
                $message .= 'From Email is not a valid email address. Value provided: ' . var_export($emailSettings['fromEmail'],true) . PHP_EOL;
            }
        } else {
            $message = 'Host is not a valid host name or IP address. Value provided: ' . var_export($emailSettings['host'],true) . PHP_EOL;
        }
    }
    if(!empty($message)) {
        throw new InvalidArgumentException($message);
    }
    return true;
}

/**
 * @param $domainName
 * @return int
 */
private function isValidDomainName($domainName) {
    $pattern = ';(?:https?://)?(?:[a-zA-z0-9]+?\.(?:com|net|org|gov|edu|co\.uk));';
    return (preg_match($pattern, $domainName));
}

TemplateConfiguration

private $template;
private $templatePrefix;
private $companyName;
private $projectName;
private $projectId;

/**
 * TemplateConfiguration constructor.
 * @param $templateSettings
 */
public function __construct($templateSettings) {
    try {
        if($this->areSettingsValid($templateSettings)) {
            $this->checkFileExist($templateSettings);
        }
    } catch(FileNotFoundException $fnfe) {
        throw new ConfigurationException('Invalid template name.',0,$fnfe);
    } catch(OutOfBoundsException $oobe) {
        throw new ConfigurationException('Invalid template setting(s).',0,$oobe);
    }
}

/**
 * @param $templateSettings
 */
private function checkFileExist($templateSettings) {
    $path = '../resources/views/emails';
    $templateName = $templateSettings['templateName'];
    if(file_exists("$path/phishing/$templateName")) {
        $this->templatePrefix = 'emails.phishing.';
        $this->setSettings($templateSettings);
    } else if(file_exists("$path/edu/$templateName")) {
        $this->templatePrefix = 'emails.edu.';
        $this->setSettings($templateSettings);
    } else {
        throw new FileNotFoundException("Failed to find template: $templateName");
    }
}

/**
 * @param $templateSettings
 * @return bool
 */
private function areSettingsValid($templateSettings) {
    $message = '';
    if(empty($templateSettings)) {
        throw new OutOfBoundsException('No settings specified.');
    }
    if(!is_array($templateSettings)) {
        throw new OutOfBoundsException('Expected array, received ' . get_class($templateSettings) . ' Object');
    }
    if(is_null($templateSettings['templateName'])) {
        $message .= 'Template Name value cannot be null.' . PHP_EOL;
    }
    if(is_null($templateSettings['companyName'])) {
        $message .= 'Company Name value cannot be null.' . PHP_EOL;
    }
    if(is_null($templateSettings['projectName'])) {
        $message .= 'Project Name value cannot be null.' . PHP_EOL;
    }
    if(is_null($templateSettings['projectId'])) {
        $message .= 'Project ID value cannot be null.' . PHP_EOL;
    }
    if(!empty($message)) {
        throw new OutOfBoundsException($message);
    }
    return true;
}

/**
 * @param $templateSettings
 */
private function setSettings($templateSettings) {
    $this->template = $templateSettings['templateName'];
    $this->companyName = $templateSettings['companyName'];
    $this->projectName = $templateSettings['projectName'];
    $this->projectId = $templateSettings['projectId'];
}

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      *optional*
 *          string                  Subject                 Subject of email                                    *optional*
 * @throws  FailureException                                Thrown from sendEmail() if mail fails to be given to mail server
 */

/**
 * @param \app\EmailConfiguration $emailConfig
 * @param TemplateConfiguration $templateConfig
 * @param $period
 * @throws EmailException
 */
public static function executeEmail(EmailConfiguration $emailConfig, TemplateConfiguration $templateConfig, $period) {
    try {
        foreach($emailConfig->getUsersIterator() as $user) {
            if(self::validateUser($templateConfig->getTemplateComplexityType(),
                $templateConfig->getTemplateTargetType(), $user, $period))
            {
                $urlId = self::getUrlId($user,$templateConfig->getProjectId());

                $headers = array(
                    'companyName'=>$templateConfig->getCompanyName(),
                    'projectName'=>$templateConfig->getProjectName(),
                    'projectId'=>$templateConfig->getProjectId(),
                    'lastName'=>$user['USR_LastName'],
                    'username'=>$user['USR_Username'],
                    'urlId'=>$urlId
                );
                self::sendEmail($templateConfig->getTemplate(),$headers,$user['USR_Email'],
                    $emailConfig->getFromEmail(),$emailConfig->getSubject());

                $projects = array($templateConfig->getProjectName(),$user['USR_ProjectMostRecent'],$user['USR_ProjectPrevious']);
                self::updateUserProjects($projects,$user);
            }
        }
    } catch(\PDOException $pdoe) {
        throw new EmailException('Failed to connect to database.',0,$pdoe);
    } catch(QueryException $qe) {
        throw new EmailException('Failed to query users from database.',0,$qe);
    } catch(FailureException $fe) {
        throw new EmailException('Failed to send email.',0,$fe);
    }
}

/**
 * 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,$projectId) {
    $db = new DBManager();
    if(!is_null($user['USR_UniqueURLId'])) {
        $urlId = $user['USR_UniqueURLId'];
    } else {
        $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           $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!

\$\endgroup\$
2
  • \$\begingroup\$ my own fault for misreading, but did you really have to post the question quickly and not write the docs first, whilst leaving half-complete documentation about? \$\endgroup\$
    – Pimgd
    Jul 14, 2016 at 23:17
  • \$\begingroup\$ @Pimgd: I was trying to get some feedback on the code itself. I was leaving work and wanted to be sure that I had something coming into tomorrow morning to work on. :P \$\endgroup\$
    – Trojan404
    Jul 14, 2016 at 23:26

2 Answers 2

1
\$\begingroup\$

First of all, kudos for progress made to this point. The code looks a lot better to me (and hopefully to you too).

Some general notes:

  • Much better approach to exceptions. The way they might bubble up your call stack seems much more manageable now.
  • Agree with sentiment from comments and other answer that you should post your code review with complete doc blocks. I would urge you to get in the habit of always changing doc blocks as necessary while working within a particular method before moving off on to another area of code.
  • Your intent on whether some of these classes and methods are intended to be used statically is really unclear. To me it looks like this is pretty much all concrete class implementation in that I don't see a lot of general purpose methods being exposed by these classes. You need a very specific set of dependencies instantiated to perform the logic in these classes, so I don't quite see the static use case. If you think of these classes as concrete implementations, you can begin to do things like store email and template config, DBManager, etc. on the actual Mail object so that you don't need to do as much passing around of parameters between your methods. You need to be explicit around your intentions. To me the only good candidates to be static methods here are PhishingController::sendEmail and Mail::executeEmail() as they use no concrete dependencies.

  • Your configuration classes have no getters (or __get() magic method implementation) and all properties are private, so you can't really read data out of them in any meaningful way now. My guess is maybe you didn't show whole classes or they aren't complete, but figured I would mention this.

Since you have cleaned things up so much, we can now start getting into the finer points and actually perform a more proper code review. My notes are with multi-line comments that are left-justified so hopefully easier to read. I am not covering doc blocks.

PhishingController::sendEmail

public function sendEmail(Request $request) {
/*
Use object style access here since this is an object you are working with.
$projectName = $request->projectName;
*/
    $projectName = $request['projectName'];
/*
Can you not modify the Request object to properly separate project id
and name such that this class does not need to understand how to do this?
*/
    $projectId = intval($projectName,strpos($projectName,'_'));
    $projectName = substr($projectName,0,strpos($projectName,'_')-1);
/*
Agree with other comment on bad variable naming here.
'$periodInWeeks' perhaps?
*/    
    $period = 4;

/*
I personally prefer to keep my try-catch blocks as closely wrapped
around the calls that will potentially trigger the exception as possible.
I don't see here why you couldn't put your catches below right after
the respective object instantiations, and perhaps a third around
the 'ExecuteEmail()' call.
*/
    try {
        $templateConfig = new TemplateConfiguration(
            array(
/*
Object style access
*/
                'templateName'=>$request['emailTemplate'],
                'companyName'=>$request['companyName'],
                'projectName'=>$projectName,
                'projectId'=>$projectId
            )
        );

        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request['hostName'],
                'port'=>$request['port'],
                'authUsername'=>$request['username'],
                'authPassword'=>$request['password'],
                'fromEmail'=>$request['fromEmail'],
                'subject'=>$request['subject']
            )
        );

        Email::executeEmail($emailConfig,$templateConfig,$period);
/*
What are you going to do with caught exceptions?
Don't just swallow them unless you have good reason.
If you aren't not going to do anything with them,
perhaps don't catch them and let them bubble up the call stack.
*/
    } catch(ConfigurationException $ce) {

    } catch(EmailException $ee) {

    }
}

EmailConfiguration

public function __construct($emailSettings) {
    try {
/*
I don't like the mixed behavior of this call to areSettingsValid().
If passing validation, you get boolean true, if failing you
get exception.  Since exception is thrown from this method anyway,
there is no reason to return true.  The code could be simplified to:
$this->areSettingsValid($emailSettings);
$this->host = ... continue code
No need for 'if' here at all.


Also, no need to nest try blocks here in this section. Again you may
think about earlier advice on wrapping try/catch as closely to possible.
But in a case like this where you may be executing relatively few lines
of code that may all generate different exception types, and all you
are going to do is wrap the underlying exceptions and chain them anyway,
having just one big block with a common handler should be OK. Your call.
I still think you have some of your DB exception and exception logging
behavior outside of that class where is belongs.
*/
        if($this->areSettingsValid($emailSettings)) {
            $this->host = $emailSettings['host'];
            $this->port = $emailSettings['port'];
            $this->authUsername = $emailSettings['authUsername'];
            $this->authPassword = $emailSettings['authPassword'];
            $this->fromEmail = $emailSettings['fromEmail'];
            $this->subject = $emailSettings['subject'];

            try {
                $db = new DBManager();
                $sql = "SELECT * FROM gaig_users.users;";
                $users = $db->query($sql,array(),array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
                $this->usersIterator = new PDOIterator($users);
            } catch(\PDOException $pdoe) {
                DBManager::logConnectError(__CLASS__,__FUNCTION__,$pdoe->getMessage(),$pdoe->getTrace());
                throw new ConfigurationException('Failed to connect to database.',0,$pdoe);
            } catch(QueryException $qe) {
                DBManager::logQueryError(__CLASS__,__FUNCTION__,$qe);
                throw new ConfigurationException('Failed to query users from database.',0,$qe);
            }
        }
/*
If you are only wrapping and chaining underlying exceptions of different
types here, you can simplify this to a single catch block
for generic Exception type.
I like that you are now abstracting underlying exception details from the
caller to this class.
*/
    } catch(OutOfBoundsException $oobe) {
        throw new ConfigurationException('Invalid email setting(s).',0,$oobe);
    } catch(InvalidArgumentException $iae) {
        throw new ConfigurationException('Invalid email setting(s).',0,$iae);
    }

}


/*
Wanted to again mention concern over mixed pass/fail behavior.
I would suggest making void return and just throwing exceptions in this
method.

There is also a lot of code in this method. It may seem tedious, but you
might consider breaking logical sets of code into more methods like you did
for the domain name validation.

Think about changing method name to validateSettings() or similar
if you have this function return void. One would typically expect
methods names like 'is*' or 'are*' to return boolean.
*/
private function areSettingsValid($emailSettings) {
    $message = '';
    if(empty($emailSettings)) {
        throw new OutOfBoundsException('No settings specified.');
    }
    if(!is_array($emailSettings)) {
/*
Consider just using 'var_export($emailSetting, true)'.
That will give you everything you need to know about the variable.
You are assuming now that you are being sent a bad object,
which may not be the case.
You could also consider combining this with above conditional into single
if block. This more specific messaging covers all use cases.
*/
        throw new OutOfBoundsException('Expected array, received ' . get_class($emailSettings) . ' Object');
    }
/*
Consider taking this section where you validate input array keys are present
into its own method ('validateSettingsKeys($emailSettings)' or similar). That new method could throw the Exception, not this method.


You could probably get this method down to a concise set of code like:

private function areSettingsValid($emailSettings) {
    if(empty($emailSettings) || !is_array($emailSettings) ) {
        throw new OutOfBoundsException(
            'Expected array, received: ' . var_export($emailSettings, true)
        );
    }
    $this->validateSettingsKeys($emailSettings);
    $this->validateSettingsValues($emailSettings);
}
*/
    if(is_null($emailSettings['host'])) {
        $message .= 'Host value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['port'])) {
        $message .= 'Port value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['authUsername'])) {
        $message .= 'Authenticating username value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['authPassword'])) {
        $message .= 'Authenticating password value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['fromEmail'])) {
        $message .= 'From email address value cannot be null.' . PHP_EOL;
    }
    if(is_null($emailSettings['subject'])) {
        $message .= 'Subject value cannot be null.' . PHP_EOL;
    }
    if(!empty($message)) {
        throw new OutOfBoundsException($message);
    }

/*
I don't understand these nested conditionals here.
Why are you checking values multiples times each?

Think about refactoring into various calls like isValidDomain.

You could even go a step farther and wrap all these methods for validating
individual settings values inside a method like 'validateSettingsValues()'
that could then throw the illegal argument exception.
*/
    if($this->isValidDomainName($emailSettings['host'])) {
        if(!filter_var($emailSettings['port'],FILTER_VALIDATE_INT)) {
            $message .= 'Port is not a valid integer. Value provided: ' . var_export(['port'],true) . PHP_EOL;
        }
        if(!filter_var($emailSettings['fromEmail'],FILTER_VALIDATE_EMAIL)) {
            $message .= 'From Email is not a valid email address. Value provided: ' . var_export($emailSettings['fromEmail'],true) . PHP_EOL;
        }
    } else {
        if(filter_var($emailSettings['host'],FILTER_VALIDATE_IP)) {
            if(!filter_var($emailSettings['port'],FILTER_VALIDATE_INT)) {
                $message .= 'Port is not a valid integer. Value provided: ' . var_export(['port'],true) . PHP_EOL;
            }
            if(!filter_var($emailSettings['fromEmail'],FILTER_VALIDATE_EMAIL)) {
                $message .= 'From Email is not a valid email address. Value provided: ' . var_export($emailSettings['fromEmail'],true) . PHP_EOL;
            }
        } else {
            $message = 'Host is not a valid host name or IP address. Value provided: ' . var_export($emailSettings['host'],true) . PHP_EOL;
        }
    }
    if(!empty($message)) {
        throw new InvalidArgumentException($message);
    }
    return true;
}

private function isValidDomainName($domainName) {
/*
You should validate that you do in fact have a non-empty string
before operating against it since this validation does not take
place before calling this method.
*/
    $pattern = ';(?:https?://)?(?:[a-zA-z0-9]+?\.(?:com|net|org|gov|edu|co\.uk));';
    return (preg_match($pattern, $domainName));
}

TemplateConfiguration

public function __construct($templateSettings) {
    try {
/*
No need for if conditional here. Just execute code and catch exceptions.
*/
        if($this->areSettingsValid($templateSettings)) {
            $this->checkFileExist($templateSettings);
        }
    } catch(FileNotFoundException $fnfe) {
        throw new ConfigurationException('Invalid template name.',0,$fnfe);
/*
Consider generalizing to just Exception if you are intending this to
be your default exception chaining message. This makes this method
more resilient against changes in related methods that may introduce
new exception types.  If you would not catch those new types, you would
leak them out to the caller without your wrapper.
*/
    } catch(OutOfBoundsException $oobe) {
        throw new ConfigurationException('Invalid template setting(s).',0,$oobe);
    }
}


private function checkFileExist($templateSettings) {
    $path = '../resources/views/emails';
    $templateName = $templateSettings['templateName'];
/*
I think this is OK if you only have a couple of templates, but
if you expand this you will probably need to refactor code so you
don't have a long string of if-else conditions.
Call to setSettings can be moved after the conditionals.
*/
    if(file_exists("$path/phishing/$templateName")) {
        $this->templatePrefix = 'emails.phishing.';
        $this->setSettings($templateSettings);
    } else if(file_exists("$path/edu/$templateName")) {
        $this->templatePrefix = 'emails.edu.';
        $this->setSettings($templateSettings);
    } else {
        throw new FileNotFoundException("Failed to find template: $templateName");
    }
}

/*
I could probably echo my comment from email configuration class validation
code to here.  This could probably by refactored into more discreet methods.
*/
private function areSettingsValid($templateSettings) {
    $message = '';
/*
Combine these two conditionals using same kind of error messaging from
second conditional and var_export().
*/
    if(empty($templateSettings)) {
        throw new OutOfBoundsException('No settings specified.');
    }
    if(!is_array($templateSettings)) {
        throw new OutOfBoundsException('Expected array, received ' . get_class($templateSettings) . ' Object');
    }
/*
You could refactor this next section into validateSettingsKeys() method
*/
    if(is_null($templateSettings['templateName'])) {
        $message .= 'Template Name value cannot be null.' . PHP_EOL;
    }
    if(is_null($templateSettings['companyName'])) {
        $message .= 'Company Name value cannot be null.' . PHP_EOL;
    }
    if(is_null($templateSettings['projectName'])) {
        $message .= 'Project Name value cannot be null.' . PHP_EOL;
    }
    if(is_null($templateSettings['projectId'])) {
        $message .= 'Project ID value cannot be null.' . PHP_EOL;
    }
    if(!empty($message)) {
        throw new OutOfBoundsException($message);
    }
/*
You are missing value validation in this method. You validate that
settings keys exist and then stop.
*/
/*
Don't return anything to be consistent in usage throughout.
*/
    return true;
}

/*
Not sure how this method works with value validation which as noted above
is missing. You are using a bit of a different approach in setting
property values in this class vs. in the email config class.
This might not be critical, but I would think you would want to take
similar coding approach across related classes in a library.
*/
private function setSettings($templateSettings) {
    $this->template = $templateSettings['templateName'];
    $this->companyName = $templateSettings['companyName'];
    $this->projectName = $templateSettings['projectName'];
    $this->projectId = $templateSettings['projectId'];
}

Email

/*
A stylistic note, but when dealing with longer message signatures
you may find it easies to read if presented with indentations where
you split onto next line. Some perhaps something like:

public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig,
    $period
) {
*/
public static function executeEmail(EmailConfiguration $emailConfig,
TemplateConfiguration $templateConfig, $period) {
/*
Validate $period and probably give it more meaningful name
*/

    try {
/*
Didn't see this getUsersIterator method in your email config class.
Why would an email configuration class provide a caller with an
iterator for working with user objects?
*/
        foreach($emailConfig->getUsersIterator() as $user) {
/*
Similar questions here.
Why would an email class (maybe I am hung up on your class naming here)
have user validation logic? Wouldn't you expect an email class to just get
a collection of valid user objects to work with?
Why would a template configuration object have have methods for
selecting templates (do you need a factory for this)?
Why would an email class generate short URL's? It may need to trigger that,
but why is the method in THIS class?
Should $user be an object? It is treated as associative array
throughout this class which means you are going to likely need to 
add validation throughout to make sure that you have valid data to work
with since you have not established a coding contract here.
*/
           if(self::validateUser($templateConfig->getTemplateComplexityType(),
                $templateConfig->getTemplateTargetType(), $user, $period))
            {
/*
getUrlId is not a static method so not sure why you are calling it
in that context.
*/
                $urlId = self::getUrlId($user,$templateConfig->getProjectId());
/*
These aren't really "headers" are they?  These are data values for injection
into the email right?  Is this variable name meaningful?
*/
                $headers = array(
                    'companyName'=>$templateConfig->getCompanyName(),
                    'projectName'=>$templateConfig->getProjectName(),
                    'projectId'=>$templateConfig->getProjectId(),
                    'lastName'=>$user['USR_LastName'],
                    'username'=>$user['USR_Username'],
                    'urlId'=>$urlId
                );
/*
sendEmail is not a static method so not sure why you are calling
it in that context.
*/
                self::sendEmail($templateConfig->getTemplate(),$headers,$user['USR_Email'],
                    $emailConfig->getFromEmail(),$emailConfig->getSubject());
/*
Should you be using numerically-indexed array here from projects?
Seems like you should either be setting associative array
or perhaps even stdClass object here
*/
                $projects = array($templateConfig->getProjectName(),$user['USR_ProjectMostRecent'],$user['USR_ProjectPrevious']);

/*
not static
*/
                self::updateUserProjects($projects,$user);
            }
        }
    } catch(\PDOException $pdoe) {
        throw new EmailException('Failed to connect to database.',0,$pdoe);
    } catch(QueryException $qe) {
        throw new EmailException('Failed to query users from database.',0,$qe);
/*
Perhaps catch Exception here instead since this seems like
default exception handler for this try block
*/
    } catch(FailureException $fe) {
        throw new EmailException('Failed to send email.',0,$fe);
    }
}

/*
Is this method really named appropriately?
It doesn't seem like you are "validating a user" but rather
DO you need to keep passing around things like template ojects
*/
private function validateUser($templateType,$templateTarget,$user,$period) {
/*
Need try-catch here around DBManager operations?
Can you inject the DBManager instance through the call stack or from
property on object rather than instantiating here?
Several lines too long in this section.
*/
    $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); 
/*
Why does this class have to know to specify PDO constant value here?
Should you not abstract this from user of DBManager class?
Perhaps expose `fetch_assoc()` method that simply calls `fetch()` with
proper constant defined.
*/
   $result = $timestampData->fetch(\PDO::FETCH_ASSOC);
/*
Shouldn't validation of a user's email address happen in the
call stack long before we get to this point?
Why even interact with the database at all if this condition isn't met?
At a minimum you should have this as "Fast fail" validation criteria right
at beginning of method if you can't better define and validate a user object.
*/
    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;
/*
fast fail.  Not understanding doing substring searches to validate
against template types.  This has some code smell to it. But I don't
know enough to comment.
*/
    } else if($templateType == substr($user['USR_ProjectMostRecent'],-5,3) &&
        $templateType == substr($user['USR_ProjectPrevious'],-5,3)) {
        return false;
/*
same as comment above
*/
    } 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;
/*
same as comment above
*/
    } else if($templateType.$templateTarget ==
        substr($user['USR_ProjectMostRecent'],strpos($user['USR_ProjectMostRecent'],'-')+1,4)) {
        return false;
    }
/*
Should you be returning boolean here in this method or would it be
more consistent to throw exceptions like done for your other
validation methods in other classes?
*/
    return true;
}

/*
This method really does two things.  It triggers generation and
return of the URL ID and it sets it in the database.
The method name does not really suggest that. This is pretty short
method, but perhaps factor DB update of value into its own method.
*/
private function getUrlId($user,$projectId) {
/*
Why instantiateDBManager here?
*/
    $db = new DBManager();
/*
Fast fail. Can user be validated upstream?
*/
    if(!is_null($user['USR_UniqueURLId'])) {
        $urlId = $user['USR_UniqueURLId'];
    } else {
/*
If you insist on instantiating DBManager in this method do it here.
Try-catch block?
Why is random_str method in this class?
*/
        $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;
}

/*
Same problem in this method that you had in previous code of sending
in numerically-indexed array as parameters vs. associative arrays or objects.
Very hard to understand meaning here.
Is boolean return needed?
Is any validation of projects or user needed here or up call stack?
*/
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);
}


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

/*
Why here?
*/
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;
}

/*
Bad method name.  You are logging bad email address occurrences here,
not generating any kind of warning.

I don't think it should be the place of this class or method to provide
logging dependencies (i.e. a writable log file).
This class/method should only know what logger it should be writing to.
You may consider creating BadEmailLogger class or similar to store
path configuration, make sure there is writable file and expose a
static method that this class can call like `BadEmailLogger::log($message)`.
That would probably eliminate need for a logging method in this class
altogether as you could just call that static method where this class
method is currently called.
*/
private function badEmailAddressWarning($message) {
/*
Be careful with relative path here. This can be fragile.
*/
    $path = '../storage/logs/badEmailAddress' . date('m-d-Y') . '.log';
    if(!file_exists($path)) {
        $file = fopen($path,'w');
        fclose($file);
    }
    error_log($message,3,$path);
}
\$\endgroup\$
4
  • \$\begingroup\$ In regards to your question about calling non-static classes using self::, $this-> cannot be accessed from a static context so the static calling method must be used. \$\endgroup\$
    – Trojan404
    Jul 18, 2016 at 15:26
  • \$\begingroup\$ The UsersIterator problem that you discuss is something that will not be easily solved. Further discussion on that might be better as its own question or in a chat room. \$\endgroup\$
    – Trojan404
    Jul 18, 2016 at 15:35
  • \$\begingroup\$ @throck95 My concern over static method usage was just one of not understanding why you are trying to work with most of these methods statically in the first place, and the being explicit in your method declarations. \$\endgroup\$
    – Mike Brant
    Jul 18, 2016 at 15:46
  • \$\begingroup\$ For right now I want to try and avoid making a major change with the DBManager implementation. For the next several code reviews it will be instantiated at every mention, however this will change in time. I just want to get more pressing matters with the project answered before I refactor that aspect of it. As of now, I'm not having memory or performance issues so it's not an urgent refactor. \$\endgroup\$
    – Trojan404
    Jul 18, 2016 at 16:31
2
\$\begingroup\$

I completely skipped over the part where you mentioned that you weren't done writing the documentation yet! My mistake. That said, perhaps you'll find some parts of this answer of use when you do have to write the documentation.


Looks like you forgot to update your documentation...

public static function executeEmail(EmailConfiguration $emailConfig, TemplateConfiguration $templateConfig, $period) {

The method signature mentions $emailConfig, $templateConfig and $period. The comments above the method describe $emailSettings, $template and $params. I think you'll be able to cut most of the comments away; but documenting what $period is for would help.

Looking in greater detail at the comments you have...

/**
 * EmailConfiguration constructor.
 * @param $emailSettings
 */
public function __construct($emailSettings) {

Obvious comment, and thus not needed. What's $emailSettings for? That's something I'd like to know, but that part of the commentary is missing. If I go in blind, I'll get an ConfigurationException back... telling me that my $emailSettings are invalid. That's ... not helpful. I guess I could read the nested exception, but it'd be great if you were to describe the constructor arguments in greater detail.

/**
 * @param $emailSettings
 * @return bool
 */
private function areSettingsValid($emailSettings) {

Ah, a validator! Stuff goes in, boolean flag "valid yes no" comes out. Looking at the function name, it seems I'll get true back if the object is valid, so I guess I'll get false back if the object is not valid.

Except you don't.

throw new OutOfBoundsException('No settings specified.');
throw new OutOfBoundsException('Expected array, received ' . get_class($emailSettings) . ' Object');
throw new OutOfBoundsException($message);
throw new InvalidArgumentException($message);
...
return true;

So, either you get back true, or exceptions. ... So what's the boolean for? If the method returns normally, it's always going to be true! What's more, you've got a function signature that seems to suggest non-throwing code, and the comments for that function make no mention of exceptions either!

/**
 * @param $domainName
 * @return int
 */
private function isValidDomainName($domainName) {

This one is weird. isX usually returns a boolean, not int. Either explain in a comment or change the type? It's good that you documented the return type, or I'd have thought it was boolean. ... still, what does a result of 0 mean? Is that good? Bad? Is the domain name valid if I get 0 back?

/**
 * TemplateConfiguration constructor.
 * @param $templateSettings
 */
public function __construct($templateSettings) {

Almost the same as the previous constructor. But this one is a ton better. Imagine I use this one blindly. I put crap in... and I get back...

    throw new ConfigurationException('Invalid template name.',0,$fnfe);

Well, at least I know I did something wrong with the template name. That's a lot better than "your settings are wrong!".

Detailed specification of the input parameters would still be nice, though.

/**
 * @param $templateSettings
 * @return bool
 */
private function areSettingsValid($templateSettings) {

Same thing here, says it returns bool, but throws exceptions instead. Don't be like that.


Non-documentation comments

private function getUrlId($user,$projectId) {
    $db = new DBManager();
    if(!is_null($user['USR_UniqueURLId'])) {
        $urlId = $user['USR_UniqueURLId'];
    } else {
        $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;
}

There's no need to create a DBManager here if the user has a USR_UniqueURLId, so delay creation until needed:

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

$period             Number of weeks

From the documentation, it turns out that $period represents a number of weeks (that some action has to be taken). If you add this to the variable name, like $periodInWeeks, then the meaning of assignments and usages becomes more obvious. Specifically this part in sendEmail:

$period = 4;

} else {
    if(filter_var($emailSettings['host'],FILTER_VALIDATE_IP)) {
        if(!filter_var($emailSettings['port'],FILTER_VALIDATE_INT)) {
            $message .= 'Port is not a valid integer. Value provided: ' . var_export(['port'],true) . PHP_EOL;
        }
        if(!filter_var($emailSettings['fromEmail'],FILTER_VALIDATE_EMAIL)) {
            $message .= 'From Email is not a valid email address. Value provided: ' . var_export($emailSettings['fromEmail'],true) . PHP_EOL;
        }
    } else {
        $message = 'Host is not a valid host name or IP address. Value provided: ' . var_export($emailSettings['host'],true) . PHP_EOL;
    }
}

Here we have an else { if (...) {} } construct. You can use else if here.

\$\endgroup\$
2
  • \$\begingroup\$ You talked about a few of the methods in the Email class. Do you have any suggestions on the best way to extrapolate some of the methods? From past feedback and personal opinion, I'm not sure the 'getURLId()' function fits in the Email class. That said, I'm struggling to find a better place to put it with the structure of my project and the required inputs. \$\endgroup\$
    – Trojan404
    Jul 15, 2016 at 12:42
  • \$\begingroup\$ @throck95 I dunno, would have to look at it in greater detail. Don't have that time atm; you'd be better off raising it as a separate point later on. I can agree with the idea that maybe getting stuff from users doesn't belong in an email class... \$\endgroup\$
    – Pimgd
    Jul 15, 2016 at 12:49

Your Answer

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

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