Organizations: transform account on confirmation #5228

chenriksson · 2018-01-03T17:32:56Z

Added embedded resource SQL script for account migration
Added UsersController confirmation action
Added UserService migration API
Added IDatabase for test mocks

Screenshot of error, if transform fails. Will confirm messages with @anangaur before merging.

scottbommarito

Looks good in general, I'm just curious why a SQL script was necessary instead of using entity framework directly. Whatever the reason, it should be in a comment in the code.

scottbommarito · 2018-01-03T21:23:06Z

src/NuGetGallery/Controllers/UsersController.cs

+
+        [HttpGet]
+        [Authorize]
+        public virtual async Task<ActionResult> ConfirmTransform(string accountName, string token)


accountName should probably be accountToTransform for clarity

scottbommarito · 2018-01-03T21:28:55Z

src/NuGetGallery/Infrastructure/AddMigrateToOrganization.Up.sql

@@ -0,0 +1,47 @@
+-- Copyright (c) .NET Foundation. All rights reserved.


Why is this a script and not done through EntityFramework?

I can't see anything here that would make it necessary to use a script. We've done transactions through EF before.

EF has a limitation with inheritance in that it doesn't support changing an object's type. It must be done with SQL. I can add a comment in UserService.TransformToOrganizationAccount.

Regarding transactions, note that by using a sproc here I also don't need a custom transaction and suspended execution strategy in the Gallery code.

The disadvantage of suspending execution strategy in Gallery code is that I believe it disables SqlAzure strategy defaults such as retry logic.

scottbommarito · 2018-01-03T21:31:00Z

src/NuGetGallery/Infrastructure/AddMigrateToOrganization.Up.sql

+
+	BEGIN TRANSACTION
+	BEGIN TRY
+		-- Ensure Organizations do not have credentials or memberships


This isn't "ensuring", this is wiping any of these existing entities if they exist.

IMO we should fail the migration if the user has any memberships--that would suggest that the user was already an organization, which is not something we should just ignore.

Sounds reasonable... I'll add a SELECT outside the transaction and fail fast if the account has any memberships.

Gallery-side, we should also check for memberships before allowing migration. Account would need to be removed from organization(s) to be eligible for migration.

scottbommarito · 2018-01-03T21:32:32Z

src/NuGetGallery/Controllers/UsersController.cs

