@almet review: use a 'validated' field on the user record

kinto/plugins/accounts/authentication.py

@@ -4,7 +4,7 @@
 from kinto.core import utils
 from kinto.core.storage import exceptions as storage_exceptions
 
-from .utils import ACCOUNT_CACHE_KEY, ACCOUNT_POLICY_NAME
+from .utils import ACCOUNT_CACHE_KEY, ACCOUNT_POLICY_NAME, is_validated
 
 
 def account_check(username, password, request):
@@ -35,8 +35,7 @@ def account_check(username, password, request):
     except storage_exceptions.ObjectNotFoundError:
         return None
 
-    if validation and "activation-key" in existing:
-        # The account hasn't been validated yet.
+    if validation and not is_validated(existing):
         return None
 
     hashed = existing["password"].encode(encoding="utf-8")

kinto/plugins/accounts/utils.py

@@ -11,3 +11,11 @@ def hash_password(password):
     pwd_str = password.encode(encoding="utf-8")
     hashed = bcrypt.hashpw(pwd_str, bcrypt.gensalt())
     return hashed.decode(encoding="utf-8")
+
+
+def is_validated(user):
+    """Is this user record validated?"""
+    # An account is "validated" if it has the `validated` field set to True, or
+    # no `validated` field at all (for accounts created before the "account
+    # validation option" was enabled).
+    return "validated" not in user or user["validated"]

kinto/plugins/accounts/views.py

@@ -16,7 +16,7 @@
 from kinto.core.events import ResourceChanged, ACTIONS
 from kinto.core.storage import exceptions as storage_exceptions
 
-from .utils import hash_password, ACCOUNT_CACHE_KEY, ACCOUNT_POLICY_NAME
+from .utils import hash_password, is_validated, ACCOUNT_CACHE_KEY, ACCOUNT_POLICY_NAME
 
 
 def _extract_posted_body_id(request):
@@ -160,6 +160,7 @@ def process_object(self, new, old=None):
 
             activation_key = str(uuid.uuid4())
             new["activation-key"] = activation_key
+            new["validated"] = False
 
             # Send an email to the user with the link to activate their account.
             message = Message(
@@ -225,17 +226,16 @@ def post_validation(request):
         error_details = {"message": "Account ID and activation key do not match"}
         raise http_error(httpexceptions.HTTPForbidden(), **error_details)
 
-    if "activation-key" not in user:
+    if is_validated(user):
         error_details = {"message": f"Account {user_id} has already been validated"}
         raise http_error(httpexceptions.HTTPForbidden(), **error_details)
 
     if user["activation-key"] != activation_key:
         error_details = {"message": "Account ID and activation key do not match"}
         raise http_error(httpexceptions.HTTPForbidden(), **error_details)
 
-    # Remove fields related to the account validation now that the user is validated.
-    del user["activation-form-url"]
-    del user["activation-key"]
+    # User is now validated.
+    user["validated"] = True
 
     request.registry.storage.update(
         parent_id=parent_id, resource_name="account", object_id=user_id, record=user

tests/plugins/test_accounts.py

@@ -7,6 +7,7 @@
 from pyramid.exceptions import ConfigurationError
 
 from kinto.plugins.accounts import scripts, ACCOUNT_CACHE_KEY
+from kinto.plugins.accounts.utils import hash_password
 from .. import support
 
 
@@ -202,7 +203,7 @@ def test_create_account_requires_activation_form_url(self):
         )
         assert "requires an `activation-form-url` field" in resp.json["message"]
 
-    def test_create_account_appends_activation_code_to_activation_form_url(self):
+    def test_create_account_stores_activated_field(self):
         activation_form_url = "https://example.com/"
         uuid_string = "20e81ab7-51c0-444f-b204-f1c4cfe1aa7a"
         with mock.patch("uuid.uuid4", return_value=uuid.UUID(uuid_string)):
@@ -219,6 +220,8 @@ def test_create_account_appends_activation_code_to_activation_form_url(self):
             )
         assert resp.json["data"]["activation-form-url"] == activation_form_url
         assert resp.json["data"]["activation-key"] == uuid_string
+        assert "validated" in resp.json["data"]
+        assert not resp.json["data"]["validated"]
         mailer_call = self.get_mailer().send.call_args_list[0]
         assert mailer_call[0][0].sender == "admin@example.com"
         assert mailer_call[0][0].subject == "activate your account"
@@ -233,6 +236,7 @@ def test_cant_authenticate_with_unactivated_account(self):
                     "id": "alice@example.com",
                     "password": "12éé6",
                     "activation-form-url": "https://example.com",
+                    "activated": False,
                 }
             },
             status=201,
@@ -265,8 +269,8 @@ def test_validation_fail_bad_activation_key(self):
         )
         assert "Account ID and activation key do not match" in resp.json["message"]
 
-    def test_validation_removes_fields(self):
-        # On user activation the 'activation-form-url' and 'activation-key' fields are removed.
+    def test_validation_validates_user(self):
+        # On user activation the 'validated' field is set to True.
         uuid_string = "20e81ab7-51c0-444f-b204-f1c4cfe1aa7a"
         with mock.patch("uuid.uuid4", return_value=uuid.UUID(uuid_string)):
             self.app.post_json(
@@ -283,8 +287,8 @@ def test_validation_removes_fields(self):
         resp = self.app.post_json(
             "/accounts/alice@example.com/validate/" + uuid_string, {}, status=200
         )
-        assert "activation-form-url" not in resp.json
-        assert "activation-key" not in resp.json
+        assert "validated" in resp.json
+        assert resp.json["validated"]
         # An active user can authenticate.
         resp = self.app.get("/", headers=get_user_headers("alice@example.com", "12éé6"))
         assert resp.json["user"]["id"] == "account:alice@example.com"
@@ -307,39 +311,23 @@ def test_validation_fail_active_user(self):
         resp = self.app.post_json(
             "/accounts/alice@example.com/validate/" + uuid_string, {}, status=200
         )
-        assert "activation-form-url" not in resp.json
-        assert "activation-key" not in resp.json
         # Try validating again.
         resp = self.app.post_json(
             "/accounts/alice@example.com/validate/" + uuid_string, {}, status=403
         )
         assert "Account alice@example.com has already been validated" in resp.json["message"]
 
-    def test_dont_check_activation_form_url_on_an_active_user(self):
-        # Once the user is active, don't check for the activation-form-url or add an activation-key.
-        uuid_string = "20e81ab7-51c0-444f-b204-f1c4cfe1aa7a"
-        with mock.patch("uuid.uuid4", return_value=uuid.UUID(uuid_string)):
-            self.app.post_json(
-                "/accounts",
-                {
-                    "data": {
-                        "id": "alice@example.com",
-                        "password": "12éé6",
-                        "activation-form-url": "https://example.com",
-                    }
-                },
-                status=201,
-            )
-        self.app.post_json("/accounts/alice@example.com/validate/" + uuid_string, {}, status=200)
-        resp = self.app.patch_json(
-            "/accounts/alice@example.com",
-            {"data": {"some-other-metadata": "foobar"}},
-            status=200,
-            headers=get_user_headers("alice@example.com", "12éé6"),
-        )
-        assert "activation-form-url" not in resp.json["data"]
-        assert "activation-key" not in resp.json["data"]
-        assert resp.json["data"]["some-other-metadata"] == "foobar"
+    def test_previously_created_accounts_can_still_authenticate(self):
+        """Accounts created before activating the 'account validation' option can still authenticate."""
+        # Create an account without going through the accounts API.
+        hashed_password = hash_password("12éé6")
+        self.app.app.registry.storage.create(
+            parent_id="alice",
+            resource_name="account",
+            record={"id": "alice", "password": hashed_password},
+        )
+        resp = self.app.get("/", headers=get_user_headers("alice", "12éé6"))
+        assert resp.json["user"]["id"] == "account:alice"
 
 
 class AccountUpdateTest(AccountsWebTest):