Adldap2 / Adldap2-Laravel

LDAP Authentication & Management for Laravel
MIT License
910 stars 184 forks source link

Cannot access adldap property of model. #7

Closed brockwddb closed 9 years ago

brockwddb commented 9 years ago

@stevebauman I've followed all of the documentation but can't seem to get Auth::user()->adldapUser to return anything but null.

I have added the trait to my user model, turned on bind_user_to_model and confirmed that authentication is working properly using the standard Auth controller with the $username attribute overridden to be samaccountname.

When doing dd(Auth::user()) I am also getting a null value on $adldapUser. Any help would be greatly appreciated.

stevebauman commented 9 years ago

When performing dd(Auth::user()) after doing authentication, what instance do you get? Make sure the returned instance of your authenticated user is the same name as configured in config/auth.php.

brockwddb commented 9 years ago
User {#168 ▼
  #table: "users"
  #fillable: array:3 [▶]
  #hidden: array:2 [▶]
  #connection: null
  #primaryKey: "id"
  #perPage: 15
  +incrementing: true
  +timestamps: true
  #attributes: array:7 [▶]
  #original: array:7 [▶]
  #relations: []
  #visible: []
  #appends: []
  #guarded: array:1 [▶]
  #dates: []
  #dateFormat: null
  #casts: []
  #touches: []
  #observables: []
  #with: []
  #morphClass: null
  +exists: true
  +wasRecentlyCreated: false
  +adldapUser: null
}
stevebauman commented 9 years ago

Can you clear your configuration cache and try again? Use php artisan config:clear

brockwddb commented 9 years ago

Still receiving the same results after clearing the cache.

stevebauman commented 9 years ago

Strange, looking into it now

brockwddb commented 9 years ago

Thank you for your help!

stevebauman commented 9 years ago

No problem at all!

Also, shoot, the adldapUser property is only set during the authentication request, which is not set on every other request. Going to look for a solution here.

brockwddb commented 9 years ago

Probably will either have to carry the information along in the session or query AD on every page load.

Might be better to ping AD in case of any information updating.

stevebauman commented 9 years ago

Yea I might implement the query on AD every page load since Eloquent already does this as well. I can cache the adldap user, but I'm not too sure how well that would continue to work in the long run.

brockwddb commented 9 years ago

I'd agree - probably best to query. One specific reason would be when trying to get the user groups. Since groups are something that frequently change in some implementations, it would probably be best for that to automatically update to save the hassle of logging out / back in for the end user.

stevebauman commented 9 years ago

Yea absolutely, and since this feature is opt-in through configuration, I'll add notes to the readme indicating that it is slightly more resource intensive due to the extra query per request. Update will be out shortly, thanks for this!

brockwddb commented 9 years ago

Appreciate the help. We're going to be implementing this into our local web apps here at work, so I'll be sure to update you with how it goes.

stevebauman commented 9 years ago

Closed with commit, but finishing up with tests and a release will be out shortly!

stevebauman commented 9 years ago

Ok, v1.2.0 is out, you'll have to republish your adldap_auth.php configuration file due to the addition of another config option. Please let me know if you encounter any other issues, thanks!

brockwddb commented 9 years ago

@stevebauman just a note - remember to update docs to reflect installation of 1.2.0. I got hung up a minute when trying to start fresh - forgot the version incremented. Testing now.

stevebauman commented 9 years ago

Yup it's there: https://github.com/Adldap2/Adldap2-laravel/blob/master/README.md#installation

EDIT: Nevermind I understand what you're saying, I'll update the docs to reflect updates.

brockwddb commented 9 years ago

Sorry - must have been my own mistake on that one! I see that now. Must have had a tab opened that wasn't up to date.

brockwddb commented 9 years ago
User {#166 ▼
  #table: "users"
  #fillable: array:3 [▶]
  #hidden: array:2 [▶]
  #connection: null
  #primaryKey: "id"
  #perPage: 15
  +incrementing: true
  +timestamps: true
  #attributes: array:7 [▶]
  #original: array:7 [▶]
  #relations: []
  #visible: []
  #appends: []
  #guarded: array:1 [▶]
  #dates: []
  #dateFormat: null
  #casts: []
  #touches: []
  #observables: []
  #with: []
  #morphClass: null
  +exists: true
  +wasRecentlyCreated: false
  +adldapUser: null
}

Am I doing something wrong? I'm still receiving a null value for the adldapUser property.

stevebauman commented 9 years ago