+
+        private bool CanTransformIntoOrganization(User user)
+        {
+            if (!user.Confirmed || user.IsAdministrator())


I would assume if that user was already an organization, we would also want to reject the transformation. Any reason this condition isn't also filtered here?

I could add additional validation here to guard against it.

scottbommarito · 2018-01-03T21:36:54Z

src/NuGetGallery/Strings.resx

@@ -688,4 +688,25 @@ For more information, please contact '{2}'.</value>
  <data name="PackageIsLocked" xml:space="preserve">
    <value>Package '{0}' has been locked. This means you cannot publish a new version or change the listing status of a published package version. Please contact support@nuget.org.</value>
  </data>
+  <data name="TransformAccount_AdminDoesNotHaveTenantId" xml:space="preserve">
+    <value>Failed to transform the account because you do not have an Azure AD sign-in with tenant ID.</value>


How about: You account must have an Azure AD sign-in with a tenant ID.

This would be shown on confirmation (admin sign-in), so it's not 'Your account'.

/cc @anangaur

Anand, let me know if you want to provide the exact error messages for these migration failure scenarios. Spec didn't go into details on this.

scottbommarito · 2018-01-03T21:39:43Z

src/NuGetGallery/Strings.resx

+    <value>Your account must be confirmed before you can create an organization.</value>
+  </data>
+  <data name="TransformAccount_DatabaseError" xml:space="preserve">
+    <value>Failed to transform the account due to a database error.</value>


"Database error" is a little bit cryptic to the average user.

How about: There was an unexpected error in transforming the account.

You can also throw in Contact support or something like that if this is something they should contact us about.

This would happen if the migration request didn't exist, a DB constraint prevented the update, etc. Probably not very likely. I'm ok with 'unexpected error'.

scottbommarito · 2018-01-03T21:41:55Z

src/NuGetGallery/Views/Users/AccountTransformFailed.cshtml

+        <h1>
+            Transform Account: Failed
+        </h1>
+        @if (TempData.ContainsKey("TransformError"))


If there isn't a TransformError, we should probably should a default message of some sort.

On migration success, this should redirect to the (future) 'Manage Organization' page. For now, I redirect to the admin account page.

I only added this view because I needed a way of displaying an error to the user.

scottbommarito · 2018-01-03T21:46:36Z

tests/NuGetGallery.Facts/Controllers/UsersControllerFacts.cs

+            {
+                // Arrange
+                var controller = GetController<UsersController>();
+                var currentUser = new User() { UnconfirmedEmailAddress = "unconfirmed@example.com" };


nit: might be easier to use users from Fakes for this

E.g.

var fakes = Get<Fakes>(); var currentUser = fakes.User;

The advantages are clearer when you need an Admin user below:

var fakes = Get<Fakes>(); var adminUser = fakes.AdminUser;

skofman1 · 2018-01-04T17:54:12Z

src/NuGetGallery/Web.config

@@ -41,6 +41,7 @@

    <add key="Gallery.AsynchronousPackageValidationEnabled" value="false" />
    <add key="Gallery.BlockingAsynchronousPackageValidationEnabled" value="false" />
+    <add key="Gallery.OrganizationsEnabledForDomains" value="microsoft.com" />


microsoft.com should be set in our private configs, not the default ones

Forgot to remove it - will do

skofman1 · 2018-01-04T18:12:06Z

src/NuGetGallery/Controllers/UsersController.cs

+            }
+            catch (TransformAccountException e)
+            {
+                TempData["TransformError"] = e.AsUserSafeException().GetUserSafeMessage();


TransformAccountException doesn't inherit from UserSafeException, so we will always show a generic message here. Is this the intention?

skofman1 · 2018-01-04T18:13:34Z

src/NuGetGallery/Services/UserService.cs

+
+            if (string.IsNullOrWhiteSpace(token))
+            {
+                throw new ArgumentNullException(nameof(token));


ArgumentException since the value isn't necessarily null

skofman1 · 2018-01-04T18:15:52Z

src/NuGetGallery/Strings.resx

@@ -688,4 +688,22 @@ For more information, please contact '{2}'.</value>
  <data name="PackageIsLocked" xml:space="preserve">
    <value>Package '{0}' has been locked. This means you cannot publish a new version or change the listing status of a published package version. Please contact support@nuget.org.</value>
  </data>
+  <data name="TransformAccount_AdminDoesNotHaveTenantId" xml:space="preserve">
+    <value>Failed to transform the account because you do not have an Azure AD sign-in with tenant ID. Contact support@nuget.org for more details.</value>


I don't think the average user knows what a tenant id is. Please go over the messages with Anand :)

chenriksson · 2018-01-04T18:43:28Z

Per discussion in stand-up, I'm going to see if I can create the migration SQL script as an embedded resource and just invoke it inline from the Gallery. Having DB tests sounds nice, but this sounds like less work.

/cc @skofman1 @joelverhagen @scottbommarito

skofman1 · 2018-01-04T18:43:33Z

src/NuGetGallery/Services/UserService.cs

+            catch (Exception ex) when (ex is SqlException || ex is DataException)
+            {
+                // EF exception when saving account transformation to the database.
+                throw new TransformAccountException(Strings.TransformAccount_DatabaseError);


The exceptions you throw here are swallowed in UsersController, so we have no log of them. Please add a trace (somewhere)

skofman1 · 2018-01-04T18:52:50Z

@chenriksson , we will need DB tests anyhow, since if the script breaks, there's no way to know about it.

skofman1

🕐

chenriksson · 2018-01-04T23:11:10Z

@skofman1 You're right - we still need the DB tests.

I've reverted the migration with sproc, and instead execute SQL via embedded resource.

joelverhagen · 2018-01-08T21:44:07Z

src/NuGetGallery.Core/Entities/DatabaseWrapper.cs

+                sqlCommand = await reader.ReadToEndAsync();
+            }
+
+            if (!string.IsNullOrEmpty(sqlCommand))


Couldn't execute an empty command be indicative of a bug? I think we should throw here -- or let ExecuteSqlCommandAsync do its thing.

joelverhagen · 2018-01-08T21:47:29Z

src/NuGetGallery/Services/UserService.cs

+
+        public async Task<bool> TransformUserToOrganization(User accountToTransform, User adminUser, string token)
+        {
+            // todo: check for tenantId and add organization policy to enforce this (future work, with manage organization)


Track TODO unless it's gonna happen very soon

It's my next work item. I've been tracking all migration with #5112, but can split this into smaller tasks.

anangaur · 2018-01-08T21:50:17Z

Proposal:

Failed to transform to an organization

'Microsoft' is already an organization on NuGet.org.

chenriksson · 2018-01-08T22:23:59Z

@anangaur Thanks! Here's updated screenshot:

Can you confirm these other messages? Note that most of these would be detected before the user can create the migration request (PR #5241), but I also safe guard against it here.

skofman1 · 2018-01-09T17:59:34Z

src/NuGetGallery/Services/UserService.cs

+            }
+            catch (Exception ex) when (ex is SqlException || ex is DataException)
+            {
+                // todo: log exception


Removed error handling, which seems more consistent with how we handle SQL exceptions in other places.

skofman1 · 2018-01-09T18:01:50Z

@chenriksson , looks good! Please add a log and then

chenriksson added 2 commits January 2, 2018 14:44

Organizations: transform on confirmation

5a02f4e

Tests and cleanup

930abd4

scottbommarito suggested changes Jan 3, 2018

View reviewed changes

Fix query

552e728

chenriksson requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, cristinamanum, skofman1 and scottbommarito January 3, 2018 22:21

PR feedback

c8978bb

skofman1 reviewed Jan 4, 2018

View reviewed changes

skofman1 requested changes Jan 4, 2018

View reviewed changes

chenriksson added 2 commits January 4, 2018 15:00

PR feedback

8cba4dd

Cleanup

12df6a3

chenriksson force-pushed the chenriks-org-transform branch from 9ee1ec5 to 12df6a3 Compare January 4, 2018 23:07

scottbommarito approved these changes Jan 8, 2018

View reviewed changes

chenriksson mentioned this pull request Jan 8, 2018

[Organizations]: UI for creating migration request #5241

Merged

joelverhagen reviewed Jan 8, 2018

View reviewed changes

joelverhagen approved these changes Jan 8, 2018

View reviewed changes

PR feedback

551ce83

skofman1 reviewed Jan 9, 2018

View reviewed changes

skofman1 approved these changes Jan 9, 2018

View reviewed changes

PR feedback

90a7d5b

chenriksson merged commit 3addad3 into dev Jan 9, 2018

chenriksson mentioned this pull request Jan 10, 2018

Obfuscate new transform account route #5260

Merged

chenriksson deleted the chenriks-org-transform branch January 25, 2018 00:12

chenriksson mentioned this pull request Jan 26, 2018

S130: Track organizations progress #5361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organizations: transform account on confirmation #5228

Organizations: transform account on confirmation #5228

chenriksson commented Jan 3, 2018 •

edited

Loading

scottbommarito left a comment

scottbommarito Jan 3, 2018

scottbommarito Jan 3, 2018

chenriksson Jan 3, 2018

chenriksson Jan 3, 2018

scottbommarito Jan 3, 2018

chenriksson Jan 3, 2018

scottbommarito Jan 3, 2018

chenriksson Jan 3, 2018

scottbommarito Jan 3, 2018

chenriksson Jan 3, 2018

scottbommarito Jan 3, 2018

chenriksson Jan 3, 2018

scottbommarito Jan 3, 2018

chenriksson Jan 3, 2018

scottbommarito Jan 3, 2018

skofman1 Jan 4, 2018

chenriksson Jan 4, 2018

skofman1 Jan 4, 2018

skofman1 Jan 4, 2018

skofman1 Jan 4, 2018

chenriksson commented Jan 4, 2018

skofman1 Jan 4, 2018

skofman1 commented Jan 4, 2018

skofman1 left a comment

chenriksson commented Jan 4, 2018

joelverhagen Jan 8, 2018

joelverhagen Jan 8, 2018

chenriksson Jan 8, 2018

anangaur commented Jan 8, 2018

chenriksson commented Jan 8, 2018

skofman1 Jan 9, 2018

chenriksson Jan 9, 2018

skofman1 commented Jan 9, 2018

		@@ -0,0 +1,47 @@
		-- Copyright (c) .NET Foundation. All rights reserved.

Organizations: transform account on confirmation #5228

Organizations: transform account on confirmation #5228

Conversation

chenriksson commented Jan 3, 2018 • edited Loading

scottbommarito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenriksson commented Jan 4, 2018

Choose a reason for hiding this comment

skofman1 commented Jan 4, 2018

skofman1 left a comment

Choose a reason for hiding this comment

chenriksson commented Jan 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anangaur commented Jan 8, 2018

Failed to transform to an organization

chenriksson commented Jan 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skofman1 commented Jan 9, 2018

chenriksson commented Jan 3, 2018 •

edited

Loading