ldap_create_user set to true by default

[cam-stitt]

I have set ldap_create_user to be true by default as I think this should be the case. Therefore minimal changes will be required by the user to have the gem work as designed.

[cam-stitt]

Just ignore this. I added and removed a debugger line for testing.

[stevenyxu]

Thanks for the pull request!

Is there a reason you're adding the override to the generator instead of just changing the default in the first place?

Default setting code here: https://github.com/cschiewek/devise_ldap_authenticatable/blob/master/lib/devise_ldap_authenticatable.rb#L18

I feel like for the sake of minimizing confusion, all the configuration variables and defaults should operate in a consistent fashion. In this case, all the other config variables are set to the sensible default with the generator generating a commented out version of a config variable set to that default.

[cam-stitt]

Thanks for that. Still learning a lot of stuff. I'll make the changes and update.

[stevenyxu]

Thanks for the update!

If you have other stuff you want to move on to, I can finish up this feature; just let me know!

Sorry to keep bugging you before merging, but there's one more thing I'd like to see before merging this.

I noticed you didn't change any tests. Do the tests still pass?

I haven't pulled over your changes to my dev machine, but I strongly suspect that your changes will make something like this go from passing to failing.

If they still pass, that's also bad because it means our original tests didn't cover the fact that the setting was false by default. In that case, that would either need an issue request or a test refactor.

Can you reorganize and possibly even refactor the tests so that they pass?

[cam-stitt]

I'll run the tests the first chance I get and let you know how they go.

[stevenyxu]

Cameron, any chance you've been able to check this out? I've been looking forward to merging in this new default.

[cam-stitt]

Sorry have had a lot on. I still intend to do this as soon as I can.

[cnk]

Why do folks think create user should be true by default? I just want to use LDAP for authentication, not to actually manage my user base. So ldap_create_user defaulting to false is great for me.

[stevenyxu]

@cnk: There might be a misunderstanding of what create_user does. It makes it so that if a user exists on LDAP but not on your application's database, it creates that user one your application's database.

Presumably, if your administrator adds a user to the LDAP database, you want them to be able to access your application without waiting for you to add a user by that username to your Devise app. If I have misinterpreted your use case, could you explain a bit more why you prefer to not have users created on the Rails side if they are authenticated by LDAP?

[cnk]

I build small applications for a large organization. The users who should have access to my apps are < 1% of the people in the LDAP database. So in general I want to do my own user management and only use LDAP so they can use their standard password to log in.

I don't think it is a very big deal - there is a configuration parameter and I can just set it to the value I prefer. I just didn't see an argument for changing what the default is.

[cnk]

The interesting wrinkle for me is that I need mostly LDAP authentication but will also need some users with local passwords (and no LDAP accounts). I have something like that already based on Restful Authentication. But thought now would be a good time to try out Devise. We'll see how it goes.

[cam-stitt]

Just a quick update. Was looking into setting this up to test tonight but am having all sorts of troubles. Needed ruby 1.8 which rvm had a panic attack with for some reason. I noticed there were some pushes to have the test app updated to 1.9/rails 3.2. Hope this gets done soon!

[stevenyxu]

@castitt There are known problems with rvm and 1.8.7 on OS X Lion. That might be you. Also, you can easily contribute to the gem because the test suite runs independently of the app. If you can only get it to run on 1.9 and contribute based on that, that's fine. We can easily deal with any backwards incompatibility later.

[cschiewek]

In all honesty I think creating users in ldap upon creation in the app more of an edge case then NOT creating them. I'm going to keep the default as it is for now, unless you can really convince me otherwise.

[stevenyxu]

@cschiewek this option doesn't create users in LDAP upon creation in the app. This option makes it so that if a user exists in LDAP but not in the app, it creates the user in the app.

Assuming that's the case, it makes sense for this to be default functionality. Consider deployment in a company with thousands of users on LDAP. A blank Rails app can use this gem and when Bob, who exists on LDAP, logs in with his LDAP credentials, the Rails app creates Bob's record in the application database. By default, this is not the case. Without ldap_create_user on, Bob (and in fact every other user) would get turned away because he doesn't already exist in the application's database.

[cschiewek]

I misunderstood. This does in fact make sense as a default, as you can still restrict application access via ldap groups or attributes, and if you really want to force user creation before login, you can. Re-opening.

[cschiewek]

Current defaults are fine.