Add initial support for user deletion request #5007

joelverhagen · 2017-11-15T01:28:29Z

Progress on https://github.com/NuGet/Engineering/issues/921.

The changes were getting big so I stopped and made a PR. Hopefully easier to review.

This PR:

Update the "report my package" form to support a new hidden drop-down (delete decision) and confirmation checkbox.
Add new notification message so that all owners know a version was deleted.
Add method to IPackageDeleteService to determine if a package can be deleted. It always returns false right now.
Add logic to soft delete the package and auto-close the support request.

TODO:

Implement "can I delete this?" logic.
Introduce configuration for the aforementioned logic.
Functional test (hopefully)

Without these TODOs, the new code paths introduced by this PR will not be available yet.

chenriksson · 2017-11-15T18:50:26Z

src/NuGetGallery/Controllers/PackagesController.cs

+            if (!allowDelete
+                && reportForm.DeleteDecision == PackageDeleteDecision.DeletePackage)
+            {
+                reportForm.DeleteDecision = null;


Does this mean that lack of permissions is going to result in the same UI validation error for when a delete decision wasn't provided?

In this case, the UI will behave as if the delete decision UI was hidden. Really, the user should never get into this case because the UI doesn't allow it. The missing delete decision validation error only occurs if allowDelete is true (which is not the case here).

chenriksson · 2017-11-15T18:55:42Z

src/NuGetGallery/Controllers/PackagesController.cs

+                {
+                    // Swallow exceptions that occur during the delete process. If this happens, we'll just
+                    // send out the support request as usual and handle it manually.
+                    QuietLog.LogHandledException(e);


Will the user receive a message that the support request was sent?

deleted will still be false so we will show the "support request sent" message as usual.

chenriksson · 2017-11-15T18:56:46Z

src/NuGetGallery/ExtensionMethods.cs

@@ -291,6 +291,19 @@ public static HtmlString ShowCheckboxFor<TModel>(this HtmlHelper<TModel> html, E
        public static MvcHtmlString ShowEnumDropDownListFor<TModel, TEnum>(
            this HtmlHelper<TModel> html,
            Expression<Func<TModel, TEnum?>> expression,
+            string emptyItemText)


There's also a HtmlExtensions class. Maybe we should move these HtmlHelper extensions there?

Hm, good point. I'll take a look at that.

It looks like this is a big mess already. I will create an issue to clean this up.
#5015

skofman1 · 2017-11-16T17:30:14Z

can you attach screenshots?

joelverhagen · 2017-11-16T17:37:22Z

Select a reason

After choosing a delete reason

After choosing the delete decision

After choosing the contact support decision

After deleted

skofman1 · 2017-11-16T17:39:16Z

src/NuGetGallery/Controllers/PackagesController.cs

+            }
+
+            // Enforce the auto-delete rules.
+            var allowDelete = await _packageDeleteService.CanPackageBeDeletedByUserAsync(package);


do we need to evaluate CanPackageBeDeletedByUserAsync is DeleteDecision is not DeletePackage? If the user just wants to contact support we are doing unnecessary work here.

Great point, no we don't.

skofman1 · 2017-11-16T17:57:49Z

src/NuGetGallery/Controllers/PackagesController.cs

            {
-                return HttpNotFound();
+                reportForm.CopySender = false;
+                reportForm.Message = "This support request is intended for automatic deletion.";


automatic package deletion

Fixed. Good point.

skofman1 · 2017-11-16T18:00:20Z

src/NuGetGallery/Controllers/PackagesController.cs

+                user,
+                package);
+
+            if (supportRequest != null


when will AddNewSupportRequestAsync return null?

If the commit to the support request DB fails. This is existing logic in the support request service.

skofman1 · 2017-11-16T18:01:21Z

src/NuGetGallery/Controllers/PackagesController.cs

+                        "change to propagate through our system.";
+
+                    return Redirect(Url.Package(id, version));
+                }


this method is to big. Can you split it to smaller methods?

skofman1 · 2017-11-16T18:02:39Z

src/NuGetGallery/Controllers/PackagesController.cs

+
+                    TempData["Message"] =
+                        "We're performing the package delete right now. It may take a while for this " +
+                        "change to propagate through our system.";


I know this controller doesn't do much use of Strings, but perhaps it time to start?

skofman1 · 2017-11-16T18:08:22Z

src/NuGetGallery/Controllers/PackagesController.cs

+                return HttpNotFound();
+            }
+
+            if (!PermissionsService.IsActionAllowed(package, User, PackageActions.ReportMyPackage))


Are the permissions required for reporting a package are the same as for deleting a package? Do we need a special check for the later?

Same permissions. The keyword here is "My" in "ReportMyPackage".

skofman1

🕐

skofman1

A subsequent commit will complete the work and fully enable this feature. Progress on NuGet/Engineering#921

scottbommarito

woops, looks like i missed the bell

scottbommarito · 2017-11-16T19:56:03Z

src/NuGetGallery/Controllers/PackagesController.cs

@@ -35,6 +36,29 @@ namespace NuGetGallery
    public partial class PackagesController
        : AppController
    {
+        private static readonly IReadOnlyList<ReportPackageReason> ReportAbuseReasons = new[]


I would add some comments on what these are for.

/// <summary> /// Reasons that users can select when reporting abuse by a package they do not own. /// </summary> private static readonly IReadOnlyList<ReportPackageReason> ReportAbuseReasons = new[] { ... } /// <summary> /// Reasons that users can select when reporting their own package. /// </summary> private static readonly IReadOnlyList<ReportPackageReason> ReportMyPackageReasons = new[] { ... } /// <summary> /// Subset of <see cref="ReportMyPackageReasons"/> that suggests a user would like to delete a package. /// </summary> private static readonly IReadOnlyList<ReportPackageReason> DeleteReasons = new[] { ... }

Yeah, good idea. Thanks for doing the work for me 😉

scottbommarito · 2017-11-16T19:56:53Z

src/NuGetGallery/Controllers/PackagesController.cs

-
-            var modelIsValid = ModelState.IsValid;
-            if (reportForm.Reason == ReportPackageReason.ViolatesALicenseIOwn)
+            reportForm.Message = HttpUtility.HtmlEncode(reportForm.Message);


I know this isn't related to this PR, but shouldn't MVC do this encoding for us?

If it's needed, we should have a comment describing why.

Out of scope of this PR. I think we are encoding it since it makes its way into the email system.

scottbommarito · 2017-11-16T22:56:50Z

src/NuGetGallery/Controllers/PackagesController.cs

-                modelIsValid = modelIsValid && !string.IsNullOrEmpty(reportForm.Signature);
+                ModelState.AddModelError(
+                    nameof(ReportAbuseViewModel.Signature),
+                    "The signature is required.");


1 - This should be in Strings.resx
2 - We should be more verbose here: for example, "A signature is required when submitting a copyright violation request."

Yes, that should be in Strings.

The context of the UI negates the need for verbosity. If the message field is visible, it is required.

scottbommarito · 2017-11-16T23:04:07Z

src/NuGetGallery/Controllers/PackagesController.cs

+                return RedirectToAction(nameof(ReportAbuse), new { id = package.PackageRegistration.Id, version = package.NormalizedVersion });
+            }
+
+            reportForm.Message = HttpUtility.HtmlEncode(reportForm.Message);


same as above: shouldn't MVC do the encoding for us?

scottbommarito · 2017-11-16T23:22:44Z

src/NuGetGallery/Controllers/PackagesController.cs

+                allowDelete = await _packageDeleteService.CanPackageBeDeletedByUserAsync(package);
+                if (!allowDelete)
+                {
+                    reportForm.DeleteDecision = null;


Seems really odd to me that we are changing the form here. It also made the logic more confusing for me.

If someone specifies they would like to delete their package, shouldn't we either
1 - return an error message that says they package does not meet the criteria
2 - keep the deletion state so that the on-call can see it in the support request

Regardless, there should be a comment on this line describing what we are doing.

1 - return an error message that says they package does not meet the criteria

Why show an error message? The only way this can happen is if someone is fiddling with the request or if they get the form while the package can be deleted and only submit it after the package can't be deleted. We shouldn't put extra effort into these cases.

2 - keep the deletion state so that the on-call can see it in the support request

The deletion state shouldn't even be persisted in this case. It's a bad request which should be rejected early on.

Regardless, there should be a comment on this line describing what we are doing.

Sounds good.

scottbommarito · 2017-11-16T23:37:51Z

src/NuGetGallery/Controllers/PackagesController.cs

+                package.PackageRegistration.Id,
+                package.NormalizedVersion);
+            var reason = EnumHelper.GetDescription(reportForm.Reason.Value);
+            var supportRequest = await _supportRequestService.AddNewSupportRequestAsync(


If the user attempts to the delete the package, why are we creating a support request?

Seems unnecessary. We should just delete it and if it fails, then create the request. Probably makes this code simpler because we don't need to pass around this support request object.

Also, it's very odd to be creating support requests we don't get emails for.

This is to track the auto-delete in a way the PMs asked for.

scottbommarito · 2017-11-16T23:41:48Z

src/NuGetGallery/Services/PackageDeleteService.cs

@@ -61,6 +61,13 @@ public class PackageDeleteService
            _auditingService = auditingService;
        }

+        public Task<bool> CanPackageBeDeletedByUserAsync(Package package)
+        {
+            // For now, don't allow user's to delete their packages.


nit: user's -> users

scottbommarito · 2017-11-16T23:52:44Z

src/NuGetGallery/Views/Packages/ReportMyPackage.cshtml

+                        @Html.ShowCheckboxFor(m => m.DeleteConfirmation)
+                        I understand that this action cannot be undone. This will permanently delete the package
+                        @Model.PackageId @Model.PackageVersion and make it unavailable for download and package restore.
+                        This does not allow uploading a new package with the same version.


To keep the same phrasing, this I will not be able to upload a new package with the same version. or something similar might be better.

Yeah, I like that better too.

scottbommarito · 2017-11-16T23:59:23Z

tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

+                    _viewModel);
+
+                // Assert
+                Assert.Contains(


Can consolidate this into a helper message to be used in a number of these other tests

private void AssertContainsMessage(PackagesController controller, string message) { Assert.Contains(message, _controller.ModelState.Values.SelectMany(x => x.Errors).Select(x => x.ErrorMessage)); }

scottbommarito · 2017-11-17T00:16:21Z

tests/NuGetGallery.Facts/Controllers/PackagesControllerFacts.cs

+            }
+
+            [Theory]
+            [InlineData(ReportPackageReason.ContainsMaliciousCode)]


two thoughts:

1 - we should make this run on all members of PackagesController.DeleteReasons
2 - if we are concerned about the members of PackageController.DeleteReasons changing, then there should be tests for that

Kind of out of scope, as there was no pre-existing validation on which reasons were being sent, but I'll add tests for it.

joelverhagen · 2017-11-17T00:39:12Z

@scottbommarito, I will address these comments in a follow-up PR.

A subsequent commit will complete the work and fully enable this feature. Progress on NuGet/Engineering#921

joelverhagen requested review from agr, loic-sharma, xavierdecoster, shishirx34, ryuyu, chenriksson, cristinamanum, skofman1 and scottbommarito November 15, 2017 01:28

dnfclas added the cla-already-signed label Nov 15, 2017

chenriksson approved these changes Nov 15, 2017

View reviewed changes

skofman1 reviewed Nov 16, 2017

View reviewed changes

skofman1 requested changes Nov 16, 2017

View reviewed changes

skofman1 approved these changes Nov 16, 2017

View reviewed changes

Add initial support for user deletion request (#5007)

a0e9cfa

A subsequent commit will complete the work and fully enable this feature. Progress on NuGet/Engineering#921

joelverhagen merged commit a0e9cfa into NuGet:dev Nov 16, 2017

joelverhagen deleted the auto-delete branch November 16, 2017 23:30

scottbommarito reviewed Nov 17, 2017

View reviewed changes

shishirx34 mentioned this pull request Nov 17, 2017

Deployment 2017-11-30 #4950

Closed

10 tasks

scottbommarito pushed a commit that referenced this pull request Jul 22, 2019

Add initial support for user deletion request (#5007)

d48bffb

A subsequent commit will complete the work and fully enable this feature. Progress on NuGet/Engineering#921

Add initial support for user deletion request #5007

Add initial support for user deletion request #5007

Conversation

joelverhagen commented Nov 15, 2017

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

skofman1 commented Nov 16, 2017

joelverhagen commented Nov 16, 2017

Select a reason

After choosing a delete reason

After choosing the delete decision

After choosing the contact support decision

After deleted

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

skofman1 left a comment

Choose a reason for hiding this comment

skofman1 left a comment

Choose a reason for hiding this comment

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

joelverhagen commented Nov 17, 2017