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

Authentication Functionality #66

Closed
wants to merge 11 commits into from
Closed

Conversation

mattv8
Copy link

@mattv8 mattv8 commented Sep 14, 2022

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:

  • A basic user can log in and view attributes for their own account, or change their password.
  • Admin users can log in and view attributes for everybody's accounts, and check/change their passwords.
  • Admin users have access to the dashboard and search menus, basic accounts do not.
  • Website managers can specify who can and cannot login, as well as who has elevated privileges to edit accounts other than their own by setting authentication variables in config.inc.php. Please review config.inc.php for list of proposed authentication variables where an administrator can have granular control over access.

Added Functionality:

  • Basic site-wide authentication functionality. Users must be logged in to view attributes and change passwords.
  • Basic users can log in and view only their own LDAP attributes.
  • Admin users can log in and view everybody's LDAP attributes.
    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:

  • Testing in Active Directory environment.
  • Testing in LDAP environment.
  • Verify my French translations make sense.

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.
Don't lock down checkpassword and resetpassword to only admins
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.
@coudot
Copy link
Member

coudot commented Sep 15, 2022

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
Copy link
Author

mattv8 commented Sep 15, 2022

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.

Users can now specify $ldap_authentication = true if they want to use built-in LDAP authentication.
@mattv8
Copy link
Author

mattv8 commented Sep 15, 2022

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
Copy link
Member

coudot commented Sep 16, 2022

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.

@coudot coudot added the enhancement New feature or request label Sep 18, 2022
@mattv8 mattv8 force-pushed the master branch 2 times, most recently from 71cae52 to 616fd7c Compare September 19, 2022 23:26
@mattv8
Copy link
Author

mattv8 commented Sep 20, 2022

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 mattv8 deleted the branch ltb-project:master October 6, 2022 17:14
@mattv8 mattv8 closed this Oct 6, 2022
@mattv8 mattv8 deleted the master branch October 6, 2022 17:14
@mattv8 mattv8 restored the master branch October 6, 2022 17:16
@mattv8
Copy link
Author

mattv8 commented Oct 6, 2022

@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
Copy link
Member

coudot commented Oct 7, 2022

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
Copy link
Author

mattv8 commented Oct 7, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants