Clean up / refactor this store() method

Question

I'm new to Laravel and trying to figure out how I can reduce/refactor this store() call in my SessionController.

Basically, the SessionController is used to authenticate a user against an LDAP server (using wells/l4-ldap-ntlm instead of the default auth driver)

On LDAP auth success, it then checks if the user has an account/profile for this webapp. If they do not, it creates it. If they already have one, it just logs them in (user/profile only created on first login).

If LDAP auth fails, it redirects back to the login page.

I know this is a lengthy method for a controller, so I want to know the best practices for:

1. Refactoring this code out, so the controller is cleaner, and adhering to the "single responsibility" rule.
2. I need to use LDAP just for authentication, but I also want to store information for this webapp that is not stored in LDAP. Right now I am creating a User and a Profile model, but I don't know a good way to "link" them to the LDAP authenticated user once logged in.

public function store()
{
 // store credentials
 $input = Input::all();
 // attempt authentication
 $attempt = Auth::attempt([
 'username' => $input['user'],
 'password' => $input['pass']
 ]);
 if ($attempt) {
 // LDAP auth success
 $un = Auth::user()->username;
 $role = Auth::user()->type;
 // check if user exists, creating a user and profile if they do not.
 try {
 $user = User::whereUsername($un)->firstOrFail();
 } 
 catch (Illuminate\Database\Eloquent\ModelNotFoundException $e) {
 $user = User::create(['username'=>$un]);
 $user->roles()->attach( $role );
 $user->createProfile();
 }
 // redirect
 return Redirect::intended('/profile')
 ->with('flash_message', 'welcome, ' . $un);
 }
 else {
 // LDAP auth failed.
 return Redirect::back()
 ->with('flash_message', 'The credentials you entered were invalid. Please try again.')
 ->withInput();
 }
}

Answer 1

Okay, let me get this right (I have no experience working with an LDAP): The Auth class you're first using for $attempt is actually the LDAP library you're implementing?

I don't think I understand the logic in your code. You attempt to log in to the user, and if they successfully login, you check to see if the user exists? I must be misunderstanding something.

Well, assuming you know how exactly this works, I'll just point out a couple refactor points I can see:

• In terms of breaking this apart into responsibilities: your method handles both authentication and registration. I can imagine registration being handled in another method.
• I doubt you need to flash forward this entire parameter: 'welcome, ' . $un. Have your Blade file check for the flash and display "welcome, ". Since you're only sending one, just pass through $un. Either that or make it an array and pass two values: type of flash, and message.
• Speaking of $un. There are better variable names, give yours some meaning and quality to them.
• You can manage to un-nest your code a little:

if ($attempt) {
 ...
} else {
 return Redirect...
}

This piece could really just be:

if (!$attempt) {
 return Redirect...
}
// *What used to be inside the if*

• tdc: Hi, Thanks a bunch for the input! It is a little strange, but essentially I just use LDAP for authentication with our systems (ie username and password). On successful auth, LDAP only returns me the username, but not other information I Need (name, location, etc). So to get around this I'm essentially creating a "profile" for my system, which is mapped to their LDAP username (basically instead of using an integer for relational ID, I'm using the LDAP username).