jotaelesalinas / laravel-adminless-ldap-auth

Authenticate users in Laravel against an adminless LDAP server
MIT License
210 stars 33 forks source link

Undefined index: password #49

Closed Abdullah-Alsuwayhiri closed 3 years ago

Abdullah-Alsuwayhiri commented 3 years ago

I followed all instructions, but with Login UI I have an issue

image

github-actions[bot] commented 3 years ago

Hi #! Welcome to this repo.

jotaelesalinas commented 3 years ago

Hi @Abdullah-Alsuwayhiri . Have you been able to solve this issue?

Abdullah-Alsuwayhiri commented 3 years ago

No, I have not

Abdullah-Alsuwayhiri commented 3 years ago

image

the credentials array contains only a username image

Abdullah-Alsuwayhiri commented 3 years ago

Hi @jotaelesalinas

It's working fine now; in current project I'm working in I have many rules which uses id from Auth->id before using ldap authentication; to fix that I convert it to quires and getting the id in each controller & views.....(the username in ldap server is equal to the one in mysql db and it's unique) I think this is not best practice. what you think?

jotaelesalinas commented 3 years ago

I'm glad you could make it work in the end. To be honest, I have no idea whether that is best practice or not. If it works for you now, that's ok!

LichP commented 3 years ago

This is still a bug. In my case I'm using the @guest and @auth helpers in a blade template to understand whether or not a user is logged in, and the problem is occurring after the user has successfully authenticated in a previous request. Here's the relevant stack trace snippet:

[previous exception] [object] (ErrorException(code: 0): Undefined index: password at vendor/jotaelesalinas/laravel-adminless-ldap-auth/src/AdminlessUserProvider.php:39)
[stacktrace]
#0 vendor/jotaelesalinas/laravel-adminless-ldap-auth/src/AdminlessUserProvider.php(39): Illuminate\\Foundation\\Bootstrap\\HandleExceptions->handleError()
#1 vendor/jotaelesalinas/laravel-adminless-ldap-auth/src/AdminlessUserProvider.php(20): JotaEleSalinas\\AdminlessLdap\\AdminlessUserProvider->retrieveByCredentials()
#2 vendor/laravel/framework/src/Illuminate/Auth/SessionGuard.php(139): JotaEleSalinas\\AdminlessLdap\\AdminlessUserProvider->retrieveById()
#3 vendor/laravel/framework/src/Illuminate/Auth/GuardHelpers.php(60): Illuminate\\Auth\\SessionGuard->user()
#4 vendor/laravel/framework/src/Illuminate/Auth/GuardHelpers.php(70): Illuminate\\Auth\\SessionGuard->check()
#5 storage/framework/views/5372c3d99182e13163d41332b141f3f63dcd017b.php(69): Illuminate\\Auth\\SessionGuard->guest()

The problem is AdminlessUserProvider::retrieveByCredentials expects to be supplied a password in the credentials array, but AdminlessUserProvider::retrieveById doesn't supply one, so an excpetion occurs as there's no null check here.

There is a more fundamental problem though: the approach appears to be to recreate the LdapUser object on every request by retrieving the detaisl from the directory, however in Active Directory land this can't be done without a bind, and we don't have the user password anymore to do that so it becomes an impossibility without an AD service account (somewhat defeats the point of this package ;-) ). Perhaps a better approach would be to serialize the LdapUser object directly on to the session so it can be retrieved in subsequent requests without having to rebind to the server?

jotaelesalinas commented 3 years ago

Hi @LichP ! Thank you for your very helpful insights. I see the issue but, in theory, Laravel's session guard should return the user in subsequent requests, without connecting to the LDAP server. Am I wrong? I will try to look into this and get it fixed... eventually. I can't promise anything. You are very welcome to submit a merge request!

LichP commented 3 years ago

I think the session guard only retains the user identifier and delegates the user retrieval to the active UserProvider via implementation of retrieveById: in a vanilla setting EloquentUserProvider would simply pull the user data from the DB and return a new model object each time.

jotaelesalinas commented 3 years ago

Fixed in https://github.com/jotaelesalinas/laravel-adminless-ldap-auth/pull/51 .