launchpad-reviewers team mailing list archive

[Colin Watson <mp+429959@xxxxxxxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:prevent-manual-activation into launchpad:master.

Commit message:
Prevent activating accounts from +reviewaccount

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/429959

This is otherwise a tempting mistake to make when dealing with account issues.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:prevent-manual-activation into launchpad:master.

diff --git a/lib/lp/registry/browser/person.py b/lib/lp/registry/browser/person.py
index be5908b..8145cc0 100644
--- a/lib/lp/registry/browser/person.py
+++ b/lib/lp/registry/browser/person.py
@@ -1391,6 +1391,23 @@ class PersonAccountAdministerView(LaunchpadFormView):
         """See `LaunchpadEditFormView`."""
         return canonical_url(self.person)
 
+    def validate(self, data):
+        """See `LaunchpadFormView`."""
+        new_status = data.get("status")
+        if (
+            new_status != self.context.status
+            and new_status == AccountStatus.ACTIVE
+        ):
+            # This typically breaks, as it bypasses the code in
+            # `PersonSet.getOrCreateByOpenIDIdentifier` that sets the user's
+            # preferred email address.  Activating an account should only be
+            # done by the user themselves logging in from a Deactivated,
+            # Unactivated, or Placeholder account status.
+            self.setFieldError(
+                "status",
+                "Only the user themselves can activate their account.",
+            )
+
     @action("Change", name="change")
     def change_action(self, action, data):
         """Update the IAccount."""
diff --git a/lib/lp/registry/browser/tests/person-admin-views.rst b/lib/lp/registry/browser/tests/person-admin-views.rst
index e711afb..4a2e30a 100644
--- a/lib/lp/registry/browser/tests/person-admin-views.rst
+++ b/lib/lp/registry/browser/tests/person-admin-views.rst
@@ -160,6 +160,7 @@ user must log in to restore the email addresses using the reactivate step.
     >>> view = create_initialized_view(user, "+reviewaccount", form=form)
     >>> print(view.errors)
     []
+    >>> transaction.commit()
     >>> user.account_status
     <DBItem AccountStatus.DEACTIVATED, ...>
     >>> user.account_status_history
@@ -169,6 +170,19 @@ user must log in to restore the email addresses using the reactivate step.
     None
 
 
+An admin cannot manually activate an account.  The user must do that
+themselves by logging in.
+
+    >>> form = {
+    ...     "field.status": "ACTIVE",
+    ...     "field.comment": "Manually reactivating.",
+    ...     "field.actions.change": "Change",
+    ... }
+    >>> view = create_initialized_view(user, "+reviewaccount", form=form)
+    >>> print(view.errors)
+    ['Only the user themselves can activate their account.']
+
+
 An admin can mark an account as belonging to a user who has died.
 
     >>> form = {