From 869d404290ffedd14dfb42577ca2c8162349ab2c Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Mon, 18 Mar 2024 15:28:02 +1000 Subject: [PATCH 1/2] [QOLDEV-744] allow sysadmins to update usernames - Username modification is a potential security risk, but sysadmins can access any account anyway --- changes/4193.feature | 1 + ckan/logic/action/update.py | 2 +- ckan/logic/validators.py | 8 +++++--- ckan/templates/user/edit_user_form.html | 6 +++++- ckan/tests/controllers/test_user.py | 17 +++++++++++++++++ 5 files changed, 29 insertions(+), 5 deletions(-) create mode 100644 changes/4193.feature diff --git a/changes/4193.feature b/changes/4193.feature new file mode 100644 index 00000000000..325ec5b7db3 --- /dev/null +++ b/changes/4193.feature @@ -0,0 +1 @@ +Allow sysadmins to change usernames of other accounts diff --git a/ckan/logic/action/update.py b/ckan/logic/action/update.py index 418f42398f7..c68e02de6db 100644 --- a/ckan/logic/action/update.py +++ b/ckan/logic/action/update.py @@ -774,7 +774,7 @@ def user_update(context: Context, data_dict: DataDict) -> ActionResult.UserUpdat '''Update a user account. Normal users can only update their own user accounts. Sysadmins can update - any user account. Can not modify exisiting user's name. + any user account and modify existing usernames. .. note:: Update methods may delete parameters not explicitly provided in the data_dict. If you want to edit only a specific attribute use `user_patch` diff --git a/ckan/logic/validators.py b/ckan/logic/validators.py index b8c9f409e99..55209751091 100644 --- a/ckan/logic/validators.py +++ b/ckan/logic/validators.py @@ -14,7 +14,7 @@ from sqlalchemy.orm.exc import NoResultFound import ckan.lib.navl.dictization_functions as df -import ckan.logic as logic +from ckan import authz, logic import ckan.logic.converters as converters import ckan.lib.helpers as h from ckan.model import (MAX_TAG_LENGTH, MIN_TAG_LENGTH, @@ -22,7 +22,6 @@ PACKAGE_VERSION_MAX_LENGTH, VOCABULARY_NAME_MAX_LENGTH, VOCABULARY_NAME_MIN_LENGTH) -import ckan.authz as authz from ckan.model.core import State from ckan.common import _ @@ -615,10 +614,13 @@ def user_name_validator(key: FlattenKey, data: FlattenDataDict, return else: # Otherwise return an error: there's already another user with that - # name, so you can create a new user with that name or update an + # name, so you can't create a new user with that name or update an # existing user's name to that name. errors[key].append(_('That login name is not available.')) elif user_obj_from_context: + requester = context.get('auth_user_obj', None) + if requester and authz.is_sysadmin(requester.name): + return old_user = model.User.get(user_obj_from_context.id) if old_user is not None and old_user.state != model.State.PENDING: errors[key].append(_('That login name can not be modified.')) diff --git a/ckan/templates/user/edit_user_form.html b/ckan/templates/user/edit_user_form.html index 3b1a3331888..01e68715288 100644 --- a/ckan/templates/user/edit_user_form.html +++ b/ckan/templates/user/edit_user_form.html @@ -8,7 +8,11 @@ {% block core_fields %}
{{ _('Change details') }} - {{ form.input('name', label=_('Username'), id='field-username', value=data.name, error=errors.name, classes=['control-medium'], attrs={'readonly': '', 'class': 'form-control'}) }} + {% if g.userobj.sysadmin %} + {{ form.input('name', label=_('Username'), id='field-username', value=data.name, error=errors.name, classes=['control-medium'], is_required=true) }} + {% else %} + {{ form.input('name', label=_('Username'), id='field-username', value=data.name, error=errors.name, classes=['control-medium'], attrs={'readonly': '', 'class': 'form-control'}) }} + {% endif %} {{ form.input('fullname', label=_('Full name'), id='field-fullname', value=data.fullname, error=errors.fullname, placeholder=_('eg. Joe Bloggs'), classes=['control-medium']) }} diff --git a/ckan/tests/controllers/test_user.py b/ckan/tests/controllers/test_user.py index 13ca9509af4..30f5b91acbd 100644 --- a/ckan/tests/controllers/test_user.py +++ b/ckan/tests/controllers/test_user.py @@ -428,6 +428,23 @@ def test_edit_user_logged_in_username_change_by_id(self, app, user): assert "That login name can not be modified" in response + def test_edit_user_logged_in_username_change_by_sysadmin( + self, app, user, sysadmin): + env = {"Authorization": sysadmin["token"]} + + response = app.post( + url=url_for("user.edit", id=user["id"]), + data={ + "email": user["email"], + "save": "", + "password1": "", + "password2": "", + "name": factories.User.stub().name, + }, + extra_environ=env + ) + assert 'Profile updated' in response + def test_perform_reset_for_key_change(self, app): password = "TestPassword1" params = {"password1": password, "password2": password} From 4f040003216abd9e2dafda6980b6746adfde74a8 Mon Sep 17 00:00:00 2001 From: ThrawnCA Date: Thu, 18 Apr 2024 21:30:23 +1000 Subject: [PATCH 2/2] adjust test syntax to match other tests, #4193 - Pass API token via 'headers' parameter instead of 'extra_environ' --- ckan/tests/controllers/test_user.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ckan/tests/controllers/test_user.py b/ckan/tests/controllers/test_user.py index 30f5b91acbd..dcc3f20af98 100644 --- a/ckan/tests/controllers/test_user.py +++ b/ckan/tests/controllers/test_user.py @@ -430,18 +430,19 @@ def test_edit_user_logged_in_username_change_by_id(self, app, user): def test_edit_user_logged_in_username_change_by_sysadmin( self, app, user, sysadmin): - env = {"Authorization": sysadmin["token"]} + headers = {"Authorization": sysadmin["token"]} response = app.post( url=url_for("user.edit", id=user["id"]), data={ "email": user["email"], "save": "", + "old_password": "correct123", "password1": "", "password2": "", "name": factories.User.stub().name, }, - extra_environ=env + headers=headers ) assert 'Profile updated' in response