-
Notifications
You must be signed in to change notification settings - Fork 21
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
Authentication Functionality #66
Conversation
Added password-protection to the Service-Desk website using in-place LDAP authentication. Website managers can specify who can and cannot login, as well as who has elevated privileges to edit accounts other than their own; set in config.inc.php.
handled by index.php
I noticed that if somebody guesses the correct admin username, the search menu is displayed. It will not show results, however, unless $authenticated=true. Now I only update the $isadmin variable upon a successful authentication.
Verifies user is an admin before allowing access to protected pages.
Now, authenticated users are routed directly to "display" for their own account, and can navigate from there. There is a welcome header at "display" now too.
Removed the ability for non-admin users and "self" to see the "checkpassword" field since it is assumed they know their password when they authenticated.
Hello @mattv8 thanks for your work and your interest in this project. However you introduce too much changes at once, we have several issues in the roadmap, some covers your PR. We focus today on 0.5 milestone: https://github.com/ltb-project/service-desk/milestone/5 Service Desk is already used by a lot of organizations, we should keep the main behaviors. Concerning authentication for example, we should still be compatible with SSO/external auth. And we do not intend for the moment to replace full IdM products like FusionDirectory or Midpoint. You're welcome to help us improve the software, but feature by feature, in separate PR. |
This is a frustrating response to me in many ways. I was unaware of the roadmap, it is not linked or referenced anywhere on the project page. I did not see a use-case for SSO/external auth since LDAP is an authentication database, so why not just build it in to the application. For my organization, we MUST have a sensitive LDAP directory behind some sort of authentication. Even if it is on a private/protected network. I assume there are others that would agree with me, which is why I spent the time making this PR. With that said, I do understand the points you are making, regarding using individual PR's to address the open issues on the roadmap and that you do not intend to replace full IdM products. Would you consider approving this PR if I added a configuration option to turn off Authentication by default? This way a site administrator could decide if they want to use it or not. We could add the option, for example, |
Users can now specify $ldap_authentication = true if they want to use built-in LDAP authentication.
Okay I have pushed another commit that will allow users to decide whether or not they want to use the built-in authentication or do something else. I'll also lessen the scope of this PR to just built-in authentication only. I've edited my initial PR description to reflect this. Hopefully this addresses some of your concerns. :) The flag can be set in config.inc.php, called |
Hello @mattv8 first of all, thanks for taking time to dig into this project and propose your help. I don't know which experience you have with free software and community, but sending a PR for such big feature without any previous discussion with the core team is not the good way to go. I'm not saying your work is not good or not interesting, but as a maintainer, I can't take it as it is. Managing authentication is addressed for the moment in the software by configuring the virtual host: https://service-desk.readthedocs.io/en/stable/configuration-apache.html#ldap-authentication-and-authorization We could indeed improve this by adding a login page as you proposed. We must first discuss about roles and configuration settings. Another point with your PR is that you seem to work with Active Directory, and you hard coded the usage of I keep this PR and discussion open for the moment. |
71cae52
to
616fd7c
Compare
Hey @coudot, the usage of Take a look at my proposed |
@coudot I am going to leave this PR as it stands here if you want the feature, but I am going to let the PR branch go stale in my fork of your project and move it to another branch (authentication-functionality), as I am actively working on adding other features and would like to be able to contribute to the Master in my own fork of your project. The ball is in your court if you want to merge it or not. I am happy to submit PR's for the new features I am adding if you would like, just let me know. They include the ability to edit individual LDAP attributes as well as ability to create new accounts from the Service Desk web interface. Leaving this note here for the record. |
Hello @mattv8 thanks for your message. I need some time on my side to organize the roadmap and see how to include all the features you want. |
Sounds good to me. Let me know when you have updated the roadmap and I will organize my contributions into separate issues/PR's to fit your roadmap's framework. Depending on what you decide. |
I have added password-protection to the Service-Desk website using in-place LDAP authentication. This PR can be merged as-is,
however I am still working on giving users ability to edit some of their LDAP attributes, as specified by the config.The summary of added functionality is as follows:
Added Functionality:
Ability for basic users to easily edit all or most of their own LDAP attributes, such as 'mobile' or 'displayname'.Ability for admins easily to edit all users' LDAP attributes.I have tested this in an Active Directory LDAP environment, however this needs testing in a purely LDAP environment as well.
Quality Control: