Merge pull request #3531 from fanjinfei/ckanmaster

do not allow to modify exsiting username

ckan/controllers/user.py

@@ -512,8 +512,11 @@ def perform_reset(self, id):
         if request.method == 'POST':
             try:
                 context['reset_password'] = True
+                user_state = user_dict['state']
                 new_password = self._get_form_password()
-                user_dict['password'] = new_password
+                username = request.params.get('name')
+                if (username is not None and username != ''):
+                    user_dict['name'] = username
                 user_dict['reset_key'] = c.reset_key
                 user_dict['state'] = model.State.ACTIVE
                 user = get_action('user_update')(context, user_dict)
@@ -531,6 +534,7 @@ def perform_reset(self, id):
                 h.flash_error(u'%r' % e.error_dict)
             except ValueError, ve:
                 h.flash_error(unicode(ve))
+            user_dict['state'] = user_state
 
         c.user_dict = user_dict
         return render('user/perform_reset.html')

ckan/logic/action/update.py

@@ -668,7 +668,7 @@ def user_update(context, data_dict):
     '''Update a user account.
 
     Normal users can only update their own user accounts. Sysadmins can update
-    any user account.
+    any user account. Can not modify exisiting user's name.
 
     For further parameters see
     :py:func:`~ckan.logic.action.create.user_create`.

ckan/logic/validators.py

@@ -568,10 +568,9 @@ def user_name_validator(key, data, errors, context):
         raise Invalid(_('User names must be strings'))
 
     user = model.User.get(new_user_name)
+    user_obj_from_context = context.get('user_obj')
     if user is not None:
         # A user with new_user_name already exists in the database.
-
-        user_obj_from_context = context.get('user_obj')
         if user_obj_from_context and user_obj_from_context.id == user.id:
             # If there's a user_obj in context with the same id as the user
             # found in the db, then we must be doing a user_update and not
@@ -582,6 +581,12 @@ def user_name_validator(key, data, errors, context):
             # name, so you can 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:
+        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.'))
+        else:
+            return
 
 def user_both_passwords_entered(key, data, errors, context):
 

ckan/templates/user/edit_user_form.html

@@ -5,7 +5,7 @@
 
   <fieldset>
     <legend>{{ _('Change details') }}</legend>
-    {{ form.input('name', label=_('Username'), id='field-username', value=data.name, error=errors.name, classes=['control-medium'], is_required=true) }}
+    {{ form.input('name', label=_('Username'), id='field-username', value=data.name, error=errors.name, classes=['control-medium'], attrs={'readonly': ''}) }}
 
     {{ form.input('fullname', label=_('Full name'), id='field-fullname', value=data.fullname, error=errors.fullname, placeholder=_('eg. Joe Bloggs'), classes=['control-medium']) }}
 

ckan/templates/user/perform_reset.html

@@ -17,6 +17,11 @@ <h1 class="module-heading">
       {% block form %}
         <form class="form-horizontal" action="" method="post">
           {{ form.errors(error_summary) }}
+          {% if c.user_dict['state'] == 'pending' %}
+            <p>{{ _('You can also change username. It can not be modified later.') }}</p>
+            {{ form.input("name", id="field-name", label=_("Username"), type="text", value=c.user_dict["name"],
+               error='', attrs={'autocomplete': 'no'}, classes=["control-medium"]) }}
+          {% endif %}
           {{ form.input("password1", id="field-password", label=_("Password"), type="password", value='', error='', attrs={'autocomplete': 'no'}, classes=["control-medium"]) }}
           {{ form.input("password2", id="field-confirm-password", label=_("Confirm"), type="password", value='', error='', attrs={'autocomplete': 'no'}, classes=["control-medium"]) }}
           <div class="form-actions">

ckan/tests/controllers/test_user.py

@@ -272,11 +272,9 @@ def test_edit_user_logged_in_username_change(self):
 
         # new values
         form['name'] = 'new-name'
-        response = submit_and_follow(app, form, name='save')
-        response = helpers.webtest_maybe_follow(response)
-
-        expected_url = url_for(controller='user', action='read', id='new-name')
-        assert response.request.path == expected_url
+        env = {'REMOTE_USER': user['name'].encode('ascii')}
+        response = webtest_submit(form, 'save', status=200, extra_environ=env)
+        assert_true('That login name can not be modified' in response)
 
     def test_edit_user_logged_in_username_change_by_name(self):
         user_pass = 'pass'
@@ -303,11 +301,9 @@ def test_edit_user_logged_in_username_change_by_name(self):
 
         # new values
         form['name'] = 'new-name'
-        response = submit_and_follow(app, form, name='save')
-        response = helpers.webtest_maybe_follow(response)
-
-        expected_url = url_for(controller='user', action='read', id='new-name')
-        assert response.request.path == expected_url
+        env = {'REMOTE_USER': user['name'].encode('ascii')}
+        response = webtest_submit(form, 'save', status=200, extra_environ=env)
+        assert_true('That login name can not be modified' in response)
 
     def test_edit_user_logged_in_username_change_by_id(self):
         user_pass = 'pass'
@@ -334,11 +330,9 @@ def test_edit_user_logged_in_username_change_by_id(self):
 
         # new values
         form['name'] = 'new-name'
-        response = submit_and_follow(app, form, name='save')
-        response = helpers.webtest_maybe_follow(response)
-
-        expected_url = url_for(controller='user', action='read', id='new-name')
-        assert response.request.path == expected_url
+        env = {'REMOTE_USER': user['name'].encode('ascii')}
+        response = webtest_submit(form, 'save', status=200, extra_environ=env)
+        assert_true('That login name can not be modified' in response)
 
     def test_perform_reset_for_key_change(self):
         password = 'password'

ckan/tests/logic/action/test_update.py

@@ -65,24 +65,12 @@ def test_user_update_name(self):
 
         # 1. Setup.
         user = factories.User()
+        user['name'] = 'updated'
 
-        # 2. Call the function that's being tested, once only.
-        # FIXME we have to pass the email address and password to user_update
-        # even though we're not updating those fields, otherwise validation
-        # fails.
-        helpers.call_action('user_update', id=user['name'],
-                            email=user['email'],
-                            password=factories.User.attributes()['password'],
-                            name='updated',
-                            )
-
-        # 3. Make assertions about the return value and/or side-effects.
-        updated_user = helpers.call_action('user_show', id=user['id'])
-        # Note that we check just the field we were trying to update, not the
-        # entire dict, only assert what we're actually testing.
-        assert updated_user['name'] == 'updated'
-
-        # 4. Do nothing else!
+        # 2. Make assertions about the return value and/or side-effects.
+        assert_raises(logic.ValidationError,
+                      helpers.call_action, 'user_update',
+                      **user)
 
     ## END-BEFORE
 
@@ -249,7 +237,7 @@ def test_user_update_activity_stream(self):
         helpers.call_action('user_update', id=user['name'],
                             email=user['email'],
                             password=factories.User.attributes()['password'],
-                            name='updated',
+                            fullname='updated full name',
                             )
 
         activity_stream = helpers.call_action('user_activity_list',
@@ -292,7 +280,7 @@ def test_user_update_with_custom_schema(self):
         helpers.call_action('user_update', context={'schema': schema},
                             id=user['name'], email=user['email'],
                             password=factories.User.attributes()['password'],
-                            name='updated',
+                            fullname='updated full name',
                             )
 
         # Since we passed user['name'] to user_update as the 'id' param,
@@ -307,7 +295,6 @@ def test_user_update_multiple(self):
 
         params = {
             'id': user['id'],
-            'name': 'updated_name',
             'fullname': 'updated full name',
             'about': 'updated about',
             # FIXME: We shouldn't have to put email here since we're not
@@ -321,7 +308,6 @@ def test_user_update_multiple(self):
         helpers.call_action('user_update', **params)
 
         updated_user = helpers.call_action('user_show', id=user['id'])
-        assert updated_user['name'] == 'updated_name'
         assert updated_user['fullname'] == 'updated full name'
         assert updated_user['about'] == 'updated about'
 
@@ -333,7 +319,6 @@ def test_user_update_does_not_return_password(self):
 
         params = {
             'id': user['id'],
-            'name': 'updated_name',
             'fullname': 'updated full name',
             'about': 'updated about',
             'email': user['email'],
@@ -351,7 +336,6 @@ def test_user_update_does_not_return_apikey(self):
 
         params = {
             'id': user['id'],
-            'name': 'updated_name',
             'fullname': 'updated full name',
             'about': 'updated about',
             'email': user['email'],
@@ -373,7 +357,6 @@ def test_user_update_does_not_return_reset_key(self):
 
         params = {
             'id': user['id'],
-            'name': 'updated_name',
             'fullname': 'updated full name',
             'about': 'updated about',
             'email': user['email'],