3
\$\begingroup\$

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();
    }

}
\$\endgroup\$
1
\$\begingroup\$

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*
\$\endgroup\$
  • \$\begingroup\$ 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). \$\endgroup\$ – user49060 Jul 11 '14 at 21:06

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.