5
\$\begingroup\$

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'];
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

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.

\$\endgroup\$
5
  • \$\begingroup\$ "Also, why not use PHPDoc blocks only in certain places? Be consistent in usage." As stated in the OP, Documentation is not yet complete. \$\endgroup\$
    – Trojan404
    Feb 9, 2017 at 18:44
  • \$\begingroup\$ putenv: Unless you have an alternate suggestion, that was the result of a weeks worth of research. Laravel Mail uses the .env file to authenticate to mail servers. So, modifying the .env for that specific email call was the only solution that I was able to find. Furthermore, the 2 way encryption was designed in that this password is REQUIRED for authentication to a third party email server. So it has to be able to be encrypted and decrypted then passed to the authentication, which is the .env. \$\endgroup\$
    – Trojan404
    Feb 9, 2017 at 18:46
  • \$\begingroup\$ As far as Model structure, that's how Laravel models work. The Model class has underlying methods that require these items to be set otherwise the Model does not work. Without primaryKey, it will treat a field called Id in your database as the primary. Without incrementing it will assume the primary key is an incrementing integer. Without fillable, you cannot mass assign any database values. \$\endgroup\$
    – Trojan404
    Feb 9, 2017 at 18:57
  • \$\begingroup\$ @Trojan404 Good clarifications regarding the underlying limitations around Laravel Mail. Sounds odd, but if that is what you have to do to make it work... I wasn't particularly concerned with the existence of those various properties in the models, but rather their mutability. To me it seems odd that the framework would allow such fields to be mutable. I would say that you might find it useful to put in PHPDocs right as you are defining your class. For me at least, I think it helpful to think in terms of class interface (your programming contract) before implementation. \$\endgroup\$
    – Mike Brant
    Feb 9, 2017 at 20:46
  • \$\begingroup\$ Would you seriously suggest to remove the \App\Mail\ from the send email functionality? My only concern with doing that is that there are only two ways to handle this: 1) Add a use statement in every time a new mailable gets created. This seems bad because you create a new template, then you have to update this class when it should be dynamic. 2) Define that directory as a use statement (i.e. use App\Mail as MailTemplates;) Even here, though, you would have to call the template like this: MailTemplates\Class. \$\endgroup\$
    – Trojan404
    Feb 12, 2017 at 18:59

Your Answer

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

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