Did you re-publish your configuration? You might need to delete the adldap_auth.php config file and then re-publish.

brockwddb commented 9 years ago

I did. I'm creating a new project to test now.

brockwddb commented 9 years ago

I can confirm it is still not working on a new install.

stevebauman commented 9 years ago

Hmmm I've tested it in various stages of my application locally and it works fine. I'll re-open this for now while I investigate.

brockwddb commented 9 years ago

Sounds good. If it helps, I did a fresh install of Laravel, configured the .env file, added a view to do authentication, installed your package and configured it as per the documentation.

stevebauman commented 9 years ago

Worked perfectly on fresh install for me. Started a brand new laravel project and configured everything normally. Installed a fresh adldap2/adldap2-laravel repo, created login form, logged in successfully, dumped the user on the welcome page and the adldapUser property on the User model is set to Adldap user.

There has to be something on your end you may be forgetting. Did you add the Adldap facade? Where are you dumping your user? Are you absolutely sure that bind_user_to_model is set to true?

brockwddb commented 9 years ago

I dumped user in the index route. Yes, I did add the facade and yes, the property is set to true. I followed the docs to the T.

stevebauman commented 9 years ago

Can you dump the user in a controller and see what that returns using dd(auth()->user()) and let me know what that returns?

brockwddb commented 9 years ago

@stevebauman Okay - just tried that. I'm still getting the same results. I'll try one more time with a fresh install and follow the docs again.

brockwddb commented 9 years ago

@stevebauman I am getting the same results with a fresh install. Following the documentation completely. I'm on Laravel version 5.1.17.

Are you sure you're not leaving anything out of the documentation?

stevebauman commented 9 years ago

I don't believe I am, I'm going to do a completely fresh install and display all of my steps, give me one moment.

brockwddb commented 9 years ago

Sounds good. I'm going to see if I can do any debugging in the classes too.

brockwddb commented 9 years ago

One quick note - I tried doing a dd of the $model in the bindAdldapToModel method. It doesn't seem that method is ever called.

    protected function bindAdldapToModel(User $user, Authenticatable $model)
    {
        $model->adldapUser = $user;

        dd($model);

        return $model;
    }
stevebauman commented 9 years ago
    \Adldap\Laravel\AdldapServiceProvider::class,
    \Adldap\Laravel\AdldapAuthServiceProvider::class,
stevebauman commented 9 years ago

It looks like it may be having issues discovering your user after login?

stevebauman commented 9 years ago

Are you by chance checking the remember me checkbox? I didn't override the retrieveByToken method. This may be the issue here

brockwddb commented 9 years ago

Further debugging

if ($user instanceof User && $this->getBindUserToModel()) {
                $model = $this->bindAdldapToModel($user, $model);
            }
            else{
                dd('test');
            }

Always returning "test".

stevebauman commented 9 years ago

Try:

dd($this->getBindUserToModel()); // Add this, see if it returns true

if ($user instanceof User && $this->getBindUserToModel()) {
    $model = $this->bindAdldapToModel($user, $model);
}
else{
    dd('test');
}
brockwddb commented 9 years ago
dd($this->getBindUserToModel()); // Add this --- returns true
dd($user instanceof User); // Returns False

It looks like the check to see if the model is a User model is failing for some reason.

For what it's worth, it is working for the login, just not subsequently after being logged in.

stevebauman commented 9 years ago

Okay so it's failing to find the user after logging in. Hmmm...

brockwddb commented 9 years ago

Yep - that's what it looks like.

dd($user); // returns false

stevebauman commented 9 years ago

Can you try:

dd($this->getUsernameAttribute());

And see what that returns?

brockwddb commented 9 years ago
array:1 [▼
  "username" => "samaccountname"
]
stevebauman commented 9 years ago

What are you logging in with? The LDAP users email or their username?

I'm using 'username_attribute' => ['email' => 'mail'] inside my configuration

brockwddb commented 9 years ago

Username - hold on... might see what the problem is here.

brockwddb commented 9 years ago

Okay, I thought I had it figured out.

Did not fix the issue. I assume I'm pretty close. What am I missing?

stevebauman commented 9 years ago

If you're using a username, keep 'username' => 'samaccountname'

brockwddb commented 9 years ago

It's now working. To fix my issue:

Thank you very much for your help with this. I really do appreciate it, and sorry for the errors on my part.

stevebauman commented 9 years ago

Awesome! Really glad you've solved the issue. No problem at all! I'll include some more documentation for this.