From d55dab93da0f5c82ac570bc56cc8f54b26de6fd2 Mon Sep 17 00:00:00 2001
From: Kevin Morris <kevr@0cost.org>
Date: Mon, 27 Dec 2021 17:42:56 -0800
Subject: [PATCH] revert account type permission changes

While this does make more sense to me personally, there is no need
to change how the AUR treats its users; it has been accepted for
ages and not found to be ridden with flaws. Stay with the tried
and true method.

Signed-off-by: Kevin Morris <kevr@0cost.org>
---
 aurweb/routers/accounts.py           |  24 +++--
 aurweb/users/validate.py             |  27 ++----
 templates/partials/account_form.html |  34 ++++---
 test/test_accounts_routes.py         | 129 ++++++++++++++++++++-------
 4 files changed, 132 insertions(+), 82 deletions(-)

diff --git a/aurweb/routers/accounts.py b/aurweb/routers/accounts.py
index 4e0ce33b..6fffd79c 100644
--- a/aurweb/routers/accounts.py
+++ b/aurweb/routers/accounts.py
@@ -10,7 +10,7 @@ from sqlalchemy import and_, or_
 import aurweb.config
 
 from aurweb import cookies, db, l10n, logging, models, util
-from aurweb.auth import account_type_required, auth_required, creds
+from aurweb.auth import account_type_required, auth_required
 from aurweb.captcha import get_captcha_salts
 from aurweb.exceptions import ValidationError
 from aurweb.l10n import get_translator_for_request
@@ -169,17 +169,15 @@ def make_account_form_context(context: dict,
     # Do not modify the original context.
     context = copy.copy(context)
 
-    context["account_types"] = [
-        (at.USER_ID, f"Normal {at.USER}"),
-        (at.TRUSTED_USER_ID, at.TRUSTED_USER)
-    ]
-
-    user_account_type_id = context.get("account_types")[0][0]
-
-    if request.user.has_credential(creds.ACCOUNT_EDIT_DEV):
-        context["account_types"].append((at.DEVELOPER_ID, at.DEVELOPER))
-        context["account_types"].append((at.TRUSTED_USER_AND_DEV_ID,
-                                         at.TRUSTED_USER_AND_DEV))
+    context["account_types"] = list(filter(
+        lambda e: request.user.AccountTypeID >= e[0],
+        [
+            (at.USER_ID, f"Normal {at.USER}"),
+            (at.TRUSTED_USER_ID, at.TRUSTED_USER),
+            (at.DEVELOPER_ID, at.DEVELOPER),
+            (at.TRUSTED_USER_AND_DEV_ID, at.TRUSTED_USER_AND_DEV)
+        ]
+    ))
 
     if request.user.is_authenticated():
         context["username"] = args.get("U", user.Username)
@@ -202,7 +200,7 @@ def make_account_form_context(context: dict,
         context["inactive"] = args.get("J", user.InactivityTS != 0)
     else:
         context["username"] = args.get("U", str())
-        context["account_type"] = args.get("T", user_account_type_id)
+        context["account_type"] = args.get("T", at.USER_ID)
         context["suspended"] = args.get("S", False)
         context["email"] = args.get("E", str())
         context["hide_email"] = args.get("H", False)
diff --git a/aurweb/users/validate.py b/aurweb/users/validate.py
index 32146094..bbd6082a 100644
--- a/aurweb/users/validate.py
+++ b/aurweb/users/validate.py
@@ -13,7 +13,6 @@ from aurweb import config, db, l10n, logging, models, time, util
 from aurweb.auth import creds
 from aurweb.captcha import get_captcha_answer, get_captcha_salts, get_captcha_token
 from aurweb.exceptions import ValidationError
-from aurweb.models import account_type as at
 from aurweb.models.account_type import ACCOUNT_TYPE_NAME
 from aurweb.models.ssh_pub_key import get_fingerprint
 
@@ -171,31 +170,23 @@ def invalid_account_type(T: int = None, request: Request = None,
                          _: l10n.Translator = None,
                          **kwargs) -> None:
     if T is not None and (T := int(T)) != user.AccountTypeID:
+        name = ACCOUNT_TYPE_NAME.get(T, None)
         has_cred = request.user.has_credential(creds.ACCOUNT_CHANGE_TYPE)
-        if T not in ACCOUNT_TYPE_NAME:
+        if name is None:
             raise ValidationError(["Invalid account type provided."])
         elif not has_cred:
             raise ValidationError([
                 "You do not have permission to change account types."])
-
-        credential_checks = {
-            at.USER_ID: request.user.is_trusted_user,
-            at.TRUSTED_USER_ID: request.user.is_trusted_user,
-            at.DEVELOPER_ID: request.user.is_developer,
-            at.TRUSTED_USER_AND_DEV_ID: (lambda: request.user.is_trusted_user()
-                                         and request.user.is_developer())
-        }
-        credential_check = credential_checks.get(T)
-
-        name = ACCOUNT_TYPE_NAME.get(T)
-        if not credential_check() or request.user == user:
+        elif T > request.user.AccountTypeID:
+            # If the chosen account type is higher than the editor's account
+            # type, the editor doesn't have permission to set the new type.
             error = _("You do not have permission to change "
                       "this user's account type to %s.") % name
             raise ValidationError([error])
-        else:
-            logger.debug(f"Trusted User '{request.user.Username}' has "
-                         f"modified '{user.Username}' account's type to"
-                         f" {name}.")
+
+        logger.debug(f"Trusted User '{request.user.Username}' has "
+                     f"modified '{user.Username}' account's type to"
+                     f" {name}.")
 
 
 def invalid_captcha(captcha_salt: str = None, captcha: str = None,
diff --git a/templates/partials/account_form.html b/templates/partials/account_form.html
index 8ed01ce7..f6a24c66 100644
--- a/templates/partials/account_form.html
+++ b/templates/partials/account_form.html
@@ -43,25 +43,23 @@
         </p>
 
         {% if request.user.has_credential(creds.ACCOUNT_CHANGE_TYPE) %}
-            {% if request.user != user %}
-                <p>
-                    <label for="id_type">
-                        {% trans %}Account Type{% endtrans %}:
-                    </label>
-                    <select name="T" id="id_type">
-                        {% for value, type in account_types %}
-                            <option value="{{ value }}"
-                                    {% if user.AccountType.ID == value %}
-                                    selected="selected"
-                                    {% endif %}
-                            >
-                                {{ type | tr }}
-                            </option>
-                        {% endfor %}
+            <p>
+                <label for="id_type">
+                    {% trans %}Account Type{% endtrans %}:
+                </label>
+                <select name="T" id="id_type">
+                    {% for value, type in account_types %}
+                        <option value="{{ value }}"
+                                {% if user.AccountType.ID == value %}
+                                selected="selected"
+                                {% endif %}
+                        >
+                            {{ type | tr }}
+                        </option>
+                    {% endfor %}
 
-                    </select>
-                </p>
-            {% endif %}
+                </select>
+            </p>
             <p>
                 <label for="id_suspended">
                     {% trans %}Account Suspended{% endtrans %}:
diff --git a/test/test_accounts_routes.py b/test/test_accounts_routes.py
index bca87d59..3b7c7161 100644
--- a/test/test_accounts_routes.py
+++ b/test/test_accounts_routes.py
@@ -613,27 +613,6 @@ def test_post_register_with_ssh_pubkey(client: TestClient):
     assert response.status_code == int(HTTPStatus.OK)
 
 
-def test_get_account_edit_self_as_tu(client: TestClient, tu_user: User):
-    """ Test edit get route of ourselves as a TU. """
-    cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
-    endpoint = f"/account/{tu_user.Username}/edit"
-
-    with client as request:
-        response = request.get(endpoint, cookies=cookies)
-    assert response.status_code == int(HTTPStatus.OK)
-
-    # Account type can't be modified when editing ourselves.
-    root = parse_root(response.text)
-    atype = root.xpath('//select[@id="id_type"]/option[@selected="selected"]')
-    assert not atype
-
-    # But other fields should be available and match up.
-    username = root.xpath('//input[@id="id_username"]')[0]
-    assert username.attrib["value"] == tu_user.Username
-    email = root.xpath('//input[@id="id_email"]')[0]
-    assert email.attrib["value"] == tu_user.Email
-
-
 def test_get_account_edit_tu_as_tu(client: TestClient, tu_user: User):
     """ Test edit get route of another TU as a TU. """
     with db.begin():
@@ -686,6 +665,33 @@ def test_get_account_edit_as_tu(client: TestClient, tu_user: User):
     assert email.attrib["value"] == user2.Email
 
 
+def test_get_account_edit_type(client: TestClient, user: User):
+    """ Test that users do not have an Account Type field. """
+    cookies = {"AURSID": user.login(Request(), "testPassword")}
+    endpoint = f"/account/{user.Username}/edit"
+
+    with client as request:
+        response = request.get(endpoint, cookies=cookies)
+    assert response.status_code == int(HTTPStatus.OK)
+    assert "id_type" not in response.text
+
+
+def test_get_account_edit_type_as_tu(client: TestClient, tu_user: User):
+    with db.begin():
+        user2 = create_user("test_tu")
+
+    cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
+    endpoint = f"/account/{user2.Username}/edit"
+
+    with client as request:
+        response = request.get(endpoint, cookies=cookies)
+    assert response.status_code == int(HTTPStatus.OK)
+
+    root = parse_root(response.text)
+    atype = root.xpath('//select[@id="id_type"]/option[@selected="selected"]')
+    assert atype[0].text.strip() == f"Normal {at.USER}"
+
+
 def test_get_account_edit_unauthorized(client: TestClient, user: User):
     request = Request()
     sid = user.login(request, "testPassword")
@@ -727,7 +733,68 @@ def test_post_account_edit(client: TestClient, user: User):
     assert expected in response.content.decode()
 
 
-def test_post_account_edit_dev(client: TestClient, user: User):
+def test_post_account_edit_type_as_tu(client: TestClient, tu_user: User):
+    with db.begin():
+        user2 = create_user("test_tu")
+
+    cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
+    endpoint = f"/account/{user2.Username}/edit"
+    data = {
+        "U": user2.Username,
+        "E": user2.Email,
+        "T": at.USER_ID,
+        "passwd": "testPassword",
+    }
+    with client as request:
+        resp = request.post(endpoint, data=data, cookies=cookies)
+    assert resp.status_code == int(HTTPStatus.OK)
+
+
+def test_post_account_edit_type_as_dev(client: TestClient, tu_user: User):
+    with db.begin():
+        user2 = create_user("test2")
+        tu_user.AccountTypeID = at.DEVELOPER_ID
+
+    cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
+    endpoint = f"/account/{user2.Username}/edit"
+    data = {
+        "U": user2.Username,
+        "E": user2.Email,
+        "T": at.DEVELOPER_ID,
+        "passwd": "testPassword",
+    }
+    with client as request:
+        resp = request.post(endpoint, data=data, cookies=cookies)
+    assert resp.status_code == int(HTTPStatus.OK)
+    assert user2.AccountTypeID == at.DEVELOPER_ID
+
+
+def test_post_account_edit_invalid_type_as_tu(client: TestClient,
+                                              tu_user: User):
+    with db.begin():
+        user2 = create_user("test_tu")
+        tu_user.AccountTypeID = at.TRUSTED_USER_ID
+
+    cookies = {"AURSID": tu_user.login(Request(), "testPassword")}
+    endpoint = f"/account/{user2.Username}/edit"
+    data = {
+        "U": user2.Username,
+        "E": user2.Email,
+        "T": at.DEVELOPER_ID,
+        "passwd": "testPassword",
+    }
+    with client as request:
+        resp = request.post(endpoint, data=data, cookies=cookies)
+    assert resp.status_code == int(HTTPStatus.BAD_REQUEST)
+    assert user2.AccountTypeID == at.USER_ID
+
+    errors = get_errors(resp.text)
+    expected = ("You do not have permission to change this user's "
+                f"account type to {at.DEVELOPER}.")
+    assert errors[0].text.strip() == expected
+
+
+def test_post_account_edit_dev(client: TestClient, tu_user: User):
     # Modify our user to be a "Trusted User & Developer"
     name = "Trusted User & Developer"
     tu_or_dev = query(AccountType, AccountType.AccountType == name).first()
@@ -735,7 +802,7 @@ def test_post_account_edit_dev(client: TestClient, user: User):
         user.AccountType = tu_or_dev
 
     request = Request()
-    sid = user.login(request, "testPassword")
+    sid = tu_user.login(request, "testPassword")
 
     post_data = {
         "U": "test",
@@ -743,11 +810,10 @@ def test_post_account_edit_dev(client: TestClient, user: User):
         "passwd": "testPassword"
     }
 
+    endpoint = f"/account/{tu_user.Username}/edit"
     with client as request:
-        response = request.post("/account/test/edit", cookies={
-            "AURSID": sid
-        }, data=post_data, allow_redirects=False)
-
+        response = request.post(endpoint, cookies={"AURSID": sid},
+                                data=post_data, allow_redirects=False)
     assert response.status_code == int(HTTPStatus.OK)
 
     expected = "The account, <strong>test</strong>, "
@@ -1102,7 +1168,7 @@ def test_post_account_edit_self_type_as_tu(client: TestClient, tu_user: User):
     with client as request:
         resp = request.get(endpoint, cookies=cookies, allow_redirects=False)
     assert resp.status_code == int(HTTPStatus.OK)
-    assert "id_type" not in resp.text
+    assert "id_type" in resp.text
 
     # We cannot modify our own account type.
     data = {
@@ -1113,12 +1179,9 @@ def test_post_account_edit_self_type_as_tu(client: TestClient, tu_user: User):
     }
     with client as request:
         resp = request.post(endpoint, data=data, cookies=cookies)
-    assert resp.status_code == int(HTTPStatus.BAD_REQUEST)
+    assert resp.status_code == int(HTTPStatus.OK)
 
-    errors = get_errors(resp.text)
-    expected = ("You do not have permission to change this "
-                "user's account type to User.")
-    assert errors[0].text.strip() == expected
+    assert tu_user.AccountTypeID == USER_ID
 
 
 def test_post_account_edit_other_user_type_as_tu(