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

Support for LDAP authentication #562

Closed
wants to merge 31 commits into from
Closed

Support for LDAP authentication #562

wants to merge 31 commits into from

Conversation

grenade
Copy link
Contributor

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

azzlack and others added 21 commits June 4, 2012 12:56
There is no need to fetch and rebase in separate commands. Pull accepts a --rebase switch which does the fetch and rebase operations in one command.
Fixed stats formatting on home page when download or package count is 0
Fixed problem with PUT and DELETE verbs not working on servers with WebDAV installed
* Modifying stats to only consider package registrations with at least one
  listed package
  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.
* 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
@@ -11,7 +12,9 @@ public class MigrationsConfiguration : DbMigrationsConfiguration<EntitiesContext

public MigrationsConfiguration()
{
AutomaticMigrationsEnabled = false;
bool enabledInConfig;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. LDAP password no longer stored.

@pranavkm
Copy link
Contributor

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 :)

@ghost ghost assigned pranavkm Sep 5, 2012
@howarddierking
Copy link
Contributor

@pranavkm please close this out during the next sprint

@analogrelay
Copy link
Contributor

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.

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.