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.

Sign up for GitHub

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

Jump to bottom

LDAP support #477

Merged
merged 1 commit into from
Aug 10, 2016
Merged

LDAP support #477

merged 1 commit into from
Aug 10, 2016

Conversation

darshan-talati
Copy link

Implementing LDAP Support.
Rebase @lindskogen commit with master.

@MaxLeiter
Copy link
Member

There are some style errors causing the checks to fail: https://travis-ci.org/thelounge/lounge/jobs/143304208#L160

@MaxLeiter
Copy link
Member

MaxLeiter commented Jul 9, 2016 •
edited
Loading

Tests are passing, please squash your commits [1] so we can keep a nice and concise git commit log. Other then that looks good to me :)

[1] http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

@maxpoulin64
Copy link
Member

Agreed for the squash. I'll try to give it a try and review later in the day. We've been a little bit behind PR's lately, sorry about the delays :/ This one proved kinda hard to test as nobody on the team knows LDAP, but with the resources shared on #170 it should be a lot easier. Thanks a lot for updating that old PR!

@maxpoulin64 maxpoulin64 added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jul 10, 2016
@maxpoulin64 maxpoulin64 self-assigned this Jul 10, 2016
@astorije
Copy link
Member

astorije commented Jul 10, 2016 •
edited
Loading

This is a bit of a silly comment, but on top of the squash, something that bothers me a bit is the authorship change here. This is not a rebase, it's essentially a copy and paste, therefore completely shadowing @lindskogen's work/contribution.
It would be great if you could keep the original commit and apply your changes to it (rebase) so it appears as a commit authored by @lindskogen and rebased by you, that would in the end appear like this commit (authored by @xPaw and rebased by me):
screen shot 2016-07-09 at 20 17 34
screen shot 2016-07-09 at 20 17 42

@darshan-talati
Copy link
Author

I have squashed my commits into one, but I am not sure how would I be able to keep the original commit.

@MaxLeiter
Copy link
Member

https://stackoverflow.com/questions/3042437/change-commit-author-at-one-specific-commit Might be of help

@darshan-talati
Copy link
Author

This is all set, changed the author to @lindskogen.

@astorije
Copy link
Member

Very nice of you, @thisisdarshan, thanks!
@lindskogen, are you OK with closing your PR in favor of this one?

@lindskogen
Copy link
Contributor

Go ahead! 👍

@lindskogen lindskogen mentioned this pull request Jul 11, 2016
Closed
maxpoulin64
maxpoulin64 reviewed Jul 16, 2016
View reviewed changes
src/server.js
}

function ldapAuth(client, user, password, callback) {
var bindDN = Helper.config.ldap.primaryKey + "=" + user + "," + Helper.config.ldap.baseDN;
Copy link
Member

Choose a reason for hiding this comment

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

I don't speak LDAP, but wouldn't the user here need to be escaped, for example if user here is Max-P,dn=evil.max-p.me\0?

Copy link
Author

Choose a reason for hiding this comment

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

@lindskogen can you comment on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I guess that could be a problem

@squidboylan
Copy link

What is the status of this? We're hoping to use this soon

@darshan-talati darshan-talati mentioned this pull request Jul 30, 2016
Closed
@darshan-talati
Copy link
Author

@maxpoulin64 i have escaped the user - this will address your concern.

@maxpoulin64
Copy link
Member

Took a lot of work, but I think I finally got the thing to work! 🎉 👍

@lindskogen
Copy link
Contributor

@thisisdarshan, @maxpoulin64 nice job guys! Is is ready for merge? 👍

@jedahan
Copy link

jedahan commented Aug 9, 2016

Tested on our corporate ldap, works like a dream!

@astorije
Copy link
Member

Alright, I couldn't test this but @maxpoulin64 did plus apparently several folks here. I don't see anything that bugs me code-wise so... it's happeniiiing 🎉
👍 and merging.

@astorije astorije merged commit 1fb1477 into thelounge:master Aug 10, 2016
@astorije astorije self-assigned this Aug 10, 2016
@astorije astorije added this to the 2.0.0 milestone Aug 10, 2016
astorije added a commit that referenced this pull request Aug 10, 2016
@astorije
Fix token persistency across server restarts
cf64cb0 
This fixes a regression introduced by LDAP support addition
(#477), which forces
users to re-login when the server restarts. This was originally
implemented in #370.
This was referenced Aug 10, 2016
Merged
Merged
@astorije astorije mentioned this pull request Aug 29, 2016
Merged
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@astorije
Merge pull request thelounge#477 from thisisdarshan/ldap
8625bd2 
LDAP support
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@astorije
Fix token persistency across server restarts
22cf600 
This fixes a regression introduced by LDAP support addition
(thelounge#477), which forces
users to re-login when the server restarts. This was originally
implemented in thelounge#370.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@astorije astorije

@maxpoulin64 maxpoulin64

Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
7 participants
@darshan-talati @MaxLeiter @maxpoulin64 @astorije @lindskogen @squidboylan @jedahan