LDAP sign-in #383
LDAP sign-in #383
Conversation
…AP and Active Directory with plain authentication.
…he referrer when we log in with LDAP.
|
This would be awesome. OpenID isn't good when you're trying to use Barkeep with confidential code; I can't have snippets being sent off to [email protected]. |
| halt 400, "LDAP provider not found." unless provider | ||
|
|
||
| begin | ||
| Timeout::timeout(5) do |
There was a problem hiding this comment.
Timeout should be treated like try/catch -- we should try to only put code inside the body which needs timeout protection. For instance, setting up the vars conn_params and auth_params does not need to be in the timeout, and makes it hard to see the code that needs the timeout protection.
|
This is overall a very solid and much-requested piece of functionality Michael. Nice job! If we can restructure it a bit and polish it up, I'd be happy to merge it in. The one key thing that's missing is some tests. Ladle sounds great and realistic, but even if we had some basic unit tests using mocks, that would help. Since I don't use ldap and don't know how to test it, I'd be very afraid that I'd break your functionality in the course of refactoring things, which would be a headache for anyone who comes along and wants to use it. |
|
Great job Michael. My development team are looking to adopt Barkeep, but require Active Directory rather than OpenID. I've been following this post closely and wonder if you have a rough idea of when your branch is likely to get merged into master? Thanks for your efforts adding this highly sought-after functionality. |
|
👍 I'm starting to test drive barkeep for an enterprise deployment and would (sadly) require ldap as well, this will be a great feature when integrated. |
|
I'm also interested in this feature. My team is interested in using barkeep for our code reviews, but using openid for authentication isn't going to work for us. Active Directory support would be great! |
|
As far as open source code review tools go, Barkeep is at the top of its game. With that said, in order for its acceptance within many software development houses, LDAP support is paramount. With only openid support, Barkeep is preventing itself from being used where it is most needed. In absence of LDAP support, I've had to suggest the introduction of the propriety tool Crucible (Atlassian), it's pretty slick, but IMO lacks the simple UI and workflow of Barkeep. It would be great to see Barkeep evolve into an open source project that could be adopted at all levels of development, as it shows great promise. |
|
I would also like to add my vote for this issue.... thanks. |
|
+1 vote. |
|
The next action for this pull request is for @michaelstorm to respond to the inlined comments and to include some tests. Michael, this functionality is really great and it looks like it's going to be a popular feature. You mentioned that you couldn't use Ladle to test because of a gem dependency issue. I've fixed this in 9a9ee62. Do you think you could take another crack at polishing this off? |
|
👍 This is currently what is stopping me from giving Barkeep a go! |
|
It's been about 8 months, any word on when this will be merged, or what's keeping it from being merged? |
|
What's keeping it from being merged is (a) the code needs cleanup, see review comments and (b) the complete lack of tests. |
Disable session hijacking protection
Conflicts: Gemfile Gemfile.lock barkeep_server.rb
|
Michael, just wanted to say thanks for this. The LDAP branch is working great for us so far. |
| :password => params[:password] })})) | ||
|
|
||
| if conn.bind | ||
| filter = Net::LDAP::Filter.contains(provider[:uid], params[:username]) |
There was a problem hiding this comment.
This Filter.contains needs to actually be Filter.eq. If, for instance, you happen to have Alice Smith (asmith) and Adam Smithson (asmithson) then Alice won't be able to log in because this seach will return both, and the entries.size > 1 error condition a few line below will activate.
This resolves #347 by adding LDAP user authentication to Barkeep. It's documented, with examples, in
doc/ldap.markdown.Here's the provider selection page. If only one provider of any type is specified, Barkeep skips this page and redirects immediately there, as before. Note that the names are specified arbitrarily in the configuration; I could've used "barkeep.com" instead of "Active Directory".
And here's the actual LDAP sign-in page. I'm not great at making things pretty, but it's functional.
I'd like to test it with Ladle, but there seems to be a version conflict between Fezzik's version of Open4 (>= 1.3.0) and Ladle's version (~> 1.0.0). I don't know Ruby best practice in these situations, so I'll leave it alone for now.
That said, I've tested it manually against OpenLDAP and Active Directory. However, the more exotic (read: anything other than
:method => :simple) are untested. I'm just too distracted to set up anything more complicated right now.Best,
Mike