Updated LDAP Integration #738
Conversation
…a/hashtopolis's adduser and other integrations that may exist that use only 16 arguments to construct user
…ration. These are required, however, there is also a more specific Description to assist the users
|
Still using this on our side, working well |
There was a problem hiding this comment.
Thanks for the pull request, I added some small comments regarding some code parts, but looking good overall.
One thing which is missing, is the addition to the update script which is used to update existing installations which pulled new changes from the repository. These would break with the current implementation. The changes needed to be done on the database should be added in src/install/updates/update_v0.12.x_v0.x.x.php.
@zyronix will do a test on our side to verify the functionality and then report back.
| @@ -19,14 +20,44 @@ class User extends AbstractModel { | |||
| private $otp2; | |||
| private $otp3; | |||
| private $otp4; | |||
|
|
|||
| function __construct($userId, $username, $email, $passwordHash, $passwordSalt, $isValid, $isComputedPassword, $lastLoginDate, $registeredSince, $sessionLifetime, $rightGroupId, $yubikey, $otp1, $otp2, $otp3, $otp4) { | |||
There was a problem hiding this comment.
While I understand what the idea is behind having two constructors, this would break the way how the Models are generated.
In src/dba/models/generator.php there are all models defined in arrays, which would be the way to go to add another column for a table. After this, just all new User( calls would need to be searched to adjust the arguments.
| @@ -0,0 +1,2 @@ | |||
| Order deny,allow | |||
There was a problem hiding this comment.
Did this file slip in by accident? As this would block installation procedure to start after placing the files. The block htaccess file should be added automatically by the installation procedure after it is completed.
|
Is there any progress on this topic? LDAP authentication would be a great feature |
|
No progress on this has been made, but in the background we have been working on a new frontend and backend. This will come with the switch to oAuth for authentication, which would make it possible to use all kinds of authentication servers like LDAP or AD based servers for authentication. |
|
Any status update on this? Super useful. |
I've taken the older LDAP Pull Request and merged it with the latest code. It is working great in my environment.
From the old code, I've found that the popular kpeiruza/hashtopolis docker image uses the User.class constructor with 16 arguments, the extra argument breaks this. So I've reworked the constructor flow to use either 16 or 17 arguments.
Also, I've made the BaseDN and UID config settings, so this can work in different environments. Some LDAP Servers use CN by default, others use UID. Setting these variables right now has to occur manually via the database.
A couple nice things that I can work on in the future, would be: