3
\$\begingroup\$

Continuing on my quest to make sure that my application is developed strong, securely, and efficiently, I've updated my code as suggested in the previous question.

To start, I've implemented a User_test class (to be renamed once it is full implemented and designed). I have not yet added value verification to the constructor.

Any suggestions as to the changes of my code as this project has progressed is appreciated. It is worth noting that I am working on redesigning my database. Therefore, the code may drastically change on the next iteration based on the new design.

User_test

private $id;
private $username;
private $email;
private $firstName;
private $lastName;
private $uniqueURLId;
private $password;
private $mostRecentProject;
private $previousProject;
private $lastProject;

private $date;

/**
 * User_test constructor.
 * @param $user
 */
public function __construct($user)
{
    $this->id = $user['USR_UserId']; //required
    $this->username = $user['USR_Username']; //required
    $this->email = $user['USR_Email']; //required
    $this->firstName = $user['USR_FirstName']; //required
    $this->lastName = $user['USR_LastName']; //required
    $this->uniqueURLId = $user['USR_UniqueURLId'];
    $this->password = $user['USR_Password']; //required
    $this->mostRecentProject = $user['USR_ProjectMostRecent'];
    $this->previousProject = $user['USR_ProjectPrevious'];
    $this->lastProject = $user['USR_ProjectLast'];
}

/**
 * checkURLId
 * Checks if UniqueURLId is null and sets it if it is.
 *
 * @param   int         $projectId          Integer ID referencing specific project to be concatenated onto the URLId
 */
private function checkURLId($projectId) {
    if(is_null($this->uniqueURLId)) {
        $db = new DBManager();
        $this->uniqueURLId = RandomObjectGeneration::random_str(15) . $projectId;
        $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=? WHERE USR_UserId=?;";
        $bindings = array($this->uniqueURLId,$this->id);
        $db->query($sql,$bindings);
    }
}

/**
 * pushUser
 * Pushes $this onto the provided array if it is valid.
 *
 * @param   array                   $validUsers             Array of User_test objects
 * @param   int                     $periodInWeeks          Period to check in validation of user
 * @param   TemplateConfiguration   $templateConfig         Template Configuration for validation
 */
public function pushUser($validUsers, $periodInWeeks, TemplateConfiguration $templateConfig) {
    try {
        $this->checkURLId($templateConfig->getProjectId());
        if($this->isValid($periodInWeeks,$templateConfig)) {
            $validUsers[] = $this;
        }
    } catch(Exception $e) {

    }
}

/**
 * isValid
 * Verifies the user is valid according to the verification algorithm defined in the check... functions.
 *
 * @param   int                     $periodInWeeks          Period to check in validation of user
 * @param   TemplateConfiguration   $templateConfig         Template Configuration for validation
 * @return  bool
 */
private function isValid($periodInWeeks, TemplateConfiguration $templateConfig) {
    try {
        $db = new DBManager();
        $sql = "
            SELECT MAX(SML_SentTimestamp) AS 'timestamp_check' 
            FROM gaig_users.sent_email 
            WHERE SML_UserId = ? AND SML_ProjectName = ?;";
        $bindings = array($this->id,$this->mostRecentProject);
        $data = $db->query($sql,$bindings);
        if($data->rowCount() > 0) {
            $result = $data->fetch();
            $this->date = date('Y-m-d',strtotime('-' . $periodInWeeks . ' weeks')) . ' 00:00:00';
            if($this->checkPeriod($this->date,$result['timestamp_check'])) {
                return true;
            }
            $sql = "SELECT * FROM gaig_users.projects WHERE PRJ_ProjectId = ?;";
            $data = $db->query($sql,array($this->mostRecentProject));
            $mostRecentProj = new Project($data->fetch());
            $newComplexity = $templateConfig->getTemplateComplexityType();
            $newTarget = $templateConfig->getTemplateTargetType();
            if($this->checkMRP($mostRecentProj,$newComplexity,$newTarget)) {
                return false;
            }
            $data = $db->query($sql,array($this->previousProject));
            $previousProj = new Project($data->fetch());
            if($this->checkPP($mostRecentProj,$previousProj,$newComplexity)) {
                return false;
            }
            $data = $db->query($sql,array($this->lastProject));
            $lastProj = new Project($data->fetch());
            if($this->checkLP($mostRecentProj,$previousProj,$lastProj,$newTarget)) {
                return false;
            }
        }
        return true;
    } catch(Exception $e) {
        //unsure how to manage any exceptions thrown yet, if at all. further design to come
    }
}

/**
 * checkPeriod - Verification Algorithm
 * Verifies if the period is outside of periodInWeeks zone.
 *
 * @param   string          $date               Date in format 'Y-m-d h:i:s'
 * @param   string          $timestamp          Date retrieved from PDOStatement
 * @return  bool
 */
private function checkPeriod($date,$timestamp) {
    return $timestamp <= $date;
}

/**
 * checkMRP - Verification Algorithm
 * Checks the Most Recent Project to see if identical.
 *
 * @param   Project         $mrp            Project object representing the Most Recent Project
 * @param   string          $complexity     Complexity type of requested template
 * @param   string          $target         Target type of requested template
 * @return  bool
 */
private function checkMRP(Project $mrp, $complexity, $target) {
    return $complexity == $mrp->getTemplateComplexityType() &&
        $target == $mrp->getTemplateTargetType();
}

/**
 * checkPP - Verification Algorithm
 * Checks the Previous Project and Most Recent Project for identical complexity type.
 *
 * @param   Project         $mrp            Project object representing the Most Recent Project
 * @param   Project         $pp             Project object representing the Previous Project
 * @param   string          $complexity     Complexity type of requested template
 * @return  bool
 */
private function checkPP(Project $mrp, Project $pp, $complexity) {
    return !is_null($pp) &&
        $complexity == $mrp->getTemplateComplexityType() &&
        $complexity == $pp->getTemplateComplexityType();
}

/**
 * checkLP - Verification Algorithm
 * Checks the Last Project, Previous Project, and Most Recent Project for identical target type.
 *
 * @param   Project         $mrp            Project object representing the Most Recent Project
 * @param   Project         $pp             Project object representing the Previous Project
 * @param   Project         $lp             Project object representing the Last Project
 * @param   string          $target         Target type of requested template
 * @return  bool
 */
private function checkLP(Project $mrp, Project $pp, Project $lp, $target) {
    return !is_null($lp) &&
        !is_null($pp) &&
        $target == $mrp->getTemplateTargetType() &&
        $target == $pp->getTemplateTargetType() &&
        $target == $lp->getTemplateTargetType();
}

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

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

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

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

I have not yet designed a UserCollection class. So right now I simply pass an array of User_test objects to the Email class inside of the EmailConfiguration object. This array is already verified as I use the TemplateConfiguration object to verify all users when the EmailConfiguration object is instantiated in PhishingController.

TemplateConfiguration Validation Function

/**
 * getValidUsers
 * Retrieves all users from the database and validates them through the User_test object.
 *
 * @param   array           $returnUsers            Array of User_test objects
 * @param   int             $periodInWeeks          Period to check for instant sending of email
 * @return  array
 */
public function getValidUsers($returnUsers, $periodInWeeks) {
    $db = new DBManager();
    $sql = "SELECT * FROM gaig_users.users;";
    $users = $db->query($sql,array(),array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
    $usersIterator = new PDOIterator($users);
    foreach($usersIterator as $user) {
        $tempUser = new User_Test($user);
        $tempUser->pushUser($returnUsers,$periodInWeeks,$this);
    }
    return $returnUsers;
}

PhishingController Instantiation and Email Execution

/**
 * 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) {
    try {
        $templateConfig = new TemplateConfiguration(
            array(
                'templateName'=>$request->input('emailTemplate'),
                'companyName'=>$request->input('companyName'),
                'projectName'=>$request->input('projectData')['projectName'],
                'projectId'=>intval($request->input('projectData')['projectId'])
            )
        );

        $periodInWeeks = 4;
        $users = array();
        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request->input('hostName'),
                'port'=>$request->input('port'),
                'authUsername'=>$request->input('username'),
                'authPassword'=>$request->input('password'),
                'fromEmail'=>$request->input('fromEmail'),
                'subject'=>$request->input('subject'),
                'users'=>$templateConfig->getValidUsers($users,$periodInWeeks)
            )
        );

        Email::executeEmail($emailConfig,$templateConfig);
    } catch(ConfigurationException $ce) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    } catch(EmailException $ee) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    }
}

Email

private static $templateConfig;
private static $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
 * @throws  EmailException                                      Custom Exception to embody any exceptions thrown in this class
 */
public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig)
{
    self::setTemplateConfig($templateConfig);
    self::setEmailConfig($emailConfig);

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

/**
 * 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(self::$templateConfig->getProjectName(),
        $user['USR_ProjectMostRecent'],
        $user['USR_ProjectPrevious'],
        $user['USR_Username']
    );
    $db->query($sql,$bindings);
}

/**
 * sendEmail
 * Sends them an email to the specified user.
 *
 * @param   User_test           $user           User object
 * @throws  FailureException
 */
private static function sendEmail($user) {
    $templateData = array(
        'companyName'=>self::$templateConfig->getCompanyName(),
        'projectName'=>self::$templateConfig->getProjectName(),
        'projectId'=>self::$templateConfig->getProjectId(),
        'lastName'=>$user->getLastName(),
        'username'=>$user->getUsername(),
        'urlId'=>$user->getUniqueURLId()
    );
    $subject = self::$emailConfig->getSubject();
    $from = self::$emailConfig->getFromEmail();
    $to = $user->getEmail();
    $mailResult = Mail::send(
        ['html' => self::$templateConfig->getTemplate()],
        $templateData,
        function($m) use ($from, $to, $subject) {
            $m->from($from);
            $m->to($to)
                ->subject($subject);
        }
    );
    if(!$mailResult) {
        throw new FailureException('Email failed to send to ' . $to . ', from ' . $from);
    }
}

private static function setTemplateConfig(TemplateConfiguration $templateConfig) {
    self::$templateConfig = $templateConfig;
}

private static function setEmailConfig(EmailConfiguration $emailConfig) {
    self::$emailConfig = $emailConfig;
}

I have also extrapolated the random_str function to a library class.

Libraries

class RandomObjectGeneration
{
/**
 * 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
 */
public static function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
    if(is_null($length) || !is_numeric($length)) {
        throw new Exception();
    }
    $str = '';
    $max = mb_strlen($keyspace) - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}
}
\$\endgroup\$
3
\$\begingroup\$

I have added thoughts within multi-line comments below.

User_test

/*
I know you mentioned that you need to add validation here,
which I agree with.

I don't really like the approach of having a DB query outside of this class
to read this information and then sending to this class to instantiate.
Why not have that query here in this class so that, given DB connection
as dependency, this class has full ability to set up the user, rather
than having some other logic in the application to do this? Why should
some other part of the code need to know things like what table the user
data is stored in, what the schema for that table looks like, etc.

Should DBManager be passed to constructor as dependency since it is used
in other methods?

Just try to be consistent as to whether this class is supposed to interact
with database or not. It seems odd to mix approaches here. If the intent
is to handle ALL database access for users through this class (i.e. this
class IS the model), then it should truly handle ALL access including
getting the user info which is currently being passed to constructor.

This model seems really unclear in terms of how projects are handled.
Should projects (and/or a collection of projects) have their own object
definitions?  At a minimum, should this be an array of projects rather
than previous, most recent, last (which seems unclear as to what the
difference between these is anyway). An ordered array would let you 
get to whatever project you were interested in. Also, should there not
be 1 to 1 relationship between projects and unique URL's since they
are based on project ID's?  Here it seems like the user object can
have multiple projects, but only one unique URL, which doesn't make
sense to me.
*/
public function __construct($user)
{
    $this->id = $user['USR_UserId']; //required
    $this->username = $user['USR_Username']; //required
    $this->email = $user['USR_Email']; //required
    $this->firstName = $user['USR_FirstName']; //required
    $this->lastName = $user['USR_LastName']; //required
    $this->uniqueURLId = $user['USR_UniqueURLId'];
    $this->password = $user['USR_Password']; //required
    $this->mostRecentProject = $user['USR_ProjectMostRecent'];
    $this->previousProject = $user['USR_ProjectPrevious'];
    $this->lastProject = $user['USR_ProjectLast'];
}


/*
Is this method named properly?  It does more than check, so perhaps it should
be called setURLIdForProjectId().
It seems unclear to me in this model if a user could have multiple URL's in
case where they have multiple project ID's. If this is not the case,
what value does projectID even have with regards to this URL?
This method only considers happy path.  What if query fails (for example if
all URL's in database must be unique and generated string matches existing
URL in database)?
*/
private function checkURLId($projectId) {
    if(is_null($this->uniqueURLId)) {
        $db = new DBManager();
        $this->uniqueURLId = RandomObjectGeneration::random_str(15) . $projectId;
        $sql = "UPDATE gaig_users.users SET USR_UniqueURLId=? WHERE USR_UserId=?;";
        $bindings = array($this->uniqueURLId,$this->id);
        $db->query($sql,$bindings);
    }
}

/*
Not a very good method name. This method doesn't return anything and
isn't operating against $validUsers by reference so it is really unclear
to me what you are intending to do.
*/
public function pushUser($validUsers, $periodInWeeks, TemplateConfiguration $templateConfig) {
    try {
        $this->checkURLId($templateConfig->getProjectId());
        if($this->isValid($periodInWeeks,$templateConfig)) {
            $validUsers[] = $this;
        }
    } catch(Exception $e) {
/*
Either do something meaningful here or don't catch. Right now you are silently
swallowing this exception.
*/
    }
}

/*
Method name does not seem useful. What does "isValid" mean? You are not
validating that the user is valid here right? But rather that the user
is eligible for some email to be sent.  I would think User::isValid() 
method would tell you if the user itself is valid.

My worry is that you are beginning to pollute the user object with methods, knowledge
that is should not have.  Why would the User class need to read through
a template object to understand if the user needs to get a mailing?
I would think this would live in a class that prepares a list of users for
email send, not within user class itself.

Pass DBManager to object as dependency (probably in constructor)
*/
private function isValid($periodInWeeks, TemplateConfiguration $templateConfig) {
    try {
        $db = new DBManager();
        $sql = "
            SELECT MAX(SML_SentTimestamp) AS 'timestamp_check' 
            FROM gaig_users.sent_email 
            WHERE SML_UserId = ? AND SML_ProjectName = ?;";
        $bindings = array($this->id,$this->mostRecentProject);
        $data = $db->query($sql,$bindings);
        if($data->rowCount() > 0) {
            $result = $data->fetch();
/*
Why do you need to set date on object? If you are going to do this,
at least use more descriptive property name.
*/
            $this->date = date('Y-m-d',strtotime('-' . $periodInWeeks . ' weeks')) . ' 00:00:00';
            if($this->checkPeriod($this->date,$result['timestamp_check'])) {
                return true;
            }
/*
Free the query resource somewhere. You just fetch(), read out the only record
and then leave the resource open.
*/
/*
If you already had all of user's project information loaded into object,
then you could avoid this following query here.
*/
            $sql = "SELECT * FROM gaig_users.projects WHERE PRJ_ProjectId = ?;";
            $data = $db->query($sql,array($this->mostRecentProject));
/*
Ah so you are thinking about having Project be it's own class. Good.
But, why would you just not load all project info into user object at
instantiation if you feel you are always going to need to information?
You could lazy load as done here,but you are potentially allowing
caller to call against methods that don't have all appropriate dependencies
established.

This is only happy path here.
*/
            $mostRecentProj = new Project($data->fetch());
            $newComplexity = $templateConfig->getTemplateComplexityType();
            $newTarget = $templateConfig->getTemplateTargetType();
            if($this->checkMRP($mostRecentProj,$newComplexity,$newTarget)) {
                return false;
            }
            $data = $db->query($sql,array($this->previousProject));
            $previousProj = new Project($data->fetch());
            if($this->checkPP($mostRecentProj,$previousProj,$newComplexity)) {
                return false;
            }
            $data = $db->query($sql,array($this->lastProject));
            $lastProj = new Project($data->fetch());
            if($this->checkLP($mostRecentProj,$previousProj,$lastProj,$newTarget)) {
                return false;
            }
        }
        return true;
    } catch(Exception $e) {
        //unsure how to manage any exceptions thrown yet, if at all. further design to come
    }
}

