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 LDAP support #6003

Closed
wants to merge 1 commit into from
Closed

Added LDAP support #6003

wants to merge 1 commit into from

Conversation

Falco20019
Copy link
Contributor

@Falco20019 Falco20019 commented May 29, 2018

Fixes #554, #5288

Since I also needed it, I added support for LDAP / Windows Active Directory. I was only able to test it within our company with our companies AD. To use it, you have to configure the Auth.LdapUser block in the Web.config file.

When Auth.LdapUser.Enabled, the register form is used to check the given credentials (username and password) and if the credentials are valid, it creates a new account with the username as ldap.user credentials instead of saving the password with password.v3 credentials. No password credentials will be created for this account. If you want to use LdapUser with the current password as LocalUser fallback, you can just add the credentials to the account in AuthenticationService.cs right after the CreateIdentity was called (around line 252). This may be something to make configurable.

If you only want to support LDAP without regular passwords (i.e. when you are setting up a new server), I advise you to set Auth.LocalUser.Enabled in Web.config to false. They can coexist, since they both have an own cookie, but in most cases, you really only want to have either LDAP or local users.

Right now, there is no migration path to change existing user credentials from password.v3 to ldap.user. In case the NuGet username already matches the LDAP username, the easiest way would be to update the dbo.Credentials entries for password.v3 and set Type= ldap.user and Value = (SELECT [Username] FROM [Users] WHERE [Key] = [UserKey]).

Tests are currently missing. I didn't want to put in the effort in case that it won't be merged. So I will add them, in case that there are no points speaking against merging.

@dnfclas
Copy link

dnfclas commented May 29, 2018

CLA assistant check
All CLA requirements met.

@awickham10
Copy link

awickham10 commented Mar 15, 2019

Do we know when this will be accepted? I could use this as well.

@scottbommarito can you help? Is there anything else that needs added to get this accepted?

@DjArt
Copy link

DjArt commented Sep 14, 2021

So?

@joelverhagen
Copy link
Member

Given the age of this PR, I'm going to close it.

I think the main concern I have about this PR is that it adds complexity to the codebase with a scenario we do not plan to use directly on NuGet.org. If the code can be factored in such a way that the ongoing maintenance costs are low, then we can consider it. In other words, we can certainly accept a contribution like this in the future, but we need to be careful not to add overhead to our team's main objective -- the improvement and maintenance of NuGet.org itself.

I wonder if there is any way to re-use NuGetGallery's existing MSA/AAD integration with LDAP. I don't know how Azure Active Directory and enterprise (Windows?) Active Directory overlap.

If anyone is interested in pushing this change through, I think it would be worth going through the formal proposal as described here: https://github.com/NuGet/Home/tree/dev/meta#nuget-proposal-process

This would allow us to align on the direction first, and later jump into the code.

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.

NuGet Gallery should support LDAP for on-premise deployments
6 participants