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.

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

Added login via LDAP #44

Merged
merged 1 commit into from Jun 1, 2019

Conversation

@HelioStrike
Copy link
Collaborator

commented May 24, 2019

Added login via LDAP. The user can click on the 'Sign In' button, to reach a (not so shiny)login page.

Screenshot from 2019-05-25 09-12-22
On entering the correct credentials, the user is logged into the Atlas, and can do Atlas stuff.

Screenshot from 2019-05-24 09-52-42

Wrong credentials give us an error message (I think I'll focus on the looks of the message in a future PR).
Screenshot from 2019-05-25 09-06-21

@cintiadr

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

I know you copied this code from ID, but I think it's unnecessary complex to look for a user by username.

I'd prefer if we refactor searchUser to be more like searchGroups, and delete the convertUser and searchRaw methods. It's not really clear to me why the code is even like that in ID.

@cintiadr

This comment has been minimized.

Copy link
Contributor

commented May 24, 2019

Probably you do not even want to load the user groups. Best case scenario you can add later to test if this user has permission to login (or if it's admin) based if it belongs to atlas-user or atlas-admin. But you have no reason to load all groups of a given user.

It might be simpler if for now you just ignore it completely.

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-auth branch 2 times, most recently from 13c4435 to f29306a May 24, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented May 24, 2019

@cintiadr I've updated the code. Please take a look at it again. :)

ldap.js Outdated Show resolved Hide resolved
ldap.js Outdated Show resolved Hide resolved
//If unable to fetch user details, redirect to login
if(err) {
console.log(err);
res.redirect('/login');

This comment has been minimized.

Copy link
@bmamlin

bmamlin May 25, 2019

Member

Will this pass the error back to the login page so it can be displayed?

This comment has been minimized.

Copy link
@HelioStrike

HelioStrike May 27, 2019

Author Collaborator

I'm storing the error in the session, so that it can be displayed later. What do you think of it?

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-auth branch 2 times, most recently from 0d1fc93 to 6d8df36 May 25, 2019

@HelioStrike

This comment has been minimized.

Copy link
Collaborator Author

commented May 25, 2019

@bmamlin I was just being polite to the more determined hackers!

Sorry, I missed that log, but the code's updated now! The error message bit wasn't implemented earlier, but I added it to the code, and you can find a screenshot in the first comment of the PR. :)

@cintiadr
Copy link
Contributor

left a comment

I think it's only missing removing var client = ldap.createClient from authentication.js

conf.example.js Outdated Show resolved Hide resolved
ldap.js Outdated Show resolved Hide resolved
conf.example.js Outdated Show resolved Hide resolved
routes/authentication.js Outdated Show resolved Hide resolved

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-auth branch 2 times, most recently from 1c93667 to b28993c May 25, 2019

@HelioStrike HelioStrike force-pushed the HelioStrike:3.x-auth branch from b28993c to cb83764 May 27, 2019

@cintiadr

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

@bmamlin / @harsha89 , are you ok merging this PR? I'd like it merged so I can try to get staging back working, even if there's work to do :)

@cintiadr cintiadr closed this Jun 1, 2019

@cintiadr cintiadr reopened this Jun 1, 2019

@cintiadr cintiadr merged commit 65a4411 into openmrs:3.x Jun 1, 2019

@HelioStrike HelioStrike deleted the HelioStrike:3.x-auth branch Jul 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.