Move foreman LDAP functionality to ldap_fluff #26

dLobatog · 2014-06-06T11:57:40Z

Most logic comes from here: https://github.com/theforeman/foreman/blob/develop/app/models/auth_sources/auth_source_ldap.rb

and theforeman/foreman#529 , which this PR is meant to facilitate.

pitr-ch · 2014-06-06T12:02:46Z

ldap_fluff.gemspec

@@ -1,6 +1,6 @@
 Gem::Specification.new do |s|
  s.name        = 'ldap_fluff'
-  s.version     = '0.2.5'
+  s.version     = '0.2.6'


this will definitely be 0.3 :)

pitr-ch · 2014-06-09T08:28:32Z

lib/ldap_fluff/generic.rb

+      return false
+    end
+    return true
+  end


this could be simplified to:

def user_exists?(uid) @member_service.find_user(uid) true rescue self.class::MemberService::UIDNotFoundException false end

please look also for other usages.

pitr-ch · 2014-06-09T08:48:39Z

I like creation of the Generic classes 👍 Thanks.

pitr-ch · 2014-06-12T09:34:32Z

lib/ldap_fluff/generic.rb

@@ -0,0 +1,67 @@
+require 'net/ldap'
+
+class Generic


the gem should not pollute global space!

dLobatog · 2014-06-13T01:40:22Z

Thanks for the comments 😄, I've fixed them and tested it with ActiveDirectory which gave me a couple of hints:

I removed ad domains, they are unnecessary and incompatible with old Windows Server versions. Users can include the "@whatever.com" in their logins and it'll work just fine. Keeping it makes ldap_fluff incompatible with Windows Server 2008+ as the logins will just be of the format "CORP\Administrator" (just an example). This way it works for all versions, the old way it only works for Windows 2000/2003, and 2008 in compatibility mode.

I also fixed get_logins to make sure we don't miss any possible login in a group, and fixed your concerns with UnauthenticatedExceptions.

I need the & in get_groups and get_logins, to keep the block both as a lambda and a block and therefore and able to receive arguments (|g|).

pitr-ch · 2014-06-16T08:12:08Z

lib/ldap_fluff/generic_member_service.rb

+  # return string will be something like
+  # CN=bros,OU=bropeeps,DC=jomara,DC=redhat,DC=com
+  def get_groups(grouplist)
+    grouplist.map(&:downcase).collect(&(proc { |g| g.sub(/.*?cn=(.*?),.*/, '\1') } ))


I am not sure what you mean.

grouplist.map(&:downcase).collect(&(proc { |g| g.sub(/.*?cn=(.*?),.*/, '\1') } )) #equals grouplist.map(&:downcase).collect { |g| g.sub(/.*?cn=(.*?),.*/, '\1') }

pitr-ch · 2014-06-16T08:16:51Z

Thanks for the updates, exceptions are looking much better now 👍 I noticed one more thing, could you create a common LdapFluff::Error ancestor for all the errors for a developer to be able to catch all LdapFluff errors easily?

domcleal · 2014-06-16T10:04:16Z

lib/ldap_fluff/posix_member_service.rb

+  def find_user(uid)
+    user = @ldap.search(:filter => name_filter(uid), :base => @group_base)
+    raise self.class::UIDNotFoundException if (user.nil? || user.empty?)
+    user
  end

  # return an ldap user with groups attached


This method is giving me problems I think when testing against OpenLDAP, as it's returning the user's CN rather than their groups.

dLobatog · 2014-06-17T11:27:04Z

@domcleal It still doesn't recurse to bring the nested users, ideally I think Foreman should try to return all users directly in the specified GID (users_for_gid takes care of that), and after that, it should check for nested user groups, and create a external user group in Foreman for each of these nested groups found in it. Does this make sense to you?

Say we have a group: "foremaners", with a direct member "dcleal", a subgroup "katellers", with a member "bk".

The flow would work like this:

Add external user group "foremaners" to user group "test" in Foreman
Refresh external user group
Refreshing adds "dcleal" to the list of users directly
Refreshing adds "katellers" to the list of external user groups
Auto-refresh "katellers", this will add "bk" to the list of users
Done

domcleal · 2014-06-18T16:13:26Z

Sounds a little clunky and more work than using the LDAP server's memberOf determination to me, but if that's what you want to do, do it.

pitr-ch · 2014-06-19T09:56:14Z

This PR resolves #27 (this should auto close 27 when this PR is merged)

dLobatog · 2014-06-26T11:39:43Z

@pitr-ch @domcleal I've pushed changes needed to recursive search groups for all 3 implementations and the Generic. Summing up:

ActiveDirectory
List of members include users, and groups. We check the objectclass of each of the members of the group in question and act accordingly, recursing if its a group, and returning the login if its an user.
POSIX
Nested groups have no special attributes in the RFC, so basically anything under the dn of the parent group could be considered a nested group. I query using cn=foremaners,ou=groups,dc=example,dc=com as the base_dn, which would return stuff like cn=katellers,cn=foremaners,ou=groups,dc=example,dc=com, and we can get the logins of all nested groups.
FreeIPA
MemberUID returns just the 'uid' or 'gid' of the users and nested groups. Since FreeIPA users 'cn=users' and 'cn=groups' to distinguish between them, I just check that part of the cn and return the userid or recurse searching for users in the nested group.

Tests will follow soon, but I thought it'd be helpful to put this out early if you want to test.
Also, Travis is trying to pick up activesupport 4.1.1 in 1.8.7, that's why the build is failing, any solutions?

dLobatog · 2014-07-01T17:13:02Z

@pitr-ch I've just updated this with some tests for the nested usergroups and an easy refactor of some methods used in tests. After this PR is merged I think a rewrite of the test suite should follow at some point.

pitr-ch · 2014-07-04T09:09:52Z

@elobato could you fix travis tests?

pitr-ch · 2014-07-04T09:14:39Z

Thanks for updating. ACK Ruby wise, I would defer to @domcleal to ack the LDAP part.

domcleal · 2014-07-07T08:43:04Z

@pitr-ch sorry, I can't, I'm not at all familiar with this project

pitr-ch · 2014-07-14T08:43:36Z

@elobato based on discussion with @domcleal later on irc, he'll look at the foreman integration and ack.

domcleal · 2014-07-24T08:48:10Z

I've tested it successfully with OpenLDAP, I'm happy. Please feel free to merge & release 0.3.0. 👍

dLobatog · 2014-07-24T09:41:59Z

I have to say @vintagepenguin 's comment on #28 worries me a bit, although I could not reproduce it.

pitr-ch · 2014-07-25T10:37:46Z

@elobato what are the possible objects returned by #authenticate?? I tried to find it but I was unsuccessful.

dLobatog · 2014-07-28T08:13:18Z

@pitr-ch

true if authentication was successful
false if username/password werent provided
an OpenStruct with the information about the error (invalid credentials, unreachable host, etc..) if authentication was unsuccessful

remove debugger Addressed Petr issues during review Removed domains and tested with ADLS LdapError class and OpenLDAP memberuid get_logins fix Relax activesupport dependency and add authors Recursive OU, posixgroups search Refactored tests and added nested groups tests Make travis pick up different activesupport for ruby 1.8 Booleanize authenticate? result

dLobatog · 2014-07-28T10:28:57Z

@domcleal @pitr-ch .authenticate? now returns true or false always. For more information about the authentication the user can query get_operation_result as it's done here dLobatog/foreman@2f63b5b#diff-3658b1e0f80d953e1d2be7093a1a9e30R52

pitr-ch · 2014-07-28T11:43:31Z

@elobato thanks true or false is much better 👍

Move foreman LDAP functionality to ldap_fluff

