Update External Account Linking Flow #5338

scottbommarito · 2018-01-24T01:12:55Z

Sign-in Flow change

Check if the email received links to existing account
- Existing NuGet.org account
  - Found the matching MSA/AAD login - all good, login successful.
  - Found but has other external logins(MSA/AAD)? - Reject Slide 14
  - No external logins? - Link to Microsoft account - Slide 15
    - Disallow email change in link external account flow.
- No accounts with specified email address.
  - Remove the "Create account page" - Slide 16
  - Show the create a new account page flow - Slide 17

… static when it is provided by model

skofman1 · 2018-01-24T17:21:34Z

src/NuGetGallery/Strings.resx

@@ -736,4 +736,13 @@ For more information, please contact '{2}'.</value>
  <data name="TransformAccount_RequestExists" xml:space="preserve">
    <value>Another tranform request was created on {0} with organization admin '{1}'.</value>
  </data>
+  <data name="LinkingMultipleExternalAccountsUnsupported" xml:space="preserve">


LinkingMultipleExternalAccountsUnsupported, LinkingMultipleExternalAccountsUnsupportedFailure - the names are too similar.
Can you rename to something clearer?

couldn't come up with better names--they're the same issue, just one is a validation error for a form and one is a validation error for a page...any ideas?

LinkingMultipleExternalAccountsUnsupported -> AccountIsLinkedToAnotherExternalAccount
LinkingMultipleExternalAccountsUnsupportedFailure -> LinkingMultipleExternalAccountsUnsupported

skofman1 · 2018-01-24T17:22:16Z

src/NuGetGallery/Controllers/AuthenticationController.cs

+                    existingUserLinkingError = Strings.LinkingMultipleExternalAccountsUnsupported;
+                }
+
+                if (existingUser is Organization)


skofman1

How was this tested?

skofman1 · 2018-01-24T17:23:51Z

src/NuGetGallery/Controllers/AuthenticationController.cs

+                {
+                    existingUserCanBeLinked = false;
+                    existingUserLinkingError = Strings.LinkingMultipleExternalAccountsUnsupported;
+                }


skofman1 · 2018-01-24T17:28:36Z

src/NuGetGallery.Core/Extensions/CredentialExtensions.cs

+
+namespace NuGetGallery
+{
+    public static class CredentialExtensions


There's a class called CredentialTypes where we have similar methods. Please put those there

scottbommarito · 2018-01-24T18:18:12Z

@skofman1 You can actually test the entire MSA/AAD integration very easily by following some instructions in the OneNote. @shishirx34 showed it to me when I asked him.

chenriksson · 2018-01-24T20:42:44Z

src/NuGetGallery/ViewModels/LogOnViewModel.cs

@@ -33,6 +33,13 @@ public class AssociateExternalAccountViewModel
        public string ProviderAccountNoun { get; set; }
        public string AccountName { get; set; }
        public bool FoundExistingUser { get; set; }
+        public bool ExistingUserCanBeLinked { get; set; }


Can this just return string.IsNullOrEmpty(ExistingUserLinkingError)? Then can remove default ctor below.

chenriksson · 2018-01-24T20:51:33Z

src/NuGetGallery/Strings.resx

+    <value>We found an account with the email address associated with this external account ({0}), but linked to a different external account.</value>
+  </data>
+  <data name="LinkingMultipleExternalAccountsUnsupported" xml:space="preserve">
+    <value>You cannot link your NuGet.org account to multiple external accounts.</value>


Should this direct user to Account page to unlink other external accounts?

@anangaur was working on the messaging--not sure the status of it. I suggested doing something similar to him or at least a message explaining how to do so.

Please run through the flow for strings with @anangaur once before merging.

chenriksson · 2018-01-24T20:58:17Z

src/NuGetGallery/Controllers/AuthenticationController.cs

@@ -373,12 +382,37 @@ public ActionResult ChallengeAuthentication(string returnUrl, string provider)
                {
                    existingUser = _userService.FindByEmailAddress(email);
                }
+
+                var existingUserCanBeLinked = true;


would remove existingUserCanBeLinked, per other comment above

chenriksson · 2018-01-24T20:59:13Z

src/NuGetGallery/Controllers/AuthenticationController.cs

+                        CultureInfo.CurrentCulture, 
+                        existingUserLinkingError, 
+                        email);
+                }


Would remove this block and just do string.Format when setting existingUserLinkingError.

When we implement more error messages with different format strings we can do that, but right now I don't see a reason to do so.

chenriksson · 2018-01-24T20:59:51Z

src/NuGetGallery/Controllers/AuthenticationController.cs

+                {
+                    existingUserCanBeLinked = false;
+                    existingUserLinkingError = Strings.LinkingOrganizationUnsupported;
+                }


Suggest using if/else if for these two existing user checks

chenriksson · 2018-01-24T21:06:05Z

src/NuGetGallery/Controllers/AuthenticationController.cs

+                if (authenticatedUser.User.Credentials.Any(c => c.IsExternal()))
+                {
+                    return SignInFailure(model, linkingAccount, Strings.LinkingMultipleExternalAccountsUnsupported);
+                }


Do you need this check here? Isn't this failure covered in LinkExternalAccount below?

Why 2 different error messages for these checks?

This is for the form POST, not the initial callback. There needs to be verification here in case somebody hacks the form to put the wrong value in.

The messages are basically the same, but adapted for the context in which they appear.

chenriksson · 2018-01-24T21:12:06Z

src/NuGetGallery/Views/Authentication/LinkExternal.cshtml

                    }
                    else
                    {
-                        @:the @Model.External.ProviderAccountNoun '@Model.External.AccountName'.
+                        if (!string.IsNullOrEmpty(Model.External.ExistingUserLinkingError))


Don't need this check if you have ExistingUserCanBeLinked just return string.IsNullOrEmpty(ExistingUserLinkingError).

chenriksson · 2018-01-24T21:17:31Z

src/NuGetGallery/Views/Authentication/_SignIn.cshtml

+        else
+        {
+            @Html.ShowTextBoxFor(m => m.SignIn.UserNameOrEmail)
+        }
        @Html.ShowValidationMessagesFor(m => m.SignIn.UserNameOrEmail)


@Html.ShowValidationMessagesFor(...) should go with @Html.ShowTextBoxfor(...), right?

I don't think so--if MVC returns errors for the validation of the UserNameOrEmail field, but the field is uneditable (with ShowHiddenFor), we still want to see the errors. The user seeing the errors can send the error to us so we can figure out the issue.

chenriksson · 2018-01-24T21:18:41Z

tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs

@@ -16,6 +16,7 @@
 using NuGetGallery.Infrastructure.Authentication;
 using Xunit;
 using System.Web;
+using System.Globalization;


nit: sort usings

chenriksson · 2018-01-24T21:19:23Z

src/NuGetGallery/Views/Authentication/_Register.cshtml

+        else
+        {
+            @Html.ShowEmailBoxFor(m => m.Register.EmailAddress)
+        }
        @Html.ShowValidationMessagesFor(m => m.Register.EmailAddress)


@Html.ShowValidationMessagesFor(...) should go with @Html.ShowEmailBoxfor(...), right?

I don't think so--if MVC returns errors for the validation of the EmailAddress field, but the field is uneditable (with ShowHiddenFor), we still want to see the errors. The user seeing the errors can send the error to us so we can figure out the issue.

I disagree here - validation errors are for user input, right? Validation errors help users take corrective actions.

If EmailAddress is not editable and fails validation, wouldn't it indicate bad data on our side. I would not show a validation error in this case, but would log and direct the user to contact support.

It could indicate bad data (honestly more likely that the user hacked the form with F12), but given that

it would be nasty to have to predict from the form submission endpoint whether or not the error was on an uneditable field or not and then log the model errors

this would happen very rarely

we can debug the issue easier if the user experiencing it can tell us the error message

the user will likely be more annoyed if the form refuses to submit but there's no visible message

I would rather keep this like this.

Ultimately this isn't that big of a deal, which is part of why I'm suggesting this easy solution that allows any users that encounter it to approach us and doesn't require any nontrivial code changes.

scottbommarito · 2018-01-25T00:49:36Z

@skofman1 @chenriksson

Addressed all feedback. Also tested with MSA/AAD (I had only tested with AAD2) and got all working.

skofman1 · 2018-01-25T18:38:02Z

src/NuGetGallery.Core/CredentialTypes.cs

+            return actualType.Equals(expectedType, StringComparison.OrdinalIgnoreCase);
+        }
+
+        private static bool IsSubType(string actualType, string expectedTypePrefix)


@scottbommarito , I understand you wanted to make the methods use shared code, but in this case, since it's only a single line (credential.Type.StartsWith()) the result is overly complex. For example: IsPassword(Credential), instead of having a single line, now calls into IsPassword(type) => IsSubType() => string.StartsWith()
It's hard to read and not necessary.
Please undo this change.

I initially agreed with you and was going to make this change, but then I realized that despite the fact that it does create deeper call stacks, it also substantially reduces the code because there is no longer the need for every method to have an if (type == null) throw new ArgumentNullException in addition to the fact that it unifies all of these StartsWith and Equals calls.

@shishirx34's PR, that came after this one, also touches this code and creates about 20 lines for something that could be closer to 10 with this restructuring. (https://github.com/NuGet/NuGetGallery/pull/5355/files#diff-624219da9e2d31ff3446f6c5be30fe20R43)

I think the point shouldn't always be to reduce the number of lines if it makes the code unreadable. If few lines to a non-often changing code are repetitive its fine if they are duplicated. I agree with @skofman1 and feel like this is an unnecessary refactoring leading to hard to read code, please undo these changes and keep the code easy to read.

changed to be one-liners that return false if credential or type is null

chenriksson · 2018-01-25T19:25:03Z

...etGallery/Authentication/Providers/AzureActiveDirectory/AzureActiveDirectoryAuthenticator.cs

