Email Controller

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->Fir

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 shoul

StackExchange Code Review Q#154875, answer score: 3