Email Controller

Question

I'm developing a Social Engineering Awareness Training Application. This is the focus of my thesis for my undergraduate degree. This will be a multi-part review request, however, if you want to see the entire application, it can be found on GitHub. For this request, I'm looking to see how my EmailController is set up and how efficient you think it might be. I open to any and all suggestions about any facet of the code. This will be a long request as I couldn't figure out how to split up a lot of the logic into multiple requests without meaning being lost.

Keep in mind that this application is nearly to testing, however, there are a few pieces still left to do (a few templates, a few views, documentation).

EmailController

/**
 * 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 static function sendPhishingEmail(Request $request)
{
    if(Auth::check()) {
        $fromEmail = Campaign_Email_Addresses::where('EmailAddress',$request->input('fromEmailText'))->first();
        $template = Template::where('FileName',$request->input('templateText'))->first();
        $campaign = Campaign::where('Id',$request->input('campaignText'))->first();
        if(!empty($fromEmail) && !empty($template) && !empty($campaign)) {
            putenv("MAIL_USERNAME=$fromEmail->EmailAddress");
            putenv("MAIL_NAME=$fromEmail->Name");
            $cryptor = new Cryptor();
            $password = $cryptor->decrypt($fromEmail->Password);
            putenv("MAIL_PASSWORD=$password");
            $templateClass = "\\App\\Mail\\$template->Mailable";
            $sendingChoice = $request->input('sendingChoiceRadio');
            if($sendingChoice == 'user') {
                $user = Mailing_List_User::where('Id',$request->input('userIdText'))->first();
                if(!empty($user)) {
                    Mail::to($user->Email,$user->FirstName . ' ' . $user->LastName)
                        ->send(new $templateClass($user,$campaign,$request->input('companyText')));
                    self::logSentEmail($user,$campaign);
                }
            } else {
                $group = MLU_Departments::where('Id',$request->input('groupIdText'))->first();
                if(!empty($group)) {
                    $bridge = Mailing_List_User_Department_Bridge::where('DepartmentId',$group->Id)->get();
                    foreach($bridge as $pair) {
                        $user = Mailing_List_User::where('Id',$pair->UserId)->first();
                        if(!empty($user)) {
                            Mail::to($user->Email,$user->FirstName . ' ' . $user->LastName)
                                ->send(new $templateClass($user,$campaign,$request->input('companyText')));
                            self::logSentEmail($user,$campaign);
                        }
                    }
                }
            }
        }
    }
    return redirect()->route('generatePhish');
}

public static function sendNewAccountEmail(User $user, $password) {
    if(Auth::adminCheck()) {
        Mail::to($user->Email,$user->FirstName . ' ' . $user->LastName)
            ->send(new NewUser($user,$password));
    }
}

public static function sendTwoFactorEmail(User $user, $code) {
    Mail::to($user->Email,$user->FirstName . ' ' . $user->LastName)
        ->send(new TwoFactorCode($user,$code));
}

/**
 * logSentEmail
 * Logs to sent_email table info about this email and associated recipient.
 *
 * @param   Mailing_List_User           $user
 */
private static function logSentEmail(Mailing_List_User $user, Campaign $campaign) {
    Sent_Mail::create(
        ['UserId'=>$user->Id,
            'CampaignId'=>$campaign->Id,
            'Timestamp'=>Carbon::now()]
    );
}

Cryptor Library

protected $method = 'AES-256-CTR';
private $key;

protected function iv_bytes()
{
    return openssl_cipher_iv_length($this->method);
}

public function __construct($key = false, $method = false)
{
    if(!$key) {
        $key = file_get_contents('../../' . getenv('CRYPTOR_SECRET_KEY'));
    }
    if(ctype_print($key)) {
        $this->key = openssl_digest($key, 'SHA256', true);
    } else {
        $this->key = $key;
    }
    if($method) {
        if(in_array($method, openssl_get_cipher_methods())) {
            $this->method = $method;
        } else {
            die(__METHOD__ . ": unrecognised encryption method: {$method}");
        }
    }
}

public function encrypt($data)
{
    $iv = openssl_random_pseudo_bytes($this->iv_bytes());
    $encrypted_string = bin2hex($iv) . openssl_encrypt($data, $this->method, $this->key, 0, $iv);
    return $encrypted_string;
}

public function decrypt($data)
{
    $iv_strlen = 2 * $this->iv_bytes();
    if(preg_match("/^(.{" . $iv_strlen . "})(.+)$/", $data, $regs)) {
        list(, $iv, $crypted_string) = $regs;
        $decrypted_string = openssl_decrypt($crypted_string, $this->method, $this->key, 0, hex2bin($iv));
        return $decrypted_string;
    }
    return false;
}

Campaign Email Addresses Model

protected $table = 'campaign_email_addresses';

