Support for LDAP authentication #562

Closed
wants to merge 31 commits into
from

Projects

None yet
@grenade
grenade commented Aug 8, 2012

This fork resolves issue #554 - #554

For our internal enterprise NuGet Gallery, we required LDAP based authentication. This fork contains the bare minimum of that implementation and is configuration driven (if the web.config contains LDAP configuration, it is seamlessly used as the authentication mechanism, users are silently added to the NuGetGallery database on first login).

The implementation works out of the box but one feature yet to be implemented is the ability to quietly update the user password in the NuGet Gallery database whenever the LDAP user changes it.
UPDATE: User passwords are no longer stored.

@pranavkm
Member

:shipit:

Haacked and others added some commits Jul 10, 2012
@Haacked Haacked Merge pull request #545 from NuGet/misc-tweaks
Misc tweaks
19ff073
@AnthonyCarl AnthonyCarl Allow caching of Nuget packages using package Hash as ETag 909e541
@AnthonyCarl AnthonyCarl Added AllowCachingOfPackage flag to set wether a FileStorageService a…
…llows caching of packages
a12a1f7
@AnthonyCarl AnthonyCarl Remove using spam 59bbcd5
@pranavkm pranavkm * Modifying GetPackages to always return listed packages
* Modifying stats to only consider package registrations with at least one
  listed package
28b11b3
@pranavkm pranavkm * Modifying Lucene to store latest and latest stable packages for
  prerelease filtering
* Fixing bug where using multiple IndexWriters would lock the index
* Improving search perf by adding boosts to fields at index time
* Removing the need to recreate index by updating the index when
  aggregate download counts are updated.
7a325ca
@pranavkm pranavkm Modifying feed service to use Lucene search 5aee8b7
@pranavkm pranavkm Work Item #405: Use Gravatar's secure URL when using SSL. d79a2c8
@osbornm
Member

Sad panda we should fix the helper :) But looks good :shipit:

Member

:shipit:

Member

@osbornm Like you'd ever get the lazy-ass owner of that code to ever fix that bug :p

@jeffhandley

I know there are hosts (AppHarbor for instance) where IsSecureConnection will be false because of being behind a reverse proxy, but if someone else who hosts the gallery is affected, they can issue a pull request to make this code still work in those cases.

pranavkm added some commits Jul 17, 2012
@pranavkm pranavkm Tweak request filtering to allow specific file extensions for ~/packages
and ~/api/vX/package paths
Work Item: #519
342febc
@pranavkm pranavkm Minor bug fixes:
* Work Item #447: Use Published Date instead of last updated date in the
  package display
* Store null values in the Download stats pag*
* Work Item #465: Allow at most 5 tags per package
89afb35
@maartenba

Will these changes be reflected in the public API? If yes, this sounds like a breaking change for many NuGet consumers out there.

Member

No, we'll continue pushing both those dates in our feed. That said this value is modified by the gallery when any metadata for a package changes. For instance, when you pushed a newer version of the package, the last updated value for both the current latest and the newly uploaded packages is set to the current time. https://github.com/NuGet/NuGetGallery/blob/89afb3508c3f03a06fb18614b603bc453c4d36ed/Website/Services/PackageService.cs#L376-380

This change makes this a little less confusing - See #447

