Add Manage/Privacy-Delete/Download functionality #1559

HaoK · 2018-01-09T22:48:42Z

Initial changes to identity default UI for Manage/Privacy/DownloadPersonalData and DeletePersonalData

Still need final tweaks to UI/text which we'll plan to get from @DamianEdwards review in person.

@blowdart @danroth27 @javiercn @ajcvickers @mkArtakMSFT

blowdart · 2018-01-09T22:52:10Z

There won't be time for in person with @DamianEdwards is off having fun in London, so let's use @danroth27 and I as his proxy.

HaoK · 2018-01-10T19:58:15Z

@danroth27 who do you want to review this PR from the perspective of proper RazorPages usage/patterns etc? I mostly just copied what the existing pages did, I only intentionally deviated slightly using the NavPages constant instead of hardcoding the value in the ActivePages setting.

DamianEdwards · 2018-01-10T20:13:07Z

Get Ryan and Pranav to look at it pls

pranavkm · 2018-01-11T01:06:59Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml

+</div>
+
+@section Scripts {
+    @await Html.PartialAsync("_ValidationScriptsPartial")


<partial name="_ValidationScriptsPartial" />

@pranavkm What's the advantage of <partial over Html.PartialAsync?

We generally prefer tag helpers to HtmlHelpers. It's cleaner, more idiomatic code.

pranavkm · 2018-01-11T01:15:39Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml.cs

+            _logger.LogInformation("User with ID '{UserId}' deleted themselves.", _userManager.GetUserId(User));
+
+            // REVIEW: should this redirect to somewhere that says user was deleted?
+            return RedirectToPage("./");


This should redirect to the website root return Redirect("~/"); or a successfully deleted page. Redirecting to the page ./ is kinda weird.

No, the right thing to do here is to pass in a return url and do a local redirect, look at the logout action for how to do it.

So most things in manage are doing some form of RedirectToPage... is one better than the other?

RedirectToPage expects to resolve a page. Since there isn't a page named ./, it ends up resolving the empty string which ends up in the app root. But that's just a weird way of going about it.

Yeah so after they get deleted, manage will no longer be valid, so I really was just trying to redirect to home or whatever. Unless we think its worth adding a dedicated page for a Deleted confirmation

pranavkm · 2018-01-11T01:16:01Z

src/UI/Areas/Identity/Pages/Account/Manage/DownloadPersonalData.cshtml

+<h4>@ViewData["Title"]</h4>
+
+@section Scripts {
+    @await Html.PartialAsync("_ValidationScriptsPartial")


<partial name="_ValidationScriptsPartial" />

pranavkm · 2018-01-11T01:16:36Z

src/UI/Areas/Identity/Pages/Account/Manage/DownloadPersonalData.cshtml.cs

+
+        public DownloadPersonalDataModel(
+            UserManager<IdentityUser> userManager,
+            ILogger<PrivacyModel> logger)


ILogger<DownloadPersonalDataModel>?

pranavkm · 2018-01-11T01:35:21Z

src/UI/Areas/Identity/Pages/Account/Manage/DownloadPersonalData.cshtml.cs

+            _logger.LogInformation("User with ID '{UserId}' asked for their personal data.", _userManager.GetUserId(User));
+
+            // REVIEW: do we want to set a force download header?
+            return new FileContentResult(Encoding.ASCII.GetBytes(JsonConvert.SerializeObject(user)), "text/json");


You likely need Encoding.UTF8. That said, this kinda feels icky, but there really doesn't seem like a more definitive way to do this.

We probably want to force the download header, but I would just do OK(user) and set the header before. We can add a file content result in MVC that takes an object. I've filed aspnet/Mvc#7232 to track this

pranavkm · 2018-01-11T01:36:37Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml.cs

+        public DeletePersonalDataModel(
+            UserManager<IdentityUser> userManager,
+            SignInManager<IdentityUser> signInManager,
+            ILogger<PrivacyModel> logger)


ILogger<DeletePersonalDataModel>

pranavkm · 2018-01-11T01:38:28Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml.cs

+            }
+
+            var result = await _userManager.DeleteAsync(user);
+            if (!result.Succeeded)


Is it still correct for a user to be signed in if this fails?

The default implementation basically never will fail, it would only blow up with an exception, but this is a valid point, I guess we should only sign out if it succeeded

pranavkm · 2018-01-11T01:39:59Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml.cs

+            var result = await _userManager.DeleteAsync(user);
+            if (!result.Succeeded)
+            {
+                throw new ApplicationException($"Unexpected error occurred deleteing user with ID '{user.Id}'.");


ApplicationException -> InvalidOperationException

pranavkm · 2018-01-11T01:41:42Z

src/UI/Areas/Identity/Pages/Account/Manage/Privacy.cshtml

+@model PrivacyModel
+@{
+    ViewData["Title"] = "Privacy";
+    ViewData["ActivePage"] = "Privacy";


ManageNavPages.Privacy

pranavkm · 2018-01-11T01:43:18Z

src/UI/Areas/Identity/Pages/Account/Manage/DownloadPersonalData.cshtml.cs

+            _logger = logger;
+        }
+
+        public async Task<IActionResult> OnGet()


When do we get to this handler? Privacy.cshtml posts to this action and there isn't a link to this page.

Yeah we don't, I'll remove

javiercn

Looks promising.

javiercn · 2018-01-11T17:18:41Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml

+        <strong>This action will delete your user.</strong>
+    </p>
+    <p>
+        This is unrecoverable.


Not sure what this sentence means

"This action will delete your user."
"This is unrecoverable."

means you can't recover your user after you delete it.

Should this elaborate and say, once deleted you cannot use your login credentials and would need to re-register?

Kinda unrelated, but is there a hook that let's the app developer also clean up other stored details about the user when this is deleted? For instance, if the user id is used to store my order history or posted comments?

Don't worry about the exact text yet, I'll update that shortly.

re hook, not really, ideally we'd have an easy way for them to just drop in a new DeletePersonalData.cshtml.cs to take over this work entirely to customize. Or I guess scaffold it all and change the code if that feature doesn't make it...

javiercn · 2018-01-11T17:19:33Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml

+</div>
+
+@section Scripts {
+    @await Html.PartialAsync("_ValidationScriptsPartial")


@pranavkm What's the advantage of <partial over Html.PartialAsync?

javiercn · 2018-01-11T17:22:22Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml.cs

+            _logger.LogInformation("User with ID '{UserId}' deleted themselves.", _userManager.GetUserId(User));
+
+            // REVIEW: should this redirect to somewhere that says user was deleted?
+            return RedirectToPage("./");


No, the right thing to do here is to pass in a return url and do a local redirect, look at the logout action for how to do it.

javiercn · 2018-01-11T17:29:14Z

src/UI/Areas/Identity/Pages/Account/Manage/DownloadPersonalData.cshtml.cs

+            _logger.LogInformation("User with ID '{UserId}' asked for their personal data.", _userManager.GetUserId(User));
+
+            // REVIEW: do we want to set a force download header?
+            return new FileContentResult(Encoding.ASCII.GetBytes(JsonConvert.SerializeObject(user)), "text/json");


We probably want to force the download header, but I would just do OK(user) and set the header before. We can add a file content result in MVC that takes an object. I've filed aspnet/Mvc#7232 to track this

mkArtakMSFT · 2018-01-11T19:18:20Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml.cs

+
+            await _signInManager.SignOutAsync();
+
+            _logger.LogInformation("User with ID '{UserId}' deleted themselves.", _userManager.GetUserId(User));


Is it ok to log the userId as is (as this can be the email address, which is PII)

The UserName is the email, the user id is a guid by default, but this is the pattern the existing templates use for logging I believe.

mkArtakMSFT · 2018-01-11T19:21:14Z

src/UI/Areas/Identity/Pages/Account/Manage/Privacy.cshtml

+</div>
+
+@section Scripts {
+    @await Html.PartialAsync("_ValidationScriptsPartial")


Similar to the rest of the comments - prefer <partial ... /> tag syntax instead

HaoK · 2018-01-11T19:43:33Z

Ok updated switching all default UI over to use the new partial and consistently use ManageNavClass constants in for the active page stuff

HaoK · 2018-01-12T19:40:40Z

Text has been updated, Privacy tab => Personal Data, will add one more change which will require users to reenter their password if they have a local password before being allowed to delete their account

HaoK · 2018-01-12T22:45:42Z

Updated flow with password confirmation on Delete page if the user has a password. Also added white list to only include Id/UserName/Email/Phone/and Confirmed properties)

HaoK · 2018-01-12T22:57:25Z

I think all feedback has been addressed now, added Response.Headers.Add("Content-Disposition", "attachment; filename=PersonalData.json"); to force download.

@pranavkm can you do a final review/sign off?

pranavkm · 2018-01-17T00:04:25Z

src/UI/Areas/Identity/Pages/Account/Manage/DeletePersonalData.cshtml.cs

+            {
+                if (!await _userManager.CheckPasswordAsync(user, Input.Password))
+                {
+                    ModelState.AddModelError(string.Empty, "Password not correct.");


AddModelError("Input.Password", ) if we'd like to show error along the password field.

The error shows up at the top in the summary and at the bottom (below the password field) with this change so I think i'll leave it as is, so its only shown at the top

HaoK · 2018-01-17T19:59:06Z

f1ed482

grahamehorner · 2018-01-18T00:01:23Z

If a user has two factor authentication enable would the second factor used for verification before deletion?

HaoK · 2018-01-18T00:24:04Z

No, we just check password right now, checking a second factor is significantly more involved, but its certainly doable if an app wants that, you basically just need to duplicate the two factor login flow

based on changes in aspnet/Identity/pull/1559

HaoK requested review from pranavkm and rynowak January 10, 2018 20:18

pranavkm reviewed Jan 11, 2018

View reviewed changes

javiercn reviewed Jan 11, 2018

View reviewed changes

mkArtakMSFT reviewed Jan 11, 2018

View reviewed changes

mkArtakMSFT approved these changes Jan 11, 2018

View reviewed changes

HaoK added 8 commits January 12, 2018 14:49

Add Privacy-Delete/Download

d310c02

Cleanup

13f5b28

Cleanup

6d17a2e

UI tweaks

662a966

Add password to DeletePersonalData, whitelist dl

36528e9

Cleanup

c0b2141

Fix redirect

5adcf3f

Force download prompt

aa1fc6c

HaoK force-pushed the haok/gdpr branch from 45c9ba0 to aa1fc6c Compare January 12, 2018 22:56

pranavkm approved these changes Jan 17, 2018

View reviewed changes

HaoK closed this Jan 17, 2018

prafullbhosale added a commit to dotnet/Scaffolding that referenced this pull request Jan 23, 2018

Add Personal data pages to identity scaffolder content.

b8d7326

based on changes in aspnet/Identity/pull/1559

prafullbhosale mentioned this pull request Jan 23, 2018

Add Personal data pages to identity scaffolder content. dotnet/Scaffolding#679

Merged

prafullbhosale added a commit to dotnet/Scaffolding that referenced this pull request Jan 24, 2018

Add Personal data pages to identity scaffolder content.

927221e

based on changes in aspnet/Identity/pull/1559

prafullbhosale added a commit to dotnet/Scaffolding that referenced this pull request Jan 24, 2018

Add Personal data pages to identity scaffolder content.

8fd518b

based on changes in aspnet/Identity/pull/1559

prafullbhosale added a commit to dotnet/Scaffolding that referenced this pull request Jan 26, 2018

Add Personal data pages to identity scaffolder content.

d2dd894

based on changes in aspnet/Identity/pull/1559

chrisckc mentioned this pull request Dec 18, 2018

Issues with Personal Data page (does not delete saved tokens when deleting user) dotnet/aspnetcore#5791

Closed


		await _signInManager.SignOutAsync();

		_logger.LogInformation("User with ID '{UserId}' deleted themselves.", _userManager.GetUserId(User));

Add Manage/Privacy-Delete/Download functionality #1559

Add Manage/Privacy-Delete/Download functionality #1559

Uh oh!

Conversation

HaoK commented Jan 9, 2018

Uh oh!

blowdart commented Jan 9, 2018

Uh oh!

HaoK commented Jan 10, 2018

Uh oh!

DamianEdwards commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javiercn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HaoK commented Jan 11, 2018

Uh oh!

HaoK commented Jan 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

HaoK Jan 11, 2018 •

edited

Loading

HaoK commented Jan 12, 2018 •

edited

Loading