protected $primaryKey = 'EmailAddress';

public $incrementing = false;

protected $fillable = ['EmailAddress',
    'Name',
    'Password'];

public static function insertEmail($email, $name, $password) {
    $cryptor = new Cryptor();
    $encrypted = $cryptor->encrypt($password);
    unset($password);
    $query = self::where('EmailAddress',$email)->first();
    if(count($query)) {
        throw new DuplicateKeyException("Email Address already exists.");
    }
    return self::create([
        'EmailAddress'=>$email,
        'Name'=>$name,
        'Password'=>$encrypted
    ]);
}

public static function updateEmail($email, $name, $password) {
    $cryptor = new Cryptor();
    $encrypted = $cryptor->encrypt($password);
    unset($password);
    $query = self::query();
    $query->where('EmailAddress',$email);
    $query->update(['Password'=>$encrypted,'Name'=>$name]);
    return $query->get();
}

public static function decryptPassword($email) {
    $cryptor = new Cryptor();
    $password = self::where('EmailAddress',$email)->first()->Password;
    return $cryptor->decrypt($password);
}

Template Model

NOTE: Templates are all stored in the same location, database is just to display available templates to user. Emails are sent using Mailables.

protected $table = 'templates';

protected $fillable = ['EmailType',
    'FileName',
    'PublicName',
    'Mailable'
];

protected $primaryKey = 'FileName';

public $incrementing = false;

Mailing List User Model

protected $table = 'mailing_list';

protected $primaryKey = 'Id';

protected $fillable =
    ['Email',
        'FirstName',
        'LastName',
        'UniqueURLId'
    ];

public static function updateMailingListUser($mlu, $email, $fname, $lname, $uniqueURLId = '') {
    $query = Mailing_List_User::query();
    $query->where('Id',$mlu->Id);
    $update = array();

    if(!empty($email)) {
        $update['Email'] = $email;
    }
    if(!empty($fname)) {
        $update['FirstName'] = $fname;
    }
    if(!empty($lname)) {
        $update['LastName'] = $lname;
    }
    if(!empty($uniqueURLId)) {
        $update['UniqueURLId'] = $uniqueURLId;
    }

    $query->update($update);
    $query->get();
}

Mailing List Departments Model

protected $table = 'mailing_list_departments';

public $timestamps = false;

protected $primaryKey = 'Id';

protected $fillable =
    ['Department'];

Mailing List User and Department Bridge Model

protected $table = 'mailing_list_users_departments_bridge';

protected $primaryKey = ['UserId','DepartmentId'];
public $incrementing = false;

public $timestamps = false;

use CompositeKeyTrait;

protected $fillable =
    ['UserId',
        'DepartmentId'
    ];

Campaign Model

protected $table = 'campaigns';

protected $primaryKey = 'Id';

protected $fillable = ['Name',
    'Description',
    'Assignee',
    'Status'];

public static function updateCampaign($campaign,$description,$assignee,$status) {
    $query = Campaign::query();
    $query->where('Id',$campaign->Id);
    $update = array();
    if(!empty($description)) {
        $update['Description'] = $description;
    }
    if(!empty($assignee)) {
        $update['Assignee'] = $assignee;
    }
    if(!empty($status)) {
        $update['Status'] = $status;
    }

    $query->update($update);
    $query->get();
}

public static function getAllActiveCampaigns() {
    return Campaign::where('Status','active')->get();
}

Sent Mail Model

protected $table = 'sent_email';

public $timestamps = false;

protected $primaryKey = 'Id';

protected $fillable = ['UserId',
    'CampaignId',
    'Timestamp'];

Answer 1

You do not get bonus points for cramming as many operations on a single line as possible. Strive to make you code more readable and you will find that your end up with code that is easier to read and maintain, with fewer bugs.

I find your code very hard to read because it is so densely packed, with long lines of code, minimal vertical whitespace, no comments, and inconsistent and intermingled use of camelCase, snake_case, StudlyCaps, and Title_Case.

I would strongly suggest you familiarize yourself with PHP Standards Recommendations, particularly PSR-1 and PSR-2 for what are the most widely accepted styles for use in PHP.

I would also suggest using a style checker to enforce whatever style you settle on.

Also, why use PHPDoc blocks only in certain places? Be consistent in usage.

---

if(Auth::check()) { ... }

You have basically your entire method logic inside this if conditional. You should think about inverting your conditions in cases like this to exit early under failure condition and remove unnecessary code nesting. It is generaly always a good idea to fail out of your method/function calls as early as possible, whether that be with a return or an exception. In this case, it might be better to do something like:

if(!Auth::check()) {
    // throw exception or return
}
// rest of method logic

The same can be said for this subsequent conditional that has no else condition:

