Repository: fractal-analytics-platform/fractal-client (language: python)

Issue:
Possible bug: `user edit` removes user's `verified` flag?
To check:

1. Use credentials of the default admin (which is verified and superuser)
2. Check that the admin is verified and superuser
3. Find current user's ID
4. Edit the current user, by changing email and password via `fractal user edit`
5. Verify that the current user has the same flags as before (verified and superuser). If not, this is likely a bug.


Discussion / hints:
```sh
$ poetry run fractal user whoami
{
  "id": 1,
  "email": "admin@fractal.xy",
  "is_active": true,
  "is_superuser": true,
  "is_verified": true,
  "slurm_user": null,
  "cache_dir": null,
  "username": "admin",
  "slurm_accounts": []
}

poetry run fractal user edit --new-email fake@email.com --new-password fakepassword 1
{
  "id": 1,
  "email": "fake@email.com",
  "is_active": true,
  "is_superuser": true,
  "is_verified": false,
  "slurm_user": null,
  "cache_dir": null,
  "username": "admin",
  "slurm_accounts": []
}
```
`is_verified` has changed.


But if we don't edit the email
```sh
$ poetry run fractal user show 1
{
  "id": 1,
  "email": "admin@fractal.xy",
  "is_active": true,
  "is_superuser": true,
  "is_verified": true,
  "slurm_user": null,
  "cache_dir": null,
  "username": "admin",
  "slurm_accounts": []
}
$ poetry run fractal user edit --new-password newpass 1
{
  "id": 1,
  "email": "admin@fractal.xy",
  "is_active": true,
  "is_superuser": true,
  "is_verified": true,
  "slurm_user": null,
  "cache_dir": null,
  "username": "admin",
  "slurm_accounts": []
}
```
`is_verified` is still true
https://github.com/fastapi-users/fastapi-users/blob/ae9f52474ba2c7baebeb923e4d03aea479765362/fastapi_users/manager.py#L674
As per the last comment, this is the default behavior of fastapi-users. It looks funny in our case just because we are not associating "is_verified" to the email address.

Current working proposal, with @ychiucco:
1. If `fractal user edit` includes the `--new-email` field, and it includes either `--make-verified` or `--remove-verified`, the expected verified-related behavior is obtained.
2. If `fractal user edit` includes the `--new-email` field, but it does not include either `--make-verified` or `--remove-verified`, then the CLI command fails before any API call.
3. If `fractal user edit` does not include the `--new-email` field, then the verified-related arguments are not treated in any special way.

TLDR: when updating a user's email, it becomes required to also specify if the user must be verified or not.

