Account deletion support for Organization members #5797

scottbommarito · 2018-04-12T21:08:58Z

User view:

Admin view:

scottbommarito · 2018-04-12T21:10:15Z

tests/NuGetGallery.Facts/ViewModels/DeleteAccountViewModelFacts.cs

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


I removed this file because it seemed redundant--it's only testing a field on ListPackageItemViewModel that is passed to DeleteAccountViewModel. The field already has its own tests in ListPackageItemViewModel.

chenriksson · 2018-04-13T15:25:54Z

src/NuGetGallery/Areas/Admin/ViewModels/DeleteUserAccountViewModel.cs

    {
        public DeleteUserAccountViewModel()
        {
+            // This constructor exists so that we can have a form that uses this model.


nit: move to doc comment?

We need this and the setters because of the _DeleteUserAccountForm.cshtml postback?

chenriksson · 2018-04-13T15:35:39Z

src/NuGetGallery/Areas/Admin/Views/DeleteAccount/DeleteUserAccount.cshtml

@@ -1,5 +1,5 @@
 @using NuGetGallery
-@model NuGetGallery.Areas.Admin.ViewModels.DeleteUserAccountViewModel
+@model DeleteAccountViewModel


Why change from DeleteUserAccountViewModel to DeleteAccountViewModel?

If this is going to be shared for deleting org accounts, should the view also change to DeleteAccount.cshtml?

I changed this because

DeleteUserAccountViewModel used to inherit from DeleteAccountViewModel.
DeleteUserAccountViewModel contained the information required by the form, while DeleteAccountViewModel had the information required by rendering the view.
I thought having DeleteUserAccountViewModel depend on DeleteAccountViewModel was confusing because the two objects served very different purposes and the improper inheritance would lead to DeleteAccountViewModels being created with empty fields.

chenriksson · 2018-04-13T15:38:35Z

src/NuGetGallery/Areas/Admin/Views/DeleteAccount/_DeleteUserAccountForm.cshtml

+                @Html.ValidationMessageFor(m => m.Signature)
+            </div>
+
+            @if (Model.HasOrphanPackages)


Was the HasOrphanPackages already updated to account for packages for the member's organizations?

chenriksson · 2018-04-13T15:39:19Z

src/NuGetGallery/Areas/Admin/Views/DeleteAccount/_DeleteUserAccountForm.cshtml

+                <div class="form-group">
+                    @Html.EditorFor(m => m.ShouldUnlist)
+                    <label class="checkbox-inline">
+                        <b>Unlist the packages without any user.</b>


Maybe Unlist the packages without any owner is better?

chenriksson · 2018-04-13T15:44:17Z

src/NuGetGallery/Services/DeleteAccountService.cs

+                migrationRequest.AdminUser.OrganizationMigrationRequests.Remove(migrationRequest);
+            }
+
+            await _entitiesContext.SaveChangesAsync();


There's logic in DeleteGalleryUserAccountAsync to commitAsTransaction. Given that,

Should this call SaveChangesAsync?

Are all of the RemoveX operations honoring the deferred commit?

As far as I understand

SaveChangesAsync "enlists in any ambient transactions" e.g. it attaches the changes to the ongoing transaction

other services that are called also call SaveChangesAsync

I was under the impression this was the right way to do things but if it's wrong then we'll need to fix other things as well. In my experience when I was testing this and it failed at this point, it rolled back any changes that it was attempting to make.

@scottbommarito is correct if called under a transaction it appends the changes and commit everything at the end. If not under a transaction each SaveChangesAsync will be committed individually

chenriksson · 2018-04-13T15:45:00Z

src/NuGetGallery/ViewModels/DeleteAccountViewModel.cs

@@ -1,6 +1,8 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

+using NuGetGallery.Areas.Admin;
+using NuGetGallery.Areas.Admin.Models;


nit: sort after System usings

chenriksson · 2018-04-13T15:46:00Z

src/NuGetGallery/ViewModels/DeleteAccountViewModel.cs

+            HasPendingRequests = supportRequestService.GetIssues()
+                .Where(issue => 
+                    (issue.UserKey.HasValue && issue.UserKey.Value == User.Key) &&
+                    string.Equals(issue.IssueTitle, Strings.AccountDelete_SupportRequestTitle) &&


Ignore case?

Existing code that I moved here. Ignore case is probably correct though.

chenriksson · 2018-04-13T15:47:52Z

src/NuGetGallery/ViewModels/DeleteAccountViewModel.cs

+                 .Select(p => new ListPackageItemViewModel(p, currentUser))
+                 .ToList();
+
+            HasPendingRequests = supportRequestService.GetIssues()


Aside from checking for support requests, are we also deleting them? Could there be PII in the support requests that necessitate deleting them as part of Delete Account?

Deletion is done inside DeleteAccountService I believe.

chenriksson · 2018-04-13T15:49:52Z

src/NuGetGallery/Views/Users/DeleteAccount.cshtml

@@ -35,6 +35,7 @@
                            <li>Revoke your api key(s).</li>
                            <li>Remove you as the owner for any packages you own.</li>
                            <li>Remove your ownership from any ID prefix reservations and delete any ID prefix reservations that you were the only owner of.</li>
+                            <li>Remove your membership from any organizations you are a member of.</li>


nit: would remove redundancy... Remove your membership from any organizations or Remove yourself from any organizations that you are a member of

chenriksson · 2018-04-13T15:58:40Z

src/NuGetGallery/Views/Users/_UserOrganizationsListForDeletedAccount.cshtml

+                                <td class="align-middle ">
+                                    @if (organization.MemberCount == 1)
+                                    {
+                                        <i class="ms-Icon ms-Icon--StatusErrorFull red-check" title="After account delete the organization will not have any members."></i>


I wonder if we should just automatically delete any orphaned organizations... to avoid support requests. We're already unlisting all the packages. This user can't easily request the orphaned org(s) be deleted after their account is deleted.

I can't imagine that anyone would be too concerned about their orphaned organization if they were willing to orphan it.
I think leaving the organization might make it easier for a real user to take over ownership of the packages (the next PR after this is to allow site admins to change any organization's membership).

anangaur · 2018-04-13T18:05:30Z

Should we show admins instead of members? Because ultimately you would need additional admins to be present for an organization.

In case the user deleting his/her account is the only Admin, we should just delete the orgs as well and make all the packages orphans. We do not want to end up in a situation where there is a dangling organization with only collaborators.

OR

We continue to show members and not just admins.

Organizations where the user account is the sole member (Admin) --> Delete the organization and orphan all the packages as if they directly belonged to the user
Organization where the user account is one of the n admins --> User is deleted. Organization exists with other Admins to manage it -- No brainer :)
Organization where the user is the sole Admin but there are collaborators --> User is deleted and all the collaborators are promoted as Admins. We should show this message on the page.

scottbommarito · 2018-04-16T15:37:15Z

@anangaur @chenriksson Currently this implementation does NOT delete orphaned organizations. This is because deleting these organizations requires actually implementing the delete flow for organizations. I don't mind doing that--I've already done most of the work required for it--but I don't see any major problems with having orphaned organizations when the main reason for implementing this feature is GDPR concerns.

anangaur · 2018-04-16T16:21:35Z

We need to have delete Organization feature and IMO we should be more liberal in releasing the namespace. As discussed earlier, delete org feature should be available on the manage organization page itself and not just on the profile page.

In general we should have the capability to release deleted orgs/accounts namespace on request (with some validations).

…eleteorgmember

scottbommarito · 2018-04-27T16:15:43Z

@anangaur @chenriksson @cristinamanum The implementation has been updated to delete any orphaned organizations as @anangaur requested above, please review again.

cristinamanum · 2018-04-27T16:27:49Z

src/NuGetGallery/Areas/Admin/Views/DeleteAccount/_DeleteUserAccountForm.cshtml

+        </div>
+        <hr />
+        <p>
+            This action <strong>CANNOT</strong> be undone. This will permanently delete the account <b>@Model.AccountName</b>.


The "permanently delete" will imply that the account nae will be released and this is not the case all the time.

Changed to

You will not be able to regain access to the account <b>@Model.AccountName</b> or any of its packages.

cristinamanum · 2018-04-27T16:31:46Z

src/NuGetGallery/Services/DeleteAccountService.cs

+                }
+                else if (memberCount - 1 <= collaborators.Count())
+                {
+                    // All other members of this organization are collaborators, so we should promote them to administrators.


Why we do this? It could result in a large number of admins.

I don't think there's any metric we can use to choose one collaborator or a subset of the collaborators to promote when the only admin has left. Therefore, we promote all collaborators because then we're guaranteed to promote at least one person who actually should be an administrator.

Agree w/ @cristinamanum - and what if multiple collaborators want to claim ownership? I'm wondering if we need a modified dispute resolution policy for obtaining admin permissions for an orphaned organization. What did @anangaur say?

@anangaur suggested this--promote all collaborators when the last admin deletes their account.

This isn't an orphaned organization, this is an organization whose last admin left that still has collaborators. I think if the owner of this organization doesn't care enough to give administrator rights to another member, I don't think it's worth detailing a fancy policy.

chenriksson

LGTM, except same concern as @cristinamanum on promoting collaborators

chenriksson · 2018-04-27T16:47:42Z

src/NuGetGallery/Services/DeleteAccountService.cs

+                }
+                else if (memberCount - 1 <= collaborators.Count())
+                {
+                    // All other members of this organization are collaborators, so we should promote them to administrators.


Agree w/ @cristinamanum - and what if multiple collaborators want to claim ownership? I'm wondering if we need a modified dispute resolution policy for obtaining admin permissions for an orphaned organization. What did @anangaur say?

chenriksson · 2018-04-27T16:49:02Z

src/NuGetGallery/Services/DeleteAccountService.cs

+            {
+                user.OrganizationMigrationRequests.Remove(transformationRequest);
+                transformationRequest.NewOrganization.OrganizationMigrationRequest = null;
+            }


Can these be simplified? i.e., user.OrganizationRequests.Clear()

I wanted to make sure that there's no dependency from either end, e.g. this user has no OrganizationMigrationRequests and there are no users that have OrganizationMigrationRequests that depend on this user. not sure if the cascade was set up correctly.

cristinamanum · 2018-04-27T22:49:26Z

src/NuGetGallery/Services/DeleteAccountService.cs

+            {
+                var organization = membership.Organization;
+                var collaborators = organization.Members.Where(m => !m.IsAdmin).ToList();
+                var memberCount = organization.Members.Count();


I think that these two lines do two queries to db? Maybe we can do one query and do the filter and the count on the same collection?

cristinamanum · 2018-04-27T22:51:22Z

src/NuGetGallery/Services/DeleteAccountService.cs

@@ -249,12 +283,6 @@ private List<Package> GetPackagesOwnedByUser(User user)
            await _userRepository.CommitChangesAsync();
        }

-        private async Task RemoveUser(Organization organization)


Is this delete action replaced by other call?

Yeah, _userRepository.DeleteOnCommit(organization) as called by RemoveUser(User user) works fine for this.

Discussed offline with @scottbommarito and this method is not needed anymore. The orgs will be deleted as user entities.

cristinamanum

Added few comments.

…eleteorgmember

Scott Bommarito added 3 commits April 11, 2018 09:30

UI/functional changes to allow user to delete organization member

c0dcccd

fix existing tests (still need new tests)

c830590

unit tests

49eedcb

scottbommarito requested review from joelverhagen, agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, anangaur, chenriksson, cristinamanum and skofman1 April 12, 2018 21:08

scottbommarito changed the title ~~Sb deleteorgmember~~ Account deletion support for Organization members Apr 12, 2018

scottbommarito commented Apr 12, 2018

View reviewed changes

chenriksson reviewed Apr 13, 2018

View reviewed changes

pr comments

2950b38

Scott Bommarito added 4 commits April 23, 2018 13:34

Merge branch 'dev' of https://github.com/NuGet/NuGetGallery into sb-d…

5f8f39b

…eleteorgmember

simplify memberships

7601b50

Merge branch 'dev' of https://github.com/NuGet/NuGetGallery into sb-d…

9916f48

…eleteorgmember

don't show delete section for collaborators

b0f5750

Scott Bommarito added 2 commits April 27, 2018 09:23

fix whitespace

3173465

fix build

b8cb879

cristinamanum reviewed Apr 27, 2018

View reviewed changes

improve wording

38bd7b7

chenriksson reviewed Apr 27, 2018

View reviewed changes

fix unit tests

823c12a

cristinamanum reviewed Apr 27, 2018

View reviewed changes

cristinamanum approved these changes Apr 27, 2018

View reviewed changes

Scott Bommarito added 2 commits April 27, 2018 16:02

do not make multiple queries

0e5a893

Merge branch 'dev' of https://github.com/NuGet/NuGetGallery into sb-d…

2b4a8c5

…eleteorgmember

scottbommarito merged commit 932026b into dev Apr 30, 2018

scottbommarito deleted the sb-deleteorgmember branch April 30, 2018 19:24

scottbommarito restored the sb-deleteorgmember branch July 22, 2019 23:20

scottbommarito deleted the sb-deleteorgmember branch July 22, 2019 23:21

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

Account deletion support for Organization members #5797

Account deletion support for Organization members #5797

Conversation

scottbommarito commented Apr 12, 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

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 Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

anangaur commented Apr 13, 2018

scottbommarito commented Apr 16, 2018

anangaur commented Apr 16, 2018

scottbommarito commented Apr 27, 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

chenriksson 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

cristinamanum left a comment

Choose a reason for hiding this comment

scottbommarito Apr 13, 2018 •

edited

Loading

scottbommarito commented Apr 27, 2018 •

edited

Loading