+            var identityInfo = ClaimsExtentions.GetIdentityInformation(claimsIdentity, DefaultAuthenticationType);
+            // The claims returned by AzureActiveDirectory have the email as the name claim are missing the email claim.
+            // Copy the object returned by the method but set the email as the name.
+            return new IdentityInformation(identityInfo.Identifier, identityInfo.Name, identityInfo.Name, identityInfo.AuthenticationType, identityInfo.TenantId);


nit: long line

chenriksson · 2018-01-25T19:28:33Z

src/NuGetGallery/Controllers/AuthenticationController.cs

+
+                    if (existingUserLinkingError != null)
+                    {
+                        existingUserLinkingError = string.Format(


Would put the string.Format inline. This doesn't improve code, and inline would be more maintainable if the resource string args ever diverge.

chenriksson · 2018-01-25T19:34:57Z

src/NuGetGallery/Views/Authentication/_Register.cshtml

+        else
+        {
+            @Html.ShowEmailBoxFor(m => m.Register.EmailAddress)
+        }
        @Html.ShowValidationMessagesFor(m => m.Register.EmailAddress)


I disagree here - validation errors are for user input, right? Validation errors help users take corrective actions.

If EmailAddress is not editable and fails validation, wouldn't it indicate bad data on our side. I would not show a validation error in this case, but would log and direct the user to contact support.

chenriksson · 2018-01-25T19:35:53Z

Please see my comment on validation, but otherwise changes look good to me.

shishirx34 · 2018-01-25T22:42:34Z

...etGallery/Authentication/Providers/AzureActiveDirectory/AzureActiveDirectoryAuthenticator.cs

+
+            // The claims returned by AzureActiveDirectory have the email as the name claim are missing the email claim.
+            // Copy the object returned by the method but set the email as the name.
+            return new IdentityInformation(


I think the correct fix here would be to extract the name claim correctly from the claimsIdentity Otherwise the identity here would be email <email>

Fixed it. All of the AAD on dev (and probably also int and prod as well) have incorrect identities as well.

shishirx34 · 2018-01-25T23:00:52Z

src/NuGetGallery/Strings.resx

+    <value>We found an account with the email address associated with this external account ({0}), but linked to a different external account.</value>
+  </data>
+  <data name="LinkingMultipleExternalAccountsUnsupported" xml:space="preserve">
+    <value>You cannot link your NuGet.org account to multiple external accounts.</value>


Please run through the flow for strings with @anangaur once before merging.

shishirx34 · 2018-01-25T23:04:22Z

tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs

@@ -243,6 +243,134 @@ public class TheSignInAction : TestContainer
                GetMock<AuthenticationService>().VerifyAll();
            }

+            public async Task WhenAttemptingToLinkExternalToExistingAccountWithNoExternalAccounts_AllowsLinking()


Also add the test case for register? when the email doesn't match any account?

There were existing tests for that flow--I didn't change the behavior there at all.

shishirx34

Looks good. Please add the register test case and check the strings with @anangaur before merging.

Scott Bommarito added 3 commits January 19, 2018 17:15

simplify msa link flow

861b9ae

reject multiple external accounts, fix link external page, make email…

5020916

… static when it is provided by model

add unit tests

83decaa

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, nugetlurker, ryuyu, chenriksson, cristinamanum and skofman1 January 24, 2018 01:12

skofman1 reviewed Jan 24, 2018

View reviewed changes

renamed multiple external accounts linking strings

ae86e64

chenriksson reviewed Jan 24, 2018

View reviewed changes

Scott Bommarito added 2 commits January 24, 2018 16:30

pr comments

f1f1e62

Remove cancel button from register with external

2c0fcf5

skofman1 reviewed Jan 25, 2018

View reviewed changes

chenriksson approved these changes Jan 25, 2018

View reviewed changes

Scott Bommarito added 2 commits January 25, 2018 13:22

admins are allowed to have multiple external credentials

eef55c8

pr fixes

268601d

shishirx34 suggested changes Jan 25, 2018

View reviewed changes

now correctly gets identity information for azure active directory

02421e3

shishirx34 approved these changes Jan 27, 2018

View reviewed changes

Scott Bommarito added 4 commits January 29, 2018 13:24

remove unnecessary credentialtypes abstraction

c59bfed

throw if credential type is null and credential is removed

a671794

fix strings

d077f72

Merge branch 'dev' into sb-msasignin

b39f83a

scottbommarito merged commit 116b8a6 into dev Jan 30, 2018

scottbommarito deleted the sb-msasignin branch January 30, 2018 20:25

scottbommarito mentioned this pull request Feb 2, 2018

Remove password credential from user when they link an external account #5382

Merged

Update External Account Linking Flow #5338

Update External Account Linking Flow #5338

Conversation

scottbommarito commented Jan 24, 2018

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

skofman1 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

scottbommarito commented Jan 24, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito commented Jan 25, 2018

skofman1 Jan 25, 2018 • edited Loading

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 25, 2018

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

shishirx34 left a comment

Choose a reason for hiding this comment

scottbommarito commented Jan 24, 2018 •

edited

Loading

skofman1 Jan 25, 2018 •

edited

Loading