Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement LDAP auth #37

Closed
wants to merge 5 commits into from
Closed

Conversation

TheNetworkIsDown
Copy link

@TheNetworkIsDown TheNetworkIsDown commented Apr 12, 2020

Fixes #14

The following enhancements and modifications implement a way to authenticate against an LDAP server.

We assume that a successful LDAP authentication means the user is allowed to access the application independent of RBAC later in the application. This initial authorization can be varied by the object filter (or the base DN). Currently there is only one keyword value pair to define such a filter to match the username (Common Name/CN) as such.

But we still need the DB! A User record is automatically created in the DB (if it does not exist) when the user first logs in, since we still need to remember some metadata. It is updated from LDAP, if the concerning attributes are configured and LDAP exposes that information. Otherwise, local data is used (name, email, role), which can be set using User Preferences dialog. This data gets overwritten as soon as the directory contains data.

So far, the identity of the user was defined by an email address, not a username. In a directory setting, that changes. However, the User model doesn't account for a username, only an email address (and a real name). I could have:

  • stored the username in the email field, thus maybe changing the semantics of that field
  • stored the username in the name field, thus no longer being able to store the actual name
  • UPN in AD looks like an email address so I could have stored it in email and used it as the main credentail
  • in other directories like eDirectory however, the username might just be contain a plain username like "jdoe"

As a consequence I've introduced a UserLDAP model, together with an AuthLDAP controller, and also a UserPreferences view, since the single form currently used via the controller no longer cuts it. There is now also a setupUserModelLDAP() trait.

Concerning roles: I'm not sure how to handle that. Currently I'm not assigning any role at all. And there doesn't seem to be anything that is not accessible in my App. In any case we give the option to retrieve the role from LDAP.

UserAdmin will not be needed much with directory authentication, except for an admin to modify attributes not provided by LDAP. In any case, username and password cannot be changed in the application. And also not in LDAP via the application. A password field doesn't even exist anymore, so we might need to update UserAdmin to account for the new User model...RoleAdmin remains viable of course. RegisterForm is also not used in this case.

The Login form got a minor update because we need to change the caption and error message depending on the authentication type.

There is still an issue with potential user enumeration during Login:

  • If the user does not exist, the form field shows an error
  • If the user exists but the password is wrong then there is an ErrorException

Usage example:

$app->add(new \atk4\login\AuthLDAP([
  'ldapUrl' => 'ldap://ldap.acme.company/',
  'ldapBaseDn' => 'o=acme',
  'ldapObjFilter' => ['objectClass', 'Person'],
  'ldapFullNameAttr' => 'fullname',
  'ldapEmailAttr' => 'mail',
  ]))->setModel(new \atk4\login\Model\UserLDAP($app->db));

@codecov
Copy link

codecov bot commented Apr 12, 2020

Codecov Report

Merging #37 into develop will decrease coverage by 0.87%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             develop     #37      +/-   ##
============================================
- Coverage       6.35%   5.48%   -0.88%     
- Complexity       184     211      +27     
============================================
  Files             18      20       +2     
  Lines            472     547      +75     
============================================
  Hits              30      30              
- Misses           442     517      +75     
Impacted Files Coverage Δ Complexity Δ
src/Auth.php 0.00% <0.00%> (ø) 26.00 <0.00> (ø)
src/AuthLDAP.php 0.00% <0.00%> (ø) 23.00 <23.00> (?)
src/Feature/SetupModel.php 0.00% <0.00%> (ø) 19.00 <6.00> (+3.00)
src/LoginForm.php 0.00% <0.00%> (ø) 7.00 <0.00> (ø)
src/Model/UserLDAP.php 0.00% <0.00%> (ø) 1.00 <1.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2efb2ce...b689d4a. Read the comment docs.

Screenshot used in README.md was not correct and confusing for the initial user.
comments and checks missing
@kiwiflyer
Copy link

@NotAProfessionalDeveloper - great contribution!

If using ldapProxyUser, I think you will need to include
$ldapConn = ldap_connect($this->ldapUrl); prior to attempting the user bind, or the ldap_bind will fail.

...
if ($info['count'] == 1) {
$ldapConn = ldap_connect($this->ldapUrl);
if (ldap_bind($ldapConn, $info[0]['dn'], utf8_encode($password))) {
ldap_unbind($ldapConn);
...

@TheNetworkIsDown
Copy link
Author

TheNetworkIsDown commented Apr 21, 2020 via email

Unbinding frees the connection descriptor, and anyway unbinding is not necessary to bind again, so removing it
@mvorisek
Copy link
Member

@NotAProfessionalDeveloper please rebase and add a test

@mvorisek mvorisek marked this pull request as draft June 8, 2022 22:50
@mvorisek
Copy link
Member

mvorisek commented Oct 2, 2022

I like this PR, but it needs to be reworked to match the current code and quality standards incl. functional test.

If you want to finish this PR, let us know, for now, closing this PR.

@mvorisek mvorisek closed this Oct 2, 2022
@freemangordon
Copy link

Seems the original author abandoned this PR, so I wonder if we can take on it: we have RL project we shall use LDAP authentication. I'll try to contact the author to see if there is still any interest in that PR.

@freemangordon
Copy link

freemangordon commented Apr 5, 2023

unfortunately commits email is in shape of xxx@users.noreply.github.com and I have no other idea how to reach him, so I'll just make another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add LDAP support
5 participants