@pranavkm pranavkm commented on the diff Aug 8, 2012
Website/Migrations/MigrationsConfiguration.cs
@@ -11,7 +12,9 @@ public class MigrationsConfiguration : DbMigrationsConfiguration<EntitiesContext
public MigrationsConfiguration()
{
- AutomaticMigrationsEnabled = false;
+ bool enabledInConfig;
@pranavkm
pranavkm Aug 8, 2012 NuGet member

Could you send this as a separate PR? Seems unrelated.

@grenade
grenade Aug 8, 2012

The LDAP stuff won't work without enabling here because of the extra property on User (DisplayName). It does make sense to disable by default though. Do you prefer that I comment out the code or disable from config (by default)?

@pranavkm
pranavkm Aug 9, 2012 NuGet member

I am not entirely sure why that piece of code disables auto-migrations, but I'd rather not change it. That said something does kick off migration. @half-ogre \ @jeffhandley might have a better understanding of how that works.

@pranavkm pranavkm and 1 other commented on an outdated diff Aug 8, 2012
Website/Services/UserService.cs
@@ -137,7 +139,8 @@ public virtual User FindByUsernameOrEmailAddressAndPassword(string usernameOrEma
// TODO: validate input
var user = FindByUsername(usernameOrEmail)
- ?? FindByEmailAddress(usernameOrEmail);
+ ?? FindByEmailAddress(usernameOrEmail)
+ ?? AutoEnroll(usernameOrEmail, password);
@pranavkm
pranavkm Aug 8, 2012 NuGet member

Could you write a unit test for this? Yank this out into an LDAPProvider type and then mock it out in the test

@grenade
grenade Aug 8, 2012

Ok, I added some unit tests, to the best of my ability but I've never used xunit before and don't really know how. I may revisit tomorrow, but I could really do with some help here. For the most part, I'm satisfied that bar the unit tests, I've implemented all your suggestions and I'm leaning towards leaving it there.

@pranavkm pranavkm and 1 other commented on an outdated diff Aug 8, 2012
Website/Services/UserService.cs
@@ -156,6 +159,50 @@ public virtual User FindByUsernameOrEmailAddressAndPassword(string usernameOrEma
return user;
}
+ private User AutoEnroll(string username, string password)
+ {
+ var ldapUri = ConfigurationManager.AppSettings.Get("Ldap:Uri");
+ if (string.IsNullOrWhiteSpace(ldapUri))
+ return null;
+ var ldapUserBase = ConfigurationManager.AppSettings.Get("Ldap:UserBase");
+ var ldapDomain = ConfigurationManager.AppSettings.Get("Ldap:Domain").ToLowerInvariant();
+
+ var ldapPropUsername = ConfigurationManager.AppSettings.Get("Ldap:Prop:Username") ?? "samaccountname";
+ var ldapPropEmail = ConfigurationManager.AppSettings.Get("Ldap:Prop:Email") ?? "mail";
+ var ldapPropDisplayName = ConfigurationManager.AppSettings.Get("Ldap:Prop:DisplayName") ?? "displayname";
+
+ if (username.ToLowerInvariant().StartsWith(string.Concat(ldapDomain, '\\')))
@pranavkm
pranavkm Aug 8, 2012 NuGet member

Use StringComparison.OrdinalIgnoreCase here instead of ToLower. Regular + instead of String.Concat

@pranavkm pranavkm and 1 other commented on an outdated diff Aug 8, 2012
Website/Services/UserService.cs
@@ -156,6 +159,50 @@ public virtual User FindByUsernameOrEmailAddressAndPassword(string usernameOrEma
return user;
}
+ private User AutoEnroll(string username, string password)
+ {
+ var ldapUri = ConfigurationManager.AppSettings.Get("Ldap:Uri");
+ if (string.IsNullOrWhiteSpace(ldapUri))
+ return null;
+ var ldapUserBase = ConfigurationManager.AppSettings.Get("Ldap:UserBase");
+ var ldapDomain = ConfigurationManager.AppSettings.Get("Ldap:Domain").ToLowerInvariant();
+
+ var ldapPropUsername = ConfigurationManager.AppSettings.Get("Ldap:Prop:Username") ?? "samaccountname";
+ var ldapPropEmail = ConfigurationManager.AppSettings.Get("Ldap:Prop:Email") ?? "mail";
+ var ldapPropDisplayName = ConfigurationManager.AppSettings.Get("Ldap:Prop:DisplayName") ?? "displayname";
+
+ if (username.ToLowerInvariant().StartsWith(string.Concat(ldapDomain, '\\')))
+ username = username.Replace(string.Concat(ldapDomain, '\\'), string.Empty);
@pranavkm
pranavkm Aug 8, 2012 NuGet member

username = username.Substring(ldapDomain + 2) would be safer. In the event that token recurs elsewhere in the username

@pranavkm pranavkm and 1 other commented on an outdated diff Aug 8, 2012
Website/Services/UserService.cs
+ if (string.IsNullOrWhiteSpace(ldapUri))
+ return null;
+ var ldapUserBase = ConfigurationManager.AppSettings.Get("Ldap:UserBase");
+ var ldapDomain = ConfigurationManager.AppSettings.Get("Ldap:Domain").ToLowerInvariant();
+
+ var ldapPropUsername = ConfigurationManager.AppSettings.Get("Ldap:Prop:Username") ?? "samaccountname";
+ var ldapPropEmail = ConfigurationManager.AppSettings.Get("Ldap:Prop:Email") ?? "mail";
+ var ldapPropDisplayName = ConfigurationManager.AppSettings.Get("Ldap:Prop:DisplayName") ?? "displayname";
+
+ if (username.ToLowerInvariant().StartsWith(string.Concat(ldapDomain, '\\')))
+ username = username.Replace(string.Concat(ldapDomain, '\\'), string.Empty);
+ var entry = new DirectoryEntry(string.Concat(ldapUri, '/', ldapUserBase), string.Concat(ldapDomain, '\\', username), password);
+ if (entry.NativeObject != null)
+ {
+ var searchResult = new DirectorySearcher(entry, string.Format("({0}={1})", ldapPropUsername, username), new[] { ldapPropUsername, ldapPropDisplayName, ldapPropEmail }).FindOne();
+ var user = new User(GetStringProperty(searchResult, ldapPropUsername).ToLowerInvariant(), cryptoSvc.GenerateSaltedHash(password, Constants.PBKDF2HashAlgorithmId))
@pranavkm
pranavkm Aug 8, 2012 NuGet member

We don't want to save the password in the gallery db. We always want to go against the current LDAP password not the one that was active when the account was created. In essence, the only information we ought to have in the db is a stub for the user (so the foreign keys work) but the rest of should be values that LDAP provides.

@grenade
grenade Aug 8, 2012

Added support for changed password. System will silently update the database whenever a new password is used by a user logging in. Will investigate not storing the password at all.

@grenade
grenade Aug 8, 2012

Done. LDAP password no longer stored.

@pranavkm pranavkm and 1 other commented on an outdated diff Aug 8, 2012
Website/Views/Shared/UserDisplay.cshtml
@@ -1,11 +1,15 @@
<div class="user-display">
@if (!User.Identity.IsAuthenticated)
{
- <span class="welcome"><a href="@Url.LogOn()">Log On</a></span>
- <a href="@Url.Action(MVC.Users.Register())" class="register">Register</a>
+ <span class="welcome"><a href="@Url.LogOn()">Log On</a></span>
+ if (string.IsNullOrWhiteSpace(System.Configuration.ConfigurationManager.AppSettings.Get("Ldap:Uri")))
@pranavkm
pranavkm Aug 8, 2012 NuGet member

Pass this info via the Model (if Model.AllowUserRegistrations).

@pranavkm pranavkm and 1 other commented on an outdated diff Aug 8, 2012
Website/Views/Shared/UserDisplay.cshtml
}
else {
- <span class="welcome"><a href="@Url.Action(MVC.Users.Account())">@User.Identity.Name</a></span>
+ var user = DependencyResolver.Current.GetService<IUserService>().FindByUsername(User.Identity.Name);
@pranavkm
pranavkm Aug 8, 2012 NuGet member

Pass it via the model.

@pranavkm pranavkm and 1 other commented on an outdated diff Aug 8, 2012
Website/Web.config
@@ -27,6 +27,13 @@
<add key="Gallery:ReleaseSha" value="" />
<add key="Gallery:ReleaseTime" value="" />
<add key="Gallery:SiteRoot" value="http://nuget.org/" />
+ <add key="Gallery:AutomaticMigrationsEnabled" value="true" />
+ <add key="Ldap:Uri" value="LDAP://ldap.example.com:389" />
@pranavkm
pranavkm Aug 8, 2012 NuGet member

Probably want this commented by default.

@pranavkm
Member
pranavkm commented Aug 9, 2012

Looks good. I'm probably going to revert the migration change and learn how that works. Additionally I'm going to squash your changes into a single commit just so that it's easier to manage it. Hope you don't mind losing out on a few commits :)

@akoeplinger

Shouldn't this be "Web.ForbiddenHandlers.config" ?

Member

Woops. Thanks for noticing this.

@pranavkm pranavkm was assigned Sep 5, 2012
@howarddierking

@pranavkm please close this out during the next sprint

@anurse
Member
anurse commented Feb 8, 2013

We're definitely interested in multiple-auth scenarios, and when we have a cleaner auth mechanism, we'd be happy to consider a PR which adds LDAP support to that. However, for now I think we're going to close this out. If it's working in your fork, awesome, and we'll direct people towards that fork if they're interested in LDAP auth.

@anurse anurse closed this Feb 8, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment