ltb-project / service-desk

Application for support team who need to check and reset user passwords
https://service-desk.readthedocs.io/
GNU General Public License v3.0
49 stars 20 forks source link

Authentication Functionality #66

Closed mattv8 closed 2 years ago

mattv8 commented 2 years ago

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. image

The summary of added functionality is as follows:

Added Functionality:

I have tested this in an Active Directory LDAP environment, however this needs testing in a purely LDAP environment as well.

Quality Control:

coudot commented 2 years ago

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.

mattv8 commented 2 years ago

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, $use_authentication = true; or $authentication = 'ldap'; to open up the possibility for other forms of SSO/external auth down the road. By taking this approach, the other organizations that use Service Desk will be completely unaffected by this PR unless they decide to activate it in the configuration.

mattv8 commented 2 years ago

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 $ldap_authentication = true;. It should be set to false by default so as to not affect existing users.

coudot commented 2 years ago

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 samAccountName attribute, which is not standard and only used with AD. We have issues opened on the subject (#48, #52) and AD support requires a lot of work. Are you able to block/unblock an account in AD with current code? I don't think it could work as we use the standard LDAP attribute pwdAccountLockedTime and the attributes from AD.

I keep this PR and discussion open for the moment.

mattv8 commented 2 years ago

Hey @coudot, the usage of samAccoutnName is only "hard coded" for getting the desired LDAP attributes for some variables I set later. All LDAP standard username attributes are allowed for login, as defined in config.inc.php: $search_attributes = array('uid', 'cn', 'mail', 'samaccountname');.

Take a look at my proposed login.php, particularly these lines. It uses the above user-definable $search_attributes array for the username field to build the search filter for looking up usernames during the auth process.

mattv8 commented 2 years ago

@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.

coudot commented 1 year ago

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.

mattv8 commented 1 year ago

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.