/*
Kind of a trivial method here if all you are doing is comparing dates and
not doing any input validation or similar. Not sure that this method should
exist here unless it is used multiple places. I think you already have
input validation for these values further up call stack, so don't know
if it even makes sense to add that here. Put comparison in parenthesis.
return ($timestamp <= $date);
*/
private function checkPeriod($date,$timestamp) {
    return $timestamp <= $date;
}

/*
Do you need to think about name here?  What is MRP?

Seems off to validate a project here in User class.

Put comparison in parenthesis.
*/
private function checkMRP(Project $mrp, $complexity, $target) {
    return $complexity == $mrp->getTemplateComplexityType() &&
        $target == $mrp->getTemplateTargetType();
}

/*
Again, why is project validation in a user class?

Again, put comparison in parenthesis.
*/
private function checkPP(Project $mrp, Project $pp, $complexity) {
    return !is_null($pp) &&
        $complexity == $mrp->getTemplateComplexityType() &&
        $complexity == $pp->getTemplateComplexityType();
}

/*
Same comments as for checkPP()
*/
private function checkLP(Project $mrp, Project $pp, Project $lp, $target) {
    return !is_null($lp) &&
        !is_null($pp) &&
        $target == $mrp->getTemplateTargetType() &&
        $target == $pp->getTemplateTargetType() &&
        $target == $lp->getTemplateTargetType();
}

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

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

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

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

TemplateConfiguration Validation Function

/*
What class is this a method on? TemplateConfiguration class?
Why would this functionality exist in that class?
You might consider a class whose specific purpose is to build the list
of recipients given all necessary dependencies.

Pass DBManager dependency to class or method.
*/
public function getValidUsers($returnUsers, $periodInWeeks) {
    $db = new DBManager();
/*
Don't use SELECT *. It is a terrible habit.
*/
    $sql = "SELECT * FROM gaig_users.users;";
/*
Don't leak PDO implementation outside of DBManager.  Why does template
class need to know about how to instantiate user objects?
*/
    $users = $db->query($sql,array(),array('\PDO::ATTR_CURSOR'),array('\PDO::CURSOR_SCROLL'));
    $usersIterator = new PDOIterator($users);
    foreach($usersIterator as $user) {
        $tempUser = new User_Test($user);
/*
As noted in other code comments, this pushUser method is not working as you expect
*/
        $tempUser->pushUser($returnUsers,$periodInWeeks,$this);
    }
    return $returnUsers;
}

PhishingController Instantiation and Email Execution

/*
It seems like PhishingContrller is most logical place to do dependency
set up, as here you are reading request, instantiating templates, etc.
Perhaps this is place where you need to instantiate your additional
dependencies such as user collection.

I would suggest that perhaps you break this sendEmail method up. I don't
see a constructor for this class, but you might want to think about a
structure such as
public function __construct(Request $request) {
    // Handle the request.
    // This method could set dependencies on object such as
    // template and email config
    this.handleRequest($request);

    // instantiate DB connection
    // this controller can then pass this dependency to classes
    // functions it calls that need it
    this.db = new DBManager();

    // set time period. Right now you hard code this but perhaps it
    // should be passed to class or derived fromn request
     this.periodInWeeks = 4;

    // get user list
    this.users = ...; // some class/method that builds your user list
}

public function sendEmail() {
    // pass all dependencies here to static executeEmail method
    // of email class
    Email::executeEmail(
        this.emailConfig,
        this.templateConfig,
        this.users
    );
}
*/
public function sendEmail(Request $request) {
    try {
        $templateConfig = new TemplateConfiguration(
            array(
                'templateName'=>$request->input('emailTemplate'),
                'companyName'=>$request->input('companyName'),
                'projectName'=>$request->input('projectData')['projectName'],
                'projectId'=>intval($request->input('projectData')['projectId'])
            )
        );

        $periodInWeeks = 4;
        $users = array();
        $emailConfig = new EmailConfiguration(
            array(
                'host'=>$request->input('hostName'),
                'port'=>$request->input('port'),
                'authUsername'=>$request->input('username'),
                'authPassword'=>$request->input('password'),
                'fromEmail'=>$request->input('fromEmail'),
                'subject'=>$request->input('subject'),
                'users'=>$templateConfig->getValidUsers($users,$periodInWeeks)
            )
        );

        Email::executeEmail($emailConfig,$templateConfig);
    } catch(ConfigurationException $ce) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    } catch(EmailException $ee) {
        //will be doing something here - what still has yet to be defined (likely just log the exception)
    }
}

Email

/*
I would try to pass the user list as argument to this method as noted above.

Maybe name should be executeEmailToUserList() or something more specific
*/
public static function executeEmail(
    EmailConfiguration $emailConfig,
    TemplateConfiguration $templateConfig)
{
    self::setTemplateConfig($templateConfig);
    self::setEmailConfig($emailConfig);


    try {
        foreach($emailConfig->getUsers() as $user) {
            self::sendEmail($user);
/*
I would suggest that updateUserProjects should be method on user class,
such that you would change this to:
$user->updateUserProjects()
*/
            self::updateUserProjects($user);
        }
    } catch(Exception $e) {
        throw new EmailException(__CLASS__ . ' Exception',0,$e);
    }
}

/*
Move this method to user class, as it has nothing to do with email sending.
Parameter passed to this method probably need to change so as to 
provide to this method the information in needs about the project. 
Perhaps pass Project object?
*/
private function updateUserProjects($user) {
    $db = new DBManager();
/*
Line is too long.  A suggestion on writing readable SQL might be to do:
$sql = "
    UPDATE gaig_users.users
    SET USR_ProjectMostRecent=?,
        USR_ProjectPrevious=?, 
        USR_ProjectLast=?
    WHERE USR_Username=?
";
I also find heredoc/nowdoc syntax helpful for more complex queries.
*/
    $sql = "UPDATE gaig_users.users SET USR_ProjectMostRecent=?, USR_ProjectPrevious=?, 
            USR_ProjectLast=? WHERE USR_Username=?;";
    $bindings = array(self::$templateConfig->getProjectName(),
        $user['USR_ProjectMostRecent'],
        $user['USR_ProjectPrevious'],
        $user['USR_Username']
    );
    $db->query($sql,$bindings);
}


private static function sendEmail($user) {    
    $templateData = array(
        'companyName'=>self::$templateConfig->getCompanyName(),
        'projectName'=>self::$templateConfig->getProjectName(),
        'projectId'=>self::$templateConfig->getProjectId(),
        'lastName'=>$user->getLastName(),
        'username'=>$user->getUsername(),
        'urlId'=>$user->getUniqueURLId()
    );
    $subject = self::$emailConfig->getSubject();
    $from = self::$emailConfig->getFromEmail();
/*
You getter name on user class might lead to some confusion here.
getEmailAddress?
*/
    $to = $user->getEmail();
    $mailResult = Mail::send(
        ['html' => self::$templateConfig->getTemplate()],
        $templateData,
        function($m) use ($from, $to, $subject) {
/*
Don't mix method chaining here. I would either chain them all (if possible)
or chain none of them.
*/
            $m->from($from);
            $m->to($to)
                ->subject($subject);
        }
    );
    if(!$mailResult) {
        throw new FailureException('Email failed to send to ' . $to . ', from ' . $from);
    }
}

/*
Probably need similar function here for setting user collection
*/
private static function setTemplateConfig(TemplateConfiguration $templateConfig) {
    self::$templateConfig = $templateConfig;
}

private static function setEmailConfig(EmailConfiguration $emailConfig) {
    self::$emailConfig = $emailConfig;
}

RandomObjectGeneration

class RandomObjectGeneration
{

/*
Consider placing default "keyspace" value as constant on class.
Method signature might then look like:
public static function random_str($length,
    $keyspace = RandomObjectGeneration::KEYSPACE)
*/
public static function random_str($length, $keyspace = '0123456789abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ')
{
/*
You should validate that $length is positive integer, not just numeric.
You don't want a values like 3.1415926, 0, -50, etc. passed in here.
These would all pass your current validation.
*/
    if(is_null($length) || !is_numeric($length)) {
/*
Give meaningful exception message.
*/
        throw new Exception();
    }
/*
Validate $keyspace as well. Non-zero length string as minimum validation.
*/
    $str = '';
    $max = mb_strlen($keyspace) - 1;
    for ($i = 0; $i < $length; ++$i) {
        $str .= $keyspace[random_int(0, $max)];
    }
    return $str;
}
}
\$\endgroup\$
  • \$\begingroup\$ The uniqueURLId was a complete screw-up in my implementation. The URL is to be unique and only generated once. The project ID is to be concatenated onto the end of the URL whenever the email is generated and sent. This is to identify the user and identify the project without exposing too much to the client. \$\endgroup\$ – Trojan404 Jul 26 '16 at 17:28
  • \$\begingroup\$ As far as a constructor for PhishingController, there isn't one. PhishingController is just that, a controller that holds methods that get called from a routes file. When a URL is hit, the routes reads which function to call and then calls it. This means preloading dependency set-up is near impossible unless you call it as a private method on every single function in the controller. \$\endgroup\$ – Trojan404 Jul 26 '16 at 17:37
  • \$\begingroup\$ "This means preloading dependency set-up is near impossible". Not sure I follow this. Right now you are setting up dependencies in your PhishingController sendEmail method. I am suggesting taking at a step farther and instantionating yuour DB dependency here (if you don't have a way to pass it from router, instantiating your user list here, etc. If you are not using this a concrete class, make sure your methods are static and forget about my suggestion to put logic in a constructor, but I still think you can set up your dependencies in the main method here and pass them to email class. \$\endgroup\$ – Mike Brant Jul 26 '16 at 17:41
  • \$\begingroup\$ I agree that I need to move most of the instantiation to this class, I'm only saying that I need to be careful about implementation. With it being a Controller that just gets statically called from a routes file, I don't want to overextend the dependency declaration here because I started this whole CR trying to pull it out of this controller. Was more a note to readers and myself when I return for further review to be careful on implementation than a question to you. \$\endgroup\$ – Trojan404 Jul 26 '16 at 17:44

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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