if(!empty($fromEmail) && !empty($template) && !empty($campaign)) {

---

I really do not understand why you would be setting variables that only have meaning in an execution context to your environment (your calls using putenv()). Do you really want to change environmental variables every time you send an email?

---

$password = $cryptor->decrypt($fromEmail->Password);

How secure is a password if you can programmatically decrypt it? Typically passwords utilize one-way encryption. Make sure you really have valid reason for two-way encryption here.

To make matters worse, you then introduce the decrypted password into an environment variable making it available to any subsequent code being executed during this request execution. This is really poor from a security standpoint.

---

Is this:

$templateClass = "\\App\\Mail\\$template->Mailable";

really more readable than this?

$templateClass = '\App\Mail\' . $template->Mailable;

Your current version is ambiguous, as it is unclear to the reader if the intent is what I have shown above or this:

$templateClass = '\App\Mail' . $template . '->Mailable';

If you insist on using double quotes for this I would think you should add {} to remove ambiguity, like:

$templateClass = "\\App\\Mail\\{$template->Mailable}";

I would suggest that perhaps the \App\Mail\' portion could even be designed away through proper use of the use statement (though you are not showing your full code).

---

if($sendingChoice == 'user') {

I would suggest getting in the habit of using exact comparisons (===, !==) by default rather than loose comparisons. This helps make your code less fragile to unexpected truthy/falsey values. I find myself using loose comparison very rarely, and when I do, I typically add a comment in the code as to why the loose comparison makes sense in that case.

I also think this conditional, which appears to switch between sending to individual use and group presents a good opportunity for refactoring. I would suggest that each side of this conditional might be a method on the class like sendToUser and sendToGroup.

---

protected function iv_bytes()
{
    return openssl_cipher_iv_length($this->method);
}

public function __construct($key = false, $method = false)
{

I would consider it best practice to always have your constructor as the first method within your class. I typically like to arrange all public methods first, then protected/private methods.

---

file_get_contents('../../' . getenv('CRYPTOR_SECRET_KEY'));

Relative path references like this can tend to add fragility to your application. If you are storing this file reference in environment (a good approach I think), why not store the full path reference?

file_get_contents(getenv('CRYPTOR_SECRET_KEY_PATH'));

What happens here if file is missing or can't be read? Beware of only considering happy path.

---

In your crypto class, you have one die() outcome that I question. You might consider throwing an exception in cases like this instead, so that failure can be bubble up call stack to logic that is better suited to provide end user messaging. This is a cryptography class, not a class designed for user output, it should only do cryptography.

---

protected $table = 'campaign_email_addresses';

protected $primaryKey = 'EmailAddress';

public $incrementing = false;

protected $fillable = ['EmailAddress',
    'Name',
    'Password'];

My guess is that these values are to be treated as immutable for this class. Perhaps these should be treated as class constants?

Same for all other model classes.

---

You have some redundant naming in class model method names. Why use update* when the * is already explictly defined in the class name? This is particularly important in model classes where you might want to override methods defined in an abstract base model, or make sure all inheriting classes implement a method defined in base model as abstract.

For me I would like your base model class to have methods defined in it like:

// a method that must be implemented in all inheriting classes
public abstract function update() {}

// a method with concrete implementation that can be overridden in inheriting class
public function getById($id) {
    // validate id as int
    // build query to show "visible" database fields
    $query = 'SELECT' .  [visible fields defined in inheriting class] .
             'FROM' .  [table as defined in inheriting class] .
             'WHERE' .  [primary key as defined in inheriting class] . '= ?';
    // execute prepared statement
}

You are missing an opportunity to leverage the power of inheritance.

---

Many of your public class methods contain no validation whatsoever, and you can get as far as attempting to make the (relatively) expense call to the database with what might be invalid data. At a minimum, you should consider basic validation checks (non-zero-length string, integers, etc.) before working with data passed through public interfaces. Preference should be given toward type-hinting parameters where appropriate (you are doing some of this), and in cases like email addresses, perhaps actual format validations.

---

You have similar a problem with inconsistent and intermingled use of camelCase, snake_case, StudlyCaps, and Title_Case in naming your database objects (tables and columns)

Since typically database objects such as these are case-insensitive in usage, I would recommend simply sticking with snake_case in naming all database objects.

This is more or less a de facto standard in professional DB development.

---

I don't really see much in the way of error handling here. Most of your code seems to be pretty much happy path oriented as noted above with lack of validation on parameters being passed, but perhaps more importantly lacking in your interactions with the database and file system. You seem to just assume all your queries will work - for example, that a record or records for a given search criteria will always be returned - or that the file will be in expected location and readable.

This will make your code very fragile and very hard to maintain and debug.

You should strive to know every possible result that could happen from calling a function or method (various values and value types that can be returned, whether exceptions can be thrown, etc.) and make sure you handle those cases appropriately.