Note that we may have to make two API calls, within the CLI-command function, to make sure that the expected `is_verified` value is always set (ref https://github.com/fastapi-users/fastapi-users/blob/ae9f52474ba2c7baebeb923e4d03aea479765362/fastapi_users/manager.py#L674).

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 36c988762..c58f80cde 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,9 @@
 **Note**: Numbers like (\#123) point to closed Pull Requests on the fractal repository.
 
+# 1.4.4 (unreleased)
+
+* Require user's verification to be specified when editing user's email (\#620).
+
 # 1.4.3
 
 * Make `fractal-client` a fully synchronous client, by removing all `async`/`await` (\#592).
diff --git a/fractal_client/cmd/_user.py b/fractal_client/cmd/_user.py
index c041b05b4..17b55492f 100644
--- a/fractal_client/cmd/_user.py
+++ b/fractal_client/cmd/_user.py
@@ -91,6 +91,17 @@ def user_edit(
 
     user_update = dict()
     if new_email is not None:
+        if (make_verified is False) and (remove_verified is False):
+            # Since `fastapi-users` sets `is_verified` to `False` each time the
+            # email is updated, we force the user to make explicit whether the
+            # account is verified or not.
+            return PrintInterface(
+                retcode=1,
+                data=(
+                    "Cannot use `--new-email` without `--make-verified` or "
+                    "`--remove-verified`"
+                ),
+            )
         user_update["email"] = new_email
     if new_password is not None:
         user_update["password"] = new_password
@@ -111,11 +122,21 @@ def user_edit(
 
     if not user_update:
         return PrintInterface(retcode=1, data="Nothing to update")
+
     res = client.patch(
         f"{settings.FRACTAL_SERVER}/auth/users/{user_id}/", json=user_update
     )
     new_user = check_response(res, expected_status_code=200)
 
+    if new_email is not None:
+        # Since `fastapi-users` sets `is_verified` to `False` each time the
+        # email is updated, we set `is_verified` as specified by the user.
+        res = client.patch(
+            f"{settings.FRACTAL_SERVER}/auth/users/{user_id}/",
+            json=dict(is_verified=user_update["is_verified"]),
+        )
+        new_user = check_response(res, expected_status_code=200)
+
     return RichJsonInterface(retcode=0, data=new_user)
 
 


Test patch:
diff --git a/tests/test_user.py b/tests/test_user.py
index ca6d99581..dc55cae21 100644
--- a/tests/test_user.py
+++ b/tests/test_user.py
@@ -96,15 +96,19 @@ def test_edit_as_user(invoke, invoke_as_superuser, register_user, caplog):
     user_id = res.data["id"]
     # Call fractal user edit
     with pytest.raises(SystemExit):
-        invoke(f"user edit {user_id} --new-email email@something.xy")
+        res = invoke(
+            f"user edit {user_id} "
+            "--new-email email@something.xy --make-verified"
+        )
     debug(caplog.text)
     assert "403" in caplog.text
 
 
 @pytest.mark.parametrize("new_is_superuser", [True, False])
+@pytest.mark.parametrize("new_is_verified", [True, False])
 @pytest.mark.parametrize("new_is_non_verified", [True, False])
 def test_edit_as_superuser(
-    invoke_as_superuser, new_is_superuser, new_is_non_verified
+    invoke_as_superuser, new_is_superuser, new_is_verified, new_is_non_verified
 ):
     # Register a new user
     res = invoke_as_superuser(f"user register {EMAIL_USER} {PWD_USER}")
@@ -125,19 +129,34 @@ def test_edit_as_superuser(
     )
     if new_is_superuser:
         cmd = f"{cmd} --make-superuser"
+    if new_is_verified:
+        cmd = f"{cmd} --make-verified"
     if new_is_non_verified:
         cmd = f"{cmd} --remove-verified"
-    debug(cmd)
-    res = invoke_as_superuser(cmd)
-    debug(res.data)
-    assert res.retcode == 0
-    assert res.data["email"] == NEW_EMAIL
-    assert res.data["cache_dir"] == NEW_CACHE_DIR
-    assert res.data["slurm_user"] == NEW_SLURM_USER
-    assert res.data["username"] == NEW_USERNAME
-    assert res.data["is_superuser"] == new_is_superuser
-    if new_is_non_verified:
-        assert not res.data["is_verified"]
+
+    if new_is_verified and new_is_non_verified:
+        with pytest.raises(SystemExit):
+            invoke_as_superuser(cmd)
+    elif new_is_verified or new_is_non_verified:
+        res = invoke_as_superuser(cmd)
+        assert res.retcode == 0
+        assert res.data["email"] == NEW_EMAIL
+        assert res.data["cache_dir"] == NEW_CACHE_DIR
+        assert res.data["slurm_user"] == NEW_SLURM_USER
+        assert res.data["username"] == NEW_USERNAME
+        assert res.data["is_superuser"] == new_is_superuser
+        assert (
+            res.data["is_verified"]
+            if new_is_verified
+            else not res.data["is_verified"]
+        )
+    else:
+        res = invoke_as_superuser(cmd)
+        assert res.retcode == 1
+        assert res.data == (
+            "Cannot use `--new-email` without `--make-verified` or "
+            "`--remove-verified`"
+        )
 
     BAD_CACHE_DIR = "not_absolute"
     with pytest.raises(SystemExit):