dLobatog mentioned this pull request Jun 6, 2014

Feature #813 Usergroups can be linked to an LDAP auth_source theforeman/foreman#529

Closed

pitr-ch reviewed Jun 6, 2014
View reviewed changes

dLobatog mentioned this pull request Jun 6, 2014

refs #813 - LDAP group support changes theforeman/foreman-packaging#265

Merged

pitr-ch reviewed Jun 9, 2014
View reviewed changes

pitr-ch reviewed Jun 12, 2014
View reviewed changes

lib/ldap_fluff/generic.rb

@@ -0,0 +1,67 @@

require 'net/ldap'

class Generic

Copy link

Member

pitr-ch Jun 12, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the gem should not pollute global space!

pitr-ch reviewed Jun 16, 2014
View reviewed changes

domcleal reviewed Jun 16, 2014
View reviewed changes

pitr-ch mentioned this pull request Jun 18, 2014

Relax ActiveSupport dependency #27

Closed

vintagepenguin mentioned this pull request Jul 15, 2014

Update to work with Rails 4 #28

Closed

dLobatog mentioned this pull request Jul 21, 2014

add support for eDir LDAP type #17

Open

pitr-ch pushed a commit that referenced this pull request Jul 29, 2014

Merge pull request #26 from eLobato/master

b739ef4

Move foreman LDAP functionality to ldap_fluff

pitr-ch merged commit b739ef4 into theforeman:master Jul 29, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move foreman LDAP functionality to ldap_fluff #26

Move foreman LDAP functionality to ldap_fluff #26

dLobatog commented Jun 6, 2014

pitr-ch Jun 6, 2014

pitr-ch Jun 9, 2014

pitr-ch commented Jun 9, 2014

pitr-ch Jun 12, 2014

dLobatog commented Jun 13, 2014

pitr-ch Jun 16, 2014

pitr-ch commented Jun 16, 2014

domcleal Jun 16, 2014

dLobatog commented Jun 17, 2014

domcleal commented Jun 18, 2014

pitr-ch commented Jun 19, 2014

dLobatog commented Jun 26, 2014

dLobatog commented Jul 1, 2014

pitr-ch commented Jul 4, 2014

pitr-ch commented Jul 4, 2014

domcleal commented Jul 7, 2014

pitr-ch commented Jul 14, 2014

domcleal commented Jul 24, 2014

dLobatog commented Jul 24, 2014

pitr-ch commented Jul 25, 2014

dLobatog commented Jul 28, 2014

dLobatog commented Jul 28, 2014

pitr-ch commented Jul 28, 2014

Move foreman LDAP functionality to ldap_fluff #26

Move foreman LDAP functionality to ldap_fluff #26

Conversation

dLobatog commented Jun 6, 2014

pitr-ch Jun 6, 2014

Choose a reason for hiding this comment

pitr-ch Jun 9, 2014

Choose a reason for hiding this comment

pitr-ch commented Jun 9, 2014

pitr-ch Jun 12, 2014

Choose a reason for hiding this comment

dLobatog commented Jun 13, 2014

pitr-ch Jun 16, 2014

Choose a reason for hiding this comment

pitr-ch commented Jun 16, 2014

domcleal Jun 16, 2014

Choose a reason for hiding this comment

dLobatog commented Jun 17, 2014

domcleal commented Jun 18, 2014

pitr-ch commented Jun 19, 2014

dLobatog commented Jun 26, 2014

dLobatog commented Jul 1, 2014

pitr-ch commented Jul 4, 2014

pitr-ch commented Jul 4, 2014

domcleal commented Jul 7, 2014

pitr-ch commented Jul 14, 2014

domcleal commented Jul 24, 2014

dLobatog commented Jul 24, 2014

pitr-ch commented Jul 25, 2014

dLobatog commented Jul 28, 2014

dLobatog commented Jul 28, 2014

pitr-ch commented Jul 28, 2014