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

Simple authorization via LDAP #310

Merged
merged 8 commits into from
Apr 17, 2017
Merged

Conversation

strangeman
Copy link
Contributor

Algorithm: #118 (comment)

I have some doubts about my code:

  • How can I do proper logging for LDAP errors/messages? Currently, I use 'dumb' println, because I can't see any logs except Gin logging
  • We do not need to edit username and password for LDAP users. I just make this fields read-only on frontend (but editing still possible via API). Should I add additional validation, or we may keep this way for editing?
  • I added the migration to the same file - 2.3.0.sql because 2.3.0 isn't released. Is it ok?

@matejkramny
Copy link
Contributor

  • eventually logrus will be used, but it's easy to refactor
  • The API should also reject/ignore changes in this case
  • 2.3.0 is ok. The versions are a bit meaningless anyway since it doesn't correlate to releases

Thanks for doing this 👍

@strangeman
Copy link
Contributor Author

All done, code available for review. :)

@matejkramny matejkramny self-assigned this Mar 28, 2017
Copy link

@ruriky ruriky left a comment

Choose a reason for hiding this comment

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

Great work! We have been waiting for LDAP/AD support. Some comments below.

api/login.go Outdated
util.Config.LdapSearchDN,
ldap.ScopeWholeSubtree, ldap.NeverDerefAliases, 0, 0, false,
fmt.Sprintf(util.Config.LdapSearchFilter, auth),
[]string{"dn", "mail", "uid", "cn"},
Copy link

Choose a reason for hiding this comment

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

Would be nice to get these searched parameters configurable. For example our AD doesn't have "uid" parameter at all for the user object and the "cn" doesn't contain user's name but some identification string.

util/config.go Outdated
@@ -231,4 +240,59 @@ func (conf *configType) Scan() {
conf.EmailAlert = false
}

var LdapAnswer string
fmt.Print(" > Enable LDAP authentificaton (y/n, default n): ")
Copy link

Choose a reason for hiding this comment

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

authentication

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, thank you.

@strangeman
Copy link
Contributor Author

@ruriky All done! Sorry for the delay, I caught the flu. :(

@ruriky
Copy link

ruriky commented Apr 4, 2017

Thank you strangeman for taking the time to implementing the changes I suggested.

I'm happy with the current state of this change.

@damex
Copy link

damex commented Apr 15, 2017

hey, is there any eta on when it will get merge to the tree?

@Echobob Echobob merged commit 7ea40d7 into semaphoreui:master Apr 17, 2017
@Echobob
Copy link
Contributor

Echobob commented Apr 17, 2017

Thanks guys!

@Echobob Echobob mentioned this pull request Apr 17, 2017
@strangeman strangeman deleted the ldap-auth branch April 17, 2017 05:53
matejkramny added a commit that referenced this pull request Apr 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants