Extract method in_group? from in_required_groups? and expose it to the model

[OriginalThing]

Extracted method to allow application developers to check group membership manually.

I'm not an expert Rails or Ruby developer and am not sure if I should have used the splat operator for calling the method. Should be fairly straight forward to remove them and expand them into parameters if I am wrong.

Note: Perhaps ldap_groups needs to be updated to work with AD or allow searching using custom group attributes (parameter or config file) because AD uses "member" rather than "uniqueMember".

[stevenyxu]

Thanks for the contribution. I'll take a look at this as soon as I can. Seems like a perfectly useful addition. Since you are the target user for this feature, would you mind contributing a test case demonstrating the specific use case you're looking for?

[OriginalThing]

Ok I have added a test case. I haven't tested the tests because I don't have an OpenLDAP set up to work with, so hopefully I have written it correctly.

[OriginalThing]

This line uses an array to pass the attribute that needs to be searched (not used for AD group membership checks). The reason for an array is because that is what in_required_groups? passes from the required groups configuration. I am not sure whether the values should be passed as an array or if there is a better way for the values to be sent.

[stevenyxu]

Thanks for the additions. There seems to be an issue with your earlier changeset. It's reporting the whole file as changed for some reason (CRLF, maybe?). I can correct that on the merge, though. I'm not sure from reading the test what the array argument form is for. Is the second element in the array the group membership attribute? If so, seems like it would make sense as an optional argument or an optional options hash. If you wanted to look into it, it'd help me. Otherwise, I'll fix the changeset and merge it in if the diff looks okay next chance I get at my station.

[OriginalThing]

Yeah I noticed that it said the whole file was changed but wasn't sure how to fix that.

Yes the second element in the array is the group membership attribute. I put it there in the tests to show how it could be used but the LDAP schema you have for testing doesn't appear to have different types of groups so it only shows that its not using "uniqueMember" to search but not that it is successfully using "member" to get results. It's not a feature that I use myself but wanted to have the functionally there for others to use.

I wasn't sure what the best way to optionally pass the group membership attribute and just tried to go with something simple that worked. I'll take a look and see if I can come up with something that is more conventional.

[OriginalThing]

Tried to fix the diffs but don't know what the problem is. What are line endings supposed to be? Because when I open the file Notepad++ reads it as CRLF. The diffs looked fine in the GitHub application up until I committed it.

I changed it so it wasn't using the array and hopefully I didn't break things when I reverted and reapplied my changes.

[OriginalThing]

Ok I had a bit of a look through and I think I sorted things out. I made a few silly mistakes but it is working again. I'm not sure about what is more conventional. If using the splat operator ( * ) with arrays is okay or if it is better to use the ||= operator with optional parameters defaulting to nil.

I managed to find out that the line endings are supposed to be LF instead of CRLF so I fixed that and now the diffs aren't saying I changed the whole file. Now hopefully you can tell me if I'm following ruby/rails conventions or not.

[jozefvaclavik]

+1 for this feature. Could it be merged? If @OriginalThing doesn't have time anymore, I could take over it.

[OriginalThing]

I was hoping this would be merged also. I wonder if this dropped under @cairo140 's radar when I accidentally closed the issue. I reopened it but did not hear anything further.

@jozefvaclavik I have time to work on this if something needs to be done but it was working how I needed it for the project I was working on so there was no further work required.

[stevenyxu]

Thanks @OriginalThing. Sorry I missed it for ages. The final diff looks good overall. I'm going to take a holistic look at the coverage and the syntax piece on the optional argument, and I'll let you know if there was anything additional I could use from you to get it in master.

[stevenyxu]

Pushed it out to issue-102 branch. I fixed a default argument issue that seems to have been a typo. Your ||= approach required both arguments to be passed. If my change misinterpreted what you meant for the interface to be, let's talk.

I'm still getting a couple test failures out of the box:

  2) Users With default settings check group membership should return true for admin being in the admins group
     Failure/Error: assert_equal true, @admin.in_ldap_group?('cn=admins,ou=groups,dc=test,dc=com')
     MiniTest::Assertion:
       <true> expected but was
       <false>.
     # (eval):2:in `assert_equal'
     # ./spec/unit/user_spec.rb:150:in `block (4 levels) in <top (required)>'

  3) Users With default settings check group membership should return true for user being in the users group
     Failure/Error: assert_equal true, @user.in_ldap_group?('cn=users,ou=groups,dc=test,dc=com')
     MiniTest::Assertion:
       <true> expected but was
       <false>.
     # (eval):2:in `assert_equal'
     # ./spec/unit/user_spec.rb:158:in `block (4 levels) in <top (required)>'

Looking into it though.

[stevenyxu]

Never mind. I see what you were getting at. Just used default arguments across the board. At some point one could potentially create an options hash, but IMO YAGNI applies here.

[stevenyxu]

@OriginalThing seemed good enough with my couple changes to the default arguments to push to master. Let me know if there's anything else I can do for you. Your editor also seems to have spammed a BOM on ldap_adapter so I got rid of that.

[OriginalThing]

Awesome. Thanks @cairo140 . I haven't got my project handy to test with but reading through the changes I don't see any thing that would cause any problems.

Just one thing though should line 47 in ldap_adapter.rb be like the other two methods? So:
def self.in_ldap_group?(login, group_name, group_attribute = nil)
Should be?:
def self.in_ldap_group?(login, group_name, group_attribute = DEFAULT_GROUP_UNIQUE_MEMBER_LIST_KEY)

Just for the sake of consistency. The model always calls that method with the full set of parameters anyway.