Switch Organizations to use TPT inheritance (was fix for Organization…

…-User FK) (#4926)

src/NuGetGallery.Core/Entities/EntitiesContext.cs

@@ -49,11 +49,6 @@ public EntitiesContext(string connectionString, bool readOnly)
         /// </summary>
         public IDbSet<User> Users { get; set; }
 
-        /// <summary>
-        /// Organization accounts.
-        /// </summary>
-        public IDbSet<Organization> Organizations { get; set; }
-
         IDbSet<T> IEntitiesContext.Set<T>()
         {
             return base.Set<T>();
@@ -127,25 +122,24 @@ protected override void OnModelCreating(DbModelBuilder modelBuilder)
                 .Map(c => c.ToTable("UserRoles")
                            .MapLeftKey("UserKey")
                            .MapRightKey("RoleKey"));
-            
+
             modelBuilder.Entity<Organization>()
-                .HasKey(o => o.Key)
-                .HasRequired<User>(o => o.Account)
-                .WithOptional(u => u.Organization)
-                .WillCascadeOnDelete(false); // Disabled to prevent multiple cascade paths.
+                .ToTable("Organizations");
 
             modelBuilder.Entity<Membership>()
-                .HasKey(m => new { m.OrganizationKey, m.MemberKey })
-                .HasRequired<Organization>(m => m.Organization)
-                .WithMany(o => o.Memberships)
-                .HasForeignKey(m => m.OrganizationKey)
-                .WillCascadeOnDelete(true); // Memberships will be deleted with the Organization.
-
-            modelBuilder.Entity<Membership>()
-                .HasRequired<User>(m => m.Member)
-                .WithMany(u => u.Memberships)
+                .HasKey(m => new { m.OrganizationKey, m.MemberKey });
+
+            modelBuilder.Entity<User>()
+                .HasMany(u => u.Organizations)
+                .WithRequired(m => m.Member)
                 .HasForeignKey(m => m.MemberKey)
-                .WillCascadeOnDelete(true); // Memberships will be deleted with the User (Member).
+                .WillCascadeOnDelete(true); // Membership will be deleted with the Member account.
+
+            modelBuilder.Entity<Organization>()
+                .HasMany(o => o.Members)
+                .WithRequired(m => m.Organization)
+                .HasForeignKey(m => m.OrganizationKey)
+                .WillCascadeOnDelete(true); // Memberships will be deleted with the Organization account.
 
             modelBuilder.Entity<Role>()
                 .HasKey(u => u.Key);

src/NuGetGallery.Core/Entities/IEntitiesContext.cs

@@ -13,19 +13,10 @@ public interface IEntitiesContext
         IDbSet<PackageRegistration> PackageRegistrations { get; set; }
         IDbSet<Credential> Credentials { get; set; }
         IDbSet<Scope> Scopes { get; set; }
+        IDbSet<User> Users { get; set; }
         IDbSet<UserSecurityPolicy> UserSecurityPolicies { get; set; }
         IDbSet<ReservedNamespace> ReservedNamespaces { get; set; }
 
-        /// <summary>
-        /// User or organization accounts.
-        /// </summary>
-        IDbSet<User> Users { get; set; }
-
-        /// <summary>
-        /// Organization accounts.
-        /// </summary>
-        IDbSet<Organization> Organizations { get; set; }
-
         Task<int> SaveChangesAsync();
         [System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1716:IdentifiersShouldNotMatchKeywords", MessageId = "Set", Justification="This is to match the EF terminology.")]
         IDbSet<T> Set<T>() where T : class;

src/NuGetGallery.Core/Entities/Organization.cs

@@ -6,27 +6,26 @@
 namespace NuGetGallery
 {
     /// <summary>
-    /// The organization associated with a <see cref="NuGetGallery.User" /> account.
+    /// Organization <see cref="NuGetGallery.User" /> account, based on TPT hierarchy.
     /// 
-    /// The Users table contains both User and Organization accounts. The Organizations table exists both
-    /// as a constraint for Membership as well as a place for possible Organization-only settings. If User
-    /// and Organization settings diverge, we can consider creating a separate UserSettings table as well.
+    /// With the addition of organizations, the users table effectively becomes an account table. Organizations accounts
+    /// are child types created using TPT inheritance. User accounts are not child types, but this could be done in the
+    /// future if we want to add constraints for user accounts or user-only settings.
     /// </summary>
-    public class Organization
+    /// <see href="https://weblogs.asp.net/manavi/inheritance-mapping-strategies-with-entity-framework-code-first-ctp5-part-2-table-per-type-tpt" />
+    public class Organization : User
     {
-        /// <summary>
-        /// Organization primary key.
-        /// </summary>
-        public int Key { get; set; }
+        public Organization() : base()
+        {
+        }
 
-        /// <summary>
-        /// Organization account (User).
-        /// </summary>
-        public User Account { get; set; }
+        public Organization(string name) : base(name)
+        {
+        }
 
         /// <summary>
-        /// Organization memberships.
+        /// Organization Memberships to this organization.
         /// </summary>
-        public virtual ICollection<Membership> Memberships { get; set; }
+        public virtual ICollection<Membership> Members { get; set; }
     }
 }

src/NuGetGallery.Core/Entities/User.cs

@@ -10,16 +10,11 @@
 namespace NuGetGallery
 {
     /// <summary>
-    /// NuGetGallery now supports both Users and Organizations which share common features such as:
-    /// - Namespace ownership
-    /// - Package ownership
-    /// - Security policies
-    /// - Name and email unique across both Users and Organizations
-    /// 
-    /// Organizations should not support UserRoles or Credentials, but this is not constrained by the
-    /// database. In the future, we could consider renaming the Users table to Accounts and adding a
-    /// Users table to constrain UserRoles and Credentials to only User accounts.
+    /// With the addition of organizations, the users table effectively becomes an account table. Organizations accounts
+    /// are child types created using TPT inheritance. User accounts are not child types, but this could be done in the
+    /// future if we want to add constraints for user accounts or user-only settings.
     /// </summary>
+    /// <see href="https://weblogs.asp.net/manavi/inheritance-mapping-strategies-with-entity-framework-code-first-ctp5-part-2-table-per-type-tpt" />
     public class User : IEntity
     {
         public User() : this(null)
@@ -36,14 +31,9 @@ public User(string username)
         }
 
         /// <summary>
-        /// <see cref="Organization"/> represented by this account, if any.
+        /// Organization memberships for a non-organization <see cref="User"/> account.
         /// </summary>
-        public Organization Organization { get; set; }
-
-        /// <summary>
-        /// Organization memberships for a <see cref="User"/> account.
-        /// </summary>
-        public virtual ICollection<Membership> Memberships { get; set; }
+        public virtual ICollection<Membership> Organizations { get; set; }
 
         [StringLength(256)]
         public string EmailAddress { get; set; }

src/NuGetGallery/Authentication/AuthenticationService.cs

@@ -127,7 +127,7 @@ await Auditing.SaveAuditRecordAsync(
                     return new PasswordAuthenticationResult(PasswordAuthenticationResult.AuthenticationResult.BadCredentials);
                 }
 
-                if (user.IsOrganization())
+                if (user is Organization)
                 {
                     _trace.Information($"Cannot authenticate organization account'{userNameOrEmail}'.");
 
@@ -219,7 +219,7 @@ await Auditing.SaveAuditRecordAsync(
                     return null;
                 }
 
-                if (matched.User.IsOrganization())
+                if (matched.User is Organization)
                 {
                     _trace.Information($"Cannot authenticate organization account '{matched.User.Username}'.");
 
@@ -490,7 +490,7 @@ public virtual ActionResult Challenge(string providerName, string redirectUrl)
 
         public virtual async Task AddCredential(User user, Credential credential)
         {
-            if  (user.IsOrganization())
+            if  (user is Organization)
             {
                 throw new InvalidOperationException(Strings.OrganizationsCannotCreateCredentials);
             }
@@ -646,7 +646,7 @@ public static ClaimsIdentity CreateIdentity(User user, string authenticationType
 
         private async Task ReplaceCredentialInternal(User user, Credential credential)
         {
-            if (user.IsOrganization())
+            if (user is Organization)
             {
                 throw new InvalidOperationException(Strings.OrganizationsCannotCreateCredentials);
             }

src/NuGetGallery/Extensions/UserExtensions.cs

@@ -37,15 +37,5 @@ public static Credential GetCurrentApiKeyCredential(this User user, IIdentity id
 
             return user.Credentials.FirstOrDefault(c => c.Value == apiKey);
         }
-
-        /// <summary>
-        /// Determines if the User (account) belongs to an organization.
-        /// </summary>
-        /// <param name="account">User (account) instance.</param>
-        /// <returns>True for organizations, false for users.</returns>
-        public static bool IsOrganization(this User account)
-        {
-            return account.Organization != null;
-        }
     }
 }

src/NuGetGallery/Migrations/201710250430326_AddOrganizations.cs → src/NuGetGallery/Migrations/201710301857232_Organizations.cs

@@ -3,7 +3,7 @@ namespace NuGetGallery.Migrations
     using System;
     using System.Data.Entity.Migrations;
 
-    public partial class AddOrganizations : DbMigration
+    public partial class Organizations : DbMigration
     {
         public override void Up()
         {
@@ -16,8 +16,8 @@ public override void Up()
                         IsAdmin = c.Boolean(nullable: false),
                     })
                 .PrimaryKey(t => new { t.OrganizationKey, t.MemberKey })
+                .ForeignKey("dbo.Organizations", t => t.OrganizationKey)
                 .ForeignKey("dbo.Users", t => t.MemberKey, cascadeDelete: true)
-                .ForeignKey("dbo.Organizations", t => t.OrganizationKey, cascadeDelete: true)
                 .Index(t => t.OrganizationKey)
                 .Index(t => t.MemberKey);
 
@@ -26,19 +26,17 @@ public override void Up()
                 c => new
                     {
                         Key = c.Int(nullable: false),
-                        AccountKey = c.Int(nullable: false),
                     })
                 .PrimaryKey(t => t.Key)
                 .ForeignKey("dbo.Users", t => t.Key)
                 .Index(t => t.Key);
-
         }
 
         public override void Down()
         {
-            DropForeignKey("dbo.Memberships", "OrganizationKey", "dbo.Organizations");
             DropForeignKey("dbo.Organizations", "Key", "dbo.Users");
             DropForeignKey("dbo.Memberships", "MemberKey", "dbo.Users");
+            DropForeignKey("dbo.Memberships", "OrganizationKey", "dbo.Organizations");
             DropIndex("dbo.Organizations", new[] { "Key" });
             DropIndex("dbo.Memberships", new[] { "MemberKey" });
             DropIndex("dbo.Memberships", new[] { "OrganizationKey" });

src/NuGetGallery/NuGetGallery.csproj

@@ -777,9 +777,9 @@
     <Compile Include="GlobalSuppressions.cs" />
     <Compile Include="Helpers\GravatarHelper.cs" />
     <Compile Include="Helpers\RegexEx.cs" />
-    <Compile Include="Migrations\201710250430326_AddOrganizations.cs" />
-    <Compile Include="Migrations\201710250430326_AddOrganizations.Designer.cs">
-      <DependentUpon>201710250430326_AddOrganizations.cs</DependentUpon>
+    <Compile Include="Migrations\201710301857232_Organizations.cs" />
+    <Compile Include="Migrations\201710301857232_Organizations.Designer.cs">
+      <DependentUpon>201710301857232_Organizations.cs</DependentUpon>
     </Compile>
     <Compile Include="Services\AsynchronousPackageValidationInitiator.cs" />
     <Compile Include="Services\CloudBlobFileStorageService.cs" />
@@ -1860,8 +1860,8 @@
     <EmbeddedResource Include="Migrations\201709202249402_AddPackageOwnershipRequestsPage.resx">
       <DependentUpon>201709202249402_AddPackageOwnershipRequestsPage.cs</DependentUpon>
     </EmbeddedResource>
-    <EmbeddedResource Include="Migrations\201710250430326_AddOrganizations.resx">
-      <DependentUpon>201710250430326_AddOrganizations.cs</DependentUpon>
+    <EmbeddedResource Include="Migrations\201710301857232_Organizations.resx">
+      <DependentUpon>201710301857232_Organizations.cs</DependentUpon>
     </EmbeddedResource>
     <EmbeddedResource Include="OData\QueryAllowed\Data\apiv1packages.json" />
     <EmbeddedResource Include="OData\QueryAllowed\Data\apiv1search.json" />

src/NuGetGallery/Services/PackagePermissionsService.cs

@@ -166,8 +166,9 @@ private static IEnumerable<PermissionLevel> GetPermissionLevels(IEnumerable<User
             }
 
             var matchingMembers = owners
-                .Where(u => u.Organization != null)
-                .SelectMany(u => u.Organization.Memberships)
+                .Where(o => o is Organization)
+                .Cast<Organization>()
+                .SelectMany(o => o.Members)
                 .Where(m => isUserMatch(m.Member));
 
             if (matchingMembers.Any(m => m.IsAdmin))

tests/NuGetGallery.Facts/Framework/Fakes.cs

@@ -39,26 +39,22 @@ public Fakes()
                 }
             };
 
-            Organization = new User("testOrganization")
+            Organization = new Organization("testOrganization")
             {
                 Key = 41,
                 EmailAddress = "confirmedOrganization@example.com",
-                Organization = new Organization()
-                {
-                    Key = 1
-                },
                 // invalid credentials for testing authentication constraints
                 Credentials = new List<Credential>
                 {
                     credentialBuilder.CreatePasswordCredential(Password)
                 }
             };
 
-            Organization.Organization.Memberships = new List<Membership>()
+            Organization.Members = new List<Membership>()
             {
                 new Membership
                 {
-                    Organization = Organization.Organization,
+                    Organization = Organization,
                     Member = User,
                     IsAdmin = true
                 }
@@ -117,7 +113,7 @@ public Fakes()
 
         public User User { get; }
 
-        public User Organization { get; }
+        public Organization Organization { get; }
 
         public User ShaUser { get; }
 

tests/NuGetGallery.Facts/Services/PackagePermissionsServiceFacts.cs

@@ -88,12 +88,17 @@ public void ReturnsExpectedPermissionLevels(IEnumerable<ReturnsExpectedPermissio
 
             private void CreateOrganizationOwnerAndAddUserAsMember(List<User> owners, User user, bool isAdmin)
             {
-                var organization = new Organization() { Memberships = new List<Membership>() };
-                var organizationOwner = new User("testorganization") { Key = _key++, Organization = organization };
-                owners.Add(organizationOwner);
-
-                var organizationMembership = new Membership() { Organization = organization, Member = user, IsAdmin = isAdmin };
-                organization.Memberships.Add(organizationMembership);
+                var organization = new Organization();
+                organization.Members = new[]
+                {
+                    new Membership()
+                    {
+                        Organization = organization,
+                        Member = user,
+                        IsAdmin = isAdmin
+                    }
+                };
+                owners.Add(organization);
             }
 
             private void AssertPermissionLevels(IEnumerable<User> owners, User user, IEnumerable<PermissionLevel> expectedLevels)